From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38440) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wui7U-00089W-7K for qemu-devel@nongnu.org; Wed, 11 Jun 2014 08:59:19 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Wui7M-00038e-Ol for qemu-devel@nongnu.org; Wed, 11 Jun 2014 08:59:12 -0400 Received: from mx1.redhat.com ([209.132.183.28]:20163) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wui7M-00038W-Fj for qemu-devel@nongnu.org; Wed, 11 Jun 2014 08:59:04 -0400 Date: Wed, 11 Jun 2014 14:58:57 +0200 From: Stefan Hajnoczi Message-ID: <20140611125857.GA4765@stefanha-thinkpad.str.redhat.com> References: <1402384906-335-1-git-send-email-benoit.canet@irqsave.net> <1402384906-335-4-git-send-email-benoit.canet@irqsave.net> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="Dxnq1zWXvFF0Q93v" Content-Disposition: inline In-Reply-To: <1402384906-335-4-git-send-email-benoit.canet@irqsave.net> Subject: Re: [Qemu-devel] [PATCH v8 3/4] block: Add replaces argument to drive-mirror List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?iso-8859-1?Q?Beno=EEt?= Canet Cc: kwolf@redhat.com, Benoit Canet , qemu-devel@nongnu.org, mreitz@redhat.com --Dxnq1zWXvFF0Q93v Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Jun 10, 2014 at 09:21:45AM +0200, Beno=EEt Canet wrote: > +BlockDriverState *check_to_replace_node(const char *node_name, Error **e= rrp) > +{ > + BlockDriverState *to_replace_bs =3D bdrv_find_node(node_name); > + if (!to_replace_bs) { > + error_setg(errp, "node_name=3D%s not found", > + node_name); Please use "Node name '%s' not found". It's more consistent with error message style. > + return NULL; > + } > + > + /* the code should only be able to replace the top first non filter > + * node of the graph. For example the first BDS under a quorum. > + */ > + if (!bdrv_is_first_non_filter(to_replace_bs)) { > + error_set(errp, QERR_FEATURE_DISABLED, > + "drive-mirror and replace node-name"); > + return NULL; > + } This seems like an arbitrary restriction. Can we drop this? > @@ -540,6 +555,23 @@ static void mirror_complete(BlockJob *job, Error **e= rrp) > return; > } > =20 > + /* check the target bs is not block and block all operations on it */ > + if (s->replaces) { > + s->to_replace =3D check_to_replace_node(s->replaces, errp); > + > + if (!s->to_replace) { > + return; > + } > + > + error_setg(&s->replace_blocker, > + "block device is in use by block-job-complete"); > + bdrv_op_block_all(s->to_replace, s->replace_blocker); > + bdrv_ref(s->to_replace); > + > + g_free(s->replaces); > + s->replaces =3D NULL; Leaks s->replaces if block-job-abort is called. --Dxnq1zWXvFF0Q93v Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQEcBAEBAgAGBQJTmFKRAAoJEJykq7OBq3PIyrwH/1BOuHnvkyRrQ+fS+pRWvc6j N2WOeChvaVUaa8TaP6ttJesqrsNQgTevm7f5iVin5GniytU7yMvEkd+kzHGv3Q3F xwMvGuppyYxjV+iHc33jSh/zVsQ8GTdGwrQzg8ikilfPYaKk17n1Azt8GIGteIai IaUF9Wmoy43ccaPxmNePpbg+2LBLflSE7o4R5pRVTEp+Al+hN6e2LYnm4e0/FQQO sbhQ1/ZM+oetHg78gsvvfRExM4HRtT7xeLVgRvo3tGl3f84ectiJW3ebgS9oQJbL GJ82DRmz1qJTyMt2I3oHIaWtWALf6d36PrIzWw62e2boqWB730C0XoE91/fQ9ls= =zqfo -----END PGP SIGNATURE----- --Dxnq1zWXvFF0Q93v--