[Qemu-devel] [PATCH v2 0/7] qga: add support for re-opening channel on error

[Qemu-devel] [PATCH v2 0/7] qga: add support for re-opening channel on error

[Bishara AbuHattoum]

Changes from v1:
 [1/7] qga: group agent init/cleanup init separate routines
         Fixed typo in the commit message and dropped the unnecessary message ID
 [2/7] qga: hang GAConfig/socket_activation off of GAState global
         Dropped the unnecessary message ID
 [3/7] qga: move w32 service handling out of run_agent()
         Fixed run_agent() extra call and dropped the unnecessary message ID 
 [4/7] qga: add --retry-path option for re-initializing channel on failure
         Dropped the unnecessary message ID
 [5/7] qga-win: install service with --retry-path set by default
         Dropped the unnecessary message ID
 [6/7] qga-win: report specific error when failing to open channel
         Dropped the unnecessary message ID
 [7/7] qga-win: changing --retry-path option behavior
         Dropped the use of two events and WaitForMultipleObjects, and used
         one 'wakeup_event'.

A patch series was firstly introduced about a year ago which resolves an issue
with qemu-ga and virtio-serial functionality. The issue was discussed in the
following thread:
  https://lists.gnu.org/archive/html/qemu-devel/2018-01/msg06256.html

In short, Sameeh sent an implementation which uses udev on Linux and device
events API on Windows. Since udev API is only supported for kernels 2.6+ it is
not a good approach and Michael suggested his alternative series which uses a
loop and checks if the serial is present or not. Since the Windows API is
supported and has backward compatibility up until Windows xp, there is no reason
for not using the Windows API. It was finally agreed by Michael and Sameeh that
a combination of both approaches should be used.

This series does just that, it rebases Michael's patches on top of upstream and
merges Sameeh's patches for Windows API into Michael's implementation.

Michael's series:
  https://github.com/mdroth/qemu/commits/qga-retry-path
Sameeh's series:
  https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg02400.html
  https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg02399.html
  https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg02401.html

Bishara AbuHattoum (1):
  qga-win: changing --retry-path option behavior

Michael Roth (6):
  qga: group agent init/cleanup init separate routines
  qga: hang GAConfig/socket_activation off of GAState global
  qga: move w32 service handling out of run_agent()
  qga: add --retry-path option for re-initializing channel on failure
  qga-win: install service with --retry-path set by default
  qga-win: report specific error when failing to open channel

 qga/channel-win32.c       |   3 +-
 qga/installer/qemu-ga.wxs |   2 +-
 qga/main.c                | 288 +++++++++++++++++++++++++++++---------
 qga/service-win32.h       |   4 +
 4 files changed, 229 insertions(+), 68 deletions(-)

-- 
2.17.0

[Qemu-devel] [PATCH v2 1/7] qga: group agent init/cleanup init separate routines

[Bishara AbuHattoum]

From: Michael Roth <mdroth@linux.vnet.ibm.com>

This patch better separates the init/cleanup routines out into
separate functions to make the start-up procedure a bit easier to
follow. This will be useful when we eventually break out the actual
start/stop of the agent's main loop into separates routines that
can be called multiple times after the init phase.

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qga/main.c | 82 +++++++++++++++++++++++++++++++++---------------------
 1 file changed, 50 insertions(+), 32 deletions(-)

diff --git a/qga/main.c b/qga/main.c
index c399320d3c..fd02086bfc 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -1211,9 +1211,21 @@ static bool check_is_frozen(GAState *s)
     return false;
 }
 
-static int run_agent(GAState *s, GAConfig *config, int socket_activation)
+static GAState *initialize_agent(GAConfig *config)
 {
-    ga_state = s;
+    GAState *s = g_new0(GAState, 1);
+
+    g_assert(ga_state == NULL);
+
+    s->log_level = config->log_level;
+    s->log_file = stderr;
+#ifdef CONFIG_FSFREEZE
+    s->fsfreeze_hook = config->fsfreeze_hook;
+#endif
+    s->pstate_filepath = g_strdup_printf("%s/qga.state", config->state_dir);
+    s->state_filepath_isfrozen = g_strdup_printf("%s/qga.state.isfrozen",
+                                                 config->state_dir);
+    s->frozen = check_is_frozen(s);
 
     g_log_set_default_handler(ga_log, s);
     g_log_set_fatal_mask(NULL, G_LOG_LEVEL_ERROR);
@@ -1229,7 +1241,7 @@ static int run_agent(GAState *s, GAConfig *config, int socket_activation)
     if (g_mkdir_with_parents(config->state_dir, S_IRWXU) == -1) {
         g_critical("unable to create (an ancestor of) the state directory"
                    " '%s': %s", config->state_dir, strerror(errno));
-        return EXIT_FAILURE;
+        return NULL;
     }
 #endif
 
@@ -1254,7 +1266,7 @@ static int run_agent(GAState *s, GAConfig *config, int socket_activation)
             if (!log_file) {
                 g_critical("unable to open specified log file: %s",
                            strerror(errno));
-                return EXIT_FAILURE;
+                return NULL;
             }
             s->log_file = log_file;
         }
@@ -1265,7 +1277,7 @@ static int run_agent(GAState *s, GAConfig *config, int socket_activation)
                                s->pstate_filepath,
                                ga_is_frozen(s))) {
         g_critical("failed to load persistent state");
-        return EXIT_FAILURE;
+        return NULL;
     }
 
     config->blacklist = ga_command_blacklist_init(config->blacklist);
@@ -1286,12 +1298,37 @@ static int run_agent(GAState *s, GAConfig *config, int socket_activation)
 #ifndef _WIN32
     if (!register_signal_handlers()) {
         g_critical("failed to register signal handlers");
-        return EXIT_FAILURE;
+        return NULL;
     }
 #endif
 
     s->main_loop = g_main_loop_new(NULL, false);
 
+    ga_state = s;
+    return s;
+}
+
+static void cleanup_agent(GAState *s)
+{
+    if (s->command_state) {
+        ga_command_state_cleanup_all(s->command_state);
+        ga_command_state_free(s->command_state);
+        json_message_parser_destroy(&s->parser);
+    }
+    if (s->channel) {
+        ga_channel_free(s->channel);
+    }
+    g_free(s->pstate_filepath);
+    g_free(s->state_filepath_isfrozen);
+    if (s->main_loop) {
+        g_main_loop_unref(s->main_loop);
+    }
+    g_free(s);
+    ga_state = NULL;
+}
+
+static int run_agent(GAState *s, GAConfig *config, int socket_activation)
+{
     if (!channel_init(ga_state, config->method, config->channel_path,
                       socket_activation ? FIRST_SOCKET_ACTIVATION_FD : -1)) {
         g_critical("failed to initialize guest agent channel");
@@ -1315,7 +1352,7 @@ static int run_agent(GAState *s, GAConfig *config, int socket_activation)
 int main(int argc, char **argv)
 {
     int ret = EXIT_SUCCESS;
-    GAState *s = g_new0(GAState, 1);
+    GAState *s;
     GAConfig *config = g_new0(GAConfig, 1);
     int socket_activation;
 
@@ -1383,44 +1420,25 @@ int main(int argc, char **argv)
         }
     }
 
-    s->log_level = config->log_level;
-    s->log_file = stderr;
-#ifdef CONFIG_FSFREEZE
-    s->fsfreeze_hook = config->fsfreeze_hook;
-#endif
-    s->pstate_filepath = g_strdup_printf("%s/qga.state", config->state_dir);
-    s->state_filepath_isfrozen = g_strdup_printf("%s/qga.state.isfrozen",
-                                                 config->state_dir);
-    s->frozen = check_is_frozen(s);
-
     if (config->dumpconf) {
         config_dump(config);
         goto end;
     }
 
+    s = initialize_agent(config);
+    if (!s) {
+        g_critical("error initializing guest agent");
+        goto end;
+    }
     ret = run_agent(s, config, socket_activation);
+    cleanup_agent(s);
 
 end:
-    if (s->command_state) {
-        ga_command_state_cleanup_all(s->command_state);
-        ga_command_state_free(s->command_state);
-        json_message_parser_destroy(&s->parser);
-    }
-    if (s->channel) {
-        ga_channel_free(s->channel);
-    }
-    g_free(s->pstate_filepath);
-    g_free(s->state_filepath_isfrozen);
-
     if (config->daemonize) {
         unlink(config->pid_filepath);
     }
 
     config_free(config);
-    if (s->main_loop) {
-        g_main_loop_unref(s->main_loop);
-    }
-    g_free(s);
 
     return ret;
 }
-- 
2.17.0

[Qemu-devel] [PATCH v2 2/7] qga: hang GAConfig/socket_activation off of GAState global

[Bishara AbuHattoum]

From: Michael Roth <mdroth@linux.vnet.ibm.com>

For w32 services we rely on the global GAState to access resources
associated with the agent within service_main(). Currently this is
sufficient for starting the agent since we open the channel once prior
to calling service_main(), and simply start the GMainLoop to start the
agent from within service_main().

Eventually we want to be able to also [re-]open the communication
channel from within service_main(), which requires access to
config/socket_activation variables, so we hang them off GAState in
preparation for that.

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
Signed-off-by: Sameeh Jubran <sameeh@daynix.com>
---
 qga/main.c | 54 +++++++++++++++++++++++++++++-------------------------
 1 file changed, 29 insertions(+), 25 deletions(-)

diff --git a/qga/main.c b/qga/main.c
index fd02086bfc..9f4dc0b2c5 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -69,6 +69,25 @@ typedef struct GAPersistentState {
     int64_t fd_counter;
 } GAPersistentState;
 
+typedef struct GAConfig {
+    char *channel_path;
+    char *method;
+    char *log_filepath;
+    char *pid_filepath;
+#ifdef CONFIG_FSFREEZE
+    char *fsfreeze_hook;
+#endif
+    char *state_dir;
+#ifdef _WIN32
+    const char *service;
+#endif
+    gchar *bliststr; /* blacklist may point to this string */
+    GList *blacklist;
+    int daemonize;
+    GLogLevelFlags log_level;
+    int dumpconf;
+} GAConfig;
+
 struct GAState {
     JSONMessageParser parser;
     GMainLoop *main_loop;
@@ -94,6 +113,8 @@ struct GAState {
 #endif
     gchar *pstate_filepath;
     GAPersistentState pstate;
+    GAConfig *config;
+    int socket_activation;
 };
 
 struct GAState *ga_state;
@@ -905,25 +926,6 @@ static GList *split_list(const gchar *str, const gchar *delim)
     return list;
 }
 
-typedef struct GAConfig {
-    char *channel_path;
-    char *method;
-    char *log_filepath;
-    char *pid_filepath;
-#ifdef CONFIG_FSFREEZE
-    char *fsfreeze_hook;
-#endif
-    char *state_dir;
-#ifdef _WIN32
-    const char *service;
-#endif
-    gchar *bliststr; /* blacklist may point to this string */
-    GList *blacklist;
-    int daemonize;
-    GLogLevelFlags log_level;
-    int dumpconf;
-} GAConfig;
-
 static void config_load(GAConfig *config)
 {
     GError *gerr = NULL;
@@ -1211,7 +1213,7 @@ static bool check_is_frozen(GAState *s)
     return false;
 }
 
-static GAState *initialize_agent(GAConfig *config)
+static GAState *initialize_agent(GAConfig *config, int socket_activation)
 {
     GAState *s = g_new0(GAState, 1);
 
@@ -1304,6 +1306,8 @@ static GAState *initialize_agent(GAConfig *config)
 
     s->main_loop = g_main_loop_new(NULL, false);
 
+    s->config = config;
+    s->socket_activation = socket_activation;
     ga_state = s;
     return s;
 }
@@ -1327,10 +1331,10 @@ static void cleanup_agent(GAState *s)
     ga_state = NULL;
 }
 
-static int run_agent(GAState *s, GAConfig *config, int socket_activation)
+static int run_agent(GAState *s)
 {
-    if (!channel_init(ga_state, config->method, config->channel_path,
-                      socket_activation ? FIRST_SOCKET_ACTIVATION_FD : -1)) {
+    if (!channel_init(s, s->config->method, s->config->channel_path,
+                      s->socket_activation ? FIRST_SOCKET_ACTIVATION_FD : -1)) {
         g_critical("failed to initialize guest agent channel");
         return EXIT_FAILURE;
     }
@@ -1425,12 +1429,12 @@ int main(int argc, char **argv)
         goto end;
     }
 
-    s = initialize_agent(config);
+    s = initialize_agent(config, socket_activation);
     if (!s) {
         g_critical("error initializing guest agent");
         goto end;
     }
-    ret = run_agent(s, config, socket_activation);
+    ret = run_agent(s);
     cleanup_agent(s);
 
 end:
-- 
2.17.0

[Qemu-devel] [PATCH v2 3/7] qga: move w32 service handling out of run_agent()

[Bishara AbuHattoum]

From: Michael Roth <mdroth@linux.vnet.ibm.com>

Eventually we want a w32 service to be able to restart the qga main
loop from within service_main(). To allow for this we move service
handling out of run_agent() such that service_main() calls
run_agent() instead of the reverse.

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
Signed-off-by: Bishara AbuHattoum <bishara@daynix.com>
---
 qga/main.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/qga/main.c b/qga/main.c
index 9f4dc0b2c5..23a0a46b84 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -136,6 +136,7 @@ DWORD WINAPI service_ctrl_handler(DWORD ctrl, DWORD type, LPVOID data,
                                   LPVOID ctx);
 VOID WINAPI service_main(DWORD argc, TCHAR *argv[]);
 #endif
+static int run_agent(GAState *s);
 
 static void
 init_dfl_pathnames(void)
@@ -729,7 +730,7 @@ VOID WINAPI service_main(DWORD argc, TCHAR *argv[])
     service->status.dwWaitHint = 0;
     SetServiceStatus(service->status_handle, &service->status);
 
-    g_main_loop_run(ga_state->main_loop);
+    run_agent(ga_state);
 
     service->status.dwCurrentState = SERVICE_STOPPED;
     SetServiceStatus(service->status_handle, &service->status);
@@ -1338,17 +1339,8 @@ static int run_agent(GAState *s)
         g_critical("failed to initialize guest agent channel");
         return EXIT_FAILURE;
     }
-#ifndef _WIN32
+
     g_main_loop_run(ga_state->main_loop);
-#else
-    if (config->daemonize) {
-        SERVICE_TABLE_ENTRY service_table[] = {
-            { (char *)QGA_SERVICE_NAME, service_main }, { NULL, NULL } };
-        StartServiceCtrlDispatcher(service_table);
-    } else {
-        g_main_loop_run(ga_state->main_loop);
-    }
-#endif
 
     return EXIT_SUCCESS;
 }
@@ -1434,7 +1426,19 @@ int main(int argc, char **argv)
         g_critical("error initializing guest agent");
         goto end;
     }
+
+#ifdef _WIN32
+    if (config->daemonize) {
+        SERVICE_TABLE_ENTRY service_table[] = {
+            { (char *)QGA_SERVICE_NAME, service_main }, { NULL, NULL } };
+        StartServiceCtrlDispatcher(service_table);
+    } else {
+        ret = run_agent(s);
+    }
+#else
     ret = run_agent(s);
+#endif
+
     cleanup_agent(s);
 
 end:
-- 
2.17.0

[Qemu-devel] [PATCH v2 4/7] qga: add --retry-path option for re-initializing channel on failure

[Bishara AbuHattoum]

From: Michael Roth <mdroth@linux.vnet.ibm.com>

This adds an option to instruct the agent to periodically attempt
re-opening the communication channel after a channel error has
occurred. The main use-case for this is providing an OS-independent
way of allowing the agent to survive situations like hotplug/unplug of
the communication channel, or initial guest set up where the agent may
be installed/started prior to the installation of the channel device's
driver.

There are nicer ways of implementing this functionality via things
like systemd services, but this option is useful for platforms like
*BSD/w32.

Currently a channel error will result in the GSource for that channel
being removed from the GMainLoop, but the main loop continuing to run.
That behavior results in a dead loop when --retry-path isn't set, and
prevents us from knowing when to attempt re-opening the channel when
it is set, so we also force the loop to exit as part of this patch.

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qga/main.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 54 insertions(+), 8 deletions(-)

diff --git a/qga/main.c b/qga/main.c
index 23a0a46b84..f359499aaa 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -58,6 +58,7 @@
 #endif
 #define QGA_SENTINEL_BYTE 0xFF
 #define QGA_CONF_DEFAULT CONFIG_QEMU_CONFDIR G_DIR_SEPARATOR_S "qemu-ga.conf"
+#define QGA_RETRY_INTERVAL 5
 
 static struct {
     const char *state_dir;
@@ -86,6 +87,7 @@ typedef struct GAConfig {
     int daemonize;
     GLogLevelFlags log_level;
     int dumpconf;
+    bool retry_path;
 } GAConfig;
 
 struct GAState {
@@ -115,6 +117,7 @@ struct GAState {
     GAPersistentState pstate;
     GAConfig *config;
     int socket_activation;
+    bool force_exit;
 };
 
 struct GAState *ga_state;
@@ -137,6 +140,7 @@ DWORD WINAPI service_ctrl_handler(DWORD ctrl, DWORD type, LPVOID data,
 VOID WINAPI service_main(DWORD argc, TCHAR *argv[]);
 #endif
 static int run_agent(GAState *s);
+static void stop_agent(GAState *s, bool requested);
 
 static void
 init_dfl_pathnames(void)
@@ -185,9 +189,7 @@ static void quit_handler(int sig)
     }
     g_debug("received signal num %d, quitting", sig);
 
-    if (g_main_loop_is_running(ga_state->main_loop)) {
-        g_main_loop_quit(ga_state->main_loop);
-    }
+    stop_agent(ga_state, true);
 }
 
 #ifndef _WIN32
@@ -272,6 +274,10 @@ QEMU_COPYRIGHT "\n"
 "                    to list available RPCs)\n"
 "  -D, --dump-conf   dump a qemu-ga config file based on current config\n"
 "                    options / command-line parameters to stdout\n"
+"  -r, --retry-path  attempt re-opening path if it's unavailable or closed\n"
+"                    due to an error which may be recoverable in the future\n"
+"                    (virtio-serial driver re-install, serial device hot\n"
+"                    plug/unplug, etc.)\n"
 "  -h, --help        display this help and exit\n"
 "\n"
 QEMU_HELP_BOTTOM "\n"
@@ -631,6 +637,7 @@ static gboolean channel_event_cb(GIOCondition condition, gpointer data)
     switch (status) {
     case G_IO_STATUS_ERROR:
         g_warning("error reading channel");
+        stop_agent(s, false);
         return false;
     case G_IO_STATUS_NORMAL:
         buf[count] = 0;
@@ -974,6 +981,10 @@ static void config_load(GAConfig *config)
         /* enable all log levels */
         config->log_level = G_LOG_LEVEL_MASK;
     }
+    if (g_key_file_has_key(keyfile, "general", "retry-path", NULL)) {
+        config->retry_path =
+            g_key_file_get_boolean(keyfile, "general", "retry-path", &gerr);
+    }
     if (g_key_file_has_key(keyfile, "general", "blacklist", NULL)) {
         config->bliststr =
             g_key_file_get_string(keyfile, "general", "blacklist", &gerr);
@@ -1035,6 +1046,8 @@ static void config_dump(GAConfig *config)
     g_key_file_set_string(keyfile, "general", "statedir", config->state_dir);
     g_key_file_set_boolean(keyfile, "general", "verbose",
                            config->log_level == G_LOG_LEVEL_MASK);
+    g_key_file_set_boolean(keyfile, "general", "retry-path",
+                           config->retry_path);
     tmp = list_join(config->blacklist, ',');
     g_key_file_set_string(keyfile, "general", "blacklist", tmp);
     g_free(tmp);
@@ -1053,7 +1066,7 @@ static void config_dump(GAConfig *config)
 
 static void config_parse(GAConfig *config, int argc, char **argv)
 {
-    const char *sopt = "hVvdm:p:l:f:F::b:s:t:D";
+    const char *sopt = "hVvdm:p:l:f:F::b:s:t:Dr";
     int opt_ind = 0, ch;
     const struct option lopt[] = {
         { "help", 0, NULL, 'h' },
@@ -1073,6 +1086,7 @@ static void config_parse(GAConfig *config, int argc, char **argv)
         { "service", 1, NULL, 's' },
 #endif
         { "statedir", 1, NULL, 't' },
+        { "retry-path", 0, NULL, 'r' },
         { NULL, 0, NULL, 0 }
     };
 
@@ -1117,6 +1131,9 @@ static void config_parse(GAConfig *config, int argc, char **argv)
         case 'D':
             config->dumpconf = 1;
             break;
+        case 'r':
+            config->retry_path = true;
+            break;
         case 'b': {
             if (is_help_option(optarg)) {
                 qmp_for_each_command(&ga_commands, ga_print_cmd, NULL);
@@ -1320,9 +1337,6 @@ static void cleanup_agent(GAState *s)
         ga_command_state_free(s->command_state);
         json_message_parser_destroy(&s->parser);
     }
-    if (s->channel) {
-        ga_channel_free(s->channel);
-    }
     g_free(s->pstate_filepath);
     g_free(s->state_filepath_isfrozen);
     if (s->main_loop) {
@@ -1332,7 +1346,7 @@ static void cleanup_agent(GAState *s)
     ga_state = NULL;
 }
 
-static int run_agent(GAState *s)
+static int run_agent_once(GAState *s)
 {
     if (!channel_init(s, s->config->method, s->config->channel_path,
                       s->socket_activation ? FIRST_SOCKET_ACTIVATION_FD : -1)) {
@@ -1342,9 +1356,41 @@ static int run_agent(GAState *s)
 
     g_main_loop_run(ga_state->main_loop);
 
+    if (s->channel) {
+        ga_channel_free(s->channel);
+    }
+
     return EXIT_SUCCESS;
 }
 
+static int run_agent(GAState *s)
+{
+    int ret = EXIT_SUCCESS;
+
+    s->force_exit = false;
+
+    do {
+        ret = run_agent_once(s);
+        if (s->config->retry_path && !s->force_exit) {
+            g_warning("agent stopped unexpectedly, restarting...");
+            sleep(QGA_RETRY_INTERVAL);
+        }
+    } while (s->config->retry_path && !s->force_exit);
+
+    return ret;
+}
+
+static void stop_agent(GAState *s, bool requested)
+{
+    if (!s->force_exit) {
+        s->force_exit = requested;
+    }
+
+    if (g_main_loop_is_running(s->main_loop)) {
+        g_main_loop_quit(s->main_loop);
+    }
+}
+
 int main(int argc, char **argv)
 {
     int ret = EXIT_SUCCESS;
-- 
2.17.0

[Qemu-devel] [PATCH v2 5/7] qga-win: install service with --retry-path set by default

[Bishara AbuHattoum]

From: Michael Roth <mdroth@linux.vnet.ibm.com>

It's nicer from a management perspective that the agent can survive
hotplug/unplug of the channel device, or be started prior to the
installation of the channel device's driver without and still be able
to resume normal function afterward. On linux there are alternatives
like systemd to support this, but on w32 --retry-path is the only
option so it makes sense to set it by default when installed as a
w32 service.

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qga/installer/qemu-ga.wxs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qga/installer/qemu-ga.wxs b/qga/installer/qemu-ga.wxs
index f751a7e9f7..64bf90bd85 100644
--- a/qga/installer/qemu-ga.wxs
+++ b/qga/installer/qemu-ga.wxs
@@ -78,7 +78,7 @@
               Account="LocalSystem"
               ErrorControl="ignore"
               Interactive="no"
-              Arguments="-d"
+              Arguments="-d --retry-path"
               >
             </ServiceInstall>
             <ServiceControl Id="StartService" Start="install" Stop="both" Remove="uninstall" Name="QEMU-GA" Wait="no" />
-- 
2.17.0

[Qemu-devel] [PATCH v2 6/7] qga-win: report specific error when failing to open channel

[Bishara AbuHattoum]

From: Michael Roth <mdroth@linux.vnet.ibm.com>

Useful in general, but especially now that errors might occur more
frequently with --retry-path set.

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qga/channel-win32.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/qga/channel-win32.c b/qga/channel-win32.c
index b3597a8a0f..c86f4388db 100644
--- a/qga/channel-win32.c
+++ b/qga/channel-win32.c
@@ -302,7 +302,8 @@ static gboolean ga_channel_open(GAChannel *c, GAChannelMethod method,
                            OPEN_EXISTING,
                            FILE_FLAG_NO_BUFFERING | FILE_FLAG_OVERLAPPED, NULL);
     if (c->handle == INVALID_HANDLE_VALUE) {
-        g_critical("error opening path %s", newpath);
+        g_critical("error opening path %s: %s", newpath,
+                   g_win32_error_message(GetLastError()));
         return false;
     }
 
-- 
2.17.0

[Qemu-devel] [PATCH v2 7/7] qga-win: changing --retry-path option behavior

[Bishara AbuHattoum]

Currently whenever the qemu-ga's service doesn't find the virtio-serial
the run_agent() loops in a QGA_RETRY_INTERVAL (default 5 seconds)
intervals and try to restart the qemu-ga which causes a synchronous loop.
Changed to wait and listen for the serial events by registering for
notifications a proper serial event handler that deals with events:
  DBT_DEVICEARRIVAL        indicates that the device has been inserted and
                           is available
  DBT_DEVICEREMOVECOMPLETE indicates that the devive has been removed
Which allow us to determine when the channel path is available for the
qemu-ga to restart.

Signed-off-by: Bishara AbuHattoum <bishara@daynix.com>
Signed-off-by: Sameeh Jubran <sameeh@daynix.com>
---
 qga/main.c          | 86 ++++++++++++++++++++++++++++++++++++++++++++-
 qga/service-win32.h |  4 +++
 2 files changed, 89 insertions(+), 1 deletion(-)

diff --git a/qga/main.c b/qga/main.c
index f359499aaa..c5bb063b1c 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -34,6 +34,7 @@
 #include "qemu/systemd.h"
 #include "qemu-version.h"
 #ifdef _WIN32
+#include <dbt.h>
 #include "qga/service-win32.h"
 #include "qga/vss-win32.h"
 #endif
@@ -101,6 +102,7 @@ struct GAState {
     bool logging_enabled;
 #ifdef _WIN32
     GAService service;
+    HANDLE wakeup_event;
 #endif
     bool delimit_response;
     bool frozen;
@@ -137,6 +139,7 @@ static const char *ga_freeze_whitelist[] = {
 #ifdef _WIN32
 DWORD WINAPI service_ctrl_handler(DWORD ctrl, DWORD type, LPVOID data,
                                   LPVOID ctx);
+DWORD WINAPI handle_serial_device_events(DWORD type, LPVOID data);
 VOID WINAPI service_main(DWORD argc, TCHAR *argv[]);
 #endif
 static int run_agent(GAState *s);
@@ -695,6 +698,36 @@ static gboolean channel_init(GAState *s, const gchar *method, const gchar *path,
 }
 
 #ifdef _WIN32
+DWORD WINAPI handle_serial_device_events(DWORD type, LPVOID data)
+{
+    DWORD ret = NO_ERROR;
+    PDEV_BROADCAST_HDR broadcast_header = (PDEV_BROADCAST_HDR)data;
+
+    if (broadcast_header->dbch_devicetype == DBT_DEVTYP_DEVICEINTERFACE) {
+        switch (type) {
+            /* Device inserted */
+        case DBT_DEVICEARRIVAL:
+            /* Start QEMU-ga's service */
+            if (!SetEvent(ga_state->wakeup_event)) {
+                ret = GetLastError();
+            }
+            break;
+            /* Device removed */
+        case DBT_DEVICEQUERYREMOVE:
+        case DBT_DEVICEREMOVEPENDING:
+        case DBT_DEVICEREMOVECOMPLETE:
+            /* Stop QEMU-ga's service */
+            if (!ResetEvent(ga_state->wakeup_event)) {
+                ret = GetLastError();
+            }
+            break;
+        default:
+            ret = ERROR_CALL_NOT_IMPLEMENTED;
+        }
+    }
+    return ret;
+}
+
 DWORD WINAPI service_ctrl_handler(DWORD ctrl, DWORD type, LPVOID data,
                                   LPVOID ctx)
 {
@@ -706,9 +739,13 @@ DWORD WINAPI service_ctrl_handler(DWORD ctrl, DWORD type, LPVOID data,
         case SERVICE_CONTROL_STOP:
         case SERVICE_CONTROL_SHUTDOWN:
             quit_handler(SIGTERM);
+            SetEvent(ga_state->wakeup_event);
             service->status.dwCurrentState = SERVICE_STOP_PENDING;
             SetServiceStatus(service->status_handle, &service->status);
             break;
+        case SERVICE_CONTROL_DEVICEEVENT:
+            handle_serial_device_events(type, data);
+            break;
 
         default:
             ret = ERROR_CALL_NOT_IMPLEMENTED;
@@ -735,10 +772,24 @@ VOID WINAPI service_main(DWORD argc, TCHAR *argv[])
     service->status.dwServiceSpecificExitCode = NO_ERROR;
     service->status.dwCheckPoint = 0;
     service->status.dwWaitHint = 0;
+    DEV_BROADCAST_DEVICEINTERFACE notification_filter;
+    ZeroMemory(&notification_filter, sizeof(notification_filter));
+    notification_filter.dbcc_devicetype = DBT_DEVTYP_DEVICEINTERFACE;
+    notification_filter.dbcc_size = sizeof(DEV_BROADCAST_DEVICEINTERFACE);
+    notification_filter.dbcc_classguid = GUID_VIOSERIAL_PORT;
+
+    service->device_notification_handle =
+        RegisterDeviceNotification(service->status_handle,
+            &notification_filter, DEVICE_NOTIFY_SERVICE_HANDLE);
+    if (!service->device_notification_handle) {
+        g_critical("Failed to register device notification handle!\n");
+        return;
+    }
     SetServiceStatus(service->status_handle, &service->status);
 
     run_agent(ga_state);
 
+    UnregisterDeviceNotification(service->device_notification_handle);
     service->status.dwCurrentState = SERVICE_STOPPED;
     SetServiceStatus(service->status_handle, &service->status);
 }
@@ -1326,12 +1377,24 @@ static GAState *initialize_agent(GAConfig *config, int socket_activation)
 
     s->config = config;
     s->socket_activation = socket_activation;
+
+#ifdef _WIN32
+    s->wakeup_event = CreateEvent(NULL, TRUE, FALSE, TEXT("WakeUp"));
+    if (s->wakeup_event == NULL) {
+        g_critical("CreateEvent failed");
+        return NULL;
+    }
+#endif
+
     ga_state = s;
     return s;
 }
 
 static void cleanup_agent(GAState *s)
 {
+#ifdef _WIN32
+    CloseHandle(s->wakeup_event);
+#endif
     if (s->command_state) {
         ga_command_state_cleanup_all(s->command_state);
         ga_command_state_free(s->command_state);
@@ -1363,6 +1426,27 @@ static int run_agent_once(GAState *s)
     return EXIT_SUCCESS;
 }
 
+static void wait_for_channel_availability(GAState *s)
+{
+    g_warning("waiting for channel path...");
+#ifndef _WIN32
+    sleep(QGA_RETRY_INTERVAL);
+#else
+    DWORD dwWaitResult;
+
+    dwWaitResult = WaitForSingleObject(s->wakeup_event, INFINITE);
+
+    switch (dwWaitResult) {
+    case WAIT_OBJECT_0:
+        break;
+    case WAIT_TIMEOUT:
+        break;
+    default:
+        g_critical("WaitForSingleObject failed");
+    }
+#endif
+}
+
 static int run_agent(GAState *s)
 {
     int ret = EXIT_SUCCESS;
@@ -1373,7 +1457,7 @@ static int run_agent(GAState *s)
         ret = run_agent_once(s);
         if (s->config->retry_path && !s->force_exit) {
             g_warning("agent stopped unexpectedly, restarting...");
-            sleep(QGA_RETRY_INTERVAL);
+            wait_for_channel_availability(s);
         }
     } while (s->config->retry_path && !s->force_exit);
 
diff --git a/qga/service-win32.h b/qga/service-win32.h
index 89e99dfede..7b16d69b57 100644
--- a/qga/service-win32.h
+++ b/qga/service-win32.h
@@ -20,9 +20,13 @@
 #define QGA_SERVICE_NAME         "qemu-ga"
 #define QGA_SERVICE_DESCRIPTION  "Enables integration with QEMU machine emulator and virtualizer."
 
+static const GUID GUID_VIOSERIAL_PORT = { 0x6fde7521, 0x1b65, 0x48ae,
+{ 0xb6, 0x28, 0x80, 0xbe, 0x62, 0x1, 0x60, 0x26 } };
+
 typedef struct GAService {
     SERVICE_STATUS status;
     SERVICE_STATUS_HANDLE status_handle;
+    HDEVNOTIFY device_notification_handle;
 } GAService;
 
 int ga_install_service(const char *path, const char *logfile,
-- 
2.17.0

Re: [Qemu-devel] [PATCH v2 0/7] qga: add support for re-opening channel on error

[Michael Roth]

Quoting Bishara AbuHattoum (2018-10-07 06:02:16)
> Changes from v1:
>  [1/7] qga: group agent init/cleanup init separate routines
>          Fixed typo in the commit message and dropped the unnecessary message ID
>  [2/7] qga: hang GAConfig/socket_activation off of GAState global
>          Dropped the unnecessary message ID
>  [3/7] qga: move w32 service handling out of run_agent()
>          Fixed run_agent() extra call and dropped the unnecessary message ID 
>  [4/7] qga: add --retry-path option for re-initializing channel on failure
>          Dropped the unnecessary message ID
>  [5/7] qga-win: install service with --retry-path set by default
>          Dropped the unnecessary message ID
>  [6/7] qga-win: report specific error when failing to open channel
>          Dropped the unnecessary message ID
>  [7/7] qga-win: changing --retry-path option behavior
>          Dropped the use of two events and WaitForMultipleObjects, and used
>          one 'wakeup_event'.
> 
> A patch series was firstly introduced about a year ago which resolves an issue
> with qemu-ga and virtio-serial functionality. The issue was discussed in the
> following thread:
>   https://lists.gnu.org/archive/html/qemu-devel/2018-01/msg06256.html
> 
> In short, Sameeh sent an implementation which uses udev on Linux and device
> events API on Windows. Since udev API is only supported for kernels 2.6+ it is
> not a good approach and Michael suggested his alternative series which uses a
> loop and checks if the serial is present or not. Since the Windows API is
> supported and has backward compatibility up until Windows xp, there is no reason
> for not using the Windows API. It was finally agreed by Michael and Sameeh that
> a combination of both approaches should be used.
> 
> This series does just that, it rebases Michael's patches on top of upstream and
> merges Sameeh's patches for Windows API into Michael's implementation.
> 
> Michael's series:
>   https://github.com/mdroth/qemu/commits/qga-retry-path
> Sameeh's series:
>   https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg02400.html
>   https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg02399.html
>   https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg02401.html

Thanks, applied to qga tree with some minor fix-ups:
  https://github.com/mdroth/qemu/commits/qga

> 
> Bishara AbuHattoum (1):
>   qga-win: changing --retry-path option behavior
> 
> Michael Roth (6):
>   qga: group agent init/cleanup init separate routines
>   qga: hang GAConfig/socket_activation off of GAState global
>   qga: move w32 service handling out of run_agent()
>   qga: add --retry-path option for re-initializing channel on failure
>   qga-win: install service with --retry-path set by default
>   qga-win: report specific error when failing to open channel
> 
>  qga/channel-win32.c       |   3 +-
>  qga/installer/qemu-ga.wxs |   2 +-
>  qga/main.c                | 288 +++++++++++++++++++++++++++++---------
>  qga/service-win32.h       |   4 +
>  4 files changed, 229 insertions(+), 68 deletions(-)
> 
> -- 
> 2.17.0
>