Re: [Qemu-devel] [PATCH V9 11/11] quorum: Add quorum_open() and quorum_close().

[Max Reitz <mreitz@redhat.com>]

On 2013-10-02 14:39, Benoît Canet wrote:
> Example of command line:
> -drive if=virtio,file.driver=quorum,\
> file.children.0.file.filename=1.qcow2,\
> file.children.1.file.filename=2.qcow2,\
> file.children.2.file.filename=3.qcow2,\
> file.vote_threshold=3
>
> file.blkverify=on with file.vote_threshold=2 and two file can be passed to
*two files

> emulated blkverify.
>
> Signed-off-by: Benoit Canet <benoit@irqsave.net>
> ---
>   block/quorum.c | 338 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 338 insertions(+)
Okay, in general: Bear in mind that the singular of "children" is "child".

Second, error_setg is a shorthand to error_set(…, 
ERROR_CLASS_GENERIC_ERROR, …), so I'd propose you might make use of it – 
if you're so inclined. ;)

Third, wouldn't it be better to use children[i] instead of children.i 
for addressing them on the command line? I guess, a QMP interface would 
represent them as an array, so using the standard array index notation 
seems better to me.

Fourth, I don't like the parsing functions. They look reasonable for 
what they're supposed to do, but they just do so much hard-coded parsing 
(e.g., expect this prefix and that suffix). This is okay for now, since 
they're just used by this driver, but there's my problem: Is there no 
other, general way of parsing the command line options? (I truly don't 
know) I guess not, since blockdev.c fetches whole arguments such as 
"cache.direct" as well.

So, these parsing functions seem necessary for now, but I don't like 
them. ;)

> diff --git a/block/quorum.c b/block/quorum.c
> index 0a28ab1..3cfc67e 100644
> --- a/block/quorum.c
> +++ b/block/quorum.c
> @@ -17,8 +17,13 @@
>   #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_SUFFIX "."
> +#define KEY_FILENAME_SUFFIX ".file.filename"
>   
>   /* This union hold a vote hash value */
>   typedef union QuorumVoteValue {
> @@ -673,12 +678,342 @@ static coroutine_fn int quorum_co_flush(BlockDriverState *bs)
>       return result;
>   }
>   
> +static int quorum_match_key(const char *key,
> +                            char **key_prefix,
> +                            bool clone_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+)\.)file\.filename)
> +     *       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 children */
> +    if (strncmp(key, KEY_PREFIX, strlen(KEY_PREFIX))) {
You could use sizeof instead of strlen here (and for every other string 
constant as well), though I don't know whether this is acceptable coding 
style for qemu. ;)

> +        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;
> +    }
> +
> +    /* match the ".file.filename" 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))) {
> +        return -1;
> +    }
> +
> +    if (clone_key_prefix) {
> +        /* include '.' */
> +        int len = next + strlen(KEY_SUFFIX) - 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, true);
> +
> +        /* if the result is -1 or any negative index value skip this 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_set(errp, ERROR_CLASS_GENERIC_ERROR,
> +                      "Children index must be between 0 and children count -1");
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_set(errp, ERROR_CLASS_GENERIC_ERROR,
> +                  "All children indexes between 0 and children count -1  must "
> +                  " be used");
Again, I'd prefer "- 1"; then, there are two spaces both left and right 
of must – is this intentional?

> +        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_set(errp, ERROR_CLASS_GENERIC_ERROR,
> +                  "threshold <= children count must be true");
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_bdrv_open_sub(BlockDriverState *bs,
> +                                QDict *bs_dict,
> +                                int flags,
> +                                Error **errp)
> +{
> +    const char *format_name = NULL;
> +    const char *filename = NULL;
> +    BlockDriver *drv = NULL;
> +    int ret = 0;
> +
> +    format_name = qdict_get_try_str(bs_dict, "file.driver");
> +
> +    if (!format_name) {
> +        filename = qdict_get_try_str(bs_dict, "file.filename");
> +        qdict_del(bs_dict, "file.filename");
> +    } else {
> +        drv = bdrv_find_format(format_name);
> +        if (!drv) {
> +            error_set(errp, ERROR_CLASS_GENERIC_ERROR,
> +                      "invalid format name");
> +            return -EINVAL;
> +        }
> +    }
> +
> +    QINCREF(bs_dict);
> +    ret = bdrv_open(bs,
> +                    filename,
> +                    bs_dict,
> +                    flags,
> +                    drv,
> +                    errp);
Why don't you write this on a single line?

> +    QDECREF(bs_dict);
> +    return ret;
> +}
> +
> +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_set(&local_err, ERROR_CLASS_GENERIC_ERROR,
> +                  "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;
> +    }
> +
> +    /* retrieve the threshold option from the command line */
> +    value = qdict_get_try_str(options, "vote_threshold");
> +    if (!value) {
> +        error_set(&local_err, ERROR_CLASS_GENERIC_ERROR,
> +                  "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);
> +
> +    /* no int found -> scan fail */
> +    if (ret < 0) {
> +        error_set(&local_err, ERROR_CLASS_GENERIC_ERROR,
> +                  "invalid vote_threshold specified");
> +        ret = -EINVAL;
> +        goto exit;
> +    }
> +
> +    /* and validate it againts s->total */
*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;
> +    }
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)) {
> +        const char *key = qdict_entry_key(ent);
> +        int idx = qdict_get_int(idx_dict, key);
> +        int ret = 0;
Remove this declaration, it shadows the outer ret. This results in that 
outer ret being 0 even if quorum_bdrv_open_sub returns an error which 
makes this whole function return 0 (instead of propagating the return 
value of quorum_bdrv_open_sub).

> +        QDict *bs_dict; /* dict used to open child */
> +        qdict_extract_subqdict(options, &bs_dict, key);
> +        ret = quorum_bdrv_open_sub(&s->bs[idx],
> +                                   bs_dict,
> +                                   flags,
> +                                   &local_err);
> +        if (ret < 0) {
> +            QDECREF(bs_dict);
I think you should remove this QDECREF. I don't really know why, but I 
think basically bdrv_open takes ownership of it. Anyway, if this QDECREF 
stays, qemu crashes when a child could not be opened due to memory 
corruption, so it's probably not correct. ;)

> +            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]);
> +    }
> +    g_free(s->bs);
> +    g_free(opened);
> +exit:
> +    /* propagate error */
> +    QDECREF(idx_dict);
I think you should swap these two lines.

Max

> +    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]);
> +    }
> +
> +    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,
> @@ -687,6 +1022,9 @@ static BlockDriver bdrv_quorum = {
>       .bdrv_aio_writev    = quorum_aio_writev,
>       .bdrv_invalidate_cache = quorum_invalidate_cache,
>       .bdrv_co_get_block_status = quorum_co_get_block_status,
> +
> +    /* disable external snapshots while waiting for block filters */
> +    .bdrv_check_ext_snapshot = bdrv_check_ext_snapshot_forbidden,
>   };
>   
>   static void bdrv_quorum_init(void)