[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v12 02/10] block: Update comments on BDRV_BLOCK_
Re: [Qemu-block] [PATCH v12 02/10] block: Update comments on BDRV_BLOCK_* meanings
Fri, 5 May 2017 22:23:57 +0200
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.0.1
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
>>> 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
>> 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.
>> With the commit message fixed or a “no it's fine”:
>> Reviewed-by: Max Reitz <address@hidden>
Description: OpenPGP digital signature
- [Qemu-block] [PATCH v12 00/10] qcow2 zero-cluster tweaks [was add blkdebug tests], Eric Blake, 2017/05/03
- [Qemu-block] [PATCH v12 01/10] qcow2: Use consistent switch indentation, Eric Blake, 2017/05/03
- [Qemu-block] [PATCH v12 05/10] qcow2: Optimize zero_single_l2() to minimize L2 churn, Eric Blake, 2017/05/03
- [Qemu-block] [PATCH v12 04/10] qcow2: Make distinction between zero cluster types obvious, Eric Blake, 2017/05/03
- [Qemu-block] [PATCH v12 07/10] iotests: Add test 179 to cover write zeroes with unmap, Eric Blake, 2017/05/03