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

[Dong Xu Wang]

于 2013-1-25 2:59, Markus Armbruster 写道:
> 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().
> 
Okay, will fix.

>>   #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.
> 
Okay, will fix.
>> +
>> +    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.
> 
Okay.

> Please wrap your lines a bit earlier.  Column 70 is a good limit.
> 
Okay, will use 70 column next version.
>> +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?
> 
Yes, without this the code below can 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.
> 
Okay.

>> +    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.
Okay.
> 
>> +    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.
> 
Okay.
>> +       }
>> +        i++;
> 
> Indentation's off.
> 
Yes, will fix.

>> +    }
>> +    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.
> 
Okay.
>> +    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.
> 
Okay.
>> +
>> +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");
> 
>> +        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.
> 
>