Re: [Qemu-devel] [PATCH V11 2/4] Create four opts list related functions
于 2013-1-26 2:13, Blue Swirl 写道: On Thu, Jan 24, 2013 at 6:59 PM, Markus Armbruster wrote: Dong Xu Wang 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 --- 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 mentioni
Re: [Qemu-devel] [PATCH V11 2/4] Create four opts list related functions
于 2013-1-25 2:59, Markus Armbruster 写道: > Dong Xu Wang 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 >> --- >> 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 befor
Re: [Qemu-devel] [PATCH V11 2/4] Create four opts list related functions
On Thu, Jan 24, 2013 at 6:59 PM, Markus Armbruster wrote: > Dong Xu Wang 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 >> --- >> 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 &&
Re: [Qemu-devel] [PATCH V11 2/4] Create four opts list related functions
Dong Xu Wang 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 > --- > 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 mo
[Qemu-devel] [PATCH V11 2/4] Create four opts list related functions
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 --- 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); #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++; +} + +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. + */ +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; +} + +dest = g_malloc0(sizeof(QemuOptsList) ++ (num_first_options + num_second_options + 1) * sizeof(QemuOptDesc)); + +dest->name = "append_opts_list"; +dest->implied_opt_name = NULL; +dest->merge_lists = false; +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; + } +i++; +} +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++; +} +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); +} + +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"); +i++; +} +} -- 1.7.11.7