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

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

Max

>> But that would mean that qemu-img's, qemu-nbd's and qemu-io's BBs would
>> have to be monitor-owned, too, and they'd all have to call
>> monitor_remove_blk() all over the place... Unless we'd allow NULL BB
>> names now and make them use it (I don't really see a reason why not;
>> them calling their BBs "hda" seems weird anyway), or implicitly call
>> monitor_remove_blk() in blk_delete(). Or maybe both, because the latter
>> seems convenient anyway.
> 
> I'm not sure. It would be correct even when we start to create BBs
> automatically, because monitor_remove_blk() doesn't do anything when
> there is no monitor reference and the monitor reference is the last
> thing that can be returned (hopefully). But I like reference counting to
> be as explicit as possible.
> 
> Kevin
> 



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