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().
> #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");
> + 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.
