From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:55955) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1U1BUD-0002Jq-1R for qemu-devel@nongnu.org; Fri, 01 Feb 2013 02:56:38 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1U1BU7-0007mF-T0 for qemu-devel@nongnu.org; Fri, 01 Feb 2013 02:56:36 -0500 Received: from e28smtp02.in.ibm.com ([122.248.162.2]:39184) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1U1BU7-0007lr-1S for qemu-devel@nongnu.org; Fri, 01 Feb 2013 02:56:31 -0500 Received: from /spool/local by e28smtp02.in.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 1 Feb 2013 13:24:01 +0530 Received: from d28relay03.in.ibm.com (d28relay03.in.ibm.com [9.184.220.60]) by d28dlp02.in.ibm.com (Postfix) with ESMTP id AC270394004E for ; Fri, 1 Feb 2013 13:26:24 +0530 (IST) Received: from d28av03.in.ibm.com (d28av03.in.ibm.com [9.184.220.65]) by d28relay03.in.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r117uL9a21299296 for ; Fri, 1 Feb 2013 13:26:21 +0530 Received: from d28av03.in.ibm.com (loopback [127.0.0.1]) by d28av03.in.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id r117uNBK013127 for ; Fri, 1 Feb 2013 18:56:23 +1100 Message-ID: <510B7501.6090601@linux.vnet.ibm.com> Date: Fri, 01 Feb 2013 15:55:45 +0800 From: Dong Xu Wang MIME-Version: 1.0 References: <1359022987-6274-1-git-send-email-wdongxu@vnet.linux.ibm.com> <1359022987-6274-3-git-send-email-wdongxu@vnet.linux.ibm.com> <87a9ryquf6.fsf@blackfin.pond.sub.org> In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH V11 2/4] Create four opts list related functions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org Cc: Blue Swirl , Dong Xu Wang , stefanha@redhat.com, Markus Armbruster , kwolf@redhat.com 于 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 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. :)