Re: [Qemu-devel] [PATCH 1/2] quorum: Simplify quorum_open()

[Benoît Canet]

The Friday 21 Feb 2014 à 20:44:07 (+0100), Max Reitz wrote :
> Although it may not look like it, this patch simplifies quorum_open().
> qdict_array_split() is now able to return QLists with different objects
> than only QDicts, therefore it will now do all the work and
> quorum_open() does not have to handle reference strings by itself.
> 
> This allows mixing full option dicts and reference strings for
> specifying the child block devices of quorum; furthermore, it improves
> handling of malformed specifications.
> 
> Signed-off-by: Max Reitz <address@hidden>
> ---
>  block/quorum.c | 62 
> +++++++++++++++++++++++++++++++++++-----------------------
>  1 file changed, 38 insertions(+), 24 deletions(-)
> 
> diff --git a/block/quorum.c b/block/quorum.c
> index 18721ba..8f5833d 100644
> --- a/block/quorum.c
> +++ b/block/quorum.c
> @@ -720,7 +720,6 @@ static int quorum_open(BlockDriverState *bs, QDict 
> *options, int flags,
>      QDict *sub = NULL;
>      QList *list = NULL;
>      const QListEntry *lentry;
> -    const QDictEntry *dentry;
>      int i;
>      int ret = 0;
>  
> @@ -728,10 +727,17 @@ static int quorum_open(BlockDriverState *bs, QDict 
> *options, int flags,
>      qdict_extract_subqdict(options, &sub, "children.");
>      qdict_array_split(sub, &list);
>  
> +    if (qdict_size(sub)) {
> +        error_setg(&local_err, "Invalid option children.%s",
> +                   qdict_first(sub)->key);
> +        ret = -EINVAL;
> +        goto exit;
> +    }
> +
>      /* count how many different children are present and validate
>       * qdict_size(sub) address the open by reference case
>       */
I think the second part of the comment about qlist_size(sub) is an extra now.

Aside from that:

Reviewed-by: Benoit Canet <address@hidden>

> -    s->num_children = !qlist_size(list) ? qdict_size(sub) : qlist_size(list);
> +    s->num_children = qlist_size(list);
>      if (s->num_children < 2) {
>          error_setg(&local_err,
>                     "Number of provided children must be greater than 1");
> @@ -767,30 +773,38 @@ static int quorum_open(BlockDriverState *bs, QDict 
> *options, int flags,
>      s->bs = g_new0(BlockDriverState *, s->num_children);
>      opened = g_new0(bool, s->num_children);
>  
> -    /* Open by file name or options dict (command line or QMP) */
> -    if (s->num_children == qlist_size(list)) {
> -        for (i = 0, lentry = qlist_first(list); lentry;
> -            lentry = qlist_next(lentry), i++) {
> -            QDict *d = qobject_to_qdict(lentry->value);
> -            QINCREF(d);
> -            ret = bdrv_open(&s->bs[i], NULL, NULL, d, flags, NULL, 
> &local_err);
> -            if (ret < 0) {
> -                goto close_exit;
> -            }
> -            opened[i] = true;
> +    for (i = 0, lentry = qlist_first(list); lentry;
> +         lentry = qlist_next(lentry), i++) {
> +        QDict *d;
> +        QString *string;
> +
> +        switch (qobject_type(lentry->value))
> +        {
> +            /* List of options */
> +            case QTYPE_QDICT:
> +                d = qobject_to_qdict(lentry->value);
> +                QINCREF(d);
> +                ret = bdrv_open(&s->bs[i], NULL, NULL, d, flags, NULL,
> +                                &local_err);
> +                break;
> +
> +            /* QMP reference */
> +            case QTYPE_QSTRING:
> +                string = qobject_to_qstring(lentry->value);
> +                ret = bdrv_open(&s->bs[i], NULL, qstring_get_str(string), 
> NULL,
> +                                flags, NULL, &local_err);
> +                break;
> +
> +            default:
> +                error_setg(&local_err, "Specification of child block device 
> %i "
> +                           "is invalid", i);
> +                ret = -EINVAL;
>          }
> -    /* Open by QMP references */
> -    } else {
> -        for (i = 0, dentry = qdict_first(sub); dentry;
> -             dentry = qdict_next(sub, dentry), i++) {
> -            QString *string = qobject_to_qstring(dentry->value);
> -            ret = bdrv_open(&s->bs[i], NULL, qstring_get_str(string), NULL,
> -                            flags, NULL, &local_err);
> -            if (ret < 0) {
> -                goto close_exit;
> -            }
> -            opened[i] = true;
> +
> +        if (ret < 0) {
> +            goto close_exit;
>          }
> +        opened[i] = true;
>      }
>  
>      g_free(opened);
> -- 
> 1.9.0
>