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

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

于 2013-1-26 2:13, Blue Swirl 写道:
> On Thu, Jan 24, 2013 at 6:59 PM, Markus Armbruster <armbru@redhat.com> wrote:
>> Dong Xu Wang <wdongxu@vnet.linux.ibm.com> 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 <wdongxu@vnet.linux.ibm.com>
>>> ---
>>> 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.
>>
>
>

Hi, I am working with V12, but did not test completely, I will take 
Chinese Spring Festival vocation next 2 weeks, I will continue once I am 
back. :)