* [Qemu-devel] [PATCH 0/3] @ 2017-08-13 15:58 Sameeh Jubran 2017-08-13 15:58 ` [Qemu-devel] [PATCH 1/3] qga: Channel: Add functions for checking serial status Sameeh Jubran ` (4 more replies) 0 siblings, 5 replies; 11+ messages in thread From: Sameeh Jubran @ 2017-08-13 15:58 UTC (permalink / raw) To: qemu-devel, mdroth; +Cc: yan 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [Qemu-devel] [PATCH 1/3] qga: Channel: Add functions for checking serial status 2017-08-13 15:58 [Qemu-devel] [PATCH 0/3] Sameeh Jubran @ 2017-08-13 15:58 ` Sameeh Jubran 2017-08-13 15:58 ` [Qemu-devel] [PATCH 2/3] qga: main: make qga config and socket activation global Sameeh Jubran ` (3 subsequent siblings) 4 siblings, 0 replies; 11+ messages in thread From: Sameeh Jubran @ 2017-08-13 15:58 UTC (permalink / raw) To: qemu-devel, mdroth; +Cc: yan 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [Qemu-devel] [PATCH 2/3] qga: main: make qga config and socket activation global 2017-08-13 15:58 [Qemu-devel] [PATCH 0/3] Sameeh Jubran 2017-08-13 15:58 ` [Qemu-devel] [PATCH 1/3] qga: Channel: Add functions for checking serial status Sameeh Jubran @ 2017-08-13 15:58 ` Sameeh Jubran 2017-08-13 15:58 ` [Qemu-devel] [PATCH 3/3] qga: Prevent qemu-ga exit if serial doesn't exist Sameeh Jubran ` (2 subsequent siblings) 4 siblings, 0 replies; 11+ messages in thread From: Sameeh Jubran @ 2017-08-13 15:58 UTC (permalink / raw) To: qemu-devel, mdroth; +Cc: yan 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [Qemu-devel] [PATCH 3/3] qga: Prevent qemu-ga exit if serial doesn't exist 2017-08-13 15:58 [Qemu-devel] [PATCH 0/3] Sameeh Jubran 2017-08-13 15:58 ` [Qemu-devel] [PATCH 1/3] qga: Channel: Add functions for checking serial status Sameeh Jubran 2017-08-13 15:58 ` [Qemu-devel] [PATCH 2/3] qga: main: make qga config and socket activation global Sameeh Jubran @ 2017-08-13 15:58 ` Sameeh Jubran 2017-08-22 11:18 ` [Qemu-devel] [PATCH 0/3] Sameeh Jubran 2017-10-26 23:51 ` Michael Roth 4 siblings, 0 replies; 11+ messages in thread From: Sameeh Jubran @ 2017-08-13 15:58 UTC (permalink / raw) To: qemu-devel, mdroth; +Cc: yan 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(¬ification_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, + ¬ification_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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH 0/3] 2017-08-13 15:58 [Qemu-devel] [PATCH 0/3] Sameeh Jubran ` (2 preceding siblings ...) 2017-08-13 15:58 ` [Qemu-devel] [PATCH 3/3] qga: Prevent qemu-ga exit if serial doesn't exist Sameeh Jubran @ 2017-08-22 11:18 ` Sameeh Jubran 2017-09-04 13:48 ` Sameeh Jubran 2017-10-26 23:51 ` Michael Roth 4 siblings, 1 reply; 11+ messages in thread From: Sameeh Jubran @ 2017-08-22 11:18 UTC (permalink / raw) To: QEMU Developers, Michael Roth; +Cc: Yan Vugenfirer 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>.* ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH 0/3] 2017-08-22 11:18 ` [Qemu-devel] [PATCH 0/3] Sameeh Jubran @ 2017-09-04 13:48 ` Sameeh Jubran 2017-10-02 13:02 ` Sameeh Jubran 0 siblings, 1 reply; 11+ messages in thread From: Sameeh Jubran @ 2017-09-04 13:48 UTC (permalink / raw) To: QEMU Developers, Michael Roth; +Cc: Yan Vugenfirer 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>.* ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH 0/3] 2017-09-04 13:48 ` Sameeh Jubran @ 2017-10-02 13:02 ` Sameeh Jubran 0 siblings, 0 replies; 11+ messages in thread From: Sameeh Jubran @ 2017-10-02 13:02 UTC (permalink / raw) To: QEMU Developers, Michael Roth; +Cc: Yan Vugenfirer 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>.* ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH 0/3] 2017-08-13 15:58 [Qemu-devel] [PATCH 0/3] Sameeh Jubran ` (3 preceding siblings ...) 2017-08-22 11:18 ` [Qemu-devel] [PATCH 0/3] Sameeh Jubran @ 2017-10-26 23:51 ` Michael Roth 2017-10-27 8:08 ` Sameeh Jubran 4 siblings, 1 reply; 11+ messages in thread From: Michael Roth @ 2017-10-26 23:51 UTC (permalink / raw) To: Sameeh Jubran, qemu-devel; +Cc: yan 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 > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH 0/3] 2017-10-26 23:51 ` Michael Roth @ 2017-10-27 8:08 ` Sameeh Jubran 2018-01-22 14:24 ` Sameeh Jubran 0 siblings, 1 reply; 11+ messages in thread From: Sameeh Jubran @ 2017-10-27 8:08 UTC (permalink / raw) To: Michael Roth; +Cc: QEMU Developers, Yan Vugenfirer 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>.* ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH 0/3] 2017-10-27 8:08 ` Sameeh Jubran @ 2018-01-22 14:24 ` Sameeh Jubran 2018-01-25 23:49 ` Michael Roth 0 siblings, 1 reply; 11+ messages in thread From: Sameeh Jubran @ 2018-01-22 14:24 UTC (permalink / raw) To: Michael Roth; +Cc: QEMU Developers, Yan Vugenfirer 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>.* ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH 0/3] 2018-01-22 14:24 ` Sameeh Jubran @ 2018-01-25 23:49 ` Michael Roth 0 siblings, 0 replies; 11+ messages in thread From: Michael Roth @ 2018-01-25 23:49 UTC (permalink / raw) To: Sameeh Jubran; +Cc: QEMU Developers, Yan Vugenfirer 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2018-01-25 23:49 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-08-13 15:58 [Qemu-devel] [PATCH 0/3] Sameeh Jubran 2017-08-13 15:58 ` [Qemu-devel] [PATCH 1/3] qga: Channel: Add functions for checking serial status Sameeh Jubran 2017-08-13 15:58 ` [Qemu-devel] [PATCH 2/3] qga: main: make qga config and socket activation global Sameeh Jubran 2017-08-13 15:58 ` [Qemu-devel] [PATCH 3/3] qga: Prevent qemu-ga exit if serial doesn't exist Sameeh Jubran 2017-08-22 11:18 ` [Qemu-devel] [PATCH 0/3] Sameeh Jubran 2017-09-04 13:48 ` Sameeh Jubran 2017-10-02 13:02 ` Sameeh Jubran 2017-10-26 23:51 ` Michael Roth 2017-10-27 8:08 ` Sameeh Jubran 2018-01-22 14:24 ` Sameeh Jubran 2018-01-25 23:49 ` Michael Roth
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.