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. > 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. >>> 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. >>> 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. >> [...] >>