From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60109) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WAQ0d-0007YG-HB for qemu-devel@nongnu.org; Mon, 03 Feb 2014 15:20:52 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WAQ0V-0005Jq-8n for qemu-devel@nongnu.org; Mon, 03 Feb 2014 15:20:47 -0500 Received: from mx1.redhat.com ([209.132.183.28]:56420) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WAQ0U-0005Ji-Gf for qemu-devel@nongnu.org; Mon, 03 Feb 2014 15:20:39 -0500 Message-ID: <52EFFA95.9090709@redhat.com> Date: Mon, 03 Feb 2014 21:22:45 +0100 From: Max Reitz MIME-Version: 1.0 References: <1391454712-14353-1-git-send-email-benoit.canet@irqsave.net> <1391454712-14353-13-git-send-email-benoit.canet@irqsave.net> In-Reply-To: <1391454712-14353-13-git-send-email-benoit.canet@irqsave.net> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH V14 12/13] quorum: Add quorum_open() and quorum_close(). List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?UTF-8?B?QmVub8OudCBDYW5ldA==?= , qemu-devel@nongnu.org Cc: kwolf@redhat.com, =?UTF-8?B?QmVub8OudCBDYW5ldA==?= , stefanha@redhat.com On 03.02.2014 20:11, Beno=C3=AEt Canet wrote: > From: Beno=C3=AEt Canet > > Example of command line: > -drive if=3Dvirtio,file.driver=3Dquorum,\ > file.children.0.file.filename=3D1.raw,\ > file.children.0.node-name=3D1.raw,\ > file.children.0.driver=3Draw,\ > file.children.1.file.filename=3D2.raw,\ > file.children.1.node-name=3D2.raw,\ > file.children.1.driver=3Draw,\ > file.children.2.file.filename=3D3.raw,\ > file.children.2.node-name=3D3.raw,\ > file.children.2.driver=3Draw,\ > file.vote_threshold=3D2 > > file.blkverify=3Don with file.vote_threshold=3D2 and two files can be p= assed to > emulated blkverify. > > Signed-off-by: Benoit Canet > --- > block/quorum.c | 169 ++++++++++++++++++++++++++++++++++++++++++++++= +++++++++ > qapi-schema.json | 21 ++++++- > 2 files changed, 189 insertions(+), 1 deletion(-) > > diff --git a/block/quorum.c b/block/quorum.c > index a4716b3..582bf2c 100644 > --- a/block/quorum.c > +++ b/block/quorum.c > @@ -17,8 +17,12 @@ > #include > #include "block/block_int.h" > #include "qapi/qmp/qjson.h" > +#include "qapi/qmp/types.h" > +#include "qemu-common.h" > =20 > #define HASH_LENGTH 32 > +#define KEY_PREFIX "children." > +#define KEY_FILENAME_SUFFIX ".file.filename" > =20 > /* This union holds a vote hash value */ > typedef union QuorumVoteValue { > @@ -720,12 +724,177 @@ static bool quorum_recurse_is_first_non_filter(B= lockDriverState *bs, > return false; > } > =20 > +static int quorum_valid_threshold(int threshold, > + int total, > + Error **errp) > +{ > + > + if (threshold < 1) { > + error_set(errp, QERR_INVALID_PARAMETER_VALUE, > + "vote-threshold", "value >=3D 1"); > + return -ERANGE; > + } > + > + if (threshold > total) { > + error_setg(errp, "threshold may not exceed children count"); > + return -ERANGE; > + } > + > + return 0; > +} > + > +static int quorum_open(BlockDriverState *bs, > + QDict *options, > + int flags, > + Error **errp) > +{ > + BDRVQuorumState *s =3D bs->opaque; > + Error *local_err =3D NULL; > + bool *opened; > + QDict *sub =3D NULL; > + QList *list =3D NULL; > + const QListEntry *lentry; > + const QDictEntry *dentry; > + const char *value; > + char *next; > + int i; > + int ret =3D 0; > + unsigned long long threshold =3D 0; > + > + qdict_extract_subqdict(options, &sub, "children."); > + qdict_array_split(sub, &list); > + > + /* count how many different children are present and validate */ > + s->total =3D !qlist_size(list) ? qdict_size(sub) : qlist_size(list= ); > + if (s->total < 2) { > + error_setg(&local_err, > + "Number of provided children must be greater than 1= "); > + ret =3D -EINVAL; > + goto exit; > + } > + > + ret =3D qdict_get_try_int(options, "vote-threshold", -1); > + /* from QMP */ > + if (ret !=3D -1) { > + qdict_del(options, "vote-threshold"); > + s->threshold =3D ret; > + /* from command line */ > + } else { > + /* retrieve the threshold option from the command line */ > + value =3D qdict_get_try_str(options, "vote_threshold"); > + if (!value) { > + error_setg(&local_err, > + "vote_threshold must be provided"); > + ret =3D -EINVAL; > + goto exit; > + } > + qdict_del(options, "vote_threshold"); > + > + ret =3D parse_uint(value, &threshold, &next, 10); > + > + /* no int found -> scan fail */ > + if (ret < 0) { > + error_setg(&local_err, > + "invalid vote_threshold specified"); > + ret =3D -EINVAL; > + goto exit; > + } > + s->threshold =3D threshold; > + } > + > + /* and validate it againt s->total */ One step closer, but "against" is still one letter away. ;-) > + ret =3D quorum_valid_threshold(s->threshold, s->total, &local_err)= ; > + if (ret < 0) { > + goto exit; > + } > + > + /* is the driver in blkverify mode */ > + value =3D qdict_get_try_str(options, "blkverify"); > + if (value && !strcmp(value, "on") && > + s->total =3D=3D 2 && s->threshold =3D=3D 2) { > + s->is_blkverify =3D true; > + } else if (value && strcmp(value, "off")) { > + fprintf(stderr, "blkverify mode is set by setting blkverify=3D= on " > + "and using two files with vote_threshold=3D2")= ; This probably needs a \n at the end of the format string. > + } > + qdict_del(options, "blkverify"); > + > + /* allocate the children BlockDriverState array */ > + s->bs =3D g_new0(BlockDriverState *, s->total); > + opened =3D g_new0(bool, s->total); > + > + /* Opening by file name or options */ > + for (i =3D 0, lentry =3D qlist_first(list); > + s->total =3D=3D qlist_size(list) && lentry; I know I said I find it clever to put such a condition into the loop=20 condition instead of wrapping an if around it, but this time, I'd prefer=20 the wrapping if for s->total =3D=3D qlist_size(size) instead. ;-) This is not a trivial loop and the compiler may be unable to verify that=20 this condition doesn't change on each iteration. Also, an if with this=20 loop in the if branch and the following loop in the else branch would be=20 much more evident in what it does, at least to me. > + lentry =3D qlist_next(lentry), i++) { > + ret =3D bdrv_open(&s->bs[i], NULL, NULL, qobject_to_qdict(lent= ry->value), > + flags, NULL, &local_err); > + if (ret < 0) { > + goto close_exit; > + } > + opened[i] =3D true; > + } > + > + /* Opening by reference */ > + for (i =3D 0, dentry =3D qdict_first(sub); > + s->total =3D=3D qdict_size(sub) && dentry; > + dentry =3D qdict_next(sub, dentry), i++) { > + ret =3D bdrv_open(&s->bs[i], NULL, > + qstring_get_str(qobject_to_qstring(dentry->val= ue)), > + NULL, flags, NULL, &local_err); > + if (ret < 0) { > + goto close_exit; > + } > + opened[i] =3D true; > + } Okay, I see=E2=80=A6 Using qdict_array_split() doesn't help you with QMP=20 invocation, so now you're not verifying the indices in that case. Well,=20 we have three options: 1) Leave it like this. 2) Check whether the QDict keys are actually contiguous (this shouldn't=20 be too much effort, I hope=E2=80=A6) and pull the index here ("i") from t= he=20 QDict key. 3) Use qdict_flatten(options) before everything and thus make the=20 difference between QMP and command-line invocation disappear. I'd go with 3, especially considering that it should have been already=20 called in qmp_blockdev_add() (and therefore we don't even have to call=20 it here). I don't know why exactly you have this special code for QMP in the first=20 place; perhaps it is because I extended qdict_flatten() to cover QLists=20 not until November or December last year. Until then, qdict_flatten()=20 stopped at QLists and only flattened QDicts; but now, it will flatten=20 QLists as well, in a manner consistent with qdict_array_split(). Thus, I think the extra handling for QMP should be unnecessary by now.=20 If you have anything to test this hypothesis, by any chance, could you=20 try it out? Max