From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38400) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZWUDH-0004mN-Nz for qemu-devel@nongnu.org; Mon, 31 Aug 2015 14:53:52 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZWUDG-0003cO-Fh for qemu-devel@nongnu.org; Mon, 31 Aug 2015 14:53:51 -0400 References: <1439279489-13338-1-git-send-email-wency@cn.fujitsu.com> <1439279489-13338-5-git-send-email-wency@cn.fujitsu.com> From: Eric Blake Message-ID: <55E4A2B2.5060709@redhat.com> Date: Mon, 31 Aug 2015 12:53:38 -0600 MIME-Version: 1.0 In-Reply-To: <1439279489-13338-5-git-send-email-wency@cn.fujitsu.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="x3t1ioXPwQFnr03NqLHObD1a4SUnAArVu" Subject: Re: [Qemu-devel] [Patch for-2.5 v2 4/6] quorum: implement bdrv_add_child() and bdrv_del_child() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Wen Congyang , qemu devel , Markus Armbruster , Alberto Garcia , Stefan Hajnoczi Cc: Kevin Wolf , zhanghailiang , qemu block , Jiang Yunhong , Dong Eddie , "Dr. David Alan Gilbert" , Gonglei , Yang Hongyang This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --x3t1ioXPwQFnr03NqLHObD1a4SUnAArVu Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 08/11/2015 01:51 AM, Wen Congyang wrote: > Signed-off-by: Wen Congyang > Signed-off-by: zhanghailiang > Signed-off-by: Gonglei > Reviewed-by: Alberto Garcia > --- > block/quorum.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++++= ++++++-- > 1 file changed, 73 insertions(+), 2 deletions(-) >=20 > diff --git a/block/quorum.c b/block/quorum.c > index 2f6c45f..1305086 100644 > --- a/block/quorum.c > +++ b/block/quorum.c > @@ -66,6 +66,9 @@ typedef struct QuorumVotes { > typedef struct BDRVQuorumState { > BlockDriverState **bs; /* children BlockDriverStates */ > int num_children; /* children count */ > + int max_children; /* The maximum children count, we need to r= eallocate > + * bs if num_children will larger than maxi= mum. s/will/grows/ > @@ -995,6 +999,70 @@ static void quorum_attach_aio_context(BlockDriverS= tate *bs, > } > } > =20 > +static void quorum_add_child(BlockDriverState *bs, QDict *options, Err= or **errp) > +{ > + BDRVQuorumState *s =3D bs->opaque; > + int ret; > + Error *local_err =3D NULL; > + > + bdrv_drain(bs); > + > + if (s->num_children =3D=3D s->max_children) { > + if (s->max_children >=3D INT_MAX) { > + error_setg(errp, "Too many children"); > + return; > + } > + > + s->bs =3D g_renew(BlockDriverState *, s->bs, s->max_children += 1); > + s->bs[s->num_children] =3D NULL; > + s->max_children +=3D 1; why not use ++? > + } > + > + ret =3D bdrv_open_image(&s->bs[s->num_children], NULL, options, "c= hild", bs, > + &child_format, false, &local_err); > + if (ret < 0) { > + error_propagate(errp, local_err); > + return; > + } > + s->num_children++; > +} > + > +static void quorum_del_child(BlockDriverState *bs, BlockDriverState *c= hild_bs, > + Error **errp) > +{ > + BDRVQuorumState *s =3D bs->opaque; > + int i; > + > + for (i =3D 0; i < s->num_children; i++) { > + if (s->bs[i] =3D=3D child_bs) { > + break; > + } > + } > + > + if (i =3D=3D s->num_children) { > + error_setg(errp, "Invalid child"); > + return; > + } The previous patch already assert()ed that the child was present; can't this one just assert(i < s->num_children)? > + > + if (s->num_children <=3D s->threshold) { > + error_setg(errp, > + "The number of children cannot be lower than the vote thre= shold"); > + return; Might be nice to include the numeric value of that threshold in the error message. > + } > + > + if (s->num_children =3D=3D 1) { > + error_setg(errp, "Cannot remove the last child"); > + return; > + } Isn't this dead code, as the vote threshold always has to be at least 1, so the previous 'if' already rejected an attempt to go lower than the threshold? > + > + bdrv_drain(bs); > + /* We can safely remove this child now */ > + memmove(&s->bs[i], &s->bs[i+1], (s->num_children - i - 1) * sizeof= (void *)); Spaces around '+'. > + s->num_children--; > + s->bs[s->num_children] =3D NULL; > + bdrv_unref(child_bs); > +} > + > static void quorum_refresh_filename(BlockDriverState *bs) > { > BDRVQuorumState *s =3D bs->opaque; > @@ -1049,6 +1117,9 @@ static BlockDriver bdrv_quorum =3D { > .bdrv_detach_aio_context =3D quorum_detach_aio_context,= > .bdrv_attach_aio_context =3D quorum_attach_aio_context,= > =20 > + .bdrv_add_child =3D quorum_add_child, > + .bdrv_del_child =3D quorum_del_child, > + > .is_filter =3D true, > .bdrv_recurse_is_first_non_filter =3D quorum_recurse_is_first_no= n_filter, > }; >=20 Overall seems reasonable. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --x3t1ioXPwQFnr03NqLHObD1a4SUnAArVu 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 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJV5KKyAAoJEKeha0olJ0NqkEYIAIhywgbK1okcaqGts9T5Eorf jVToOmuLV5W1Q/VJuW0ZC+S/3QqzBI7lqXPAL4Sfkt/ft38/dsCB8BVkIuyF+gUD f1gx+9yaqr6CUkXcRiR+VmbYVIohkMMOhlS+sI3qDtIvJmJ7NerKrR5PeQ1jChJa +hBFr0BDmyAHcitFD3p6nOcniTeLADt4HIRKE6shYJ6WcFBlxznQDds4x35mNG2b ykekDx5tyFD+SVvTZxpG7efp5nVtv94XTCwS/gj9jkcw2Nfv3WWdLMrgEFpDwvuw ubkYW1iWVmTgsEzgTmLyhA2nPdCjNy8u1CayGOP1NWe22OLA5NUyIy8hf4fPR9M= =vdZ5 -----END PGP SIGNATURE----- --x3t1ioXPwQFnr03NqLHObD1a4SUnAArVu--