From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58381) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VS75V-0006AR-0P for qemu-devel@nongnu.org; Fri, 04 Oct 2013 11:14:46 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VS75P-0001wA-7P for qemu-devel@nongnu.org; Fri, 04 Oct 2013 11:14:40 -0400 Received: from mx1.redhat.com ([209.132.183.28]:61234) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VS75O-0001vv-SF for qemu-devel@nongnu.org; Fri, 04 Oct 2013 11:14:35 -0400 Message-ID: <524EDB51.40605@redhat.com> Date: Fri, 04 Oct 2013 17:14:25 +0200 From: Max Reitz MIME-Version: 1.0 References: <1380717564-11098-1-git-send-email-benoit@irqsave.net> <1380717564-11098-12-git-send-email-benoit@irqsave.net> In-Reply-To: <1380717564-11098-12-git-send-email-benoit@irqsave.net> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH V9 11/11] quorum: Add quorum_open() and quorum_close(). List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?UTF-8?B?QmVub8OudCBDYW5ldA==?= Cc: kwolf@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com On 2013-10-02 14:39, Beno=C3=AEt Canet wrote: > Example of command line: > -drive if=3Dvirtio,file.driver=3Dquorum,\ > file.children.0.file.filename=3D1.qcow2,\ > file.children.1.file.filename=3D2.qcow2,\ > file.children.2.file.filename=3D3.qcow2,\ > file.vote_threshold=3D3 > > file.blkverify=3Don with file.vote_threshold=3D2 and two file can be pa= ssed to *two files > emulated blkverify. > > Signed-off-by: Benoit Canet > --- > block/quorum.c | 338 ++++++++++++++++++++++++++++++++++++++++++++++++= +++++++++ > 1 file changed, 338 insertions(+) Okay, in general: Bear in mind that the singular of "children" is "child". Second, error_setg is a shorthand to error_set(=E2=80=A6,=20 ERROR_CLASS_GENERIC_ERROR, =E2=80=A6), so I'd propose you might make use = of it =E2=80=93=20 if you're so inclined. ;) Third, wouldn't it be better to use children[i] instead of children.i=20 for addressing them on the command line? I guess, a QMP interface would=20 represent them as an array, so using the standard array index notation=20 seems better to me. Fourth, I don't like the parsing functions. They look reasonable for=20 what they're supposed to do, but they just do so much hard-coded parsing=20 (e.g., expect this prefix and that suffix). This is okay for now, since=20 they're just used by this driver, but there's my problem: Is there no=20 other, general way of parsing the command line options? (I truly don't=20 know) I guess not, since blockdev.c fetches whole arguments such as=20 "cache.direct" as well. So, these parsing functions seem necessary for now, but I don't like=20 them. ;) > diff --git a/block/quorum.c b/block/quorum.c > index 0a28ab1..3cfc67e 100644 > --- a/block/quorum.c > +++ b/block/quorum.c > @@ -17,8 +17,13 @@ > #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_SUFFIX "." > +#define KEY_FILENAME_SUFFIX ".file.filename" > =20 > /* This union hold a vote hash value */ > typedef union QuorumVoteValue { > @@ -673,12 +678,342 @@ static coroutine_fn int quorum_co_flush(BlockDri= verState *bs) > return result; > } > =20 > +static int quorum_match_key(const char *key, > + char **key_prefix, > + bool clone_key_prefix) > +{ > + const char *start; > + char *next; > + unsigned long long idx; > + int ret; > + > + *key_prefix =3D NULL; > + > + /* the following code is a translation of the following pseudo cod= e: > + * match =3D key.match('(^children\.(\d+)\.)file\.filename) > + * if not match: > + * return -1; > + * key_prefix =3D match.outer_match() > + * idx =3D match.inner_match() > + * > + * note: we also match the .file suffix to avoid futher checkings > + */ > + > + /* this key is not a children */ > + if (strncmp(key, KEY_PREFIX, strlen(KEY_PREFIX))) { You could use sizeof instead of strlen here (and for every other string=20 constant as well), though I don't know whether this is acceptable coding=20 style for qemu. ;) > + return -1; > + } > + > + /* take first char after prefix */ > + start =3D key + strlen(KEY_PREFIX); > + > + /* if the string end here -> scan fail */ > + if (start[0] =3D=3D '\0') { > + return -1; > + } > + > + /* try to retrieve the index */ > + ret =3D parse_uint(start, &idx, &next, 10); > + > + /* no int found -> scan fail */ > + if (ret < 0) { > + return -1; > + } > + > + /* match the ".file.filename" suffix to avoid matching the same id= x > + * multiple times and be required to do more checks later > + */ > + if (strncmp(next, KEY_FILENAME_SUFFIX, strlen(KEY_FILENAME_SUFFIX)= )) { > + return -1; > + } > + > + if (clone_key_prefix) { > + /* include '.' */ > + int len =3D next + strlen(KEY_SUFFIX) - key; > + *key_prefix =3D g_strndup(key, len); > + } > + > + return idx; > +} > + > +static QDict *quorum_get_children_idx(const QDict *options) > +{ > + const QDictEntry *ent; > + QDict *result; > + char *key_prefix; > + int idx; > + > + result =3D qdict_new(); > + > + for (ent =3D qdict_first(options); ent; ent =3D qdict_next(options= , ent)) { > + const char *key =3D qdict_entry_key(ent); > + idx =3D quorum_match_key(key, &key_prefix, true); > + > + /* if the result is -1 or any negative index value skip this k= ey */ > + if (idx < 0) { > + continue; > + } > + > + qdict_put(result, key_prefix, qint_from_int(idx)); > + } > + > + return result; > +} > + > +static int quorum_fill_validation_array(bool *array, > + const QDict *dict, > + int total, > + Error **errp) > +{ > + const QDictEntry *ent; > + > + /* fill the checking array with children indexes */ > + for (ent =3D qdict_first(dict); ent; ent =3D qdict_next(dict, ent)= ) { > + const char *key =3D qdict_entry_key(ent); > + int idx =3D qdict_get_int(dict, key); > + > + if (idx < 0 || idx >=3D total) { > + error_set(errp, ERROR_CLASS_GENERIC_ERROR, > + "Children index must be between 0 and children c= ount -1"); I'd prefer "- 1" instead of "-1" (or even "Child index must be an=20 unsigned integer smaller than the children count"). > + return -ERANGE; > + } > + > + array[idx] =3D true; > + } > + > + return 0; > +} > + > +static int quorum_valid_indexes(const bool *array, int total, Error **= errp) > +{ > + int i; > + > + for (i =3D 0; i < total; i++) { > + if (array[i] =3D=3D true) { > + continue; > + } > + > + error_set(errp, ERROR_CLASS_GENERIC_ERROR, > + "All children indexes between 0 and children count -= 1 must " > + " be used"); Again, I'd prefer "- 1"; then, there are two spaces both left and right=20 of must =E2=80=93 is this intentional? > + return -ERANGE; > + } > + > + return 0; > +} > + > +static int quorum_valid_children_indexes(const QDict *dict, > + int total, > + Error **errp) > +{ > + bool *array; > + int ret =3D 0;; > + > + /* allocate indexes checking array and put false in it */ > + array =3D g_new0(bool, total); > + > + ret =3D quorum_fill_validation_array(array, dict, total, errp); > + if (ret < 0) { > + goto free_exit; > + } > + > + ret =3D quorum_valid_indexes(array, total, errp); > +free_exit: > + g_free(array); > + return ret; > +} > + > +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_set(errp, ERROR_CLASS_GENERIC_ERROR, > + "threshold <=3D children count must be true"); Well, while this might technically be true, I'd rather go for "threshold=20 may not exceed children count" instead. ;) > + return -ERANGE; > + } > + > + return 0; > +} > + > +static int quorum_bdrv_open_sub(BlockDriverState *bs, > + QDict *bs_dict, > + int flags, > + Error **errp) > +{ > + const char *format_name =3D NULL; > + const char *filename =3D NULL; > + BlockDriver *drv =3D NULL; > + int ret =3D 0; > + > + format_name =3D qdict_get_try_str(bs_dict, "file.driver"); > + > + if (!format_name) { > + filename =3D qdict_get_try_str(bs_dict, "file.filename"); > + qdict_del(bs_dict, "file.filename"); > + } else { > + drv =3D bdrv_find_format(format_name); > + if (!drv) { > + error_set(errp, ERROR_CLASS_GENERIC_ERROR, > + "invalid format name"); > + return -EINVAL; > + } > + } > + > + QINCREF(bs_dict); > + ret =3D bdrv_open(bs, > + filename, > + bs_dict, > + flags, > + drv, > + errp); Why don't you write this on a single line? > + QDECREF(bs_dict); > + return ret; > +} > + > +static int quorum_open(BlockDriverState *bs, > + QDict *options, > + int flags, > + Error **errp) > +{ > + BDRVQuorumState *s =3D bs->opaque; > + Error *local_err =3D NULL; > + const QDictEntry *ent; > + QDict *idx_dict; > + bool *opened; > + const char *value; > + char *next; > + int i; > + int ret =3D 0; > + > + /* get a dict of children indexes for validation */ > + idx_dict =3D quorum_get_children_idx(options); > + > + /* count how many different children indexes are present and valid= ate */ > + s->total =3D qdict_size(idx_dict); > + if (s->total < 2) { > + error_set(&local_err, ERROR_CLASS_GENERIC_ERROR, > + "Number of provided children must be greater than 1"= ); > + ret =3D -EINVAL; > + goto exit; > + } > + > + /* validate that the set of index is coherent */ > + ret =3D quorum_valid_children_indexes(idx_dict, s->total, &local_e= rr); > + if (ret < 0) { > + goto exit; > + } > + > + /* retrieve the threshold option from the command line */ > + value =3D qdict_get_try_str(options, "vote_threshold"); > + if (!value) { > + error_set(&local_err, ERROR_CLASS_GENERIC_ERROR, > + "vote_threshold must be provided"); > + ret =3D -EINVAL; > + goto exit; > + } > + qdict_del(options, "vote_threshold"); > + > + ret =3D parse_uint(value, (unsigned long long *) &s->threshold, &n= ext, 10); > + > + /* no int found -> scan fail */ > + if (ret < 0) { > + error_set(&local_err, ERROR_CLASS_GENERIC_ERROR, > + "invalid vote_threshold specified"); > + ret =3D -EINVAL; > + goto exit; > + } > + > + /* and validate it againts s->total */ *against > + 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; > + } If the user specifies anything different from "on" or if he does and=20 s->total or s->threshold is not 2, quorum will silently work in=20 non-blkverify mode without telling the user. So I'd emit a message here=20 if blkverify has been specified and its value is different from "off"=20 but s->is_blkverify remains false (i.e., =E2=80=9Celse if (value &&=20 strcmp(value, "off")) { fprintf(stderr, "[Some warning]"); }=E2=80=9D). > + qdict_del(options, "blkverify"); > + > + /* allocate the children BlockDriverState array */ > + s->bs =3D g_new0(BlockDriverState, s->total); > + opened =3D g_new0(bool, s->total); > + > + /* open children bs */ > + for (ent =3D qdict_first(idx_dict); > + ent; ent =3D qdict_next(idx_dict, ent)) { > + const char *key =3D qdict_entry_key(ent); > + int idx =3D qdict_get_int(idx_dict, key); > + int ret =3D 0; Remove this declaration, it shadows the outer ret. This results in that=20 outer ret being 0 even if quorum_bdrv_open_sub returns an error which=20 makes this whole function return 0 (instead of propagating the return=20 value of quorum_bdrv_open_sub). > + QDict *bs_dict; /* dict used to open child */ > + qdict_extract_subqdict(options, &bs_dict, key); > + ret =3D quorum_bdrv_open_sub(&s->bs[idx], > + bs_dict, > + flags, > + &local_err); > + if (ret < 0) { > + QDECREF(bs_dict); I think you should remove this QDECREF. I don't really know why, but I=20 think basically bdrv_open takes ownership of it. Anyway, if this QDECREF=20 stays, qemu crashes when a child could not be opened due to memory=20 corruption, so it's probably not correct. ;) > + goto close_exit; > + } > + opened[idx] =3D true; > + } > + > + g_free(opened); > + goto exit; > + > +close_exit: > + /* cleanup on error */ > + for (i =3D 0; i < s->total; i++) { > + if (!opened[i]) { > + continue; > + } > + bdrv_close(&s->bs[i]); > + } > + g_free(s->bs); > + g_free(opened); > +exit: > + /* propagate error */ > + QDECREF(idx_dict); I think you should swap these two lines. Max > + if (error_is_set(&local_err)) { > + error_propagate(errp, local_err); > + } > + return ret; > +} > + > +static void quorum_close(BlockDriverState *bs) > +{ > + BDRVQuorumState *s =3D bs->opaque; > + int i; > + > + for (i =3D 0; i < s->total; i++) { > + /* Ensure writes reach stable storage */ > + bdrv_flush(&s->bs[i]); > + /* close manually because we'll free s->bs */ > + bdrv_close(&s->bs[i]); > + } > + > + g_free(s->bs); > +} > + > static BlockDriver bdrv_quorum =3D { > .format_name =3D "quorum", > .protocol_name =3D "quorum", > =20 > .instance_size =3D sizeof(BDRVQuorumState), > =20 > + .bdrv_file_open =3D quorum_open, > + .bdrv_close =3D quorum_close, > + > .bdrv_co_flush_to_disk =3D quorum_co_flush, > =20 > .bdrv_getlength =3D quorum_getlength, > @@ -687,6 +1022,9 @@ static BlockDriver bdrv_quorum =3D { > .bdrv_aio_writev =3D quorum_aio_writev, > .bdrv_invalidate_cache =3D quorum_invalidate_cache, > .bdrv_co_get_block_status =3D quorum_co_get_block_status, > + > + /* disable external snapshots while waiting for block filters */ > + .bdrv_check_ext_snapshot =3D bdrv_check_ext_snapshot_forbidden, > }; > =20 > static void bdrv_quorum_init(void)