[V9,11/11] quorum: Add quorum_open() and quorum_close().

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
emulated blkverify.

Signed-off-by: Benoit Canet <benoit@irqsave.net>
---
 block/quorum.c | 338 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 338 insertions(+)

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)

On 10/04/2013 09:14 AM, Max Reitz wrote:
> 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
>>

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

No, we already discussed this.  file.child[0].file.filename=1.qcow2
requires shell quoting to avoid unintentional shell globbing;
file.child.0.file.filename=1.qcow2 is just as legible and can still be
reconstructed into an array under the hood, and has the benefit of not
needing quoting to protect against shell globbing.

On 2013-10-04 17:31, Eric Blake wrote:
> On 10/04/2013 09:14 AM, Max Reitz wrote:
>> 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
>>>
>> 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.
> No, we already discussed this.  file.child[0].file.filename=1.qcow2
> requires shell quoting to avoid unintentional shell globbing;
> file.child.0.file.filename=1.qcow2 is just as legible and can still be
> reconstructed into an array under the hood, and has the benefit of not
> needing quoting to protect against shell globbing.

Oh, sorry.I thought escaping wouldn't hurt too much; but well, on second 
thought, for an interface that's purely designed for the command line 
it's probably best to avoid the need for it as much as possible.

Max