Am 29.03.2017 um 16:18 hat Denis V. Lunev geschrieben: > On 03/29/2017 05:11 PM, Kevin Wolf wrote: > > Am 29.03.2017 um 15:53 hat Denis V. Lunev geschrieben: > >> On 03/29/2017 01:41 PM, Kevin Wolf wrote: > >>> The question is what the contract of bdrv_truncate() is. I think "you > >>> can only call this when you got resize permissions" is the clearest > >>> interface, and the current code enforces it. > >> but in the original patch I have made check exactly over this simple > >> condition and you says that it was not accurate. If this is wrong, I'll be > >> rejected later on with EACCESS and will still be on the safe side. > >> Original patch just avoids the assert(). > > No, you checked BDRV_O_RESIZE in bs->open_flags, not BLK_PERM_RESIZE in > > child->perm. If you checked for BLK_PERM_RESIZE, that would work (though > > I still think that checking for read-only gets closer to the actual > > intent). > OK. That is clear now, I'll send a fixup. > Thank you for the explanation.
Ok, thanks. > >>>> Another thing, should we add assert like added into bdrv_co_pwritev, > >>>> namely > >>>> assert(!(bs->open_flags & BDRV_O_INACTIVE)); > >>>> in the same place below access check. > >>> You mean asserting that we have write permission? We already do that in > >>> bdrv_aligned_pwritev(), which is called by bdrv_co_pwritev(). > >> I mean that we should disallow image change if it is disallowed > >> by the contract. Current contract says that we can not change > >> image content once BDRV_O_INACTIVE is set. Should we > >> Do we have implicit rule that (child->perm & BLK_PERM_RESIZE) > >> is set only when INACTIVE is not set? > > Ah, you want to assert cleared BDRV_O_INACTIVE in bdrv_truncate()? > > That sounds like a good idea to me. > but this is for 2.10. I think it is too late to do that right now. Yes, definitely for 2.10. If you like, you can already send a patch anyway, I always have a block-next queue while we're in freeze. Kevin
