Am 14.02.2018 um 15:44 hat Eric Blake geschrieben: > On 02/14/2018 06:05 AM, Kevin Wolf wrote: > > Am 13.02.2018 um 21:26 hat Eric Blake geschrieben: > > > We are gradually moving away from sector-based interfaces, towards > > > byte-based. Update the null driver accordingly. > > > > > > Signed-off-by: Eric Blake <ebl...@redhat.com> > > > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> > > > Reviewed-by: Fam Zheng <f...@redhat.com> > > > > > > > if (s->read_zeroes) { > > > - return BDRV_BLOCK_OFFSET_VALID | start | BDRV_BLOCK_ZERO; > > > - } else { > > > - return BDRV_BLOCK_OFFSET_VALID | start; > > > + ret |= BDRV_BLOCK_ZERO; > > > } > > > + return ret; > > > } > > > > Preexisting, but I think this return value is wrong. OFFSET_VALID > > without DATA is to documented to have the following semantics: > > > > * DATA ZERO OFFSET_VALID > > * f t t sectors preallocated, read as zero, returned > > file not > > * necessarily zero at offset > > * f f t sectors preallocated but read from backing_hd, > > * returned file contains garbage at offset > > > > I'm not sure what OFFSET_VALID is even supposed to mean for null. > > Yeah, and I was even thinking about that a bit yesterday when figuring out > what to do with nvme. It does highlight the fact that you get garbage when > reading from the null driver (unless the zero option was enabled, then ZERO > is set and you know you read zeros instead) - but there no pointer that is > preallocated (whether it contains garbage or otherwise) that you can > actually dereference to read what the guest would see. > > > > > Or in fact, what it is supposed to mean for any protocol driver, because > > normally it just means I can use this offset for accessing bs->file. But > > protocol drivers don't have a bs->file, so it's interesting to see that > > they still all set this flag. > > > > OFFSET_VALID | DATA might be excusable because I can see that it's > > convenient that a protocol driver refers to itself as *file instead of > > returning NULL there and then the offset is valid (though it would be > > pointless to actually follow the file pointer), but OFFSET_VALID without > > DATA probably isn't. > > Hmm, you're probably right. Maybe that means I should tweak the > documentation to be more explicit: for a format driver, OFFSET_VALID can > always be used (and *file will be set to the underlying protocol driver); > but for a protocol driver, OFFSET_VALID only makes sense if *file is the BDS > itself and there is an actual buffer to read (that is, the protocol driver > must also be returning DATA and/or ZERO). Or maybe we can indeed state that > protocol drivers always set *file to NULL (there is no further backing file > to reference), and thus never need to return OFFSET_VALID (but I'm not sure > whether that will accidentally propagate back up the call stack and > negatively affect status queries of format drivers). > > Since it is pre-existing, should I respin to address the issue in a separate > patch, or should that be a followup after this series?
It's a more fundamental question that shouldn't hold up this series. I just wanted to raise it while I was looking at it. So yes, a followup is fine. Kevin