Commit Message

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)
Signed-off-by: Mark Wu <wudxw@linux.vnet.ibm.com>
---
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(-)

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 <wudxw@linux.vnet.ibm.com>> ---> 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;
}
> -struct GuestAgentInfo *qmp_guest_info(Error **err)> +static void qmp_command_info(QmpCommand *cmd, void *opaque)> {> - GuestAgentInfo *info = g_malloc0(sizeof(GuestAgentInfo));> + GuestAgentInfo *info = (GuestAgentInfo *)opaque;
This is C, not C++. The cast is not necessary.
> - cmd_info->enabled = qmp_command_is_enabled(cmd_info->name);> > - cmd_info_list = g_malloc0(sizeof(GuestAgentCommandInfoList));> - cmd_info_list->value = cmd_info;> - cmd_info_list->next = info->supported_commands;> - info->supported_commands = cmd_info_list;> + cmd_info = g_malloc0(sizeof(GuestAgentCommandInfo));> + cmd_info->name = g_strdup(cmd->name);> + cmd_info->enabled = 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 = (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 <eblake@redhat.com>