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

[Kevin Wolf]

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.

> >> 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 <address@hidden>
> > 
> > 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.

> 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

pgpYV9M5VbVoP.pgp
Description: PGP signature