Re: [Qemu-devel] [PATCH v10 01/17] block: Update comments on BDRV_BLOCK_* meanings

[Eric Blake]

On 04/26/2017 08:46 PM, Eric Blake wrote:
> We had some conflicting documentation: a nice 8-way table that
> described all possible combinations of DATA, ZERO, and
> OFFSET_VALID, couple with text that implied that OFFSET_VALID
> always meant raw data could be read directly.  As the 8-way
> table is the intended semantics, simplify the rest of the text
> to get rid of the confusion.
> 
> Suggested-by: Max Reitz <address@hidden>
> Signed-off-by: Eric Blake <address@hidden>
> 
> ---

>  /*
>   * Allocation status flags
> - * BDRV_BLOCK_DATA: data is read from a file returned by 
> bdrv_get_block_status.
> + * BDRV_BLOCK_DATA: data is read from a file returned by 
> bdrv_get_block_status

I guess we always return the BDS where the data is read from, even when
we can't actually provide an offset to that data (such as when the
actual data is encrypted or compressed, and therefore has no raw
offset).  But I'm still wondering if this line can be shortened.

>   * BDRV_BLOCK_ZERO: sectors read as zero
> - * BDRV_BLOCK_OFFSET_VALID: sector stored as raw data in a file returned by
> - *                          bdrv_get_block_status.

I think when this was originally written, we blindly assumed that offset
pointed into bs->file.  Then with the exposure of the BDS graph, we
changed bdrv_get_block_status() to have a BlockDriverState **file
parameter (offset points into the returned file), since bs->file need
not always be the right BDS.  This old comment is on track...

> + * BDRV_BLOCK_OFFSET_VALID: an associated offset exists for accessing raw 
> data
>   * BDRV_BLOCK_ALLOCATED: the content of the block is determined by this
>   *                       layer (as opposed to the backing file)
>   * BDRV_BLOCK_RAW: used internally to indicate that the request
>   *                 was answered by the raw driver and that one
>   *                 should look in bs->file directly.
>   *
> - * If BDRV_BLOCK_OFFSET_VALID is set, bits 9-62 represent the offset in
> - * bs->file where sector data can be read from as raw data.

...but this one is stale...

> - *
>   * DATA == 0 && ZERO == 0 means that data is read from backing_hd if present.
>   *
> + * If BDRV_BLOCK_OFFSET_VALID is set, bits 9-62 represent the offset
> + * in bs->file that is allocated for the corresponding raw data;

...and I reused the stale wording.  Oops.

> + * however, whether that offset actually contains data also depends on
> + * BDRV_BLOCK_DATA, as follows:
> + *
>   * DATA ZERO OFFSET_VALID
>   *  t    t        t       sectors read as zero, bs->file is zero at offset
>   *  t    f        t       sectors read as valid from bs->file at offset

And these references to bs->file are stale too.

Guess I have more to fix on the respin.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

signature.asc
Description: OpenPGP digital signature