Am 24.02.2016 um 10:28 hat Markus Armbruster geschrieben: > Max Reitz <mre...@redhat.com> writes: > > > On 23.02.2016 10:48, Markus Armbruster wrote: > >> Max Reitz <mre...@redhat.com> writes: > >> > >>> On 22.02.2016 09:24, Markus Armbruster wrote: > >>>> Max Reitz <mre...@redhat.com> 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. > > > > Side note: I consider it very important and there was no other way to do > > it before this series, because there is no list of all block backends. > > > >>> 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. > > > > I did. It's "monitor_block_backends". > > > >>>>> 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. > > > > First difference: The name. That's enough reason for me. > > > > Second difference: blk_new() adds any BB to blk_backends automatically. > > It doesn't do so for monitor_block_backends. > > > > Third difference: Often the monitor doesn't take care of signalling that > > it's releasing its reference, only sometimes (where "sometimes" means > > every time blk_hide...() is called). blk_delete() will instead > > automatically remove it from blk_backends, because it's assuming that > > the last reference had been held by the monitor. > > The reference held in trust for lookup by name exists as long as lookup > by name is permitted. > > Therefore, it goes away when the BB dies or when it loses its name. > > The only way for a BB to lose its name is (was?) the messed up HMP > drive_del: it wants to kill the BB right away, but can't, so it needs to > hide it instead until it can. > > New functionality may lead to a more complex live cycle where BBs may > acquire and lose their name in new ways. > > Note that I carefully avoid calling the reference the monitor's > reference. Because you made me realize it's not the monitor's alone! > Lookup by name has more customers than just the monitor.
Legitimate ones or bugs to fix? > > The second and third difference are what I referred to as "magic". You > > could also call them "convenience", if you prefer, but in any case as we > > can see by the existence blk_hide...(), removing the monitor reference > > in blk_delete() appears to be wrong. Both should be separate operations, > > and this is what this patch does. > > I can't see how the existence of blk_hide_on_behalf_of_hmp_drive_del() > shows that the name going away in blk_delete() is wrong. It must go > away there, because the thing it names goes away. It must go away there _the latest_, but it can (should?) go away earlier. > Additional operations to add / remove a BBs name may make sense; I'd > have to see their users. See "more complex live cycle" above. > > > Assuming that any blk_new() is ultimately done by the monitor, we maybe > > actually do not need an own monitor_add_blk() function; except that > > Kevin stated that he does deem it useful when I proposed inlining it > > (back) into blk_new(). > > As Kevin noted, that's not a good assumption. > > > All in all, you have convinced me that the commit message is incorrect. > > It should state that blk_backends is effectively repurposed to contain > > the list of all BBs, and that a more explicit monitor_block_backends > > list will take its place, with explicit operations for the monitor to > > signal when it takes or releases the reference to a BB. > > A member of monitor_block_backends is always a member of blk_backends. > Correct? > > Is a member of blk_backends with a name always a member of > monitor_block_backends? I would have expected that yes, but apparently you found other places that use the name, so it might not be as clear as I thought it was. Kevin
