Re: [Qemu-devel] [PATCH V14 12/13] quorum: Add quorum_open() and quorum_close().

["Benoît Canet" <benoit.canet@irqsave.net>]

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