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

[Markus Armbruster]

Max Reitz <address@hidden> writes:

> On 22.02.2016 09:24, Markus Armbruster wrote:
>> Max Reitz <address@hidden> 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).

block-backend.c holds a reference from blk_backends.  By *why* does it
do that?  Let's go through the uses of blk_backends.

0. blk_backends maintenance: blk_new(), blk_delete(),
   blk_hide_on_behalf_of_hmp_drive_del()

1. To permit lookup by name, with blk_by_name().

   In my view, block-backend.c holds this reference in trust for those
   parts of QEMU that reference by name rather than by pointer, most
   prominently the monitor.

   I can't see the point of backing up the reference by name with a
   reference by pointer in the monitor, like your patch seems to do.
   What's the difference between the two?  Can one ever exist without
   the other?  Why in the monitor, and not in any other part looking up
   by name?

2. To iterate over all named ones, with blk_next().

   Since this can only return named BBs, the reference held in trust for
   named lookup suffices.

3. To permit lookup by DriveInfo, with blk_by_legacy_dinfo()

   Since this must only return named BBs, the reference held in trust
   for named lookup suffices.

4. To do something so unimportant that it's not worth explaining, with
   blk_remove_bs().

   I made a point of giving every single external function a carefully
   worded contract, to hopefully shame future contributors into
   following suit.  Didn't work.

> 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.

Naming is hard.  Feel free to propose better comments and/or better
names.

>>> 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?

Can you explain the *actual* difference between blk_backends and
monitor_block_backends?  "Actual" in the sense of difference in contents
over time.

>>> 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.
>> [...]
>>