Re: [Qemu-devel] [PATCH V11 2/4] Create four opts list related functions

[Blue Swirl]

On Thu, Jan 24, 2013 at 6:59 PM, Markus Armbruster <address@hidden> wrote:
> Dong Xu Wang <address@hidden> writes:
>
>> This patch will create 4 functions, count_opts_list, append_opts_list,
>> free_opts_list and print_opts_list, they will used in following commits.
>>
>> Signed-off-by: Dong Xu Wang <address@hidden>
>> ---
>> v6->v7):
>> 1) Fix typo.
>>
>> v5->v6):
>> 1) allocate enough space in append_opts_list function.
>>
>>  include/qemu/option.h |  4 +++
>>  util/qemu-option.c    | 90 
>> +++++++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 94 insertions(+)
>>
>> diff --git a/include/qemu/option.h b/include/qemu/option.h
>> index 394170a..f784c2e 100644
>> --- a/include/qemu/option.h
>> +++ b/include/qemu/option.h
>> @@ -156,4 +156,8 @@ 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);
>
> Please stick to the existing naming convention: qemu_opts_append(),
> qemu_opts_free(), qemu_opts_print().
>
>>  #endif
>> diff --git a/util/qemu-option.c b/util/qemu-option.c
>> index 1aed418..f4bbbf8 100644
>> --- a/util/qemu-option.c
>> +++ b/util/qemu-option.c
>> @@ -1152,3 +1152,93 @@ 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++;
>> +    }
>
> Please don't invent your own way to interate over list->desc[]!  Imitate
> the existing code.
>
>     for (i = 0; list && list->desc[i].name; i++) ;
>
> Same several times below.
>
>> +
>> +    return i;
>> +}
>> +
>> +/* Create a new QemuOptsList and make its desc to the merge of first and 
>> second.
>> + * It will allocate space for one new QemuOptsList plus enough space for
>> + * QemuOptDesc in first and second QemuOptsList. First argument's 
>> QemuOptDesc
>> + * members take precedence over second's.
>> + */
>
> Unlike most functions dealing with QemuOptsLists, this one can take null
> arguments.  Worth mentioning.
>
> Please wrap your lines a bit earlier.  Column 70 is a good limit.
>
>> +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;
>> +    }
>
> Why do you need this extra case?  Shouldn't the code below just work?
>
>> +
>> +    dest = g_malloc0(sizeof(QemuOptsList)
>> +        + (num_first_options + num_second_options + 1) * 
>> sizeof(QemuOptDesc));
>> +
>> +    dest->name = "append_opts_list";
>> +    dest->implied_opt_name = NULL;
>
> Not copied from an argument.  Tolerable, because all you lose is the
> convenience to omit name= in one place, but worth mentioning in the
> function comment.
>
>> +    dest->merge_lists = false;
>
> Not copied from an argument.  I'm afraid the result will make no sense
> if either argument has it set.  Consider asserting they don't, and
> documenting the requirement in the function comment.
>
>> +    QTAILQ_INIT(&dest->head);
>> +    while (first && (first->desc[i].name)) {
>> +        if (!find_desc_by_name(dest->desc, first->desc[i].name)) {
>> +            dest->desc[index].name = g_strdup(first->desc[i].name);
>> +            dest->desc[index].help = g_strdup(first->desc[i].help);
>> +            dest->desc[index].type = first->desc[i].type;
>> +            dest->desc[index].def_value_str =
>> +                g_strdup(first->desc[i].def_value_str);
>> +            ++index;
>
> index++ please, for consistency with the similar increment two lines
> below.
>
>> +       }
>> +        i++;
>
> Indentation's off.
>
>> +    }
>> +    i = 0;
>> +    while (second && (second->desc[i].name)) {
>> +        if (!find_desc_by_name(dest->desc, second->desc[i].name)) {
>> +            dest->desc[index].name = g_strdup(first->desc[i].name);
>> +            dest->desc[index].help = g_strdup(first->desc[i].help);
>> +            dest->desc[index].type = second->desc[i].type;
>> +            dest->desc[index].def_value_str =
>> +                g_strdup(second->desc[i].def_value_str);
>> +            ++index;
>> +        }
>> +        i++;
>> +    }
>
> We've seen this loop before.  Please avoid the code duplication, as I
> asked you before.
>
>> +    dest->desc[index].name = NULL;
>> +    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);
>> +        g_free((char *)list->desc[i].def_value_str);
>> +        i++;
>> +    }
>> +
>> +    g_free(list);
>> +}
>
> Unlike most functions dealing with QemuOptsLists, this one can take null
> arguments.  Makes sense, but it's worth mentioning in a function
> comment.
>
>> +
>> +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");
>
> Clearer:
>
>                   list->desc[i].help ?: "No description available");

It's a GNU extension with very little chance of becoming standard. I
don't think it should be used.

In this case, if you want to avoid typing (and readers reading,
possibly wondering if they are identical in all cases) list->desc[i]
several times, introduce a pointer variable as a shortcut.

>
>> +        i++;
>> +    }
>> +}
>
> Unlike most functions dealing with QemuOptsLists, this one can take null
> arguments.  Later patches feed it the result of append_opts_list(),
> which can be null, but that makes little sense to me.
>