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: 2856 bytes --]

On 20.02.2016 14:34, Max Reitz wrote:
> 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.
> 
> 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.
> 
> 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).
> 
> 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.
> 
>>>>> a list of those BBs; blk_backends is a different list, as it contains
>>>>> references to all BBs (after a follow-up patch, that is), and that
>>>>> should not be changed because we do need such a list.
>>>>>
>>>>> monitor_remove_blk() is idempotent so that we can call it in
>>>>> blockdev_auto_del() without having to care whether it had been called in
>>>>> do_drive_del() before. monitor_add_blk() is idempotent for symmetry
>>>>> reasons (monitor_remove_blk() is, so it would be strange for
>>>>> monitor_add_blk() not to be).
>>>>>
>>>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>>>
>>>> I think hmp_drive_add() needs a monitor_remove_blk() in its error path.
>>>
>>> You're right, thanks.
>>>
>>> In addition, if we really do say that a BB having a name equals being
>>> referenced by the monitor, then maybe we don't need explicit calls to
>>> monitor_add_blk() because any BB that is created with a non-NULL name
>>> should be automatically added to the list of monitor BBs.
>>
>> While probably workable, I'd rather avoid this kind of magic where the
>> presence of a name parameter decides whether a reference is taken or
>> not. It makes the interface of the function a lot less obvious.
> 
> Still you want the name to be the monitor's reference to the BB. Thus,
> if monitor_add_blk() should not be called implicitly by blk_new(), then
> I'd instead move the @name parameter from blk_new() to monitor_add_blk().

...aaand I noticed that this is what you said for patch 8 anyway. Good.

Max


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