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:

  *  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

Reply via email to