| Message ID | 1391454712-14353-13-git-send-email-benoit.canet@irqsave.net |
|---|---|
| State | New |
| Headers | show |
On 03.02.2014 20:11, Benoît Canet wrote: > From: Benoît Canet <benoit@irqsave.net> > > Example of command line: > -drive if=virtio,file.driver=quorum,\ > file.children.0.file.filename=1.raw,\ > file.children.0.node-name=1.raw,\ > file.children.0.driver=raw,\ > file.children.1.file.filename=2.raw,\ > file.children.1.node-name=2.raw,\ > file.children.1.driver=raw,\ > file.children.2.file.filename=3.raw,\ > file.children.2.node-name=3.raw,\ > file.children.2.driver=raw,\ > file.vote_threshold=2 > > file.blkverify=on with file.vote_threshold=2 and two files can be passed to > emulated blkverify. > > Signed-off-by: Benoit Canet <benoit@irqsave.net> > --- > 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 <gnutls/crypto.h> > #include "block/block_int.h" > #include "qapi/qmp/qjson.h" > +#include "qapi/qmp/types.h" > +#include "qemu-common.h" > > #define HASH_LENGTH 32 > +#define KEY_PREFIX "children." > +#define KEY_FILENAME_SUFFIX ".file.filename" > > /* This union holds a vote hash value */ > typedef union QuorumVoteValue { > @@ -720,12 +724,177 @@ static bool quorum_recurse_is_first_non_filter(BlockDriverState *bs, > return false; > } > > +static int quorum_valid_threshold(int threshold, > + int total, > + Error **errp) > +{ > + > + if (threshold < 1) { > + error_set(errp, QERR_INVALID_PARAMETER_VALUE, > + "vote-threshold", "value >= 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 = bs->opaque; > + Error *local_err = NULL; > + bool *opened; > + QDict *sub = NULL; > + QList *list = NULL; > + const QListEntry *lentry; > + const QDictEntry *dentry; > + const char *value; > + char *next; > + int i; > + int ret = 0; > + unsigned long long threshold = 0; > + > + qdict_extract_subqdict(options, &sub, "children."); > + qdict_array_split(sub, &list); > + > + /* count how many different children are present and validate */ > + s->total = !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 = -EINVAL; > + goto exit; > + } > + > + ret = qdict_get_try_int(options, "vote-threshold", -1); > + /* from QMP */ > + if (ret != -1) { > + qdict_del(options, "vote-threshold"); > + s->threshold = ret; > + /* from command line */ > + } else { > + /* retrieve the threshold option from the command line */ > + value = qdict_get_try_str(options, "vote_threshold"); > + if (!value) { > + error_setg(&local_err, > + "vote_threshold must be provided"); > + ret = -EINVAL; > + goto exit; > + } > + qdict_del(options, "vote_threshold"); > + > + ret = parse_uint(value, &threshold, &next, 10); > + > + /* no int found -> scan fail */ > + if (ret < 0) { > + error_setg(&local_err, > + "invalid vote_threshold specified"); > + ret = -EINVAL; > + goto exit; > + } > + s->threshold = threshold; > + } > + > + /* and validate it againt s->total */ One step closer, but "against" is still one letter away. ;-) > + ret = quorum_valid_threshold(s->threshold, s->total, &local_err); > + if (ret < 0) { > + goto exit; > + } > + > + /* is the driver in blkverify mode */ > + value = qdict_get_try_str(options, "blkverify"); > + if (value && !strcmp(value, "on") && > + s->total == 2 && s->threshold == 2) { > + s->is_blkverify = true; > + } else if (value && strcmp(value, "off")) { > + fprintf(stderr, "blkverify mode is set by setting blkverify=on " > + "and using two files with vote_threshold=2"); This probably needs a \n at the end of the format string. > + } > + qdict_del(options, "blkverify"); > + > + /* allocate the children BlockDriverState array */ > + s->bs = g_new0(BlockDriverState *, s->total); > + opened = g_new0(bool, s->total); > + > + /* Opening by file name or options */ > + for (i = 0, lentry = qlist_first(list); > + s->total == qlist_size(list) && lentry; I know I said I find it clever to put such a condition into the loop condition instead of wrapping an if around it, but this time, I'd prefer the wrapping if for s->total == qlist_size(size) instead. ;-) This is not a trivial loop and the compiler may be unable to verify that this condition doesn't change on each iteration. Also, an if with this loop in the if branch and the following loop in the else branch would be much more evident in what it does, at least to me. > + lentry = qlist_next(lentry), i++) { > + ret = bdrv_open(&s->bs[i], NULL, NULL, qobject_to_qdict(lentry->value), > + flags, NULL, &local_err); > + if (ret < 0) { > + goto close_exit; > + } > + opened[i] = true; > + } > + > + /* Opening by reference */ > + for (i = 0, dentry = qdict_first(sub); > + s->total == qdict_size(sub) && dentry; > + dentry = qdict_next(sub, dentry), i++) { > + ret = bdrv_open(&s->bs[i], NULL, > + qstring_get_str(qobject_to_qstring(dentry->value)), > + NULL, flags, NULL, &local_err); > + if (ret < 0) { > + goto close_exit; > + } > + opened[i] = true; > + } Okay, I see… Using qdict_array_split() doesn't help you with QMP invocation, so now you're not verifying the indices in that case. Well, we have three options: 1) Leave it like this. 2) Check whether the QDict keys are actually contiguous (this shouldn't be too much effort, I hope…) and pull the index here ("i") from the QDict key. 3) Use qdict_flatten(options) before everything and thus make the difference between QMP and command-line invocation disappear. I'd go with 3, especially considering that it should have been already called in qmp_blockdev_add() (and therefore we don't even have to call it here). I don't know why exactly you have this special code for QMP in the first place; perhaps it is because I extended qdict_flatten() to cover QLists not until November or December last year. Until then, qdict_flatten() stopped at QLists and only flattened QDicts; but now, it will flatten QLists as well, in a manner consistent with qdict_array_split(). Thus, I think the extra handling for QMP should be unnecessary by now. If you have anything to test this hypothesis, by any chance, could you try it out? Max
Le Monday 03 Feb 2014 à 21:22:45 (+0100), Max Reitz a écrit : > On 03.02.2014 20:11, Benoît Canet wrote: > >From: Benoît Canet <benoit@irqsave.net> > > > >Example of command line: > >-drive if=virtio,file.driver=quorum,\ > >file.children.0.file.filename=1.raw,\ > >file.children.0.node-name=1.raw,\ > >file.children.0.driver=raw,\ > >file.children.1.file.filename=2.raw,\ > >file.children.1.node-name=2.raw,\ > >file.children.1.driver=raw,\ > >file.children.2.file.filename=3.raw,\ > >file.children.2.node-name=3.raw,\ > >file.children.2.driver=raw,\ > >file.vote_threshold=2 > > > >file.blkverify=on with file.vote_threshold=2 and two files can be passed to > >emulated blkverify. > > > >Signed-off-by: Benoit Canet <benoit@irqsave.net> > >--- > > 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 <gnutls/crypto.h> > > #include "block/block_int.h" > > #include "qapi/qmp/qjson.h" > >+#include "qapi/qmp/types.h" > >+#include "qemu-common.h" > > #define HASH_LENGTH 32 > >+#define KEY_PREFIX "children." > >+#define KEY_FILENAME_SUFFIX ".file.filename" > > /* This union holds a vote hash value */ > > typedef union QuorumVoteValue { > >@@ -720,12 +724,177 @@ static bool quorum_recurse_is_first_non_filter(BlockDriverState *bs, > > return false; > > } > >+static int quorum_valid_threshold(int threshold, > >+ int total, > >+ Error **errp) > >+{ > >+ > >+ if (threshold < 1) { > >+ error_set(errp, QERR_INVALID_PARAMETER_VALUE, > >+ "vote-threshold", "value >= 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 = bs->opaque; > >+ Error *local_err = NULL; > >+ bool *opened; > >+ QDict *sub = NULL; > >+ QList *list = NULL; > >+ const QListEntry *lentry; > >+ const QDictEntry *dentry; > >+ const char *value; > >+ char *next; > >+ int i; > >+ int ret = 0; > >+ unsigned long long threshold = 0; > >+ > >+ qdict_extract_subqdict(options, &sub, "children."); > >+ qdict_array_split(sub, &list); > >+ > >+ /* count how many different children are present and validate */ > >+ s->total = !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 = -EINVAL; > >+ goto exit; > >+ } > >+ > >+ ret = qdict_get_try_int(options, "vote-threshold", -1); > >+ /* from QMP */ > >+ if (ret != -1) { > >+ qdict_del(options, "vote-threshold"); > >+ s->threshold = ret; > >+ /* from command line */ > >+ } else { > >+ /* retrieve the threshold option from the command line */ > >+ value = qdict_get_try_str(options, "vote_threshold"); > >+ if (!value) { > >+ error_setg(&local_err, > >+ "vote_threshold must be provided"); > >+ ret = -EINVAL; > >+ goto exit; > >+ } > >+ qdict_del(options, "vote_threshold"); > >+ > >+ ret = parse_uint(value, &threshold, &next, 10); > >+ > >+ /* no int found -> scan fail */ > >+ if (ret < 0) { > >+ error_setg(&local_err, > >+ "invalid vote_threshold specified"); > >+ ret = -EINVAL; > >+ goto exit; > >+ } > >+ s->threshold = threshold; > >+ } > >+ > >+ /* and validate it againt s->total */ > > One step closer, but "against" is still one letter away. ;-) > > >+ ret = quorum_valid_threshold(s->threshold, s->total, &local_err); > >+ if (ret < 0) { > >+ goto exit; > >+ } > >+ > >+ /* is the driver in blkverify mode */ > >+ value = qdict_get_try_str(options, "blkverify"); > >+ if (value && !strcmp(value, "on") && > >+ s->total == 2 && s->threshold == 2) { > >+ s->is_blkverify = true; > >+ } else if (value && strcmp(value, "off")) { > >+ fprintf(stderr, "blkverify mode is set by setting blkverify=on " > >+ "and using two files with vote_threshold=2"); > > This probably needs a \n at the end of the format string. > > >+ } > >+ qdict_del(options, "blkverify"); > >+ > >+ /* allocate the children BlockDriverState array */ > >+ s->bs = g_new0(BlockDriverState *, s->total); > >+ opened = g_new0(bool, s->total); > >+ > >+ /* Opening by file name or options */ > >+ for (i = 0, lentry = qlist_first(list); > >+ s->total == qlist_size(list) && lentry; > > I know I said I find it clever to put such a condition into the loop > condition instead of wrapping an if around it, but this time, I'd > prefer the wrapping if for s->total == qlist_size(size) instead. ;-) > > This is not a trivial loop and the compiler may be unable to verify > that this condition doesn't change on each iteration. Also, an if > with this loop in the if branch and the following loop in the else > branch would be much more evident in what it does, at least to me. > > >+ lentry = qlist_next(lentry), i++) { > >+ ret = bdrv_open(&s->bs[i], NULL, NULL, qobject_to_qdict(lentry->value), > >+ flags, NULL, &local_err); > >+ if (ret < 0) { > >+ goto close_exit; > >+ } > >+ opened[i] = true; > >+ } > >+ > >+ /* Opening by reference */ > >+ for (i = 0, dentry = qdict_first(sub); > >+ s->total == qdict_size(sub) && dentry; > >+ dentry = qdict_next(sub, dentry), i++) { > >+ ret = bdrv_open(&s->bs[i], NULL, > >+ qstring_get_str(qobject_to_qstring(dentry->value)), > >+ NULL, flags, NULL, &local_err); > >+ if (ret < 0) { > >+ goto close_exit; > >+ } > >+ opened[i] = true; > >+ } > > Okay, I see… Using qdict_array_split() doesn't help you with QMP > invocation, so now you're not verifying the indices in that case. > Well, we have three options: > 1) Leave it like this. > 2) Check whether the QDict keys are actually contiguous (this > shouldn't be too much effort, I hope…) and pull the index here ("i") > from the QDict key. > 3) Use qdict_flatten(options) before everything and thus make the > difference between QMP and command-line invocation disappear. > > I'd go with 3, especially considering that it should have been > already called in qmp_blockdev_add() (and therefore we don't even > have to call it here). > > I don't know why exactly you have this special code for QMP in the > first place; perhaps it is because I extended qdict_flatten() to > cover QLists not until November or December last year. Until then, > qdict_flatten() stopped at QLists and only flattened QDicts; but > now, it will flatten QLists as well, in a manner consistent with > qdict_array_split(). This special code is not for QMP but for the QMP by references string. dict.children will be a list of qstring and qdict_array_split will leave them in the original qdict. qdict.children is not a dict of string list. Moving the qstring in the qlist will require special case handling of the correct QTYPE in the bdrv_open loop. I don't know what is the best way to handle this. Best regards Benoît > > Thus, I think the extra handling for QMP should be unnecessary by > now. If you have anything to test this hypothesis, by any chance, > could you try it out? > > Max
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 <gnutls/crypto.h> #include "block/block_int.h" #include "qapi/qmp/qjson.h" +#include "qapi/qmp/types.h" +#include "qemu-common.h" #define HASH_LENGTH 32 +#define KEY_PREFIX "children." +#define KEY_FILENAME_SUFFIX ".file.filename" /* This union holds a vote hash value */ typedef union QuorumVoteValue { @@ -720,12 +724,177 @@ static bool quorum_recurse_is_first_non_filter(BlockDriverState *bs, return false; } +static int quorum_valid_threshold(int threshold, + int total, + Error **errp) +{ + + if (threshold < 1) { + error_set(errp, QERR_INVALID_PARAMETER_VALUE, + "vote-threshold", "value >= 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 = bs->opaque; + Error *local_err = NULL; + bool *opened; + QDict *sub = NULL; + QList *list = NULL; + const QListEntry *lentry; + const QDictEntry *dentry; + const char *value; + char *next; + int i; + int ret = 0; + unsigned long long threshold = 0; + + qdict_extract_subqdict(options, &sub, "children."); + qdict_array_split(sub, &list); + + /* count how many different children are present and validate */ + s->total = !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 = -EINVAL; + goto exit; + } + + ret = qdict_get_try_int(options, "vote-threshold", -1); + /* from QMP */ + if (ret != -1) { + qdict_del(options, "vote-threshold"); + s->threshold = ret; + /* from command line */ + } else { + /* retrieve the threshold option from the command line */ + value = qdict_get_try_str(options, "vote_threshold"); + if (!value) { + error_setg(&local_err, + "vote_threshold must be provided"); + ret = -EINVAL; + goto exit; + } + qdict_del(options, "vote_threshold"); + + ret = parse_uint(value, &threshold, &next, 10); + + /* no int found -> scan fail */ + if (ret < 0) { + error_setg(&local_err, + "invalid vote_threshold specified"); + ret = -EINVAL; + goto exit; + } + s->threshold = threshold; + } + + /* and validate it againt s->total */ + ret = quorum_valid_threshold(s->threshold, s->total, &local_err); + if (ret < 0) { + goto exit; + } + + /* is the driver in blkverify mode */ + value = qdict_get_try_str(options, "blkverify"); + if (value && !strcmp(value, "on") && + s->total == 2 && s->threshold == 2) { + s->is_blkverify = true; + } else if (value && strcmp(value, "off")) { + fprintf(stderr, "blkverify mode is set by setting blkverify=on " + "and using two files with vote_threshold=2"); + } + qdict_del(options, "blkverify"); + + /* allocate the children BlockDriverState array */ + s->bs = g_new0(BlockDriverState *, s->total); + opened = g_new0(bool, s->total); + + /* Opening by file name or options */ + for (i = 0, lentry = qlist_first(list); + s->total == qlist_size(list) && lentry; + lentry = qlist_next(lentry), i++) { + ret = bdrv_open(&s->bs[i], NULL, NULL, qobject_to_qdict(lentry->value), + flags, NULL, &local_err); + if (ret < 0) { + goto close_exit; + } + opened[i] = true; + } + + /* Opening by reference */ + for (i = 0, dentry = qdict_first(sub); + s->total == qdict_size(sub) && dentry; + dentry = qdict_next(sub, dentry), i++) { + ret = bdrv_open(&s->bs[i], NULL, + qstring_get_str(qobject_to_qstring(dentry->value)), + NULL, flags, NULL, &local_err); + if (ret < 0) { + goto close_exit; + } + opened[i] = true; + } + + g_free(opened); + goto exit; + +close_exit: + /* cleanup on error */ + for (i = 0; i < s->total; i++) { + if (!opened[i]) { + continue; + } + bdrv_unref(s->bs[i]); + } + g_free(s->bs); + g_free(opened); +exit: + /* propagate error */ + if (error_is_set(&local_err)) { + error_propagate(errp, local_err); + } + QDECREF(sub); + QDECREF(list); + return ret; +} + +static void quorum_close(BlockDriverState *bs) +{ + BDRVQuorumState *s = bs->opaque; + int i; + + for (i = 0; i < s->total; i++) { + /* Ensure writes reach stable storage */ + bdrv_flush(s->bs[i]); + /* close manually because we'll free s->bs */ + bdrv_unref(s->bs[i]); + } + + g_free(s->bs); +} + static BlockDriver bdrv_quorum = { .format_name = "quorum", .protocol_name = "quorum", .instance_size = sizeof(BDRVQuorumState), + .bdrv_file_open = quorum_open, + .bdrv_close = quorum_close, + .bdrv_co_flush_to_disk = quorum_co_flush, .bdrv_getlength = quorum_getlength, diff --git a/qapi-schema.json b/qapi-schema.json index 05ced9d..903a3a0 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -4352,6 +4352,24 @@ 'raw': 'BlockdevRef' } } ## +# @BlockdevOptionsQuorum +# +# Driver specific block device options for Quorum +# +# @blkverify: #optional true if the driver must print content mismatch +# +# @children: the children block device to use +# +# @vote_threshold: the vote limit under which a read will fail +# +# Since: 2.0 +## +{ 'type': 'BlockdevOptionsQuorum', + 'data': { '*blkverify': 'bool', + 'children': [ 'BlockdevRef' ], + 'vote-threshold': 'int' } } + +## # @BlockdevOptions # # Options for creating a block device. @@ -4389,7 +4407,8 @@ 'vdi': 'BlockdevOptionsGenericFormat', 'vhdx': 'BlockdevOptionsGenericFormat', 'vmdk': 'BlockdevOptionsGenericCOWFormat', - 'vpc': 'BlockdevOptionsGenericFormat' + 'vpc': 'BlockdevOptionsGenericFormat', + 'quorum': 'BlockdevOptionsQuorum' } } ##