From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51069) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aX7gQ-0001mk-C3 for qemu-devel@nongnu.org; Sat, 20 Feb 2016 08:34:51 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aX7gP-0004Ik-ES for qemu-devel@nongnu.org; Sat, 20 Feb 2016 08:34:50 -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> From: Max Reitz Message-ID: <56C86B70.8090102@redhat.com> Date: Sat, 20 Feb 2016 14:34:40 +0100 MIME-Version: 1.0 In-Reply-To: <20160217162053.GK29494@noname.str.redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="I4hbtm3TlsaLtkPCu1rUdQe26LKivPQGM" 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: Kevin Wolf Cc: Stefan Hajnoczi , qemu-devel@nongnu.org, qemu-block@nongnu.org, Markus Armbruster This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --I4hbtm3TlsaLtkPCu1rUdQe26LKivPQGM Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable 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. >=20 > For me it seems to imply something like "contrary to your expectations"= , > but maybe that's just my non-native English needing a fix. >=20 > 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 contain= s >>>> 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 calle= d 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 >>> >>> I think hmp_drive_add() needs a monitor_remove_blk() in its error pat= h. >> >> 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. >=20 > 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().= Max >> But that would mean that qemu-img's, qemu-nbd's and qemu-io's BBs woul= d >> have to be monitor-owned, too, and they'd all have to call >> monitor_remove_blk() all over the place... Unless we'd allow NULL BB >> names now and make them use it (I don't really see a reason why not; >> them calling their BBs "hda" seems weird anyway), or implicitly call >> monitor_remove_blk() in blk_delete(). Or maybe both, because the latte= r >> seems convenient anyway. >=20 > I'm not sure. It would be correct even when we start to create BBs > automatically, because monitor_remove_blk() doesn't do anything when > there is no monitor reference and the monitor reference is the last > thing that can be returned (hopefully). But I like reference counting t= o > be as explicit as possible. >=20 > Kevin >=20 --I4hbtm3TlsaLtkPCu1rUdQe26LKivPQGM 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 iQEcBAEBCAAGBQJWyGtwAAoJEDuxQgLoOKytflEH/05UFsbu8CNnIDjoKNlPW1Yg g4GOk61GkeMyVhPKmfsSIdqVo/g28p5Nz/t3qmdK1ZLYDvJKPBgTuC71EBMsm0YU 1jSeM27YH3jvDx6BP0xAomFlRQyJK9p3bycUX/DsRaLzlHqor+D/M2Jic6FYZ0OB VfQvvRdJG1TBtlJ2Z+swgaOS3mextYqXkJXctrWWp2bG6pU1VBVM4hzhfK6S+3VS iRqyx8pOkhsLEzIaUWG/JA/I1N9F4Qpt92AGbV1YZDST8MwmxGLO043VdLZu0xZp lpAHA5ZQ4G1vslvT+vkvWvPw7jEpa/erUyKxvArNdHt5H6/a8iVaOBZOE5ek58k= =uxbW -----END PGP SIGNATURE----- --I4hbtm3TlsaLtkPCu1rUdQe26LKivPQGM--