Re: [Qemu-devel] [PATCH v3 07/14] blockdev: Add list of monitor-owned BlockBackends

[Max Reitz <mreitz@redhat.com>]

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

On 22.02.2016 09:24, Markus Armbruster wrote:
> Max Reitz <mreitz@redhat.com> writes:
> 
>> On 17.02.2016 17:20, Kevin Wolf wrote:
>>> Am 17.02.2016 um 16:41 hat Max Reitz geschrieben:
>>>> On 17.02.2016 11:53, Kevin Wolf wrote:
>>>>> Am 16.02.2016 um 19:08 hat Max Reitz geschrieben:
>>>>>> The monitor does hold references to some BlockBackends so it should have
>>>>>
>>>>> s/does hold/holds/?
>>>>
>>>> It was intentional, so I'd keep it unless you drop the question mark.
>>>
>>> For me it seems to imply something like "contrary to your expectations",
>>> but maybe that's just my non-native English needing a fix.
>>>
>>> I don't find it surprising anyway that the monitor holds BB references.
> 
> Me neither.
> 
>> The contrast I tried to point out is that while we do have these
>> references in theory, and they are reflected by a refcount, too, we do
>> not actually have these references because the monitor does not yet have
>> a list of the BBs it owns.
> 
> Oh yes, it has.  It's just outsources their actual storage to
> block-backend.c, but that's detail.

In my opinion it's not a reference made by the monitor, then, especially
because it's done through magic. With this interpretation,
block-backend.c considers every BB to be monitor-owned (until
blk_hide...() is called).

Also, if blk_backends is supposed to be the list of monitor-owned BBs,
then it's a really bad name and I put all the blame on that.

>> So it's not an "emphasize the verb because it may be contrary to your
>> expectations", but an "emphasize it because it is contrary to what the
>> current code does" (which is not actually referencing these BBs).
> 
> I disagree :)

Then I'd say "It's contrary to my interpretation of what the current
code wants to do." Now it's my personal opinion! ;-)

I wouldn't mind removing the "does" from the commit message (obviously),
but that is not the only thing there which conflicts with your opinion.
It clearly states that blk_backends is not the list of monitor-owned
BlockBackends, whereas you are saying that it very much is.

So...? Rephrase it entirely? State that blk_backends is a bad name and
this commit is basically duplicating blk_backends as
monitor_block_backends, which is the correct name, and that a follow-up
commit will make blk_backends contain what it name implies it does? Or
just call the commit "Remove magic", because it adds explicit functions
for saying that a BB is monitor-owned or that it is not?

Max

>> Like: It is supposed to have references. It says it does. But it
>> actually doesn't. It does "hold" them, however, because they are
>> accounted for in the BBs' refcounts.
> [...]
> 



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