From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51724) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aYDOV-0000ik-EJ for qemu-devel@nongnu.org; Tue, 23 Feb 2016 08:52:52 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aYDOU-00014E-7W for qemu-devel@nongnu.org; Tue, 23 Feb 2016 08:52:51 -0500 References: <1455646106-2047-1-git-send-email-mreitz@redhat.com> <1455646106-2047-8-git-send-email-mreitz@redhat.com> <20160217105345.GE29494@noname.str.redhat.com> <56C494B4.8020707@redhat.com> <20160217162053.GK29494@noname.str.redhat.com> <56C86B70.8090102@redhat.com> <87oab9i24b.fsf@blackfin.pond.sub.org> <56CB3773.1030107@redhat.com> <874mcz92q1.fsf@blackfin.pond.sub.org> From: Max Reitz Message-ID: <56CC6429.3050008@redhat.com> Date: Tue, 23 Feb 2016 14:52:41 +0100 MIME-Version: 1.0 In-Reply-To: <874mcz92q1.fsf@blackfin.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="h3GjmDVFN715tXkjs4FNtBdR0HNuAWvoo" Subject: Re: [Qemu-devel] [PATCH v3 07/14] blockdev: Add list of monitor-owned BlockBackends List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: Kevin Wolf , Stefan Hajnoczi , qemu-devel@nongnu.org, qemu-block@nongnu.org This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --h3GjmDVFN715tXkjs4FNtBdR0HNuAWvoo Content-Type: text/plain; charset=iso-8859-15 Content-Transfer-Encoding: quoted-printable On 23.02.2016 10:48, Markus Armbruster wrote: > Max Reitz writes: >=20 >> On 22.02.2016 09:24, Markus Armbruster wrote: >>> Max Reitz 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 sho= uld have >>>>>>> >>>>>>> s/does hold/holds/? >>>>>> >>>>>> It was intentional, so I'd keep it unless you drop the question ma= rk. >>>>> >>>>> For me it seems to imply something like "contrary to your expectati= ons", >>>>> but maybe that's just my non-native English needing a fix. >>>>> >>>>> I don't find it surprising anyway that the monitor holds BB referen= ces. >>> >>> 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, especial= ly >> because it's done through magic. With this interpretation, >> block-backend.c considers every BB to be monitor-owned (until >> blk_hide...() is called). >=20 > block-backend.c holds a reference from blk_backends. By *why* does it > do that? Let's go through the uses of blk_backends. >=20 > 0. blk_backends maintenance: blk_new(), blk_delete(), > blk_hide_on_behalf_of_hmp_drive_del() >=20 > 1. To permit lookup by name, with blk_by_name(). >=20 > 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. >=20 > 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? >=20 > 2. To iterate over all named ones, with blk_next(). >=20 > Since this can only return named BBs, the reference held in trust fo= r > named lookup suffices. >=20 > 3. To permit lookup by DriveInfo, with blk_by_legacy_dinfo() >=20 > Since this must only return named BBs, the reference held in trust > for named lookup suffices. >=20 > 4. To do something so unimportant that it's not worth explaining, with > blk_remove_bs(). >=20 > 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. >=20 > 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 you= r >>>> expectations", but an "emphasize it because it is contrary to what t= he >>>> 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= =2E >> 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-u= p >> commit will make blk_backends contain what it name implies it does? Or= >> just call the commit "Remove magic", because it adds explicit function= s >> for saying that a BB is monitor-owned or that it is not? >=20 > Can you explain the *actual* difference between blk_backends and > monitor_block_backends? "Actual" in the sense of difference in content= s > 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 --h3GjmDVFN715tXkjs4FNtBdR0HNuAWvoo Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAEBCAAGBQJWzGQpAAoJEDuxQgLoOKytOc4H/i/4Uy3Gdz8c/WlU++ErtHaO nSSMdHCjUeQzNQpoCd3ZT6ZJlL1AKX3UvuUJLzX9rMzIlRybRBt2k5WI3xIp4MWv kfIz+xZZ0j9ZX9E48c+OzexQtALTYyvO8r8SK9Jwm5CVJVyD5TGioU+BNq+/d8oS F/9sFAsa1wD+/Bk7XBMZF1nERc6Zf4/8fbbm5i/kYR37XocJJn/+J3nbqSSoZ8Xv CPbPVN8QtTOePtZaFtTyhnCHmyGMG7enqBg23LphrOF7u4KHrpLvNR5yLc9Ossgp wE8CRgtf9IYWkUxsNFIY0NaJzLctcTD23gvEf3Jsj/p82XwRp1ThGTwyFW2oiQM= =hrIa -----END PGP SIGNATURE----- --h3GjmDVFN715tXkjs4FNtBdR0HNuAWvoo--