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 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. 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(). 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. However, I still believe this patch itself to be useful. Max
signature.asc
Description: OpenPGP digital signature
