* [Qemu-devel] [PATCH] Add interface to traverse the qmp command list by QmpCommand
@ 2013-09-26 1:56 Mark Wu
2013-09-26 2:36 ` Mark Wu
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Mark Wu @ 2013-09-26 1:56 UTC (permalink / raw)
To: Qemu-devel; +Cc: Mark Wu, Michael Roth, Luiz Capitulino
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(-)
diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
index 1ce11f5..fb174f6 100644
--- a/include/qapi/qmp/dispatch.h
+++ b/include/qapi/qmp/dispatch.h
@@ -48,8 +48,9 @@ QObject *qmp_dispatch(QObject *request);
void qmp_disable_command(const char *name);
void qmp_enable_command(const char *name);
bool qmp_command_is_enabled(const char *name);
-char **qmp_get_command_list(void);
QObject *qmp_build_error_object(Error *errp);
+typedef void (*qmp_cmd_callback_fn)(QmpCommand *cmd, void *opaque);
+void qmp_for_each_command(qmp_cmd_callback_fn fn, void *opaque);
#endif
diff --git a/qapi/qmp-registry.c b/qapi/qmp-registry.c
index 28bbbe8..844d597 100644
--- a/qapi/qmp-registry.c
+++ b/qapi/qmp-registry.c
@@ -66,35 +66,11 @@ void qmp_enable_command(const char *name)
qmp_toggle_command(name, true);
}
-bool qmp_command_is_enabled(const char *name)
+void qmp_for_each_command(qmp_cmd_callback_fn fn, void *opaque)
{
QmpCommand *cmd;
QTAILQ_FOREACH(cmd, &qmp_commands, node) {
- if (strcmp(cmd->name, name) == 0) {
- return cmd->enabled;
- }
+ fn(cmd, opaque);
}
-
- return false;
-}
-
-char **qmp_get_command_list(void)
-{
- QmpCommand *cmd;
- int count = 1;
- char **list_head, **list;
-
- QTAILQ_FOREACH(cmd, &qmp_commands, node) {
- count++;
- }
-
- list_head = list = g_malloc0(count * sizeof(char *));
-
- QTAILQ_FOREACH(cmd, &qmp_commands, node) {
- *list = g_strdup(cmd->name);
- list++;
- }
-
- return list_head;
}
diff --git a/qga/commands.c b/qga/commands.c
index 528b082..602cd47 100644
--- a/qga/commands.c
+++ b/qga/commands.c
@@ -45,35 +45,26 @@ void qmp_guest_ping(Error **err)
slog("guest-ping called");
}
-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;
GuestAgentCommandInfo *cmd_info;
GuestAgentCommandInfoList *cmd_info_list;
- char **cmd_list_head, **cmd_list;
-
- info->version = g_strdup(QEMU_VERSION);
-
- cmd_list_head = cmd_list = qmp_get_command_list();
- if (*cmd_list_head == NULL) {
- goto out;
- }
-
- 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_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;
+ 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;
+}
- g_free(*cmd_list);
- cmd_list++;
- }
+struct GuestAgentInfo *qmp_guest_info(Error **err)
+{
+ GuestAgentInfo *info = g_malloc0(sizeof(GuestAgentInfo));
-out:
- g_free(cmd_list_head);
+ info->version = g_strdup(QEMU_VERSION);
+ qmp_for_each_command(qmp_command_info, info);
return info;
}
diff --git a/qga/main.c b/qga/main.c
index 6c746c8..1741d3f 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -347,48 +347,34 @@ static gint ga_strcmp(gconstpointer str1, gconstpointer str2)
}
/* disable commands that aren't safe for fsfreeze */
-static void ga_disable_non_whitelisted(void)
+static void ga_disable_non_whitelisted(QmpCommand *cmd, void *opaque)
{
- char **list_head, **list;
bool whitelisted;
int i;
- list_head = list = qmp_get_command_list();
- while (*list != NULL) {
- whitelisted = false;
- i = 0;
- while (ga_freeze_whitelist[i] != NULL) {
- if (strcmp(*list, ga_freeze_whitelist[i]) == 0) {
- whitelisted = true;
- }
- i++;
- }
- if (!whitelisted) {
- g_debug("disabling command: %s", *list);
- qmp_disable_command(*list);
+ whitelisted = false;
+ i = 0;
+ while (ga_freeze_whitelist[i] != NULL) {
+ if (strcmp(cmd->name, ga_freeze_whitelist[i]) == 0) {
+ whitelisted = true;
}
- g_free(*list);
- list++;
+ i++;
+ }
+ if (!whitelisted) {
+ g_debug("disabling command: %s", cmd->name);
+ qmp_disable_command(cmd->name);
}
- g_free(list_head);
}
/* [re-]enable all commands, except those explicitly blacklisted by user */
-static void ga_enable_non_blacklisted(GList *blacklist)
+static void ga_enable_non_blacklisted(QmpCommand *cmd, void *opaque)
{
- char **list_head, **list;
-
- list_head = list = qmp_get_command_list();
- while (*list != NULL) {
- if (g_list_find_custom(blacklist, *list, ga_strcmp) == NULL &&
- !qmp_command_is_enabled(*list)) {
- g_debug("enabling command: %s", *list);
- qmp_enable_command(*list);
- }
- g_free(*list);
- list++;
+ GList *blacklist = (GList *)opaque;
+ if (g_list_find_custom(blacklist, cmd->name, ga_strcmp) == NULL &&
+ !cmd->enabled) {
+ g_debug("enabling command: %s", cmd->name);
+ qmp_enable_command(cmd->name);
}
- g_free(list_head);
}
static bool ga_create_file(const char *path)
@@ -424,7 +410,7 @@ void ga_set_frozen(GAState *s)
return;
}
/* disable all non-whitelisted (for frozen state) commands */
- ga_disable_non_whitelisted();
+ qmp_for_each_command(ga_disable_non_whitelisted, NULL);
g_warning("disabling logging due to filesystem freeze");
ga_disable_logging(s);
s->frozen = true;
@@ -460,7 +446,7 @@ void ga_unset_frozen(GAState *s)
}
/* enable all disabled, non-blacklisted commands */
- ga_enable_non_blacklisted(s->blacklist);
+ qmp_for_each_command(ga_enable_non_blacklisted, s->blacklist);
s->frozen = false;
if (!ga_delete_file(s->state_filepath_isfrozen)) {
g_warning("unable to delete %s, fsfreeze may not function properly",
@@ -920,6 +906,11 @@ int64_t ga_get_fd_handle(GAState *s, Error **errp)
return handle;
}
+static void ga_print_cmd(QmpCommand *cmd, void *opaque)
+{
+ printf("%s\n", cmd->name);
+}
+
int main(int argc, char **argv)
{
const char *sopt = "hVvdm:p:l:f:F::b:s:t:";
@@ -996,15 +987,8 @@ int main(int argc, char **argv)
daemonize = 1;
break;
case 'b': {
- char **list_head, **list;
if (is_help_option(optarg)) {
- list_head = list = qmp_get_command_list();
- while (*list != NULL) {
- printf("%s\n", *list);
- g_free(*list);
- list++;
- }
- g_free(list_head);
+ qmp_for_each_command(ga_print_cmd, NULL);
return 0;
}
for (j = 0, i = 0, len = strlen(optarg); i < len; i++) {
@@ -1126,7 +1110,7 @@ int main(int argc, char **argv)
s->deferred_options.log_filepath = log_filepath;
}
ga_disable_logging(s);
- ga_disable_non_whitelisted();
+ qmp_for_each_command(ga_disable_non_whitelisted, NULL);
} else {
if (daemonize) {
become_daemon(pid_filepath);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [Qemu-devel] [PATCH] Add interface to traverse the qmp command list by QmpCommand
2013-09-26 1:56 [Qemu-devel] [PATCH] Add interface to traverse the qmp command list by QmpCommand Mark Wu
@ 2013-09-26 2:36 ` Mark Wu
2013-09-26 2:57 ` Eric Blake
2013-09-27 16:16 ` Michael Roth
2 siblings, 0 replies; 5+ messages in thread
From: Mark Wu @ 2013-09-26 2:36 UTC (permalink / raw)
To: Mark Wu; +Cc: qemu-devel, Michael Roth, Luiz Capitulino
It's verified by the following tests:
1. run ./qemu-ga -b help,
it print a list of qga command names as expected
2. run ./guest-sync -b guest-ping, guest-sync ....
command 'guest-info' show that guest-ping and guest-sync are disabled
On Thu 26 Sep 2013 09:56:39 AM CST, 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)
>
> 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(-)
>
> diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
> index 1ce11f5..fb174f6 100644
> --- a/include/qapi/qmp/dispatch.h
> +++ b/include/qapi/qmp/dispatch.h
> @@ -48,8 +48,9 @@ QObject *qmp_dispatch(QObject *request);
> void qmp_disable_command(const char *name);
> void qmp_enable_command(const char *name);
> bool qmp_command_is_enabled(const char *name);
> -char **qmp_get_command_list(void);
> QObject *qmp_build_error_object(Error *errp);
> +typedef void (*qmp_cmd_callback_fn)(QmpCommand *cmd, void *opaque);
> +void qmp_for_each_command(qmp_cmd_callback_fn fn, void *opaque);
>
> #endif
>
> diff --git a/qapi/qmp-registry.c b/qapi/qmp-registry.c
> index 28bbbe8..844d597 100644
> --- a/qapi/qmp-registry.c
> +++ b/qapi/qmp-registry.c
> @@ -66,35 +66,11 @@ void qmp_enable_command(const char *name)
> qmp_toggle_command(name, true);
> }
>
> -bool qmp_command_is_enabled(const char *name)
> +void qmp_for_each_command(qmp_cmd_callback_fn fn, void *opaque)
> {
> QmpCommand *cmd;
>
> QTAILQ_FOREACH(cmd, &qmp_commands, node) {
> - if (strcmp(cmd->name, name) == 0) {
> - return cmd->enabled;
> - }
> + fn(cmd, opaque);
> }
> -
> - return false;
> -}
> -
> -char **qmp_get_command_list(void)
> -{
> - QmpCommand *cmd;
> - int count = 1;
> - char **list_head, **list;
> -
> - QTAILQ_FOREACH(cmd, &qmp_commands, node) {
> - count++;
> - }
> -
> - list_head = list = g_malloc0(count * sizeof(char *));
> -
> - QTAILQ_FOREACH(cmd, &qmp_commands, node) {
> - *list = g_strdup(cmd->name);
> - list++;
> - }
> -
> - return list_head;
> }
> diff --git a/qga/commands.c b/qga/commands.c
> index 528b082..602cd47 100644
> --- a/qga/commands.c
> +++ b/qga/commands.c
> @@ -45,35 +45,26 @@ void qmp_guest_ping(Error **err)
> slog("guest-ping called");
> }
>
> -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;
> GuestAgentCommandInfo *cmd_info;
> GuestAgentCommandInfoList *cmd_info_list;
> - char **cmd_list_head, **cmd_list;
> -
> - info->version = g_strdup(QEMU_VERSION);
> -
> - cmd_list_head = cmd_list = qmp_get_command_list();
> - if (*cmd_list_head == NULL) {
> - goto out;
> - }
> -
> - 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_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;
> + 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;
> +}
>
> - g_free(*cmd_list);
> - cmd_list++;
> - }
> +struct GuestAgentInfo *qmp_guest_info(Error **err)
> +{
> + GuestAgentInfo *info = g_malloc0(sizeof(GuestAgentInfo));
>
> -out:
> - g_free(cmd_list_head);
> + info->version = g_strdup(QEMU_VERSION);
> + qmp_for_each_command(qmp_command_info, info);
> return info;
> }
> diff --git a/qga/main.c b/qga/main.c
> index 6c746c8..1741d3f 100644
> --- a/qga/main.c
> +++ b/qga/main.c
> @@ -347,48 +347,34 @@ static gint ga_strcmp(gconstpointer str1, gconstpointer str2)
> }
>
> /* disable commands that aren't safe for fsfreeze */
> -static void ga_disable_non_whitelisted(void)
> +static void ga_disable_non_whitelisted(QmpCommand *cmd, void *opaque)
> {
> - char **list_head, **list;
> bool whitelisted;
> int i;
>
> - list_head = list = qmp_get_command_list();
> - while (*list != NULL) {
> - whitelisted = false;
> - i = 0;
> - while (ga_freeze_whitelist[i] != NULL) {
> - if (strcmp(*list, ga_freeze_whitelist[i]) == 0) {
> - whitelisted = true;
> - }
> - i++;
> - }
> - if (!whitelisted) {
> - g_debug("disabling command: %s", *list);
> - qmp_disable_command(*list);
> + whitelisted = false;
> + i = 0;
> + while (ga_freeze_whitelist[i] != NULL) {
> + if (strcmp(cmd->name, ga_freeze_whitelist[i]) == 0) {
> + whitelisted = true;
> }
> - g_free(*list);
> - list++;
> + i++;
> + }
> + if (!whitelisted) {
> + g_debug("disabling command: %s", cmd->name);
> + qmp_disable_command(cmd->name);
> }
> - g_free(list_head);
> }
>
> /* [re-]enable all commands, except those explicitly blacklisted by user */
> -static void ga_enable_non_blacklisted(GList *blacklist)
> +static void ga_enable_non_blacklisted(QmpCommand *cmd, void *opaque)
> {
> - char **list_head, **list;
> -
> - list_head = list = qmp_get_command_list();
> - while (*list != NULL) {
> - if (g_list_find_custom(blacklist, *list, ga_strcmp) == NULL &&
> - !qmp_command_is_enabled(*list)) {
> - g_debug("enabling command: %s", *list);
> - qmp_enable_command(*list);
> - }
> - g_free(*list);
> - list++;
> + GList *blacklist = (GList *)opaque;
> + if (g_list_find_custom(blacklist, cmd->name, ga_strcmp) == NULL &&
> + !cmd->enabled) {
> + g_debug("enabling command: %s", cmd->name);
> + qmp_enable_command(cmd->name);
> }
> - g_free(list_head);
> }
>
> static bool ga_create_file(const char *path)
> @@ -424,7 +410,7 @@ void ga_set_frozen(GAState *s)
> return;
> }
> /* disable all non-whitelisted (for frozen state) commands */
> - ga_disable_non_whitelisted();
> + qmp_for_each_command(ga_disable_non_whitelisted, NULL);
> g_warning("disabling logging due to filesystem freeze");
> ga_disable_logging(s);
> s->frozen = true;
> @@ -460,7 +446,7 @@ void ga_unset_frozen(GAState *s)
> }
>
> /* enable all disabled, non-blacklisted commands */
> - ga_enable_non_blacklisted(s->blacklist);
> + qmp_for_each_command(ga_enable_non_blacklisted, s->blacklist);
> s->frozen = false;
> if (!ga_delete_file(s->state_filepath_isfrozen)) {
> g_warning("unable to delete %s, fsfreeze may not function properly",
> @@ -920,6 +906,11 @@ int64_t ga_get_fd_handle(GAState *s, Error **errp)
> return handle;
> }
>
> +static void ga_print_cmd(QmpCommand *cmd, void *opaque)
> +{
> + printf("%s\n", cmd->name);
> +}
> +
> int main(int argc, char **argv)
> {
> const char *sopt = "hVvdm:p:l:f:F::b:s:t:";
> @@ -996,15 +987,8 @@ int main(int argc, char **argv)
> daemonize = 1;
> break;
> case 'b': {
> - char **list_head, **list;
> if (is_help_option(optarg)) {
> - list_head = list = qmp_get_command_list();
> - while (*list != NULL) {
> - printf("%s\n", *list);
> - g_free(*list);
> - list++;
> - }
> - g_free(list_head);
> + qmp_for_each_command(ga_print_cmd, NULL);
> return 0;
> }
> for (j = 0, i = 0, len = strlen(optarg); i < len; i++) {
> @@ -1126,7 +1110,7 @@ int main(int argc, char **argv)
> s->deferred_options.log_filepath = log_filepath;
> }
> ga_disable_logging(s);
> - ga_disable_non_whitelisted();
> + qmp_for_each_command(ga_disable_non_whitelisted, NULL);
> } else {
> if (daemonize) {
> become_daemon(pid_filepath);
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [Qemu-devel] [PATCH] Add interface to traverse the qmp command list by QmpCommand
2013-09-26 1:56 [Qemu-devel] [PATCH] Add interface to traverse the qmp command list by QmpCommand Mark Wu
2013-09-26 2:36 ` Mark Wu
@ 2013-09-26 2:57 ` Eric Blake
2013-09-27 16:07 ` Michael Roth
2013-09-27 16:16 ` Michael Roth
2 siblings, 1 reply; 5+ messages in thread
From: Eric Blake @ 2013-09-26 2:57 UTC (permalink / raw)
To: Mark Wu; +Cc: Michael Roth, Qemu-devel, Luiz Capitulino
[-- Attachment #1: Type: text/plain, Size: 2621 bytes --]
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>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [Qemu-devel] [PATCH] Add interface to traverse the qmp command list by QmpCommand
2013-09-26 2:57 ` Eric Blake
@ 2013-09-27 16:07 ` Michael Roth
0 siblings, 0 replies; 5+ messages in thread
From: Michael Roth @ 2013-09-27 16:07 UTC (permalink / raw)
To: Eric Blake, Mark Wu; +Cc: Qemu-devel, 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 <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;
> }
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 = g_malloc0(sizeof(GuestAgentInfo));
> > + GuestAgentInfo *info = (GuestAgentInfo *)opaque;
>
>
> > - 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>
>
> --
> Eric Blake eblake redhat com +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] Add interface to traverse the qmp command list by QmpCommand
2013-09-26 1:56 [Qemu-devel] [PATCH] Add interface to traverse the qmp command list by QmpCommand Mark Wu
2013-09-26 2:36 ` Mark Wu
2013-09-26 2:57 ` Eric Blake
@ 2013-09-27 16:16 ` Michael Roth
2 siblings, 0 replies; 5+ messages in thread
From: Michael Roth @ 2013-09-27 16:16 UTC (permalink / raw)
To: Mark Wu, Qemu-devel; +Cc: Luiz Capitulino
Quoting Mark Wu (2013-09-25 20:56:39)
> 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>
Thanks, the qmp_for_each_command stuff is a nice cleanup.
Looks good other than previous comments.
> ---
> 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(-)
>
> diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
> index 1ce11f5..fb174f6 100644
> --- a/include/qapi/qmp/dispatch.h
> +++ b/include/qapi/qmp/dispatch.h
> @@ -48,8 +48,9 @@ QObject *qmp_dispatch(QObject *request);
> void qmp_disable_command(const char *name);
> void qmp_enable_command(const char *name);
> bool qmp_command_is_enabled(const char *name);
> -char **qmp_get_command_list(void);
> QObject *qmp_build_error_object(Error *errp);
> +typedef void (*qmp_cmd_callback_fn)(QmpCommand *cmd, void *opaque);
> +void qmp_for_each_command(qmp_cmd_callback_fn fn, void *opaque);
>
> #endif
>
> diff --git a/qapi/qmp-registry.c b/qapi/qmp-registry.c
> index 28bbbe8..844d597 100644
> --- a/qapi/qmp-registry.c
> +++ b/qapi/qmp-registry.c
> @@ -66,35 +66,11 @@ void qmp_enable_command(const char *name)
> qmp_toggle_command(name, true);
> }
>
> -bool qmp_command_is_enabled(const char *name)
> +void qmp_for_each_command(qmp_cmd_callback_fn fn, void *opaque)
> {
> QmpCommand *cmd;
>
> QTAILQ_FOREACH(cmd, &qmp_commands, node) {
> - if (strcmp(cmd->name, name) == 0) {
> - return cmd->enabled;
> - }
> + fn(cmd, opaque);
> }
> -
> - return false;
> -}
> -
> -char **qmp_get_command_list(void)
> -{
> - QmpCommand *cmd;
> - int count = 1;
> - char **list_head, **list;
> -
> - QTAILQ_FOREACH(cmd, &qmp_commands, node) {
> - count++;
> - }
> -
> - list_head = list = g_malloc0(count * sizeof(char *));
> -
> - QTAILQ_FOREACH(cmd, &qmp_commands, node) {
> - *list = g_strdup(cmd->name);
> - list++;
> - }
> -
> - return list_head;
> }
> diff --git a/qga/commands.c b/qga/commands.c
> index 528b082..602cd47 100644
> --- a/qga/commands.c
> +++ b/qga/commands.c
> @@ -45,35 +45,26 @@ void qmp_guest_ping(Error **err)
> slog("guest-ping called");
> }
>
> -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;
> GuestAgentCommandInfo *cmd_info;
> GuestAgentCommandInfoList *cmd_info_list;
> - char **cmd_list_head, **cmd_list;
> -
> - info->version = g_strdup(QEMU_VERSION);
> -
> - cmd_list_head = cmd_list = qmp_get_command_list();
> - if (*cmd_list_head == NULL) {
> - goto out;
> - }
> -
> - 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_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;
> + 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;
> +}
>
> - g_free(*cmd_list);
> - cmd_list++;
> - }
> +struct GuestAgentInfo *qmp_guest_info(Error **err)
> +{
> + GuestAgentInfo *info = g_malloc0(sizeof(GuestAgentInfo));
>
> -out:
> - g_free(cmd_list_head);
> + info->version = g_strdup(QEMU_VERSION);
> + qmp_for_each_command(qmp_command_info, info);
> return info;
> }
> diff --git a/qga/main.c b/qga/main.c
> index 6c746c8..1741d3f 100644
> --- a/qga/main.c
> +++ b/qga/main.c
> @@ -347,48 +347,34 @@ static gint ga_strcmp(gconstpointer str1, gconstpointer str2)
> }
>
> /* disable commands that aren't safe for fsfreeze */
> -static void ga_disable_non_whitelisted(void)
> +static void ga_disable_non_whitelisted(QmpCommand *cmd, void *opaque)
> {
> - char **list_head, **list;
> bool whitelisted;
> int i;
>
> - list_head = list = qmp_get_command_list();
> - while (*list != NULL) {
> - whitelisted = false;
> - i = 0;
> - while (ga_freeze_whitelist[i] != NULL) {
> - if (strcmp(*list, ga_freeze_whitelist[i]) == 0) {
> - whitelisted = true;
> - }
> - i++;
> - }
> - if (!whitelisted) {
> - g_debug("disabling command: %s", *list);
> - qmp_disable_command(*list);
> + whitelisted = false;
> + i = 0;
> + while (ga_freeze_whitelist[i] != NULL) {
> + if (strcmp(cmd->name, ga_freeze_whitelist[i]) == 0) {
> + whitelisted = true;
> }
> - g_free(*list);
> - list++;
> + i++;
> + }
> + if (!whitelisted) {
> + g_debug("disabling command: %s", cmd->name);
> + qmp_disable_command(cmd->name);
> }
> - g_free(list_head);
> }
>
> /* [re-]enable all commands, except those explicitly blacklisted by user */
> -static void ga_enable_non_blacklisted(GList *blacklist)
> +static void ga_enable_non_blacklisted(QmpCommand *cmd, void *opaque)
> {
> - char **list_head, **list;
> -
> - list_head = list = qmp_get_command_list();
> - while (*list != NULL) {
> - if (g_list_find_custom(blacklist, *list, ga_strcmp) == NULL &&
> - !qmp_command_is_enabled(*list)) {
> - g_debug("enabling command: %s", *list);
> - qmp_enable_command(*list);
> - }
> - g_free(*list);
> - list++;
> + GList *blacklist = (GList *)opaque;
> + if (g_list_find_custom(blacklist, cmd->name, ga_strcmp) == NULL &&
> + !cmd->enabled) {
> + g_debug("enabling command: %s", cmd->name);
> + qmp_enable_command(cmd->name);
> }
> - g_free(list_head);
> }
>
> static bool ga_create_file(const char *path)
> @@ -424,7 +410,7 @@ void ga_set_frozen(GAState *s)
> return;
> }
> /* disable all non-whitelisted (for frozen state) commands */
> - ga_disable_non_whitelisted();
> + qmp_for_each_command(ga_disable_non_whitelisted, NULL);
> g_warning("disabling logging due to filesystem freeze");
> ga_disable_logging(s);
> s->frozen = true;
> @@ -460,7 +446,7 @@ void ga_unset_frozen(GAState *s)
> }
>
> /* enable all disabled, non-blacklisted commands */
> - ga_enable_non_blacklisted(s->blacklist);
> + qmp_for_each_command(ga_enable_non_blacklisted, s->blacklist);
> s->frozen = false;
> if (!ga_delete_file(s->state_filepath_isfrozen)) {
> g_warning("unable to delete %s, fsfreeze may not function properly",
> @@ -920,6 +906,11 @@ int64_t ga_get_fd_handle(GAState *s, Error **errp)
> return handle;
> }
>
> +static void ga_print_cmd(QmpCommand *cmd, void *opaque)
> +{
> + printf("%s\n", cmd->name);
> +}
> +
> int main(int argc, char **argv)
> {
> const char *sopt = "hVvdm:p:l:f:F::b:s:t:";
> @@ -996,15 +987,8 @@ int main(int argc, char **argv)
> daemonize = 1;
> break;
> case 'b': {
> - char **list_head, **list;
> if (is_help_option(optarg)) {
> - list_head = list = qmp_get_command_list();
> - while (*list != NULL) {
> - printf("%s\n", *list);
> - g_free(*list);
> - list++;
> - }
> - g_free(list_head);
> + qmp_for_each_command(ga_print_cmd, NULL);
> return 0;
> }
> for (j = 0, i = 0, len = strlen(optarg); i < len; i++) {
> @@ -1126,7 +1110,7 @@ int main(int argc, char **argv)
> s->deferred_options.log_filepath = log_filepath;
> }
> ga_disable_logging(s);
> - ga_disable_non_whitelisted();
> + qmp_for_each_command(ga_disable_non_whitelisted, NULL);
> } else {
> if (daemonize) {
> become_daemon(pid_filepath);
> --
> 1.8.3.1
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-09-27 16:16 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-09-26 1:56 [Qemu-devel] [PATCH] Add interface to traverse the qmp command list by QmpCommand Mark Wu
2013-09-26 2:36 ` Mark Wu
2013-09-26 2:57 ` Eric Blake
2013-09-27 16:07 ` Michael Roth
2013-09-27 16:16 ` Michael Roth
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).