Re: [Qemu-devel] [PATCH RFC] remove QEMUOptionParameter

[Dong Xu Wang <wdongxu@vnet.linux.ibm.com>]

Missed cc qemu-devel, added CC, sorry.

-------- 原始消息 --------
主题: Re: [Qemu-devel] [PATCH RFC] remove QEMUOptionParameter
日期: Fri, 21 Sep 2012 10:43:12 +0800
发件人: Dong Xu Wang <wdongxu@vnet.linux.ibm.com>
收件人: Markus Armbruster <armbru@redhat.com>

于 9/20/2012 5:16 PM, Markus Armbruster 写道:
> Dong Xu Wang <wdongxu@linux.vnet.ibm.com> writes:
> 
>> Markus, I am working with v2 and have some questions based your comments.
> 
> Your replies are very hard to read, because whatever you use to send
> them wraps quoted lines.  Please fix that.
> 
Sorry for that, will notice next time.

>> On Fri, Sep 7, 2012 at 4:42 PM, Markus Armbruster <armbru@redhat.com> wrote:
>>> Some overlap with what I'm working on, but since you have code to show,
>>> and I don't, I'll review yours as if mine didn't exist.
>>>
>>>
>>> Dong Xu Wang <wdongxu@linux.vnet.ibm.com> writes:
> [...]
>>>> diff --git a/qemu-option.c b/qemu-option.c
>>>> index 27891e7..b83ca52 100644
>>>> --- a/qemu-option.c
>>>> +++ b/qemu-option.c
> [...]
>>>>   static QemuOpt *qemu_opt_find(QemuOpts *opts, const char *name)
>>>> @@ -832,14 +547,36 @@ void qemu_opts_del(QemuOpts *opts)
>>>>
>>>>   int qemu_opts_print(QemuOpts *opts, void *dummy)
>>>>   {
>>>> -    QemuOpt *opt;
>>>> +    QemuOpt *opt = NULL;
>>>> +    QemuOptDesc *desc = opts->list->desc;
>>>>
>>>> -    fprintf(stderr, "%s: %s:", opts->list->name,
>>>> -            opts->id ? opts->id : "<noid>");
>>>> -    QTAILQ_FOREACH(opt, &opts->head, next) {
>>>> -        fprintf(stderr, " %s=\"%s\"", opt->name, opt->str);
>>>> +    while (desc && desc->name) {
>>>> +        opt = qemu_opt_find(opts, desc->name);
>>>> +        switch (desc->type) {
>>>> +        case QEMU_OPT_STRING:
>>>> +            if (opt != NULL) {
>>>> +                printf("%s='%s' ", opt->name, opt->str);
>>>> +            }
>>>> +            break;
>>>> +        case QEMU_OPT_BOOL:
>>>> +            printf("%s=%s ", desc->name, (opt && opt->str) ? "on" : "off");
>>>> +            break;
>>>> +        case QEMU_OPT_NUMBER:
>>>> +        case QEMU_OPT_SIZE:
>>>> +            if (strcmp(desc->name, "cluster_size")) {
>>>> +                printf("%s=%" PRId64 " ", desc->name,
>>>> +                    (opt && opt->value.uint) ? opt->value.uint : 0);
>>>> +            } else {
>>>> +                printf("%s=%" PRId64 " ", desc->name,
>>>> + (opt && opt->value.uint) ? opt->value.uint : desc->def_value);
>>>> +            }
>>>> +            break;
>>>> +        default:
>>>> +            printf("%s=(unknown type) ", desc->name);
>>>> +            break;
>>>> +        }
>>>> +        desc++;
>>>>       }
>>>> -    fprintf(stderr, "\n");
>>>>       return 0;
>>>>   }
>>>>
>>>
>>> Why do you need to change this function?
>>
>> I just want to have the same output as before, and I noticed that
>> qemu_opts_print function
>> has no user. Is it Okay to change it?
> 
> Can you give examples where unchanged qemu_opts_print() prints something
> else than your code?
> 

After changing is:
[wdongxu]$ qemu-img create -f qcow2 t.qcow2 8G
Formatting 't.qcow2', fmt=qcow2 size=8589934592 encryption=off
cluster_size=65536 lazy_refcounts=off

If use qemu_otps_print directly:
[wdongxu]$ qemu-img create -f qcow2 t.qcow2 8G
qcow2-create-opts: <noid>: size="(null)"
Formatting 't.qcow2', fmt=qcow2

Without this patch, block.c uses print_option_parameters, output is as
the same as my code.

>>>> @@ -1110,3 +847,140 @@ int qemu_opts_foreach(QemuOptsList *list,
>> qemu_opts_loopfunc func, void *opaque,
>>>>       loc_pop(&loc);
>>>>       return rc;
>>>>   }
>>>> +
>>>> +static size_t count_opts_list(QemuOptsList *list)
>>>> +{
>>>> +    size_t i = 0;
>>>> +
>>>> +    while (list && list->desc[i].name) {
>>>> +        i++;
>>>> +    }
>>>> +
>>>> +    return i;
>>>> +}
>>>> +
>>>> +static bool opts_desc_find(QemuOptsList *list, const char *name)
>>>> +{
>>>> +    const QemuOptDesc *desc = list->desc;
>>>> +    int i;
>>>> +
>>>> +    for (i = 0; desc[i].name != NULL; i++) {
>>>> +        if (strcmp(desc[i].name, name) == 0) {
>>>> +            return true;;
>>>
>>> Extra ;
>>>
>>>> +        }
>>>> +    }
>>>> +    return false;
>>>> +}
>>>
>>> Duplicates the loop searching list->desc[] yet again.  What about
>>> factoring out all the copies into a function?  Separate cleanup patch
>>> preceding this one.
>>
>> Okay.
>>
>>>
>>>> +
>>>> +/* merge two QemuOptsLists to one and return it. */
>>>> +QemuOptsList *append_opts_list(QemuOptsList *first,
>>>> +    QemuOptsList *second)
>>>> +{
>>>> +    size_t num_first_options, num_second_options;
>>>> +    QemuOptsList *dest = NULL;
>>>> +    int i = 0;
>>>> +    int index = 0;
>>>> +
>>>> +    num_first_options = count_opts_list(first);
>>>> +    num_second_options = count_opts_list(second);
>>>> +    if (num_first_options + num_second_options == 0) {
>>>> +        return NULL;
>>>> +    }
>>>> +
>>>> +    dest = g_malloc0(sizeof(QemuOptsList)
>>>> +        + (num_first_options + num_second_options) * sizeof(QemuOptDesc));
>>>
>>> Allocate space for both first and second.
>>
>> I just make sure we have enough to hold struct QemuOptsList and
>> QemuOptDesc members.
> 
> Let me correct myself: allocate space for one new QemuOptsList plus
> enough space for the QemuOptDesc in first and second.
> 
>>>> +
>>>> +    if (first) {
>>>> +        memcpy(dest, first, sizeof(QemuOptsList));
>>>> +    } else if (second) {
>>>> +        memcpy(dest, second, sizeof(QemuOptsList));
>>>> +    }
>>>
>>> Copy either first or second.
>>>
>>> If both are non-null, the space for second remains blank.
>>>
>>> Why copy at all?  The loops below copy again, don't they?
>>>
>>
>> struct QemuOptsList has a member  "QemuOptDesc desc[]", what I want to do is:
>>
>> 1) Copy QemuOptsList, excluding "QemuOptDesc desc[]", because it is
>> "flexible array member",
>> does not take any space in QemuOptsList. Then dest will have the same
>> name and implied_opt_name
>> as first QemuoptsList's(if first is null, will be the same as second's).
>>
>> 2) Copy desc member, I have to allocate space for the "flexible array member",
>>   g_strdup name and  help, if it already exists in desc, then skip g_strdup.
>>
>> Am I right?
> 
> Your code confused me.  Let me try again.
> 
> dest->desc is set below.  All the other members of dest get copied from
> first if it's non-null, else from second if it's non-null, else remain
> blank.
> 
> Issues with copying from first or second:
> 
> * Copying dest->head from is evil.  It makes dest look like it was a
>    member of tail queue dest->head.tqh_first, but that's not true.
> 
>    I figure you need to QTAILQ_EMPTY(&dest->head).
> 
> * Copying dest->name isn't quite kosher, either, but probably harmless.
> 
> * Copying dest->implied_opt_name isn't optimal.  What if
>    first->implied_opt_name is null, but second->implied_opt_name is
>    non-null?  Then dest->implied_opt_name = second->implied_opt_name
>    would be nicer.
> 
>    I guess your particular use doesn't care, because all your
>    implied_opt_name are null.
> 
> * dest->merge_lists is fishy, too.  It governs how multiple
>    qemu_opts_create(dest, ...) with the same ID behave.  What should
>    happen if first->merge_lists != second->merge_lists?
> 
>    I guess all your merge_list are false.
> 
> * When both first and second are null, *dest remains blank.  Why is that
>    okay?
> 
> Perhaps a less problematic operation than "merge two QemuOptsLists"
> would be "set QemuOptsList C's desc to the merge of A's and B's desc."
> That way, the caller has to set up all the problematic members of the
> new QemuOptsList.  Then use it to build a special function to create
> options for a pair of BlockDrivers.
> 
Okay, you are right, I did not think about it so carefully. I will
follow your advice, using new name and initializing other struct members
in a new function.

>>>> +
>>>> +    while (first && (first->desc[i].name)) {
>>>> +        if (!opts_desc_find(dest, first->desc[i].name)) {
>>>> +            dest->desc[index].name = strdup(first->desc[i].name);
>>>> +            dest->desc[index].help = strdup(first->desc[i].help);
>>>
>>> g_strdup()
>>>
>>> Why do you need to copy name and help?
> 
> No answer?
>

I want dest->desc has the same members as first plus second, so I copy
name and help, and desc->name will be used in opts_desc_find
function(see below).

>>>> +            dest->desc[index].type = first->desc[i].type;
>>>> +            dest->desc[index].def_value = first->desc[i].def_value;
>>>> +            dest->desc[++index].name = NULL;
>>>
>>> I'd terminate dest->desc[] after the loop, not in every iteration.
>>
>> Okay.
>>
>>>
>>>> +        }
>>>> +        i++;
>>>> +    }
>>>> +    i = 0;
>>>> +    while (second && (second->desc[i].name)) {
>>>> +        if (!opts_desc_find(dest, second->desc[i].name)) {
>>>> +            dest->desc[index].name = strdup(first->desc[i].name);
>>>> +            dest->desc[index].help = strdup(first->desc[i].help);
>>>> +            dest->desc[index].type = second->desc[i].type;
>>>> +            dest->desc[index].def_value = second->desc[i].def_value;
>>>> +            dest->desc[++i].name = NULL;
>>>> +        }
>>>> +        i++;
>>>> +    }
>>>
>>> Please avoid duplicating the loop.
>>>
>>> What if first and second both contain the same parameter name?
>>
>> Then the second will be skipped, I used opts_desc_find to determine
>> whether it already exists or not.
> 
> Ah, missed that, thanks.
> 
> First argument's options take precedence over second's, same as in the
> function this replaces (append_option_parameters()).  Okay.
> 
> Document it in the function comment?
Okay, will add in v2.
> 
>>>> +    return dest;
>>>> +}
>>>> +
>>>> +void free_opts_list(QemuOptsList *list)
>>>> +{
>>>> +    int i = 0;
>>>> +
>>>> +    while (list && list->desc[i].name) {
>>>> +        g_free((char *)list->desc[i].name);
>>>> +        g_free((char *)list->desc[i].help);
>>>> +        i++;
>>>> +    }
>>>> +
>>>> +    g_free(list);
>>>> +}
>>>> +
>>>> +void print_opts_list(QemuOptsList *list)
>>>> +{
>>>> +    int i = 0;
>>>> +    printf("Supported options:\n");
>>>> +    while (list && list->desc[i].name) {
>>>> +        printf("%-16s %s\n", list->desc[i].name,
>>>> + list->desc[i].help ? list->desc[i].help : "No description
>> available");
>>>> +        i++;
>>>> +    }
>>>> +}
>>>> +
>>>> +QemuOpts *parse_opts_list(const char *param,
>>>> +    QemuOptsList *list, QemuOpts *dest)
>>>> +{
>>>> +    char name[256];
>>>> +    char value[256];
>>>> +    char *param_delim, *value_delim;
>>>> +    char next_delim;
>>>> +
>>>> +    if (list == NULL) {
>>>> +        return NULL;
>>>> +    }
>>>> +    while (*param) {
>>>> +        param_delim = strchr(param, ',');
>>>> +        value_delim = strchr(param, '=');
>>>> +
>>>> +        if (value_delim && (value_delim < param_delim || !param_delim)) {
>>>> +            next_delim = '=';
>>>> +        } else {
>>>> +            next_delim = ',';
>>>> +            value_delim = NULL;
>>>> +        }
>>>> +
>>>> +        param = get_opt_name(name, sizeof(name), param, next_delim);
>>>> +        if (value_delim) {
>>>> +            param = get_opt_value(value, sizeof(value), param + 1);
>>>> +        }
>>>> +        if (*param != '\0') {
>>>> +            param++;
>>>> +        }
>>>> +
>>>> +        if (qemu_opt_set(dest, name, value_delim ? value : NULL)) {
>>>> +            goto fail;
>>>> +        }
>>>> +    }
>>>> +
>>>> +    return dest;
>>>> +
>>>> +fail:
>>>> +    return NULL;
>>>> +}
>>>
>>> Can you explain why the existing QemuOpts parsers won't do?
>>
>> I think exsiting parsers is for QEMUOptionParameter, I have not found
>> them for QemuOpts?
> 
> Check out opts_do_parse() and its callers.
> 
Okay.

>>>> diff --git a/qemu-option.h b/qemu-option.h
>>>> index ca72986..75718fe 100644
>>>> --- a/qemu-option.h
>>>> +++ b/qemu-option.h
>>>> @@ -31,24 +31,6 @@
>>>>   #include "error.h"
>>>>   #include "qdict.h"
>>>>
>>>> -enum QEMUOptionParType {
>>>> -    OPT_FLAG,
>>>> -    OPT_NUMBER,
>>>> -    OPT_SIZE,
>>>> -    OPT_STRING,
>>>> -};
>>>> -
>>>> -typedef struct QEMUOptionParameter {
>>>> -    const char *name;
>>>> -    enum QEMUOptionParType type;
>>>> -    union {
>>>> -        uint64_t n;
>>>> -        char* s;
>>>> -    } value;
>>>> -    const char *help;
>>>> -} QEMUOptionParameter;
>>>> -
>>>> -
>>>> const char *get_opt_name(char *buf, int buf_size, const char *p,
>> char delim);
>>>>   const char *get_opt_value(char *buf, int buf_size, const char *p);
>>>>   int get_next_param_value(char *buf, int buf_size,
>>>> @@ -58,27 +40,6 @@ int get_param_value(char *buf, int buf_size,
>>>>   int check_params(char *buf, int buf_size,
>>>>                    const char * const *params, const char *str);
>>>>
>>>> -
>>>> -/*
>>>> - * The following functions take a parameter list as input. This is
>> a pointer to
>>>> - * the first element of a QEMUOptionParameter array which is
>> terminated by an
>>>> - * entry with entry->name == NULL.
>>>> - */
>>>> -
>>>> -QEMUOptionParameter *get_option_parameter(QEMUOptionParameter *list,
>>>> -    const char *name);
>>>> -int set_option_parameter(QEMUOptionParameter *list, const char *name,
>>>> -    const char *value);
>>>> -int set_option_parameter_int(QEMUOptionParameter *list, const char *name,
>>>> -    uint64_t value);
>>>> -QEMUOptionParameter *append_option_parameters(QEMUOptionParameter *dest,
>>>> -    QEMUOptionParameter *list);
>>>> -QEMUOptionParameter *parse_option_parameters(const char *param,
>>>> -    QEMUOptionParameter *list, QEMUOptionParameter *dest);
>>>> -void free_option_parameters(QEMUOptionParameter *list);
>>>> -void print_option_parameters(QEMUOptionParameter *list);
>>>> -void print_option_help(QEMUOptionParameter *list);
>>>> -
>>>>   /* ------------------------------------------------------------------ */
>>>>
>>>>   typedef struct QemuOpt QemuOpt;
>>>> @@ -96,6 +57,7 @@ typedef struct QemuOptDesc {
>>>>       const char *name;
>>>>       enum QemuOptType type;
>>>>       const char *help;
>>>> +    uint64_t def_value;
>>>
>>> The only user I can see is qemu_opts_print(), which can't be right.
>>
>> I want to pass default value, such as QCOW2_DEFAULT_CLUSTER_SIZE, it
>> will be used
>> while qemu-img create, or qcow2_create can not get correct default
>> cluster size. Is it Okay?
> 
> Maybe, but I can't see where def_value gets used, by qemu-img create or
> anything else.  Please point me to the code that uses it.
> 
Yep, it can be removed, I used such code to pass default value:
cluster_size = qemu_opt_get_size(opts, BLOCK_OPT_CLUSTER_SIZE,
DEFAULT_CLUSTER_SIZE);

>>>>   } QemuOptDesc;
>>>>
>>>>   struct QemuOptsList {
>>>> @@ -152,5 +114,10 @@ typedef int (*qemu_opts_loopfunc)(QemuOpts
>> *opts, void *opaque);
>>>>   int qemu_opts_print(QemuOpts *opts, void *dummy);
>>>> int qemu_opts_foreach(QemuOptsList *list, qemu_opts_loopfunc func,
>> void *opaque,
>>>>                         int abort_on_failure);
>>>> -
>>>> +QemuOptsList *append_opts_list(QemuOptsList *dest,
>>>> +    QemuOptsList *list);
>>>> +void free_opts_list(QemuOptsList *list);
>>>> +void print_opts_list(QemuOptsList *list);
>>>> +QemuOpts *parse_opts_list(const char *param,
>>>> +    QemuOptsList *list, QemuOpts *dest);
>>>>   #endif
>>>
>>
>> Thank you Markus for you so detail comments, :)
> 
> You're welcome.
> 
>> Luiz, I think I need to use your 1-3 patchs in your series.
>> http://lists.nongnu.org/archive/html/qemu-devel/2012-07/msg02716.html
> 

Thank you Markus.