[Qemu-devel] [PATCH 0/3]

[Qemu-devel] [PATCH 0/3]

[Sameeh Jubran]

From: Sameeh Jubran <sjubran@redhat.com>

This series fixes qemu-ga's behaviour upon facing a missing serial/serial
driver by listening to the serial device's events.

For more info on why this series is needed checkout the commit message
of the third patch and the following bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=990629.

Sameeh Jubran (3):
  qga: Channel: Add functions for checking serial status
  qga: main: make qga config and socket activation global
  qga: Prevent qemu-ga exit if serial doesn't exist

 Makefile            |   4 +
 qga/channel-posix.c |  54 ++++++++++
 qga/channel-win32.c |  60 +++++++++++
 qga/channel.h       |   9 ++
 qga/main.c          | 284 ++++++++++++++++++++++++++++++++++++++++++++++------
 qga/service-win32.h |   4 +
 6 files changed, 385 insertions(+), 30 deletions(-)

-- 
2.9.4

[Qemu-devel] [PATCH 1/3] qga: Channel: Add functions for checking serial status

[Sameeh Jubran]

From: Sameeh Jubran <sjubran@redhat.com>

This commit adds functions to check if the serial is
connected/disconnected or else if it has been attached or detached.

Signed-off-by: Sameeh Jubran <sjubran@redhat.com>
---
 qga/channel-posix.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++
 qga/channel-win32.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 qga/channel.h       |  9 ++++++++
 3 files changed, 123 insertions(+)

diff --git a/qga/channel-posix.c b/qga/channel-posix.c
index 3f34465..d307cf4 100644
--- a/qga/channel-posix.c
+++ b/qga/channel-posix.c
@@ -295,3 +295,57 @@ void ga_channel_free(GAChannel *c)
     }
     g_free(c);
 }
+
+static bool is_serial_present(GAChannelMethod method, const gchar *path,
+    int *error_code)
+{
+    int fd = -1;
+    bool ret = true;
+
+    assert(error_code);
+    *error_code = 0;
+
+    switch (method) {
+    case GA_CHANNEL_VIRTIO_SERIAL:
+        fd = qemu_open(path, O_RDWR | O_NONBLOCK
+#ifndef CONFIG_SOLARIS
+            | O_ASYNC
+#endif
+        );
+        break;
+    case GA_CHANNEL_ISA_SERIAL:
+        fd = qemu_open(path, O_RDWR | O_NOCTTY | O_NONBLOCK);
+        break;
+    default:
+        ret = false;
+    }
+    if (fd < 0) {
+        *error_code = errno;
+        ret = false;
+    } else {
+        close(fd);
+    }
+    return ret;
+}
+
+bool ga_channel_serial_is_present(GAChannelMethod method, const gchar *path)
+{
+    int error_code = 0;
+    return is_serial_present(method, path, &error_code) ||
+        error_code == EBUSY;
+}
+
+bool ga_channel_was_serial_attached(GAChannelMethod method, const gchar *path,
+    bool is_serial_attached)
+{
+    int error_code = 0;
+    return !is_serial_attached &&
+        is_serial_present(method, path, &error_code);
+}
+bool ga_channel_was_serial_detached(GAChannelMethod method, const gchar *path,
+    bool is_serial_attached)
+{
+    int error_code = 0;
+    return is_serial_attached && !is_serial_present(method, path, &error_code)
+        && error_code == ENOENT;
+}
diff --git a/qga/channel-win32.c b/qga/channel-win32.c
index 7e6dc4d..2d51bee 100644
--- a/qga/channel-win32.c
+++ b/qga/channel-win32.c
@@ -354,3 +354,63 @@ void ga_channel_free(GAChannel *c)
     g_free(c->rstate.buf);
     g_free(c);
 }
+
+static bool is_serial_present(GAChannelMethod method, const gchar *path,
+    DWORD *err)
+{
+    gchar newpath[MAXPATHLEN] = { 0 };
+    bool ret = false;
+
+    assert(err);
+
+    if (method != GA_CHANNEL_VIRTIO_SERIAL && method != GA_CHANNEL_ISA_SERIAL) {
+        g_critical("unsupported communication method");
+        return false;
+    }
+
+    if (method == GA_CHANNEL_ISA_SERIAL) {
+        snprintf(newpath, sizeof(newpath), "\\\\.\\%s", path);
+    } else {
+        g_strlcpy(newpath, path, sizeof(newpath));
+    }
+
+    HANDLE handle = CreateFile(newpath, GENERIC_READ | GENERIC_WRITE, 0, NULL,
+        OPEN_EXISTING,
+        FILE_FLAG_NO_BUFFERING | FILE_FLAG_OVERLAPPED, NULL);
+
+    if (handle == INVALID_HANDLE_VALUE) {
+        *err = GetLastError();
+        ret = false;
+    } else {
+        ret = true;
+    }
+
+    CloseHandle(handle);
+    return ret;
+}
+
+bool ga_channel_serial_is_present(GAChannelMethod method, const gchar *path)
+{
+    DWORD err_code;
+    return is_serial_present(method, path, &err_code) ||
+        err_code == ERROR_ACCESS_DENIED;
+}
+
+bool ga_channel_was_serial_attached(GAChannelMethod method, const gchar *path,
+    bool is_serial_attached)
+{
+    DWORD err_code;
+    return !is_serial_attached && is_serial_present(method, path, &err_code);
+}
+
+bool ga_channel_was_serial_detached(GAChannelMethod method, const gchar *path,
+    bool is_serial_attached)
+{
+    DWORD err_code = NO_ERROR;
+        /* In order to make sure the serial that qemu-ga uses is the one that
+         * was detached. We'll get the error ERROR_FILE_NOT_FOUND when
+         * attempting to call CreateFile with the serial path.
+        */
+       return is_serial_attached && !is_serial_present(method, path, &err_code)
+           && err_code == ERROR_FILE_NOT_FOUND;
+}
diff --git a/qga/channel.h b/qga/channel.h
index 1778416..acb3d73 100644
--- a/qga/channel.h
+++ b/qga/channel.h
@@ -12,6 +12,10 @@
 #ifndef QGA_CHANNEL_H
 #define QGA_CHANNEL_H
 
+#ifndef _WIN32
+#define SUBSYSTEM_VIRTIO_SERIAL "virtio-ports";
+#define SUBSYSTEM_ISA_SERIAL "isa-serial";
+#endif
 
 typedef struct GAChannel GAChannel;
 
@@ -30,5 +34,10 @@ GAChannel *ga_channel_new(GAChannelMethod method, const gchar *path,
 void ga_channel_free(GAChannel *c);
 GIOStatus ga_channel_read(GAChannel *c, gchar *buf, gsize size, gsize *count);
 GIOStatus ga_channel_write_all(GAChannel *c, const gchar *buf, gsize size);
+bool ga_channel_serial_is_present(GAChannelMethod method, const gchar *path);
+bool ga_channel_was_serial_attached(GAChannelMethod method, const gchar *path,
+    bool is_serial_attached);
+bool ga_channel_was_serial_detached(GAChannelMethod method, const gchar *path,
+    bool is_serial_attached);
 
 #endif
-- 
2.9.4

[Qemu-devel] [PATCH 2/3] qga: main: make qga config and socket activation global

[Sameeh Jubran]

From: Sameeh Jubran <sjubran@redhat.com>

Signed-off-by: Sameeh Jubran <sjubran@redhat.com>
---
 qga/main.c | 44 +++++++++++++++++++++++---------------------
 1 file changed, 23 insertions(+), 21 deletions(-)

diff --git a/qga/main.c b/qga/main.c
index 1b381d0..cf312b9 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -92,7 +92,28 @@ struct GAState {
     GAPersistentState pstate;
 };
 
+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 *ga_state;
+struct GAConfig *ga_config;
+int ga_socket_activation;
 QmpCommandList ga_commands;
 
 /* commands that are safe to issue while filesystems are frozen */
@@ -942,25 +963,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;
@@ -1353,7 +1355,7 @@ int main(int argc, char **argv)
 {
     int ret = EXIT_SUCCESS;
     GAState *s = g_new0(GAState, 1);
-    GAConfig *config = g_new0(GAConfig, 1);
+    GAConfig *config = ga_config = g_new0(GAConfig, 1);
     int socket_activation;
 
     config->log_level = G_LOG_LEVEL_ERROR | G_LOG_LEVEL_CRITICAL;
@@ -1376,7 +1378,7 @@ int main(int argc, char **argv)
         config->method = g_strdup("virtio-serial");
     }
 
-    socket_activation = check_socket_activation();
+    ga_socket_activation = socket_activation = check_socket_activation();
     if (socket_activation > 1) {
         g_critical("qemu-ga only supports listening on one socket");
         ret = EXIT_FAILURE;
-- 
2.9.4

[Qemu-devel] [PATCH 3/3] qga: Prevent qemu-ga exit if serial doesn't exist

[Sameeh Jubran]

From: Sameeh Jubran <sjubran@redhat.com>

Currently whenever the qemu-ga's service doesn't find the virtio-serial
it terminates. This commit addresses this issue by listening to the serial events
by registering for notifications for the chosen serial and it handles channel
initialization accordingily.

A list of possible scenarios of which could lead to this behavour:

* qemu-ga's service is started while the virtio-serial driver hasn't been installed yet
* hotplug/ unplug of the virtio-serial device
* upgrading the virtio-serial driver

Note: This problem is much more common on Windows as the virtio-serial
driver should be installed by the user and isn't shipped with the Windows OS.

Signed-off-by: Sameeh Jubran <sjubran@redhat.com>
---
 Makefile            |   4 +
 qga/main.c          | 240 ++++++++++++++++++++++++++++++++++++++++++++++++++--
 qga/service-win32.h |   4 +
 3 files changed, 239 insertions(+), 9 deletions(-)

diff --git a/Makefile b/Makefile
index ef72148..82f26d5 100644
--- a/Makefile
+++ b/Makefile
@@ -203,6 +203,10 @@ $(call set-vpath, $(SRC_PATH))
 
 LIBS+=-lz $(LIBS_TOOLS)
 
+ifndef CONFIG_WIN32
+LIBS_QGA+=-ludev
+endif
+
 HELPERS-$(CONFIG_LINUX) = qemu-bridge-helper$(EXESUF)
 
 ifdef BUILD_DOCS
diff --git a/qga/main.c b/qga/main.c
index cf312b9..d727880 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -16,6 +16,8 @@
 #ifndef _WIN32
 #include <syslog.h>
 #include <sys/wait.h>
+#include <libudev.h>
+#include <sys/types.h>
 #endif
 #include "qapi/qmp/json-streamer.h"
 #include "qapi/qmp/json-parser.h"
@@ -30,6 +32,7 @@
 #include "qemu/sockets.h"
 #include "qemu/systemd.h"
 #ifdef _WIN32
+#include <dbt.h>
 #include "qga/service-win32.h"
 #include "qga/vss-win32.h"
 #endif
@@ -74,6 +77,10 @@ struct GAState {
     GLogLevelFlags log_level;
     FILE *log_file;
     bool logging_enabled;
+    bool serial_connected;
+#ifndef _WIN32
+    struct udev_monitor *udev_monitor;
+#endif
 #ifdef _WIN32
     GAService service;
 #endif
@@ -130,9 +137,15 @@ 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 bool get_channel_method(GAChannelMethod *channel_method,
+    const gchar *method, GAState *s);
+static gboolean channel_init(GAState *s, const gchar *method, const gchar *path,
+    int listen_fd, bool *serial_connected);
+
 static void
 init_dfl_pathnames(void)
 {
@@ -186,6 +199,136 @@ static void quit_handler(int sig)
 }
 
 #ifndef _WIN32
+static int get_method_udev_subsystem(const char **serial_subsystem)
+{
+    if (strcmp(ga_config->method, "virtio-serial") == 0) {
+        *serial_subsystem = SUBSYSTEM_VIRTIO_SERIAL;
+    } else if (strcmp(ga_config->method, "isa-serial") == 0) {
+        /* try the default path for the serial port - COM1 */
+        *serial_subsystem = SUBSYSTEM_ISA_SERIAL;
+    } else {
+        serial_subsystem = NULL;
+        return -1;
+    }
+    return 0;
+}
+
+static gboolean serial_event_callback(GIOChannel *source,
+    GIOCondition condition, gpointer data)
+{
+    struct udev_monitor *mon = ga_state->udev_monitor;
+    const char *serial_subsystem = NULL;
+    struct udev_device *dev;
+
+    if (get_method_udev_subsystem(&serial_subsystem) == -1) {
+        return false;
+    }
+    dev = udev_monitor_receive_device(mon);
+
+    if (dev && serial_subsystem && strcmp(udev_device_get_subsystem(dev),
+        serial_subsystem) ==  0) {
+
+        GAChannelMethod channel_method;
+        get_channel_method(&channel_method, ga_config->method, ga_state);
+        if (ga_channel_was_serial_attached(channel_method,
+            ga_config->channel_path, ga_state->serial_connected)) {
+            ga_state->serial_connected = true;
+            if (!channel_init(ga_state,
+                ga_config->method, ga_config->channel_path,
+                ga_socket_activation ? FIRST_SOCKET_ACTIVATION_FD : -1,
+                &ga_state->serial_connected)) {
+                g_critical("failed to initialize guest agent channel");
+            }
+        }
+
+        if (ga_channel_was_serial_detached(channel_method,
+            ga_config->channel_path, ga_state->serial_connected)) {
+            ga_state->serial_connected = false;
+            ga_channel_free(ga_state->channel);
+        }
+        udev_device_unref(dev);
+    }
+    return true;
+}
+
+static int monitor_serial_events(void)
+{
+    int ret = 0;
+    const char *serial_subsystem = NULL;
+    struct udev *udev = NULL;
+    ga_state->udev_monitor = NULL;
+    GIOChannel *channel = NULL;
+    GSource *watch_source = NULL;
+    if (get_method_udev_subsystem(&serial_subsystem) == -1) {
+        ret = -1;
+        goto out;
+    }
+
+    udev = udev_new();
+    if (!udev) {
+        g_error("Couldn't create udev\n");
+        ret = -1;
+        goto out;
+    }
+
+    ga_state->udev_monitor =
+        udev_monitor_new_from_netlink(udev, "udev");
+    if (!ga_state->udev_monitor) {
+        ret = -1;
+        goto out;
+    } else {
+        /* We don't want the udev_monitor to be freed on out, so increase
+         * ref count
+         */
+        udev_monitor_ref(ga_state->udev_monitor);
+    }
+
+    if (udev_monitor_filter_add_match_subsystem_devtype(ga_state->udev_monitor,
+        serial_subsystem, NULL) < 0 ||
+        udev_monitor_enable_receiving(ga_state->udev_monitor) < 0) {
+        ret = -1;
+        goto out;
+    }
+
+    channel =
+        g_io_channel_unix_new(udev_monitor_get_fd(ga_state->udev_monitor));
+    if (!channel) {
+        ret = -1;
+        goto out;
+    }
+    watch_source = g_io_create_watch(channel, G_IO_IN);
+    if (!watch_source) {
+        ret = -1;
+        goto out;
+    }
+    g_source_set_callback(watch_source, (GSourceFunc)serial_event_callback,
+        ga_state->udev_monitor, NULL);
+    g_source_attach(watch_source, g_main_loop_get_context(ga_state->main_loop));
+
+out:
+    if (udev) {
+        udev_unref(udev);
+    }
+    if (ga_state->udev_monitor) {
+        udev_monitor_unref(ga_state->udev_monitor);
+    }
+    if (channel) {
+        g_io_channel_unref(channel);
+    }
+    if (watch_source) {
+        g_source_unref(watch_source);
+    }
+
+    return ret;
+}
+
+static void free_monitor_resources(void)
+{
+    if (ga_state->udev_monitor) {
+        udev_monitor_unref(ga_state->udev_monitor);
+    }
+}
+
 static gboolean register_signal_handlers(void)
 {
     struct sigaction sigact;
@@ -694,24 +837,38 @@ static gboolean channel_event_cb(GIOCondition condition, gpointer data)
     return true;
 }
 
-static gboolean channel_init(GAState *s, const gchar *method, const gchar *path,
-                             int listen_fd)
+static bool get_channel_method(GAChannelMethod *channel_method,
+    const gchar *method, GAState *s)
 {
-    GAChannelMethod channel_method;
+    assert(channel_method);
+    assert(method);
 
     if (strcmp(method, "virtio-serial") == 0) {
+        assert(s);
         s->virtio = true; /* virtio requires special handling in some cases */
-        channel_method = GA_CHANNEL_VIRTIO_SERIAL;
+        *channel_method = GA_CHANNEL_VIRTIO_SERIAL;
     } else if (strcmp(method, "isa-serial") == 0) {
-        channel_method = GA_CHANNEL_ISA_SERIAL;
+        *channel_method = GA_CHANNEL_ISA_SERIAL;
     } else if (strcmp(method, "unix-listen") == 0) {
-        channel_method = GA_CHANNEL_UNIX_LISTEN;
+        *channel_method = GA_CHANNEL_UNIX_LISTEN;
     } else if (strcmp(method, "vsock-listen") == 0) {
-        channel_method = GA_CHANNEL_VSOCK_LISTEN;
+        *channel_method = GA_CHANNEL_VSOCK_LISTEN;
     } else {
         g_critical("unsupported channel method/type: %s", method);
         return false;
     }
+    return true;
+}
+
+static gboolean channel_init(GAState *s, const gchar *method, const gchar *path,
+                             int listen_fd, bool *serial_connected)
+{
+    assert(serial_connected);
+    GAChannelMethod channel_method;
+    if (!get_channel_method(&channel_method, method, s)) {
+        return false;
+    }
+    *serial_connected = ga_channel_serial_is_present(channel_method, path);
 
     s->channel = ga_channel_new(channel_method, path, listen_fd,
                                 channel_event_cb, s);
@@ -724,6 +881,48 @@ 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;
+    GAChannelMethod channel_method;
+
+    if (broadcast_header->dbch_devicetype == DBT_DEVTYP_DEVICEINTERFACE) {
+        get_channel_method(&channel_method, ga_config->method, ga_state);
+        switch (type) {
+            /* Device inserted */
+        case DBT_DEVICEARRIVAL:
+            /* Start QEMU-ga's service */
+            if (ga_channel_was_serial_attached(channel_method,
+                ga_config->channel_path, ga_state->serial_connected)) {
+                if (!channel_init(ga_state,
+                    ga_config->method, ga_config->channel_path,
+                    ga_socket_activation ? FIRST_SOCKET_ACTIVATION_FD : -1,
+                    &ga_state->serial_connected)) {
+                    g_critical("failed to initialize guest agent channel");
+                    ret = EXIT_FAILURE;
+                }
+            }
+            break;
+            /* Device removed */
+        case DBT_DEVICEQUERYREMOVE:
+        case DBT_DEVICEREMOVEPENDING:
+        case DBT_DEVICEREMOVECOMPLETE:
+
+            /* Stop QEMU-ga's service */
+            if (ga_channel_was_serial_detached(channel_method,
+                ga_config->channel_path, ga_state->serial_connected)) {
+                ga_state->serial_connected = false;
+                ga_channel_free(ga_state->channel);
+            }
+            break;
+        default:
+            ret = ERROR_CALL_NOT_IMPLEMENTED;
+        }
+    }
+    return ret;
+}
+
 DWORD WINAPI service_ctrl_handler(DWORD ctrl, DWORD type, LPVOID data,
                                   LPVOID ctx)
 {
@@ -738,6 +937,9 @@ DWORD WINAPI service_ctrl_handler(DWORD ctrl, DWORD type, LPVOID data,
             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;
@@ -764,10 +966,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);
 
     g_main_loop_run(ga_state->main_loop);
 
+    UnregisterDeviceNotification(service->device_notification_handle);
     service->status.dwCurrentState = SERVICE_STOPPED;
     SetServiceStatus(service->status_handle, &service->status);
 }
@@ -1330,20 +1546,26 @@ static int run_agent(GAState *s, GAConfig *config, int socket_activation)
 #endif
 
     s->main_loop = g_main_loop_new(NULL, false);
-
     if (!channel_init(ga_state, config->method, config->channel_path,
-                      socket_activation ? FIRST_SOCKET_ACTIVATION_FD : -1)) {
+                      socket_activation ? FIRST_SOCKET_ACTIVATION_FD : -1,
+                      &ga_state->serial_connected) &&
+                      ga_state->serial_connected) {
         g_critical("failed to initialize guest agent channel");
         return EXIT_FAILURE;
     }
 #ifndef _WIN32
+    monitor_serial_events();
     g_main_loop_run(ga_state->main_loop);
+    free_monitor_resources();
 #else
     if (config->daemonize) {
         SERVICE_TABLE_ENTRY service_table[] = {
             { (char *)QGA_SERVICE_NAME, service_main }, { NULL, NULL } };
         StartServiceCtrlDispatcher(service_table);
     } else {
+        if (!ga_state->serial_connected) {
+            return EXIT_FAILURE;
+        }
         g_main_loop_run(ga_state->main_loop);
     }
 #endif
diff --git a/qga/service-win32.h b/qga/service-win32.h
index 89e99df..7b16d69 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.9.4

Re: [Qemu-devel] [PATCH 0/3]

[Sameeh Jubran]

Ping.

On Sun, Aug 13, 2017 at 6:58 PM, Sameeh Jubran <sameeh@daynix.com> wrote:

> From: Sameeh Jubran <sjubran@redhat.com>
>
> This series fixes qemu-ga's behaviour upon facing a missing serial/serial
> driver by listening to the serial device's events.
>
> For more info on why this series is needed checkout the commit message
> of the third patch and the following bugzilla:
> https://bugzilla.redhat.com/show_bug.cgi?id=990629.
>
> Sameeh Jubran (3):
>   qga: Channel: Add functions for checking serial status
>   qga: main: make qga config and socket activation global
>   qga: Prevent qemu-ga exit if serial doesn't exist
>
>  Makefile            |   4 +
>  qga/channel-posix.c |  54 ++++++++++
>  qga/channel-win32.c |  60 +++++++++++
>  qga/channel.h       |   9 ++
>  qga/main.c          | 284 ++++++++++++++++++++++++++++++
> ++++++++++++++++------
>  qga/service-win32.h |   4 +
>  6 files changed, 385 insertions(+), 30 deletions(-)
>
> --
> 2.9.4
>
>


-- 
Respectfully,
*Sameeh Jubran*
*Linkedin <https://il.linkedin.com/pub/sameeh-jubran/87/747/a8a>*
*Software Engineer @ Daynix <http://www.daynix.com>.*

Re: [Qemu-devel] [PATCH 0/3]

[Sameeh Jubran]

Ping.

On Tue, Aug 22, 2017 at 2:18 PM, Sameeh Jubran <sameeh@daynix.com> wrote:

> Ping.
>
> On Sun, Aug 13, 2017 at 6:58 PM, Sameeh Jubran <sameeh@daynix.com> wrote:
>
>> From: Sameeh Jubran <sjubran@redhat.com>
>>
>> This series fixes qemu-ga's behaviour upon facing a missing serial/serial
>> driver by listening to the serial device's events.
>>
>> For more info on why this series is needed checkout the commit message
>> of the third patch and the following bugzilla:
>> https://bugzilla.redhat.com/show_bug.cgi?id=990629.
>>
>> Sameeh Jubran (3):
>>   qga: Channel: Add functions for checking serial status
>>   qga: main: make qga config and socket activation global
>>   qga: Prevent qemu-ga exit if serial doesn't exist
>>
>>  Makefile            |   4 +
>>  qga/channel-posix.c |  54 ++++++++++
>>  qga/channel-win32.c |  60 +++++++++++
>>  qga/channel.h       |   9 ++
>>  qga/main.c          | 284 ++++++++++++++++++++++++++++++
>> ++++++++++++++++------
>>  qga/service-win32.h |   4 +
>>  6 files changed, 385 insertions(+), 30 deletions(-)
>>
>> --
>> 2.9.4
>>
>>
>
>
> --
> Respectfully,
> *Sameeh Jubran*
> *Linkedin <https://il.linkedin.com/pub/sameeh-jubran/87/747/a8a>*
> *Software Engineer @ Daynix <http://www.daynix.com>.*
>



-- 
Respectfully,
*Sameeh Jubran*
*Linkedin <https://il.linkedin.com/pub/sameeh-jubran/87/747/a8a>*
*Software Engineer @ Daynix <http://www.daynix.com>.*

Re: [Qemu-devel] [PATCH 0/3]

[Sameeh Jubran]

Pong.

On Mon, Sep 4, 2017 at 4:48 PM, Sameeh Jubran <sameeh@daynix.com> wrote:

> Ping.
>
> On Tue, Aug 22, 2017 at 2:18 PM, Sameeh Jubran <sameeh@daynix.com> wrote:
>
>> Ping.
>>
>> On Sun, Aug 13, 2017 at 6:58 PM, Sameeh Jubran <sameeh@daynix.com> wrote:
>>
>>> From: Sameeh Jubran <sjubran@redhat.com>
>>>
>>> This series fixes qemu-ga's behaviour upon facing a missing serial/serial
>>> driver by listening to the serial device's events.
>>>
>>> For more info on why this series is needed checkout the commit message
>>> of the third patch and the following bugzilla:
>>> https://bugzilla.redhat.com/show_bug.cgi?id=990629.
>>>
>>> Sameeh Jubran (3):
>>>   qga: Channel: Add functions for checking serial status
>>>   qga: main: make qga config and socket activation global
>>>   qga: Prevent qemu-ga exit if serial doesn't exist
>>>
>>>  Makefile            |   4 +
>>>  qga/channel-posix.c |  54 ++++++++++
>>>  qga/channel-win32.c |  60 +++++++++++
>>>  qga/channel.h       |   9 ++
>>>  qga/main.c          | 284 ++++++++++++++++++++++++++++++
>>> ++++++++++++++++------
>>>  qga/service-win32.h |   4 +
>>>  6 files changed, 385 insertions(+), 30 deletions(-)
>>>
>>> --
>>> 2.9.4
>>>
>>>
>>
>>
>> --
>> Respectfully,
>> *Sameeh Jubran*
>> *Linkedin <https://il.linkedin.com/pub/sameeh-jubran/87/747/a8a>*
>> *Software Engineer @ Daynix <http://www.daynix.com>.*
>>
>
>
>
> --
> Respectfully,
> *Sameeh Jubran*
> *Linkedin <https://il.linkedin.com/pub/sameeh-jubran/87/747/a8a>*
> *Software Engineer @ Daynix <http://www.daynix.com>.*
>



-- 
Respectfully,
*Sameeh Jubran*
*Linkedin <https://il.linkedin.com/pub/sameeh-jubran/87/747/a8a>*
*Software Engineer @ Daynix <http://www.daynix.com>.*

Re: [Qemu-devel] [PATCH 0/3]

[Michael Roth]

Quoting Sameeh Jubran (2017-08-13 10:58:46)
> From: Sameeh Jubran <sjubran@redhat.com>
> 
> This series fixes qemu-ga's behaviour upon facing a missing serial/serial
> driver by listening to the serial device's events.
> 
> For more info on why this series is needed checkout the commit message
> of the third patch and the following bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=990629.
> 
> Sameeh Jubran (3):
>   qga: Channel: Add functions for checking serial status
>   qga: main: make qga config and socket activation global
>   qga: Prevent qemu-ga exit if serial doesn't exist

Hi Sameeh,

The event handling stuff is spiffy and could be useful for other use-cases
(e.g. cpu/mem hotplug events that can be consumed by management), but since
the actual bug here is somewhat of an edge case (we *could* just tell
people that installing the agent before virtio-serial drivers is a bug,
or that unplugging the agent's communication channel is a bad idea),
I'm not too comfortable with adding this much complexity unless there's
a stronger argument for it.

There's also a couple issues I had with this series as it stands, namely
the lack of a ./configure check for udev (which could cause build
breakage in some environments), and a lot of spillage of GAConfig into
qga/channel-*, which I think could be avoided.

I've sent an alternative series that I think we should consider as it
uses a much simpler mechanism to implement this support (basically
just periodically retrying the channel if it doesn't exist, or if it
disappears for whatever reason). I've tested it on Windows, but would
be good to confirm that it adequately addresses the use-case you were
looking at. Thanks!

> 
>  Makefile            |   4 +
>  qga/channel-posix.c |  54 ++++++++++
>  qga/channel-win32.c |  60 +++++++++++
>  qga/channel.h       |   9 ++
>  qga/main.c          | 284 ++++++++++++++++++++++++++++++++++++++++++++++------
>  qga/service-win32.h |   4 +
>  6 files changed, 385 insertions(+), 30 deletions(-)
> 
> -- 
> 2.9.4
> 

Re: [Qemu-devel] [PATCH 0/3]

[Sameeh Jubran]

On Fri, Oct 27, 2017 at 2:51 AM, Michael Roth <mdroth@linux.vnet.ibm.com>
wrote:

> Quoting Sameeh Jubran (2017-08-13 10:58:46)
> > From: Sameeh Jubran <sjubran@redhat.com>
> >
> > This series fixes qemu-ga's behaviour upon facing a missing serial/serial
> > driver by listening to the serial device's events.
> >
> > For more info on why this series is needed checkout the commit message
> > of the third patch and the following bugzilla:
> https://bugzilla.redhat.com/show_bug.cgi?id=990629.
> >
> > Sameeh Jubran (3):
> >   qga: Channel: Add functions for checking serial status
> >   qga: main: make qga config and socket activation global
> >   qga: Prevent qemu-ga exit if serial doesn't exist
>
> Hi Sameeh,
>
> The event handling stuff is spiffy and could be useful for other use-cases
> (e.g. cpu/mem hotplug events that can be consumed by management), but since
> the actual bug here is somewhat of an edge case (we *could* just tell
> people that installing the agent before virtio-serial drivers is a bug,
> or that unplugging the agent's communication channel is a bad idea),
> I'm not too comfortable with adding this much complexity unless there's
> a stronger argument for it.
>
I can relate to your concerns, it is somehow an edge case but I think that
this
is the elegant way to handle it instead of just polling forever. This patch
series
is more related to Windows than Linux as this edge case is much more common
on Windows since when the virtio-serial driver is installed sometimes
usually
it requires a post-installation reboot and when the system is up, qemu-ga
runs before
the virtio-serial driver is fully configured and it fails to load and then
another reboot is needed.


>
> There's also a couple issues I had with this series as it stands, namely
> the lack of a ./configure check for udev (which could cause build
> breakage in some environments), and a lot of spillage of GAConfig into
> qga/channel-*, which I think could be avoided.
>
I think we can use the --retry with linux clients and use the device
notifications
API provided by Windows as it is supported since xp.

>
> I've sent an alternative series that I think we should consider as it
> uses a much simpler mechanism to implement this support (basically
> just periodically retrying the channel if it doesn't exist, or if it
> disappears for whatever reason). I've tested it on Windows, but would
> be good to confirm that it adequately addresses the use-case you were
> looking at. Thanks!
>
I haven't tested it yet, but I think it might solve the issue. Your series
is much
simpler and less intrusive to the code but I don't think this is the right
approach.

>
> >
> >  Makefile            |   4 +
> >  qga/channel-posix.c |  54 ++++++++++
> >  qga/channel-win32.c |  60 +++++++++++
> >  qga/channel.h       |   9 ++
> >  qga/main.c          | 284 ++++++++++++++++++++++++++++++
> ++++++++++++++++------
> >  qga/service-win32.h |   4 +
> >  6 files changed, 385 insertions(+), 30 deletions(-)
> >
> > --
> > 2.9.4
> >
>
>


-- 
Respectfully,
*Sameeh Jubran*
*Linkedin <https://il.linkedin.com/pub/sameeh-jubran/87/747/a8a>*
*Software Engineer @ Daynix <http://www.daynix.com>.*

Re: [Qemu-devel] [PATCH 0/3]

[Sameeh Jubran]

Ping.

On Fri, Oct 27, 2017 at 10:08 AM, Sameeh Jubran <sameeh@daynix.com> wrote:

>
>
> On Fri, Oct 27, 2017 at 2:51 AM, Michael Roth <mdroth@linux.vnet.ibm.com>
> wrote:
>
>> Quoting Sameeh Jubran (2017-08-13 10:58:46)
>> > From: Sameeh Jubran <sjubran@redhat.com>
>> >
>> > This series fixes qemu-ga's behaviour upon facing a missing
>> serial/serial
>> > driver by listening to the serial device's events.
>> >
>> > For more info on why this series is needed checkout the commit message
>> > of the third patch and the following bugzilla:
>> https://bugzilla.redhat.com/show_bug.cgi?id=990629.
>> >
>> > Sameeh Jubran (3):
>> >   qga: Channel: Add functions for checking serial status
>> >   qga: main: make qga config and socket activation global
>> >   qga: Prevent qemu-ga exit if serial doesn't exist
>>
>> Hi Sameeh,
>>
>> The event handling stuff is spiffy and could be useful for other use-cases
>> (e.g. cpu/mem hotplug events that can be consumed by management), but
>> since
>> the actual bug here is somewhat of an edge case (we *could* just tell
>> people that installing the agent before virtio-serial drivers is a bug,
>> or that unplugging the agent's communication channel is a bad idea),
>> I'm not too comfortable with adding this much complexity unless there's
>> a stronger argument for it.
>>
> I can relate to your concerns, it is somehow an edge case but I think that
> this
> is the elegant way to handle it instead of just polling forever. This
> patch series
> is more related to Windows than Linux as this edge case is much more common
> on Windows since when the virtio-serial driver is installed sometimes
> usually
> it requires a post-installation reboot and when the system is up, qemu-ga
> runs before
> the virtio-serial driver is fully configured and it fails to load and then
> another reboot is needed.
>
>
>>
>> There's also a couple issues I had with this series as it stands, namely
>> the lack of a ./configure check for udev (which could cause build
>> breakage in some environments), and a lot of spillage of GAConfig into
>> qga/channel-*, which I think could be avoided.
>>
> I think we can use the --retry with linux clients and use the device
> notifications
> API provided by Windows as it is supported since xp.
>
>>
>> I've sent an alternative series that I think we should consider as it
>> uses a much simpler mechanism to implement this support (basically
>> just periodically retrying the channel if it doesn't exist, or if it
>> disappears for whatever reason). I've tested it on Windows, but would
>> be good to confirm that it adequately addresses the use-case you were
>> looking at. Thanks!
>>
> I haven't tested it yet, but I think it might solve the issue. Your series
> is much
> simpler and less intrusive to the code but I don't think this is the right
> approach.
>
>>
>> >
>> >  Makefile            |   4 +
>> >  qga/channel-posix.c |  54 ++++++++++
>> >  qga/channel-win32.c |  60 +++++++++++
>> >  qga/channel.h       |   9 ++
>> >  qga/main.c          | 284 ++++++++++++++++++++++++++++++
>> ++++++++++++++++------
>> >  qga/service-win32.h |   4 +
>> >  6 files changed, 385 insertions(+), 30 deletions(-)
>> >
>> > --
>> > 2.9.4
>> >
>>
>>
>
>
> --
> Respectfully,
> *Sameeh Jubran*
> *Linkedin <https://il.linkedin.com/pub/sameeh-jubran/87/747/a8a>*
> *Software Engineer @ Daynix <http://www.daynix.com>.*
>



-- 
Respectfully,
*Sameeh Jubran*
*Linkedin <https://il.linkedin.com/pub/sameeh-jubran/87/747/a8a>*
*Software Engineer @ Daynix <http://www.daynix.com>.*

Re: [Qemu-devel] [PATCH 0/3]

[Michael Roth]

Quoting Sameeh Jubran (2018-01-22 08:24:29)
> Ping.

I think I would still prefer the alternative series I sent. Your
approach may be more correct/elegant but the asynchronous event-handling
nature of it makes it inherently more different to debug than the
boring synchronous loop in my series. There's also more work needed to
get this in shape and I honestly don't think it's worth the extra effort
it will take.

But if you feel strongly enough about it I wouldn't be opposed to a hybrid
of the 2 approaches that basically boils down to the following:

1) Base your code on top of my full series here:

     https://lists.gnu.org/archive/html/qemu-devel/2017-10/msg06157.html

2) Drop the udev side of your code and only implement the
   win32 side of your code.

3) Modify your handle_serial_device_events() logic to not actually
   manage closing/opening the Channel, but to simply set a global flag,
   e.g. ga_state->channel_path_available appropriately based on
   DBT_DEVICEARRIVAL/DBT_DEVICEREMOVECOMPLETE. And since it's a
   win32-only optimization make sure it's just set to true permanently
   for !win32.

   Then use that flag as an optimization to the retry loop
   in run_agent() that was introduced in patch 4 of my series:
   
   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;
   }
   
   by having your code changes introduce something like this:

   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...");
               while (!ga_state->channel_path_available) {
                 g_warning("waiting for channel path...");
                 sleep(QGA_RETRY_INTERVAL);
               }
           }
       } while (s->config->retry_path && !s->force_exit);
   
       return ret;
   }

   It's still a loop but at least it's not trial-and-error based. If
   you want to get fancy with it though you can use something based
   around WaitForSingleObject or SetEvent to avoid gratuitous looping.

I think that would be much easier to make sense of, and it also lets us
handle this for POSIX without necessarily commiting to udev event loops
(though it also leaves that option open if we really wanted to).

It should also allow you to drop all your changes to the Channel code
since it's based around the changes I made to have Channel
implementations exit cleanly if their underlying device dies.

> 
> On Fri, Oct 27, 2017 at 10:08 AM, Sameeh Jubran <sameeh@daynix.com> wrote:
> 
> 
> 
>     On Fri, Oct 27, 2017 at 2:51 AM, Michael Roth <mdroth@linux.vnet.ibm.com>
>     wrote:
> 
>         Quoting Sameeh Jubran (2017-08-13 10:58:46)
>         > From: Sameeh Jubran <sjubran@redhat.com>
>         >
>         > This series fixes qemu-ga's behaviour upon facing a missing serial/
>         serial
>         > driver by listening to the serial device's events.
>         >
>         > For more info on why this series is needed checkout the commit
>         message
>         > of the third patch and the following bugzilla: https://
>         bugzilla.redhat.com/show_bug.cgi?id=990629.
>         >
>         > Sameeh Jubran (3):
>         >   qga: Channel: Add functions for checking serial status
>         >   qga: main: make qga config and socket activation global
>         >   qga: Prevent qemu-ga exit if serial doesn't exist
> 
>         Hi Sameeh,
> 
>         The event handling stuff is spiffy and could be useful for other
>         use-cases
>         (e.g. cpu/mem hotplug events that can be consumed by management), but
>         since
>         the actual bug here is somewhat of an edge case (we *could* just tell
>         people that installing the agent before virtio-serial drivers is a bug,
>         or that unplugging the agent's communication channel is a bad idea),
>         I'm not too comfortable with adding this much complexity unless there's
>         a stronger argument for it.
> 
>     I can relate to your concerns, it is somehow an edge case but I think that
>     this
>     is the elegant way to handle it instead of just polling forever. This patch
>     series
>     is more related to Windows than Linux as this edge case is much more common
>     on Windows since when the virtio-serial driver is installed sometimes
>     usually
>     it requires a post-installation reboot and when the system is up, qemu-ga
>     runs before
>     the virtio-serial driver is fully configured and it fails to load and then
>     another reboot is needed. 
>      
> 
> 
>         There's also a couple issues I had with this series as it stands,
>         namely
>         the lack of a ./configure check for udev (which could cause build
>         breakage in some environments), and a lot of spillage of GAConfig into
>         qga/channel-*, which I think could be avoided.
> 
>     I think we can use the --retry with linux clients and use the device
>     notifications
>     API provided by Windows as it is supported since xp.
>    
> 
>         I've sent an alternative series that I think we should consider as it
>         uses a much simpler mechanism to implement this support (basically
>         just periodically retrying the channel if it doesn't exist, or if it
>         disappears for whatever reason). I've tested it on Windows, but would
>         be good to confirm that it adequately addresses the use-case you were
>         looking at. Thanks!
> 
>     I haven't tested it yet, but I think it might solve the issue. Your series
>     is much
>     simpler and less intrusive to the code but I don't think this is the right
>     approach.
>    
> 
>         >
>         >  Makefile            |   4 +
>         >  qga/channel-posix.c |  54 ++++++++++
>         >  qga/channel-win32.c |  60 +++++++++++
>         >  qga/channel.h       |   9 ++
>         >  qga/main.c          | 284 ++++++++++++++++++++++++++++++
>         ++++++++++++++++------
>         >  qga/service-win32.h |   4 +
>         >  6 files changed, 385 insertions(+), 30 deletions(-)
>         >
>         > --
>         > 2.9.4
>         >
> 
> 
>    
> 
>    
>     --
>     Respectfully,
>     Sameeh Jubran
>     Linkedin
>     Software Engineer @ Daynix.
> 
> 
> 
> 
> --
> Respectfully,
> Sameeh Jubran
> Linkedin
> Software Engineer @ Daynix.