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?
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
