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

[Kevin Wolf <kwolf@redhat.com>]

Am 03.02.2014 um 22:51 hat Benoît Canet geschrieben:
> 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   | 171 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  qapi-schema.json |  21 ++++++-
>  2 files changed, 191 insertions(+), 1 deletion(-)
> 
> diff --git a/block/quorum.c b/block/quorum.c
> index 1e683f8..d2bea29 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 {
> @@ -712,12 +716,179 @@ 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_flatten(options);
> +    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);

Which case does qdict_size(sub) address?

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

This part looks seriously wrong. I think you should consider using an
QemuOpts like other drivers do (have a look at qcow2, for example), that
should parse the integer for you.

> +    /* and validate it against 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\n");
> +    }
> +    qdict_del(options, "blkverify");

And the QemuOpts would also know how to parse a boolean.

> +
> +    /* allocate the children BlockDriverState array */
> +    s->bs = g_new0(BlockDriverState *, s->total);
> +    opened = g_new0(bool, s->total);
> +
> +    /* Open by file name or options dict (command line or QMP) */
> +    if (s->total == qlist_size(list)) {
> +        for (i = 0, lentry = qlist_first(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;
> +        }
> +    /* Open by QMP references */
> +    } else {
> +        for (i = 0, dentry = qdict_first(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;
> +        }
> +    }

What does the reference case look like? (Should work on the command
line, too) I imagine something like this:

    -drive if=virtio,file.driver=quorum,\
    file.children.0=image0,\
    file.children.1=image1,\
    file.children.2=image2,\
    file.vote_threshold=2

Doesn't qdict_array_split() split this into a list of strings? The whole
thing with directly accessing QDict entries for references confuses me.

> +    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]);

Not necessary, block.c code will flush for us.

> +        /* close manually because we'll free s->bs */
> +        bdrv_unref(s->bs[i]);
> +    }
> +
> +    g_free(s->bs);
> +}

Kevin