| Message ID | 20170427014626.11553-2-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 27.04.2017 03:46, 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 <mreitz@redhat.com> > Signed-off-by: Eric Blake <eblake@redhat.com> > > --- > v10: new patch > --- > include/block/block.h | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) Reviewed-by: Max Reitz <mreitz@redhat.com> Thanks!
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 <mreitz@redhat.com> > Signed-off-by: Eric Blake <eblake@redhat.com> > > --- > /* > * 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.
diff --git a/include/block/block.h b/include/block/block.h index 862eb56..04fcbd0 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -121,21 +121,22 @@ typedef struct HDGeometry { /* * 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 * 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. + * 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. - * * 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; + * 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
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 <mreitz@redhat.com> Signed-off-by: Eric Blake <eblake@redhat.com> --- v10: new patch --- include/block/block.h | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)