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 <mre...@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
signature.asc
Description: OpenPGP digital signature
