* Re: [Qemu-devel] [PATCH V11 2/4] Create four opts list related functions
[not found] ` <CAAu8pHvVnRSgiHnErmjyB33OfqdTU-LidVxt9FmBgd+_KKag8g@mail.gmail.com>
@ 2013-02-01 7:55 ` Dong Xu Wang
0 siblings, 0 replies; only message in thread
From: Dong Xu Wang @ 2013-02-01 7:55 UTC (permalink / raw)
To: qemu-devel; +Cc: Blue Swirl, Dong Xu Wang, stefanha, Markus Armbruster, kwolf
于 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. :)
^ permalink raw reply [flat|nested] only message in thread