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

[Max Reitz <mreitz@redhat.com>]

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().

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