qemu-devel
[Top][All Lists]
Advanced

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

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


From: Benoît Canet
Subject: Re: [Qemu-devel] [PATCH V15 12/13] quorum: Add quorum_open() and quorum_close().
Date: Wed, 5 Feb 2014 17:11:03 +0100
User-agent: Mutt/1.5.21 (2010-09-15)

Le Tuesday 04 Feb 2014 à 17:08:07 (+0100), Kevin Wolf a écrit :
> Am 03.02.2014 um 22:51 hat Benoît Canet geschrieben:
> > 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   | 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.

No references are left into sub.
Also they are string and this means that even if they ended up in list the
code would have to discriminate on object types. It would not be much cleanner.

Best regards

Benoît

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



reply via email to

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