Re: [Qemu-devel] [PATCH v3] Add interface to traverse the qmp command list by QmpCommand

[Mark Wu <wudxw@linux.vnet.ibm.com>]

Thanks a lot for your all kind and helpful comments.  I would fix it by 
myself to save the time of maintainer :)
Then I have to beg your another review.    BTW,  I would like to ask if 
there's good tools or practice to perform
an incremental review in qemu patch works.

Thanks.
Mark.


On Wed 09 Oct 2013 10:58:04 AM CST, Eric Blake wrote:
> On 10/08/2013 08:39 PM, Mark Wu wrote:
>> In the original code, qmp_get_command_list is used to construct
>> a list of all commands' name. To get the information of all qga
>> commands, it traverses the name list and search the command info
>> with its name.  So it can cause O(n^2) in the number of commands.
>>
>> This patch adds an interface to traverse the qmp command list by
>> QmpCommand to replace qmp_get_command_list. It can decrease the
>> complexity from O(n^2) to O(n).
>>
>> Signed-off-by: Mark Wu <wudxw@linux.vnet.ibm.com>
>> ---
>> v3:
>>      Add an accessor for cmd->name to avoid exposing internals of QmpCommand
>
> As your two patches are related, I'd send them threaded under one cover
> letter if you have to respin.  But maybe we don't need that...
>
>> v2:
>> 	1. Keep the signature of qmp_command_is_enabled (per Eric and Michael)
>>      2. Remove the unnecessary pointer castings (per Eric)
>>
>>   include/qapi/qmp/dispatch.h |  6 ++--
>>   qapi/qmp-registry.c         | 30 +++++-------------
>>   qga/commands.c              | 38 +++++++++--------------
>>   qga/main.c                  | 75 ++++++++++++++++++---------------------------
>>   4 files changed, 57 insertions(+), 92 deletions(-)
>>
>
>> -struct GuestAgentInfo *qmp_guest_info(Error **err)
>> +static void qmp_command_info(QmpCommand *cmd, void *opaque)
>>   {
>
>> -    while (*cmd_list) {
>> -        cmd_info = g_malloc0(sizeof(GuestAgentCommandInfo));
>> -        cmd_info->name = g_strdup(*cmd_list);
>> -        cmd_info->enabled = qmp_command_is_enabled(cmd_info->name);
>> +    cmd_info = g_malloc0(sizeof(GuestAgentCommandInfo));
>> +    cmd_info->name = g_strdup(cmd->name);
>
> Oops, not using qmp_command_name().
>
> Seems minor enough that if it is the only change after this much churn,
> and the maintainer is willing to fix it on your behalf, I could live with:
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>