qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Max Reitz
Subject: Re: [Qemu-devel] [PATCH V14 12/13] quorum: Add quorum_open() and quorum_close().
Date: Mon, 03 Feb 2014 21:22:45 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.2.0

On 03.02.2014 20:11, Benoît Canet wrote:
From: Benoît Canet <address@hidden>

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 <address@hidden>
---
  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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]