Re: [Qemu-devel] [PATCH v12 02/10] block: Update comments on BDRV_BLOCK_* meanings

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2780 bytes --]

On 05.05.2017 22:13, Eric Blake wrote:
> On 05/05/2017 03:06 PM, Max Reitz wrote:
>> On 04.05.2017 05:07, Eric Blake wrote:
>>> We had some conflicting documentation: a nice 8-way table that
>>> described all possible combinations of DATA, ZERO, and
>>> OFFSET_VALID, contrasted with text that implied that OFFSET_VALID
>>> always meant raw data could be read directly.  Furthermore, the
>>> text refers a lot to bs->file, even though the interface was
>>> updated back in 67a0fd2a to let the driver pass back which BDS (not
>>> necessarily bs->file).
>>
>> Not sure about my English skills here, but is this missing a verb? ("to
>> pass back which BDS...")
> 
> maybe s/which/a specific/

Or that, yes. :-)

> 
>>
>>>                         As the 8-way table is the intended
>>> semantics, simplify the rest of the text to get rid of the
>>> confusion.
>>>
>>> ALLOCATED is always set by the block layer for convenience (drivers
>>> do not have to worry about it). RAW is used only internally, but
>>
>> Just one space after the period? How inconsistent! :-)
> 
> But do commit messages really count?  :)

It's a critical bug, I'm telling you!!!

>>> + * Internal flag:
>>> + * BDRV_BLOCK_RAW: used internally to indicate that the request was
>>> + *                 answered by a passthrough driver such as raw and that the
>>
>> s/passthrough/filter/? But I'm not even sure myself. Well, "passthrough"
>> is a safe bet, so let's just go with it.
> 
> Calling 'raw' a filter driver is a bit weird - but in one sense, it is a
> no-op filter (filter the protocol layer into the format layer by doing
> nothing).  Meanwhile 'commit' certainly sounds like more of a filter
> than a passthrough.  I could go either way, and filter is slightly
> shorter.  If there's a real reason to respin the series, 'filter' seems
> reasonable if we're worried about line length, otherwise I'm just as
> inclined to leave it alone.

raw certainly is a filter driver; the thing I wasn't sure about is that
I'm not sure whether filter drivers are required to set this flag. But
neither the comment nor the code require it necessarily, so using
"filter" instead of "passthrough" should be OK.

The main reason for using "filter" over "passthrough" is that "filter"
is a "real" class of block drivers (just "real" in the sense that we
actually only have child-less protocol drivers and non-protocol drivers
that do have children; further dividing into "format" and "filter" is
just a convention).

But it should be clear anyway, so I don't mind either way. Leaving it as
it is certainly is simpler.

Max

> 
>>
>> With the commit message fixed or a “no it's fine”:
>>
>> Reviewed-by: Max Reitz <mreitz@redhat.com>
>>
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]