Le Monday 03 Feb 2014 à 00:09:28 (+0100), Max Reitz a écrit :
> On 28.01.2014 17:52, Benoît Canet wrote:
> >From: Benoît Canet <ben...@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 <ben...@irqsave.net>
> >---
> > block/quorum.c | 308
> > +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > qapi-schema.json | 21 +++-
> > 2 files changed, 328 insertions(+), 1 deletion(-)
> >
> >diff --git a/block/quorum.c b/block/quorum.c
> >index e7b2090..0c0d630 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 {
> >@@ -702,12 +706,316 @@ static bool
> >quorum_recurse_is_first_non_filter(BlockDriverState *bs,
> > return false;
> > }
>
As a general stance sorry to have missed this mail of review from october.
It was not done on purpose.
Best regards
Benoît
> Perhaps you could make use of qdict_extract_subqdict() and
> qdict_array_split() functions here...? That is,
> qdict_extract_subqdict(..., "children.") and then
> qdict_array_split() on the result?
>
> >+static int quorum_match_key(const char *key,
> >+ char **key_prefix)
> >+{
> >+ const char *start;
> >+ char *next;
> >+ unsigned long long idx;
> >+ int ret;
> >+
> >+ *key_prefix = NULL;
> >+
> >+ /* the following code is a translation of the following pseudo code:
> >+ * match = key.match('(^children\.(\d+)\.)$suffix)
> >+ * if not match:
> >+ * return -1;
> >+ * key_prefix = match.outer_match()
> >+ * idx = match.inner_match()
> >+ *
> >+ * note: we also match the .file suffix to avoid futher checkings
> >+ */
> >+
> >+ /* this key is not a child */
> >+ if (strncmp(key, KEY_PREFIX, strlen(KEY_PREFIX))) {
> >+ return -1;
> >+ }
> >+
> >+ /* take first char after prefix */
> >+ start = key + strlen(KEY_PREFIX);
> >+
> >+ /* if the string end here -> scan fail */
> >+ if (start[0] == '\0') {
> >+ return -1;
> >+ }
> >+
> >+ /* try to retrieve the index */
> >+ ret = parse_uint(start, &idx, &next, 10);
> >+
> >+ /* no int found -> scan fail */
> >+ if (ret < 0) {
> >+ return -1;
> >+ }
> >+
> >+ /* we are taking a reference via QMP */
> >+ if (next - key == strlen(key)) {
>
> "if (!*next)" would probably accomplish the same (or "if next[0] ==
> '\0')" to keep your coding style) without having to call strlen().
>
> >+ *key_prefix = g_strdup(key);
> >+ return idx;
> >+ }
> >+
> >+ /* match the suffix to avoid matching the same idx
> >+ * multiple times and be required to do more checks later
> >+ */
> >+ if (strncmp(next, KEY_FILENAME_SUFFIX, strlen(KEY_FILENAME_SUFFIX))) {
>
> As stated in my review from October, you may use sizeof() to avoid strlen().
>
> >+ return -1;
> >+ }
> >+
> >+ /* do not include '.' */
> >+ int len = next - key;
> >+ *key_prefix = 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 = qdict_new();
> >+
> >+ for (ent = qdict_first(options); ent; ent = qdict_next(options, ent)) {
> >+ const char *key = qdict_entry_key(ent);
> >+ idx = quorum_match_key(key,
> >+ &key_prefix);
> >+
> >+ /* if the result zero or positive we got a key */
> >+ 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 = qdict_first(dict); ent; ent = qdict_next(dict, ent)) {
> >+ const char *key = qdict_entry_key(ent);
> >+ int idx = qdict_get_int(dict, key);
> >+
> >+ if (idx < 0 || idx >= total) {
> >+ error_setg(errp,
> >+ "Children index must be between 0 and children count -1");
>
> Quote from October: I'd prefer "- 1" instead of "-1" (or even "Child
> index must be an unsigned integer smaller than the children count").
>
> >+ return -ERANGE;
> >+ }
> >+
> >+ array[idx] = true;
> >+ }
> >+
> >+ return 0;
> >+}
> >+
> >+static int quorum_valid_indexes(const bool *array, int total, Error **errp)
> >+{
> >+ int i;
> >+
> >+ for (i = 0; i < total; i++) {
> >+ if (array[i] == true) {
> >+ continue;
> >+ }
> >+
> >+ error_setg(errp,
> >+ "All child indexes between 0 and children count -1 must be "
> >+ " used");
>
> Again, I'd prefer "- 1"; then, there are two spaces to the left of
> "must" and two spaces after "be".
>
> >+ return -ERANGE;
> >+ }
> >+
> >+ return 0;
> >+}
> >+
> >+static int quorum_valid_children_indexes(const QDict *dict,
> >+ int total,
> >+ Error **errp)
> >+{
> >+ bool *array;
> >+ int ret = 0;;
> >+
> >+ /* allocate indexes checking array and put false in it */
> >+ array = g_new0(bool, total);
> >+
> >+ ret = quorum_fill_validation_array(array, dict, total, errp);
> >+ if (ret < 0) {
> >+ goto free_exit;
> >+ }
> >+
> >+ ret = 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 >= 1");
> >+ return -ERANGE;
> >+ }
> >+
> >+ if (threshold > total) {
> >+ error_setg(errp, "threshold <= children count must be true");
>
> Quote from October: Well, while this might technically be true, I'd
> rather go for "threshold may not exceed children count" instead. ;-)
>
> >+ return -ERANGE;
> >+ }
> >+
> >+ return 0;
> >+}
> >+
> >+static int quorum_open(BlockDriverState *bs,
> >+ QDict *options,
> >+ int flags,
> >+ Error **errp)
> >+{
> >+ BDRVQuorumState *s = bs->opaque;
> >+ Error *local_err = NULL;
> >+ const QDictEntry *ent;
> >+ QDict *idx_dict;
> >+ bool *opened;
> >+ const char *value;
> >+ char *next;
> >+ int i;
> >+ int ret = 0;
> >+
> >+ /* get a dict of children indexes for validation */
> >+ idx_dict = quorum_get_children_idx(options);
> >+
> >+ /* count how many different children indexes are present and validate */
> >+ s->total = qdict_size(idx_dict);
> >+ if (s->total < 2) {
> >+ error_setg(&local_err,
> >+ "Number of provided children must be greater than 1");
> >+ ret = -EINVAL;
> >+ goto exit;
> >+ }
> >+
> >+ /* validate that the set of index is coherent */
> >+ ret = quorum_valid_children_indexes(idx_dict, s->total, &local_err);
> >+ if (ret < 0) {
> >+ 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, (unsigned long long *) &s->threshold,
> >&next, 10);
>
> I don't like this cast at all...
>
> >+
> >+ /* no int found -> scan fail */
> >+ if (ret < 0) {
> >+ error_setg(&local_err,
> >+ "invalid voter_threshold specified");
>
> *vote_threshold
>
> >+ ret = -EINVAL;
> >+ goto exit;
> >+ }
> >+ }
> >+
> >+ /* and validate it againts s->total */
>
> Still *against
>
> >+ 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;
> >+ }
>
> Quote from October: If the user specifies anything different from
> "on" or if he does and s->total or s->threshold is not 2, quorum
> will silently work in non-blkverify mode without telling the user.
> So I'd emit a message here if blkverify has been specified and its
> value is different from "off" but s->is_blkverify remains false
> (i.e., “else if (value && strcmp(value, "off")) { fprintf(stderr,
> "[Some warning]"); }”).
>
> >+ qdict_del(options, "blkverify");
> >+
> >+ /* allocate the children BlockDriverState array */
> >+ s->bs = g_new0(BlockDriverState *, s->total);
> >+ opened = g_new0(bool, s->total);
> >+
> >+ /* open children bs */
> >+ for (ent = qdict_first(idx_dict);
> >+ ent; ent = qdict_next(idx_dict, ent)) {
>
> This condition fits on a single line.
>
> >+ const char *key = qdict_entry_key(ent);
> >+ int idx = qdict_get_int(idx_dict, key);
> >+ ret = bdrv_open_image(&s->bs[idx],
> >+ NULL,
> >+ options,
> >+ key,
> >+ flags,
> >+ false,
> >+ &local_err);
>
> This takes two, but definitely not seven.
>
> >+ if (ret < 0) {
> >+ goto close_exit;
> >+ }
> >+ opened[idx] = true;
> >+ }
> >+
> >+ g_free(opened);
> >+ goto exit;
> >+
> >+close_exit:
> >+ /* cleanup on error */
> >+ for (i = 0; i < s->total; i++) {
> >+ if (!opened[i]) {
> >+ continue;
> >+ }
> >+ bdrv_close(s->bs[i]);
>
> You'll probably want bdrv_unref() here.
>
> >+ }
> >+ g_free(s->bs);
> >+ g_free(opened);
> >+exit:
> >+ /* propagate error */
> >+ if (error_is_set(&local_err)) {
> >+ error_propagate(errp, local_err);
> >+ }
> >+ 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_close(s->bs[i]);
>
> Again, bdrv_unref().
>
> >+ }
> >+
> >+ 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'
> > } }
> > ##
>
> As I've said before, I'd really like it if you could abandon all
> those functions for parsing the incoming options QDict in favor of
> qdict_extract_subqdict() and qdict_array_split() which will most
> likely do the job just fine; and, in fact, I have written
> qdict_array_split() with Quorum in mind. ;-)
>
> Max
>
