qemu-devel
[Top][All Lists]
Advanced

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

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


From: Benoît Canet
Subject: Re: [Qemu-devel] [PATCH V10 12/13] quorum: Add quorum_open() and quorum_close().
Date: Mon, 3 Feb 2014 13:49:23 +0100
User-agent: Mutt/1.5.21 (2010-09-15)

Le Monday 03 Feb 2014 à 13:21:18 (+0100), Benoît Canet a écrit :
> 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 <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   | 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;
> > >  }
> > 
> > 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.
> 
> I don't think so because to simplify memory management s->bs is an array
> containing all the allocated bs and is freed at once.
> So should bdrv_unref still be used ?
I double checked you are right :)

> 
> > 
> > >+    }
> > >+    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
> > 
> 



reply via email to

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