From e5d9adbdab972a2172815c1174aed3fabcc448f1 Mon Sep 17 00:00:00 2001 From: Tomoki Sekiyama Date: Tue, 1 Oct 2013 17:09:53 -0400 Subject: [PATCH 1/3] qemu-ga: execute fsfreeze-freeze in reverse order of mounts Currently, fsfreeze-freeze may cause deadlock if a guest has loopback mounts of image files in its disk; e.g.: # mount | grep ^/ /dev/vda1 / type ext4 (rw,noatime,seclabel,data=ordered) /tmp/disk.img on /mnt type ext4 (rw,relatime,seclabel) To avoid the deadlock, this freezes filesystems in reverse order of mounts. Signed-off-by: Tomoki Sekiyama Reviewed-by: Eric Blake *fix up commit msg Signed-off-by: Michael Roth --- qga/commands-posix.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/qga/commands-posix.c b/qga/commands-posix.c index e199738c7..f453132b9 100644 --- a/qga/commands-posix.c +++ b/qga/commands-posix.c @@ -566,7 +566,7 @@ typedef struct FsMount { QTAILQ_ENTRY(FsMount) next; } FsMount; -typedef QTAILQ_HEAD(, FsMount) FsMountList; +typedef QTAILQ_HEAD(FsMountList, FsMount) FsMountList; static void free_fs_mount_list(FsMountList *mounts) { @@ -728,7 +728,7 @@ int64_t qmp_guest_fsfreeze_freeze(Error **err) /* cannot risk guest agent blocking itself on a write in this state */ ga_set_frozen(ga_state); - QTAILQ_FOREACH(mount, &mounts, next) { + QTAILQ_FOREACH_REVERSE(mount, &mounts, FsMountList, next) { fd = qemu_open(mount->dirname, O_RDONLY); if (fd == -1) { error_setg_errno(err, errno, "failed to open %s", mount->dirname); From 8dc4d915dd6ea347a47557f5aa75a648555fe253 Mon Sep 17 00:00:00 2001 From: Mark Wu Date: Wed, 9 Oct 2013 11:25:07 +0800 Subject: [PATCH 2/3] qemu-ga: Add interface to traverse the qmp command list by QmpCommand 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 Reviewed-by: Eric Blake *fix up commit subject Signed-off-by: Michael Roth --- include/qapi/qmp/dispatch.h | 6 ++-- qapi/qmp-registry.c | 38 +++++++------------- qga/commands.c | 42 +++++++++------------- qga/main.c | 71 +++++++++++++++---------------------- 4 files changed, 61 insertions(+), 96 deletions(-) diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h index 1ce11f5df..7d759ef28 100644 --- a/include/qapi/qmp/dispatch.h +++ b/include/qapi/qmp/dispatch.h @@ -47,9 +47,11 @@ QmpCommand *qmp_find_command(const char *name); 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); +bool qmp_command_is_enabled(const QmpCommand *cmd); +const char *qmp_command_name(const QmpCommand *cmd); 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 28bbbe849..5e26710d8 100644 --- a/qapi/qmp-registry.c +++ b/qapi/qmp-registry.c @@ -66,35 +66,21 @@ void qmp_enable_command(const char *name) qmp_toggle_command(name, true); } -bool qmp_command_is_enabled(const char *name) +bool qmp_command_is_enabled(const QmpCommand *cmd) +{ + return cmd->enabled; +} + +const char *qmp_command_name(const QmpCommand *cmd) +{ + return cmd->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 528b082fa..e87cbf862 100644 --- a/qga/commands.c +++ b/qga/commands.c @@ -45,35 +45,27 @@ void qmp_guest_ping(Error **err) slog("guest-ping called"); } +static void qmp_command_info(QmpCommand *cmd, void *opaque) +{ + GuestAgentInfo *info = opaque; + GuestAgentCommandInfo *cmd_info; + GuestAgentCommandInfoList *cmd_info_list; + + cmd_info = g_malloc0(sizeof(GuestAgentCommandInfo)); + cmd_info->name = g_strdup(qmp_command_name(cmd)); + cmd_info->enabled = qmp_command_is_enabled(cmd); + + 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; +} + struct GuestAgentInfo *qmp_guest_info(Error **err) { GuestAgentInfo *info = g_malloc0(sizeof(GuestAgentInfo)); - 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; - - g_free(*cmd_list); - cmd_list++; - } - -out: - g_free(cmd_list_head); + qmp_for_each_command(qmp_command_info, info); return info; } diff --git a/qga/main.c b/qga/main.c index 6c746c8f3..c58b26a9a 100644 --- a/qga/main.c +++ b/qga/main.c @@ -347,48 +347,35 @@ 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; + bool whitelisted = false; + int i = 0; + const char *name = qmp_command_name(cmd); - 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++; + while (ga_freeze_whitelist[i] != NULL) { + if (strcmp(name, ga_freeze_whitelist[i]) == 0) { + whitelisted = true; } - if (!whitelisted) { - g_debug("disabling command: %s", *list); - qmp_disable_command(*list); - } - g_free(*list); - list++; + i++; + } + if (!whitelisted) { + g_debug("disabling command: %s", name); + qmp_disable_command(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; + GList *blacklist = opaque; + const char *name = qmp_command_name(cmd); - 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++; + if (g_list_find_custom(blacklist, name, ga_strcmp) == NULL && + !qmp_command_is_enabled(cmd)) { + g_debug("enabling command: %s", name); + qmp_enable_command(name); } - g_free(list_head); } static bool ga_create_file(const char *path) @@ -424,7 +411,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 +447,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 +907,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", qmp_command_name(cmd)); +} + int main(int argc, char **argv) { const char *sopt = "hVvdm:p:l:f:F::b:s:t:"; @@ -996,15 +988,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 +1111,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); From 0106dc4f05231b44f54fae5d0ee42031298588bd Mon Sep 17 00:00:00 2001 From: Mark Wu Date: Wed, 9 Oct 2013 10:37:26 +0800 Subject: [PATCH 3/3] qemu-ga: Extend 'guest-info' command to expose flag 'success-response' Now we have several qemu-ga commands not returning response on success. It has been documented in qga/qapi-schema.json already. This patch exposes the 'success-response' flag by extending 'guest-info' command. With this change, the clients can handle the command response more flexibly. Signed-off-by: Mark Wu Reviewed-by: Eric Blake Reviewed-by: Michael Roth *fixed up commit subject Signed-off-by: Michael Roth --- include/qapi/qmp/dispatch.h | 1 + qapi/qmp-registry.c | 5 +++++ qga/commands.c | 1 + qga/qapi-schema.json | 5 ++++- 4 files changed, 11 insertions(+), 1 deletion(-) diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h index 7d759ef28..cea38181b 100644 --- a/include/qapi/qmp/dispatch.h +++ b/include/qapi/qmp/dispatch.h @@ -49,6 +49,7 @@ void qmp_disable_command(const char *name); void qmp_enable_command(const char *name); bool qmp_command_is_enabled(const QmpCommand *cmd); const char *qmp_command_name(const QmpCommand *cmd); +bool qmp_has_success_response(const QmpCommand *cmd); 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); diff --git a/qapi/qmp-registry.c b/qapi/qmp-registry.c index 5e26710d8..3e4498a3f 100644 --- a/qapi/qmp-registry.c +++ b/qapi/qmp-registry.c @@ -76,6 +76,11 @@ const char *qmp_command_name(const QmpCommand *cmd) return cmd->name; } +bool qmp_has_success_response(const QmpCommand *cmd) +{ + return !(cmd->options & QCO_NO_SUCCESS_RESP); +} + void qmp_for_each_command(qmp_cmd_callback_fn fn, void *opaque) { QmpCommand *cmd; diff --git a/qga/commands.c b/qga/commands.c index e87cbf862..a0c2de07e 100644 --- a/qga/commands.c +++ b/qga/commands.c @@ -54,6 +54,7 @@ static void qmp_command_info(QmpCommand *cmd, void *opaque) cmd_info = g_malloc0(sizeof(GuestAgentCommandInfo)); cmd_info->name = g_strdup(qmp_command_name(cmd)); cmd_info->enabled = qmp_command_is_enabled(cmd); + cmd_info->success_response = qmp_has_success_response(cmd); cmd_info_list = g_malloc0(sizeof(GuestAgentCommandInfoList)); cmd_info_list->value = cmd_info; diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json index 7155b7ab5..245f968bc 100644 --- a/qga/qapi-schema.json +++ b/qga/qapi-schema.json @@ -141,10 +141,13 @@ # # @enabled: whether command is currently enabled by guest admin # +# @success-response: whether command returns a response on success +# (since 1.7) +# # Since 1.1.0 ## { 'type': 'GuestAgentCommandInfo', - 'data': { 'name': 'str', 'enabled': 'bool' } } + 'data': { 'name': 'str', 'enabled': 'bool', 'success-response': 'bool' } } ## # @GuestAgentInfo