Re: [Qemu-block] [PATCH v8 09/21] null: Switch to .bdrv_co_block_status()

[Eric Blake]

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 <address@hidden>
> > Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> > Reviewed-by: Fam Zheng <address@hidden>

> >       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