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

[Markus Armbruster]

Dong Xu Wang <address@hidden> 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.

> On Fri, Sep 7, 2012 at 4:42 PM, Markus Armbruster <address@hidden> 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 <address@hidden> 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?

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

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

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

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

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

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