From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36249) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VPaZw-0005Mf-5c for Qemu-devel@nongnu.org; Fri, 27 Sep 2013 12:07:45 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VPaZq-0000tI-Jx for Qemu-devel@nongnu.org; Fri, 27 Sep 2013 12:07:40 -0400 Received: from mail-oa0-x22d.google.com ([2607:f8b0:4003:c02::22d]:52745) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VPaZq-0000ss-Er for Qemu-devel@nongnu.org; Fri, 27 Sep 2013 12:07:34 -0400 Received: by mail-oa0-f45.google.com with SMTP id o17so2184754oag.4 for ; Fri, 27 Sep 2013 09:07:31 -0700 (PDT) Sender: fluxion Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable From: Michael Roth In-Reply-To: <5243A2A3.2000502@redhat.com> References: <1380160599-15659-1-git-send-email-wudxw@linux.vnet.ibm.com> <5243A2A3.2000502@redhat.com> Message-ID: <20130927160726.20300.75624@loki> Date: Fri, 27 Sep 2013 11:07:26 -0500 Subject: Re: [Qemu-devel] [PATCH] Add interface to traverse the qmp command list by QmpCommand List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake , Mark Wu Cc: Qemu-devel@nongnu.org, Luiz Capitulino Quoting Eric Blake (2013-09-25 21:57:39) > On 09/25/2013 07:56 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) to O(n^2) > = > from O(n^2) to O(n) > = > > = > > Signed-off-by: Mark Wu > > --- > > include/qapi/qmp/dispatch.h | 3 +- > > qapi/qmp-registry.c | 28 ++----------------- > > qga/commands.c | 39 ++++++++++---------------- > > qga/main.c | 68 +++++++++++++++++--------------------= -------- > > 4 files changed, 45 insertions(+), 93 deletions(-) > > = > = > > = > > -bool qmp_command_is_enabled(const char *name) > = > I think one of Michael's suggestions was that you may still want > qmp_command_is_enabled, but with a new signature: > = > bool qmp_command_is_enabled(const QmpCommand *cmd) > { > return cmd->enabled; > } That would still be my preference, as I can see the underlying fields changing somewhat in a future where QMP starts using the same infrastructure, and new users come along like qemu-ga guest->host commands we did as part of GSoC. Wouldn't hold things up in the meantime, but if we're re-spinning anyway let's maintain the precedence of using getter/setter funcs for QmpCommand. > = > > -struct GuestAgentInfo *qmp_guest_info(Error **err) > > +static void qmp_command_info(QmpCommand *cmd, void *opaque) > > { > > - GuestAgentInfo *info =3D g_malloc0(sizeof(GuestAgentInfo)); > > + GuestAgentInfo *info =3D (GuestAgentInfo *)opaque; = > = > = > > - cmd_info->enabled =3D qmp_command_is_enabled(cmd_info->name); > > = > > - cmd_info_list =3D g_malloc0(sizeof(GuestAgentCommandInfoList)); > > - cmd_info_list->value =3D cmd_info; > > - cmd_info_list->next =3D info->supported_commands; > > - info->supported_commands =3D cmd_info_list; > > + cmd_info =3D g_malloc0(sizeof(GuestAgentCommandInfo)); > > + cmd_info->name =3D g_strdup(cmd->name); > > + cmd_info->enabled =3D cmd->enabled; > = > I guess it all depends on whether we want QmpCommand to be an opaque > type outside of a single file. But I don't have a strong argument for > making it opaque, and your approach works if we don't mind exposing the > details of QmpCommand across multiple files. So I can live with your > patch as-is. > = > = > > +static void ga_enable_non_blacklisted(QmpCommand *cmd, void *opaque) > > { > = > > + GList *blacklist =3D (GList *)opaque; > = > Again, the cast is not necessary. > = > Overall, I like the patch. Just a few tweaks suggested, but I can live > with you adding: > = > Reviewed-by: Eric Blake > = > -- = > Eric Blake eblake redhat com +1-919-301-3266 > Libvirt virtualization library http://libvirt.org