δΊ 2013-1-25 2:59, Markus Armbruster ει:
> Dong Xu Wang <wdon...@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 <wdon...@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().
>
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.
>
>
