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

[Mark Wu]

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 <address@hidden>
> > ---
> > 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 <address@hidden>