From bad0227d3ac4f706673df9690b77840ed89dec40 Mon Sep 17 00:00:00 2001 From: Jonathon Reinhart Date: Wed, 1 Aug 2018 19:10:59 -0400 Subject: [PATCH 01/24] qga: Support Unicode paths in guest-file-open on win32 Currently, the win32 port of QEMU Guest Agent does not properly handle Unicode paths. The JSON decoder produces a valid UTF-8 path string, but this is passed directly to CreateFileA, which is expecting an ANSI string and not UTF-8. This leads to mangled filenames. This patch follows the example of qmp_guest_set_user_password() and uses g_utf8_to_utf16() to convert the string to UTF-16 and calls CreateFileW() explicitly. Signed-off-by: Jonathon Reinhart Signed-off-by: Michael Roth --- qga/commands-win32.c | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/qga/commands-win32.c b/qga/commands-win32.c index 98d9735389..416343b97b 100644 --- a/qga/commands-win32.c +++ b/qga/commands-win32.c @@ -160,13 +160,15 @@ static void handle_set_nonblocking(HANDLE fh) int64_t qmp_guest_file_open(const char *path, bool has_mode, const char *mode, Error **errp) { - int64_t fd; + int64_t fd = -1; HANDLE fh; HANDLE templ_file = NULL; DWORD share_mode = FILE_SHARE_READ; DWORD flags_and_attr = FILE_ATTRIBUTE_NORMAL; LPSECURITY_ATTRIBUTES sa_attr = NULL; OpenFlags *guest_flags; + GError *gerr = NULL; + wchar_t *w_path = NULL; if (!has_mode) { mode = "r"; @@ -175,16 +177,21 @@ int64_t qmp_guest_file_open(const char *path, bool has_mode, guest_flags = find_open_flag(mode); if (guest_flags == NULL) { error_setg(errp, "invalid file open mode"); - return -1; + goto done; } - fh = CreateFile(path, guest_flags->desired_access, share_mode, sa_attr, + w_path = g_utf8_to_utf16(path, -1, NULL, NULL, &gerr); + if (!w_path) { + goto done; + } + + fh = CreateFileW(w_path, guest_flags->desired_access, share_mode, sa_attr, guest_flags->creation_disposition, flags_and_attr, templ_file); if (fh == INVALID_HANDLE_VALUE) { error_setg_win32(errp, GetLastError(), "failed to open file '%s'", path); - return -1; + goto done; } /* set fd non-blocking to avoid common use cases (like reading from a @@ -196,10 +203,17 @@ int64_t qmp_guest_file_open(const char *path, bool has_mode, if (fd < 0) { CloseHandle(fh); error_setg(errp, "failed to add handle to qmp handle table"); - return -1; + goto done; } slog("guest-file-open, handle: % " PRId64, fd); + +done: + if (gerr) { + error_setg(errp, QERR_QGA_COMMAND_FAILED, gerr->message); + g_error_free(gerr); + } + g_free(w_path); return fd; } From 0692b03ee135f6295168082199af55c6f289794d Mon Sep 17 00:00:00 2001 From: Chen Hanxiao Date: Fri, 31 Aug 2018 14:22:50 +0800 Subject: [PATCH 02/24] qga-win: add support for qmp_guest_fsfreeze_freeze_list This patch add support for freeze specified fs. The valid mountpoints list member are [1]: The path of a mounted folder, for example, Y:\MountX\ A drive letter, for example, D:\ A volume GUID path of the form \\?\Volume{GUID}\, where GUID identifies the volume A UNC path that specifies a remote file share, for example, \\Clusterx\Share1\ [1] https://docs.microsoft.com/en-us/windows/desktop/api/vsbackup/nf-vsbackup-ivssbackupcomponents-addtosnapshotset Cc: Michael Roth Signed-off-by: Chen Hanxiao Signed-off-by: Michael Roth --- qga/commands-win32.c | 21 ++++----- qga/main.c | 2 +- qga/vss-win32.c | 5 +- qga/vss-win32.h | 3 +- qga/vss-win32/requester.cpp | 92 ++++++++++++++++++++++++++----------- qga/vss-win32/requester.h | 13 ++++-- 6 files changed, 91 insertions(+), 45 deletions(-) diff --git a/qga/commands-win32.c b/qga/commands-win32.c index 416343b97b..347577f2a4 100644 --- a/qga/commands-win32.c +++ b/qga/commands-win32.c @@ -790,6 +790,13 @@ GuestFsfreezeStatus qmp_guest_fsfreeze_status(Error **errp) * The frozen state is limited for up to 10 seconds by VSS. */ int64_t qmp_guest_fsfreeze_freeze(Error **errp) +{ + return qmp_guest_fsfreeze_freeze_list(false, NULL, errp); +} + +int64_t qmp_guest_fsfreeze_freeze_list(bool has_mountpoints, + strList *mountpoints, + Error **errp) { int i; Error *local_err = NULL; @@ -804,7 +811,7 @@ int64_t qmp_guest_fsfreeze_freeze(Error **errp) /* cannot risk guest agent blocking itself on a write in this state */ ga_set_frozen(ga_state); - qga_vss_fsfreeze(&i, true, &local_err); + qga_vss_fsfreeze(&i, true, mountpoints, &local_err); if (local_err) { error_propagate(errp, local_err); goto error; @@ -822,15 +829,6 @@ error: return 0; } -int64_t qmp_guest_fsfreeze_freeze_list(bool has_mountpoints, - strList *mountpoints, - Error **errp) -{ - error_setg(errp, QERR_UNSUPPORTED); - - return 0; -} - /* * Thaw local file systems using Volume Shadow-copy Service. */ @@ -843,7 +841,7 @@ int64_t qmp_guest_fsfreeze_thaw(Error **errp) return 0; } - qga_vss_fsfreeze(&i, false, errp); + qga_vss_fsfreeze(&i, false, NULL, errp); ga_unset_frozen(ga_state); return i; @@ -1660,7 +1658,6 @@ GList *ga_command_blacklist_init(GList *blacklist) "guest-set-vcpus", "guest-get-memory-blocks", "guest-set-memory-blocks", "guest-get-memory-block-size", - "guest-fsfreeze-freeze-list", NULL}; char **p = (char **)list_unsupported; diff --git a/qga/main.c b/qga/main.c index c399320d3c..afcd268ee3 100644 --- a/qga/main.c +++ b/qga/main.c @@ -151,7 +151,7 @@ static void quit_handler(int sig) WaitForSingleObject(hEventTimeout, 0); CloseHandle(hEventTimeout); } - qga_vss_fsfreeze(&i, false, &err); + qga_vss_fsfreeze(&i, false, NULL, &err); if (err) { g_debug("Error unfreezing filesystems prior to exiting: %s", error_get_pretty(err)); diff --git a/qga/vss-win32.c b/qga/vss-win32.c index a541f3ae01..f444a25a70 100644 --- a/qga/vss-win32.c +++ b/qga/vss-win32.c @@ -147,7 +147,8 @@ void ga_uninstall_vss_provider(void) } /* Call VSS requester and freeze/thaw filesystems and applications */ -void qga_vss_fsfreeze(int *nr_volume, bool freeze, Error **errp) +void qga_vss_fsfreeze(int *nr_volume, bool freeze, + strList *mountpoints, Error **errp) { const char *func_name = freeze ? "requester_freeze" : "requester_thaw"; QGAVSSRequesterFunc func; @@ -164,5 +165,5 @@ void qga_vss_fsfreeze(int *nr_volume, bool freeze, Error **errp) return; } - func(nr_volume, &errset); + func(nr_volume, mountpoints, &errset); } diff --git a/qga/vss-win32.h b/qga/vss-win32.h index 4f8e39aa5c..ce2abe5a72 100644 --- a/qga/vss-win32.h +++ b/qga/vss-win32.h @@ -22,6 +22,7 @@ bool vss_initialized(void); int ga_install_vss_provider(void); void ga_uninstall_vss_provider(void); -void qga_vss_fsfreeze(int *nr_volume, bool freeze, Error **errp); +void qga_vss_fsfreeze(int *nr_volume, bool freeze, + strList *mountpints, Error **errp); #endif diff --git a/qga/vss-win32/requester.cpp b/qga/vss-win32/requester.cpp index 3d9c9716c0..5378c55d23 100644 --- a/qga/vss-win32/requester.cpp +++ b/qga/vss-win32/requester.cpp @@ -234,7 +234,7 @@ out: } } -void requester_freeze(int *num_vols, ErrorSet *errset) +void requester_freeze(int *num_vols, void *mountpoints, ErrorSet *errset) { COMPointer pAsync; HANDLE volume; @@ -246,6 +246,7 @@ void requester_freeze(int *num_vols, ErrorSet *errset) WCHAR short_volume_name[64], *display_name = short_volume_name; DWORD wait_status; int num_fixed_drives = 0, i; + int num_mount_points = 0; if (vss_ctx.pVssbc) { /* already frozen */ *num_vols = 0; @@ -337,39 +338,73 @@ void requester_freeze(int *num_vols, ErrorSet *errset) goto out; } - volume = FindFirstVolumeW(short_volume_name, sizeof(short_volume_name)); - if (volume == INVALID_HANDLE_VALUE) { - err_set(errset, hr, "failed to find first volume"); - goto out; - } - for (;;) { - if (GetDriveTypeW(short_volume_name) == DRIVE_FIXED) { + if (mountpoints) { + PWCHAR volume_name_wchar; + for (volList *list = (volList *)mountpoints; list; list = list->next) { + size_t len = strlen(list->value) + 1; + size_t converted = 0; VSS_ID pid; - hr = vss_ctx.pVssbc->AddToSnapshotSet(short_volume_name, + + volume_name_wchar = new wchar_t[len]; + mbstowcs_s(&converted, volume_name_wchar, len, + list->value, _TRUNCATE); + + hr = vss_ctx.pVssbc->AddToSnapshotSet(volume_name_wchar, g_gProviderId, &pid); if (FAILED(hr)) { - WCHAR volume_path_name[PATH_MAX]; - if (GetVolumePathNamesForVolumeNameW( - short_volume_name, volume_path_name, - sizeof(volume_path_name), NULL) && *volume_path_name) { - display_name = volume_path_name; - } err_set(errset, hr, "failed to add %S to snapshot set", - display_name); - FindVolumeClose(volume); + volume_name_wchar); + delete volume_name_wchar; goto out; } - num_fixed_drives++; + num_mount_points++; + + delete volume_name_wchar; } - if (!FindNextVolumeW(volume, short_volume_name, - sizeof(short_volume_name))) { - FindVolumeClose(volume); - break; + + if (num_mount_points == 0) { + /* If there is no valid mount points, just exit. */ + goto out; } } - if (num_fixed_drives == 0) { - goto out; /* If there is no fixed drive, just exit. */ + if (!mountpoints) { + volume = FindFirstVolumeW(short_volume_name, sizeof(short_volume_name)); + if (volume == INVALID_HANDLE_VALUE) { + err_set(errset, hr, "failed to find first volume"); + goto out; + } + + for (;;) { + if (GetDriveTypeW(short_volume_name) == DRIVE_FIXED) { + VSS_ID pid; + hr = vss_ctx.pVssbc->AddToSnapshotSet(short_volume_name, + g_gProviderId, &pid); + if (FAILED(hr)) { + WCHAR volume_path_name[PATH_MAX]; + if (GetVolumePathNamesForVolumeNameW( + short_volume_name, volume_path_name, + sizeof(volume_path_name), NULL) && + *volume_path_name) { + display_name = volume_path_name; + } + err_set(errset, hr, "failed to add %S to snapshot set", + display_name); + FindVolumeClose(volume); + goto out; + } + num_fixed_drives++; + } + if (!FindNextVolumeW(volume, short_volume_name, + sizeof(short_volume_name))) { + FindVolumeClose(volume); + break; + } + } + + if (num_fixed_drives == 0) { + goto out; /* If there is no fixed drive, just exit. */ + } } hr = vss_ctx.pVssbc->PrepareForBackup(pAsync.replace()); @@ -435,7 +470,12 @@ void requester_freeze(int *num_vols, ErrorSet *errset) goto out; } - *num_vols = vss_ctx.cFrozenVols = num_fixed_drives; + if (mountpoints) { + *num_vols = vss_ctx.cFrozenVols = num_mount_points; + } else { + *num_vols = vss_ctx.cFrozenVols = num_fixed_drives; + } + return; out: @@ -449,7 +489,7 @@ out1: } -void requester_thaw(int *num_vols, ErrorSet *errset) +void requester_thaw(int *num_vols, void *mountpints, ErrorSet *errset) { COMPointer pAsync; diff --git a/qga/vss-win32/requester.h b/qga/vss-win32/requester.h index 2a39d734a2..5a8e8faf0c 100644 --- a/qga/vss-win32/requester.h +++ b/qga/vss-win32/requester.h @@ -34,9 +34,16 @@ typedef struct ErrorSet { STDAPI requester_init(void); STDAPI requester_deinit(void); -typedef void (*QGAVSSRequesterFunc)(int *, ErrorSet *); -void requester_freeze(int *num_vols, ErrorSet *errset); -void requester_thaw(int *num_vols, ErrorSet *errset); +typedef struct volList volList; + +struct volList { + volList *next; + char *value; +}; + +typedef void (*QGAVSSRequesterFunc)(int *, void *, ErrorSet *); +void requester_freeze(int *num_vols, void *volList, ErrorSet *errset); +void requester_thaw(int *num_vols, void *volList, ErrorSet *errset); #ifdef __cplusplus } From b4bf912a6c19449e68af7b4173a8c6da21904d99 Mon Sep 17 00:00:00 2001 From: Igor Mammedov Date: Thu, 6 Sep 2018 14:51:54 +0200 Subject: [PATCH 03/24] qga: ignore non present cpus when handling qmp_guest_get_vcpus() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If VM has VCPUs plugged sparselly (for example a VM started with 3 VCPUs (cpu0, cpu1 and cpu2) and then cpu1 was hotunplugged so only cpu0 and cpu2 are present), QGA will rise a error error: internal error: unable to execute QEMU agent command 'guest-get-vcpus': open("/sys/devices/system/cpu/cpu1/"): No such file or directory when virsh vcpucount FOO --guest is executed. Fix it by ignoring non present CPUs when fetching CPUs status from sysfs. Signed-off-by: Igor Mammedov Reviewed-by: Laszlo Ersek Reviewed-by: Marc-André Lureau Signed-off-by: Michael Roth --- qga/commands-posix.c | 111 ++++++++++++++++++++++--------------------- 1 file changed, 57 insertions(+), 54 deletions(-) diff --git a/qga/commands-posix.c b/qga/commands-posix.c index 37e8a2d791..42d30f078e 100644 --- a/qga/commands-posix.c +++ b/qga/commands-posix.c @@ -2035,61 +2035,56 @@ static long sysconf_exact(int name, const char *name_str, Error **errp) * Written members remain unmodified on error. */ static void transfer_vcpu(GuestLogicalProcessor *vcpu, bool sys2vcpu, - Error **errp) + char *dirpath, Error **errp) { - char *dirpath; + int fd; + int res; int dirfd; + static const char fn[] = "online"; - dirpath = g_strdup_printf("/sys/devices/system/cpu/cpu%" PRId64 "/", - vcpu->logical_id); dirfd = open(dirpath, O_RDONLY | O_DIRECTORY); if (dirfd == -1) { error_setg_errno(errp, errno, "open(\"%s\")", dirpath); + return; + } + + fd = openat(dirfd, fn, sys2vcpu ? O_RDONLY : O_RDWR); + if (fd == -1) { + if (errno != ENOENT) { + error_setg_errno(errp, errno, "open(\"%s/%s\")", dirpath, fn); + } else if (sys2vcpu) { + vcpu->online = true; + vcpu->can_offline = false; + } else if (!vcpu->online) { + error_setg(errp, "logical processor #%" PRId64 " can't be " + "offlined", vcpu->logical_id); + } /* otherwise pretend successful re-onlining */ } else { - static const char fn[] = "online"; - int fd; - int res; + unsigned char status; - fd = openat(dirfd, fn, sys2vcpu ? O_RDONLY : O_RDWR); - if (fd == -1) { - if (errno != ENOENT) { - error_setg_errno(errp, errno, "open(\"%s/%s\")", dirpath, fn); - } else if (sys2vcpu) { - vcpu->online = true; - vcpu->can_offline = false; - } else if (!vcpu->online) { - error_setg(errp, "logical processor #%" PRId64 " can't be " - "offlined", vcpu->logical_id); - } /* otherwise pretend successful re-onlining */ - } else { - unsigned char status; + res = pread(fd, &status, 1, 0); + if (res == -1) { + error_setg_errno(errp, errno, "pread(\"%s/%s\")", dirpath, fn); + } else if (res == 0) { + error_setg(errp, "pread(\"%s/%s\"): unexpected EOF", dirpath, + fn); + } else if (sys2vcpu) { + vcpu->online = (status != '0'); + vcpu->can_offline = true; + } else if (vcpu->online != (status != '0')) { + status = '0' + vcpu->online; + if (pwrite(fd, &status, 1, 0) == -1) { + error_setg_errno(errp, errno, "pwrite(\"%s/%s\")", dirpath, + fn); + } + } /* otherwise pretend successful re-(on|off)-lining */ - res = pread(fd, &status, 1, 0); - if (res == -1) { - error_setg_errno(errp, errno, "pread(\"%s/%s\")", dirpath, fn); - } else if (res == 0) { - error_setg(errp, "pread(\"%s/%s\"): unexpected EOF", dirpath, - fn); - } else if (sys2vcpu) { - vcpu->online = (status != '0'); - vcpu->can_offline = true; - } else if (vcpu->online != (status != '0')) { - status = '0' + vcpu->online; - if (pwrite(fd, &status, 1, 0) == -1) { - error_setg_errno(errp, errno, "pwrite(\"%s/%s\")", dirpath, - fn); - } - } /* otherwise pretend successful re-(on|off)-lining */ - - res = close(fd); - g_assert(res == 0); - } - - res = close(dirfd); + res = close(fd); g_assert(res == 0); } - g_free(dirpath); + res = close(dirfd); + g_assert(res == 0); } GuestLogicalProcessorList *qmp_guest_get_vcpus(Error **errp) @@ -2107,17 +2102,21 @@ GuestLogicalProcessorList *qmp_guest_get_vcpus(Error **errp) while (local_err == NULL && current < sc_max) { GuestLogicalProcessor *vcpu; GuestLogicalProcessorList *entry; + int64_t id = current++; + char *path = g_strdup_printf("/sys/devices/system/cpu/cpu%" PRId64 "/", + id); - vcpu = g_malloc0(sizeof *vcpu); - vcpu->logical_id = current++; - vcpu->has_can_offline = true; /* lolspeak ftw */ - transfer_vcpu(vcpu, true, &local_err); - - entry = g_malloc0(sizeof *entry); - entry->value = vcpu; - - *link = entry; - link = &entry->next; + if (g_file_test(path, G_FILE_TEST_EXISTS)) { + vcpu = g_malloc0(sizeof *vcpu); + vcpu->logical_id = id; + vcpu->has_can_offline = true; /* lolspeak ftw */ + transfer_vcpu(vcpu, true, path, &local_err); + entry = g_malloc0(sizeof *entry); + entry->value = vcpu; + *link = entry; + link = &entry->next; + } + g_free(path); } if (local_err == NULL) { @@ -2138,7 +2137,11 @@ int64_t qmp_guest_set_vcpus(GuestLogicalProcessorList *vcpus, Error **errp) processed = 0; while (vcpus != NULL) { - transfer_vcpu(vcpus->value, false, &local_err); + char *path = g_strdup_printf("/sys/devices/system/cpu/cpu%" PRId64 "/", + vcpus->value->logical_id); + + transfer_vcpu(vcpus->value, false, path, &local_err); + g_free(path); if (local_err != NULL) { break; } From 3efac6ebb88e4d099f07fef65178ebaa595ae770 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Golembiovsk=C3=BD?= Date: Tue, 23 Oct 2018 13:23:10 +0200 Subject: [PATCH 04/24] configure: add test for libudev MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Tomáš Golembiovský Reviewed-by: Marc-André Lureau *make libudev optional to avoid breaking existing build/test environments *disable libudev for --static builds Signed-off-by: Michael Roth --- configure | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/configure b/configure index a898e21c68..61276982c6 100755 --- a/configure +++ b/configure @@ -474,6 +474,7 @@ libxml2="" docker="no" debug_mutex="no" libpmem="" +libudev="no" # cross compilers defaults, can be overridden with --cross-cc-ARCH cross_cc_aarch64="aarch64-linux-gnu-gcc" @@ -870,6 +871,7 @@ Linux) vhost_vsock="yes" QEMU_INCLUDES="-I\$(SRC_PATH)/linux-headers -I$(pwd)/linux-headers $QEMU_INCLUDES" supported_os="yes" + libudev="yes" ;; esac @@ -5609,6 +5611,17 @@ if test "$libnfs" != "no" ; then fi fi +########################################## +# Do we have libudev +if test "$libudev" != "no" ; then + if $pkg_config libudev && test "$static" != "yes"; then + libudev="yes" + libudev_libs=$($pkg_config --libs libudev) + else + libudev="no" + fi +fi + # Now we've finished running tests it's OK to add -Werror to the compiler flags if test "$werror" = "yes"; then QEMU_CFLAGS="-Werror $QEMU_CFLAGS" @@ -6033,6 +6046,7 @@ echo "VxHS block device $vxhs" echo "capstone $capstone" echo "docker $docker" echo "libpmem support $libpmem" +echo "libudev $libudev" if test "$sdl_too_old" = "yes"; then echo "-> Your SDL version is too old - please upgrade to have SDL support" @@ -6868,6 +6882,11 @@ if test "$docker" != "no"; then echo "HAVE_USER_DOCKER=y" >> $config_host_mak fi +if test "$libudev" != "no"; then + echo "CONFIG_LIBUDEV=y" >> $config_host_mak + echo "LIBUDEV_LIBS=$libudev_libs" >> $config_host_mak +fi + # use included Linux headers if test "$linux" = "yes" ; then mkdir -p linux-headers From b616105a904bc350a409e12c39af0ae210900124 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Golembiovsk=C3=BD?= Date: Tue, 23 Oct 2018 13:23:11 +0200 Subject: [PATCH 05/24] qga: linux: report disk serial number MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add reporting of disk serial number on Linux guests. The feature depends on libudev. Example: { "name": "dm-2", "mountpoint": "/", ... "disk": [ { "serial": "SAMSUNG_MZ7LN512HCHP-000L1_S1ZKNXAG822493", ... } ], } Signed-off-by: Tomáš Golembiovský Signed-off-by: Michael Roth --- qga/Makefile.objs | 1 + qga/commands-posix.c | 32 ++++++++++++++++++++++++++++++-- qga/qapi-schema.json | 4 +++- 3 files changed, 34 insertions(+), 3 deletions(-) diff --git a/qga/Makefile.objs b/qga/Makefile.objs index ed08c5917c..80e6bb3c2e 100644 --- a/qga/Makefile.objs +++ b/qga/Makefile.objs @@ -1,3 +1,4 @@ +commands-posix.o-libs := $(LIBUDEV_LIBS) qga-obj-y = commands.o guest-agent-command-state.o main.o qga-obj-$(CONFIG_POSIX) += commands-posix.o channel-posix.o qga-obj-$(CONFIG_WIN32) += commands-win32.o channel-win32.o service-win32.o diff --git a/qga/commands-posix.c b/qga/commands-posix.c index 42d30f078e..41bd11b2b5 100644 --- a/qga/commands-posix.c +++ b/qga/commands-posix.c @@ -48,6 +48,10 @@ extern char **environ; #include #include +#ifdef CONFIG_LIBUDEV +#include +#endif + #ifdef FIFREEZE #define CONFIG_FSFREEZE #endif @@ -872,6 +876,10 @@ static void build_guest_fsinfo_for_real_device(char const *syspath, GuestDiskAddressList *list = NULL; bool has_ata = false, has_host = false, has_tgt = false; char *p, *q, *driver = NULL; +#ifdef CONFIG_LIBUDEV + struct udev *udev = NULL; + struct udev_device *udevice = NULL; +#endif p = strstr(syspath, "/devices/pci"); if (!p || sscanf(p + 12, "%*x:%*x/%x:%x:%x.%x%n", @@ -936,6 +944,21 @@ static void build_guest_fsinfo_for_real_device(char const *syspath, list = g_malloc0(sizeof(*list)); list->value = disk; +#ifdef CONFIG_LIBUDEV + udev = udev_new(); + udevice = udev_device_new_from_syspath(udev, syspath); + if (udev == NULL || udevice == NULL) { + g_debug("failed to query udev"); + } else { + const char *serial; + serial = udev_device_get_property_value(udevice, "ID_SERIAL"); + if (serial != NULL && *serial != 0) { + disk->serial = g_strdup(serial); + disk->has_serial = true; + } + } +#endif + if (strcmp(driver, "ata_piix") == 0) { /* a host per ide bus, target*:0::0 */ if (!has_host || !has_tgt) { @@ -995,14 +1018,19 @@ static void build_guest_fsinfo_for_real_device(char const *syspath, list->next = fs->disk; fs->disk = list; - g_free(driver); - return; + goto out; cleanup: if (list) { qapi_free_GuestDiskAddressList(list); } +out: g_free(driver); +#ifdef CONFIG_LIBUDEV + udev_unref(udev); + udev_device_unref(udevice); +#endif + return; } static void build_guest_fsinfo_for_device(char const *devpath, diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json index dfbc4a5e32..3bcda6257e 100644 --- a/qga/qapi-schema.json +++ b/qga/qapi-schema.json @@ -834,13 +834,15 @@ # @bus: bus id # @target: target id # @unit: unit id +# @serial: serial number (since: 3.1) # # Since: 2.2 ## { 'struct': 'GuestDiskAddress', 'data': {'pci-controller': 'GuestPCIAddress', 'bus-type': 'GuestDiskBusType', - 'bus': 'int', 'target': 'int', 'unit': 'int'} } + 'bus': 'int', 'target': 'int', 'unit': 'int', + '*serial': 'str'} } ## # @GuestFilesystemInfo: From 6589ce35734e7e71463485650e5fb6e4bbe64729 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Golembiovsk=C3=BD?= Date: Tue, 23 Oct 2018 13:23:12 +0200 Subject: [PATCH 06/24] qga: linux: return disk device in guest-get-fsinfo MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Report device node of the disk on Linux (e.g. "/dev/sda2"). Requirs libudev. Signed-off-by: Tomáš Golembiovský Signed-off-by: Michael Roth --- qga/commands-posix.c | 7 ++++++- qga/qapi-schema.json | 3 ++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/qga/commands-posix.c b/qga/commands-posix.c index 41bd11b2b5..1877976522 100644 --- a/qga/commands-posix.c +++ b/qga/commands-posix.c @@ -950,7 +950,12 @@ static void build_guest_fsinfo_for_real_device(char const *syspath, if (udev == NULL || udevice == NULL) { g_debug("failed to query udev"); } else { - const char *serial; + const char *devnode, *serial; + devnode = udev_device_get_devnode(udevice); + if (devnode != NULL) { + disk->dev = g_strdup(devnode); + disk->has_dev = true; + } serial = udev_device_get_property_value(udevice, "ID_SERIAL"); if (serial != NULL && *serial != 0) { disk->serial = g_strdup(serial); diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json index 3bcda6257e..c6725b3ec8 100644 --- a/qga/qapi-schema.json +++ b/qga/qapi-schema.json @@ -835,6 +835,7 @@ # @target: target id # @unit: unit id # @serial: serial number (since: 3.1) +# @dev: device node (POSIX) or device UNC (Windows) (since: 3.1) # # Since: 2.2 ## @@ -842,7 +843,7 @@ 'data': {'pci-controller': 'GuestPCIAddress', 'bus-type': 'GuestDiskBusType', 'bus': 'int', 'target': 'int', 'unit': 'int', - '*serial': 'str'} } + '*serial': 'str', '*dev': 'str'} } ## # @GuestFilesystemInfo: From 0d7f937e2a3ef6d80e8cb3d2cbca95f7365b451e Mon Sep 17 00:00:00 2001 From: Sameeh Jubran Date: Tue, 23 Oct 2018 13:23:13 +0200 Subject: [PATCH 07/24] qga-win: prevent crash when executing fsinfo command MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The fsinfo command is currently implemented for Windows only and it's disk parameter can be enabled by adding the define "CONFIG_QGA_NTDDSCSI" to the qga code. When enabled and executed the qemu-ga crashed with the following message: ------------------------------------------------ File qapi/qapi-visit-core.c, Line 49 Expression: !(v->type & VISITOR_OUTPUT) || *obj) ------------------------------------------------ After some digging, turns out that the GuestPCIAddress is null and the qapi visitor doesn't like that, so we can always allocate it instead and initiate all it's members to -1. Signed-off-by: Sameeh Jubran Reviewed-by: Philippe Mathieu-Daudé Signed-off-by: Michael Roth Signed-off-by: Tomáš Golembiovský Signed-off-by: Michael Roth --- qga/commands-win32.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/qga/commands-win32.c b/qga/commands-win32.c index 347577f2a4..f0e6f6128b 100644 --- a/qga/commands-win32.c +++ b/qga/commands-win32.c @@ -499,6 +499,11 @@ static GuestPCIAddress *get_pci_info(char *guid, Error **errp) char *buffer = NULL; GuestPCIAddress *pci = NULL; char *name = g_strdup(&guid[4]); + pci = g_malloc0(sizeof(*pci)); + pci->domain = -1; + pci->slot = -1; + pci->function = -1; + pci->bus = -1; if (!QueryDosDevice(name, dev_name, ARRAY_SIZE(dev_name))) { error_setg_win32(errp, GetLastError(), "failed to get dos device name"); @@ -570,7 +575,6 @@ static GuestPCIAddress *get_pci_info(char *guid, Error **errp) func = addr & 0x0000FFFF; dev = (addr >> 16) & 0x0000FFFF; - pci = g_malloc0(sizeof(*pci)); pci->domain = dev; pci->slot = slot; pci->function = func; From 6880b94f8a53e34833bf895b3004b2e48c0ffe74 Mon Sep 17 00:00:00 2001 From: Sameeh Jubran Date: Tue, 23 Oct 2018 13:23:14 +0200 Subject: [PATCH 08/24] qga-win: fsinfo: pci-info: allow partial info MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The call to SetupDiGetDeviceRegistryProperty might fail because the value doesn't exist in the registry, in this case we shouldn't exit from the loop but instead continue to look for other available values in the registry and set this value as unavailable (-1). Signed-off-by: Sameeh Jubran Signed-off-by: Michael Roth Signed-off-by: Tomáš Golembiovský *squash in fix for when get_pci_info() returns NULL pci_controller field *fix handling for error_set() cases in get_pci_info(), not just NULL return *force all -1 PCI addr fields if any single one of them isn't found Signed-off-by: Michael Roth --- qga/commands-win32.c | 46 +++++++++++++++++++++++++++++++------------- 1 file changed, 33 insertions(+), 13 deletions(-) diff --git a/qga/commands-win32.c b/qga/commands-win32.c index f0e6f6128b..4fe1517778 100644 --- a/qga/commands-win32.c +++ b/qga/commands-win32.c @@ -499,6 +499,7 @@ static GuestPCIAddress *get_pci_info(char *guid, Error **errp) char *buffer = NULL; GuestPCIAddress *pci = NULL; char *name = g_strdup(&guid[4]); + bool partial_pci = false; pci = g_malloc0(sizeof(*pci)); pci->domain = -1; pci->slot = -1; @@ -519,7 +520,8 @@ static GuestPCIAddress *get_pci_info(char *guid, Error **errp) dev_info_data.cbSize = sizeof(SP_DEVINFO_DATA); for (i = 0; SetupDiEnumDeviceInfo(dev_info, i, &dev_info_data); i++) { - DWORD addr, bus, slot, func, dev, data, size2; + DWORD addr, bus, slot, data, size2; + int func, dev; while (!SetupDiGetDeviceRegistryProperty(dev_info, &dev_info_data, SPDRP_PHYSICAL_DEVICE_OBJECT_NAME, &data, (PBYTE)buffer, size, @@ -549,21 +551,24 @@ static GuestPCIAddress *get_pci_info(char *guid, Error **errp) */ if (!SetupDiGetDeviceRegistryProperty(dev_info, &dev_info_data, SPDRP_BUSNUMBER, &data, (PBYTE)&bus, size, NULL)) { - break; + bus = -1; + partial_pci = true; } /* The function retrieves the device's address. This value will be * transformed into device function and number */ if (!SetupDiGetDeviceRegistryProperty(dev_info, &dev_info_data, SPDRP_ADDRESS, &data, (PBYTE)&addr, size, NULL)) { - break; + addr = -1; + partial_pci = true; } /* This call returns UINumber of DEVICE_CAPABILITIES structure. * This number is typically a user-perceived slot number. */ if (!SetupDiGetDeviceRegistryProperty(dev_info, &dev_info_data, SPDRP_UI_NUMBER, &data, (PBYTE)&slot, size, NULL)) { - break; + slot = -1; + partial_pci = true; } /* SetupApi gives us the same information as driver with @@ -573,12 +578,19 @@ static GuestPCIAddress *get_pci_info(char *guid, Error **errp) * DeviceNumber = (USHORT)(((propertyAddress) >> 16) & 0x0000FFFF); * SPDRP_ADDRESS is propertyAddress, so we do the same.*/ - func = addr & 0x0000FFFF; - dev = (addr >> 16) & 0x0000FFFF; - pci->domain = dev; - pci->slot = slot; - pci->function = func; - pci->bus = bus; + if (partial_pci) { + pci->domain = -1; + pci->slot = -1; + pci->function = -1; + pci->bus = -1; + } else { + func = ((int) addr == -1) ? -1 : addr & 0x0000FFFF; + dev = ((int) addr == -1) ? -1 : (addr >> 16) & 0x0000FFFF; + pci->domain = dev; + pci->slot = (int) slot; + pci->function = func; + pci->bus = (int) bus; + } break; } @@ -622,6 +634,7 @@ static GuestDiskAddressList *build_guest_disk_info(char *guid, Error **errp) DWORD len; int bus; HANDLE vol_h; + Error *local_err = NULL; scsi_ad = &addr; char *name = g_strndup(guid, strlen(guid)-1); @@ -640,6 +653,16 @@ static GuestDiskAddressList *build_guest_disk_info(char *guid, Error **errp) disk = g_malloc0(sizeof(*disk)); disk->bus_type = find_bus_type(bus); + /* always set pci_controller as required by schema. get_pci_info() should + * report -1 values for non-PCI buses rather than fail. fail the command + * if that doesn't hold since that suggests some other unexpected + * breakage + */ + disk->pci_controller = get_pci_info(name, &local_err); + if (local_err) { + error_propagate(errp, local_err); + goto out_close; + } if (bus == BusTypeScsi || bus == BusTypeAta || bus == BusTypeRAID #if (_WIN32_WINNT >= 0x0600) /* This bus type is not supported before Windows Server 2003 SP1 */ @@ -654,12 +677,9 @@ static GuestDiskAddressList *build_guest_disk_info(char *guid, Error **errp) disk->unit = addr.Lun; disk->target = addr.TargetId; disk->bus = addr.PathId; - disk->pci_controller = get_pci_info(name, errp); } /* We do not set error in this case, because we still have enough * information about volume. */ - } else { - disk->pci_controller = NULL; } list = g_malloc0(sizeof(*list)); From 76dc75ca549109499a7a6e275a80662f2390f686 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Golembiovsk=C3=BD?= Date: Tue, 23 Oct 2018 13:23:16 +0200 Subject: [PATCH 09/24] build: rename CONFIG_QGA_NTDDDISK to CONFIG_QGA_NTDDSCSI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There was inconsistency between commits: 50cbebb9a3 configure: add configure check for ntdddisk.h a3ef3b2272 qga: added bus type and disk location path The first commit added #define CONFIG_QGA_NTDDDISK but the second commit expected the name to be CONFIG_QGA_NTDDSCSI. As a result the code in second patch was never used. Renaming the option to CONFIG_QGA_NTDDSCSI to match the name of header file that is being checked for. Signed-off-by: Tomáš Golembiovský Reviewed-by: Marc-André Lureau Reviewed-by: Sameeh Jubran Signed-off-by: Michael Roth --- configure | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/configure b/configure index 61276982c6..95f3d44ecd 100755 --- a/configure +++ b/configure @@ -6142,7 +6142,7 @@ if test "$mingw32" = "yes" ; then echo "WIN_SDK=\"$win_sdk\"" >> $config_host_mak fi if test "$guest_agent_ntddscsi" = "yes" ; then - echo "CONFIG_QGA_NTDDDISK=y" >> $config_host_mak + echo "CONFIG_QGA_NTDDSCSI=y" >> $config_host_mak fi if test "$guest_agent_msi" = "yes"; then echo "QEMU_GA_MSI_ENABLED=yes" >> $config_host_mak From 222682abb3800b9c644b7832543510ade3d5138b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Golembiovsk=C3=BD?= Date: Mon, 29 Oct 2018 18:42:19 -0500 Subject: [PATCH 10/24] qga-win: add debugging information MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The windows code generaly lacks debug information (compared to posix code). This patch adds some related to HW info in guest-get-fsinfo command. Signed-off-by: Tomáš Golembiovský Reviewed-by: Marc-André Lureau Signed-off-by: Michael Roth --- qga/commands-win32.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/qga/commands-win32.c b/qga/commands-win32.c index 4fe1517778..1a21aac5ad 100644 --- a/qga/commands-win32.c +++ b/qga/commands-win32.c @@ -89,6 +89,12 @@ static OpenFlags guest_file_open_modes[] = { {"a+b", FILE_GENERIC_APPEND|GENERIC_READ, OPEN_ALWAYS } }; +#define debug_error(msg) do { \ + char *suffix = g_win32_error_message(GetLastError()); \ + g_debug("%s: %s", (msg), suffix); \ + g_free(suffix); \ +} while (0) + static OpenFlags *find_open_flag(const char *mode_str) { int mode; @@ -518,6 +524,7 @@ static GuestPCIAddress *get_pci_info(char *guid, Error **errp) goto out; } + g_debug("enumerating devices"); dev_info_data.cbSize = sizeof(SP_DEVINFO_DATA); for (i = 0; SetupDiEnumDeviceInfo(dev_info, i, &dev_info_data); i++) { DWORD addr, bus, slot, data, size2; @@ -543,6 +550,7 @@ static GuestPCIAddress *get_pci_info(char *guid, Error **errp) if (g_strcmp0(buffer, dev_name)) { continue; } + g_debug("found device %s", dev_name); /* There is no need to allocate buffer in the next functions. The size * is known and ULONG according to @@ -551,6 +559,7 @@ static GuestPCIAddress *get_pci_info(char *guid, Error **errp) */ if (!SetupDiGetDeviceRegistryProperty(dev_info, &dev_info_data, SPDRP_BUSNUMBER, &data, (PBYTE)&bus, size, NULL)) { + debug_error("failed to get bus"); bus = -1; partial_pci = true; } @@ -559,6 +568,7 @@ static GuestPCIAddress *get_pci_info(char *guid, Error **errp) * transformed into device function and number */ if (!SetupDiGetDeviceRegistryProperty(dev_info, &dev_info_data, SPDRP_ADDRESS, &data, (PBYTE)&addr, size, NULL)) { + debug_error("failed to get address"); addr = -1; partial_pci = true; } @@ -567,6 +577,7 @@ static GuestPCIAddress *get_pci_info(char *guid, Error **errp) * This number is typically a user-perceived slot number. */ if (!SetupDiGetDeviceRegistryProperty(dev_info, &dev_info_data, SPDRP_UI_NUMBER, &data, (PBYTE)&slot, size, NULL)) { + debug_error("failed to get slot"); slot = -1; partial_pci = true; } @@ -639,6 +650,7 @@ static GuestDiskAddressList *build_guest_disk_info(char *guid, Error **errp) scsi_ad = &addr; char *name = g_strndup(guid, strlen(guid)-1); + g_debug("getting disk info for: %s", name); vol_h = CreateFile(name, 0, FILE_SHARE_READ, NULL, OPEN_EXISTING, 0, NULL); if (vol_h == INVALID_HANDLE_VALUE) { @@ -646,6 +658,7 @@ static GuestDiskAddressList *build_guest_disk_info(char *guid, Error **errp) goto out_free; } + g_debug("getting bus type"); bus = get_disk_bus_type(vol_h, errp); if (bus < 0) { goto out_close; @@ -653,6 +666,7 @@ static GuestDiskAddressList *build_guest_disk_info(char *guid, Error **errp) disk = g_malloc0(sizeof(*disk)); disk->bus_type = find_bus_type(bus); + g_debug("bus type %d", disk->bus_type); /* always set pci_controller as required by schema. get_pci_info() should * report -1 values for non-PCI buses rather than fail. fail the command * if that doesn't hold since that suggests some other unexpected @@ -672,6 +686,7 @@ static GuestDiskAddressList *build_guest_disk_info(char *guid, Error **errp) /* We are able to use the same ioctls for different bus types * according to Microsoft docs * https://technet.microsoft.com/en-us/library/ee851589(v=ws.10).aspx */ + g_debug("getting pci-controller info"); if (DeviceIoControl(vol_h, IOCTL_SCSI_GET_ADDRESS, NULL, 0, scsi_ad, sizeof(SCSI_ADDRESS), &len, NULL)) { disk->unit = addr.Lun; From c76d70f498bd2dea9dafb7fcc2aa1b7a418f0f31 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Golembiovsk=C3=BD?= Date: Tue, 23 Oct 2018 13:23:18 +0200 Subject: [PATCH 11/24] qga-win: refactor disk properties (bus) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Refactor code that queries bus type to be more generic. The function get_disk_bus_type() has been renamed to build_guest_disk_info(). Following commit(s) will extend this function. Signed-off-by: Tomáš Golembiovský Signed-off-by: Michael Roth --- qga/commands-win32.c | 47 ++++++++++++++++++++++++++------------------ 1 file changed, 28 insertions(+), 19 deletions(-) diff --git a/qga/commands-win32.c b/qga/commands-win32.c index 1a21aac5ad..1e91aa2343 100644 --- a/qga/commands-win32.c +++ b/qga/commands-win32.c @@ -613,25 +613,28 @@ out: return pci; } -static int get_disk_bus_type(HANDLE vol_h, Error **errp) +static void get_disk_properties(HANDLE vol_h, GuestDiskAddress *disk, + Error **errp) { STORAGE_PROPERTY_QUERY query; STORAGE_DEVICE_DESCRIPTOR *dev_desc, buf; DWORD received; + ULONG size = sizeof(buf); dev_desc = &buf; - dev_desc->Size = sizeof(buf); query.PropertyId = StorageDeviceProperty; query.QueryType = PropertyStandardQuery; if (!DeviceIoControl(vol_h, IOCTL_STORAGE_QUERY_PROPERTY, &query, sizeof(STORAGE_PROPERTY_QUERY), dev_desc, - dev_desc->Size, &received, NULL)) { + size, &received, NULL)) { error_setg_win32(errp, GetLastError(), "failed to get bus type"); - return -1; + return; } + disk->bus_type = find_bus_type(dev_desc->BusType); + g_debug("bus type %d", disk->bus_type); - return dev_desc->BusType; + return; } /* VSS provider works with volumes, thus there is no difference if @@ -643,7 +646,6 @@ static GuestDiskAddressList *build_guest_disk_info(char *guid, Error **errp) GuestDiskAddress *disk; SCSI_ADDRESS addr, *scsi_ad; DWORD len; - int bus; HANDLE vol_h; Error *local_err = NULL; @@ -655,17 +657,16 @@ static GuestDiskAddressList *build_guest_disk_info(char *guid, Error **errp) 0, NULL); if (vol_h == INVALID_HANDLE_VALUE) { error_setg_win32(errp, GetLastError(), "failed to open volume"); - goto out_free; - } - - g_debug("getting bus type"); - bus = get_disk_bus_type(vol_h, errp); - if (bus < 0) { - goto out_close; + goto err; } disk = g_malloc0(sizeof(*disk)); - disk->bus_type = find_bus_type(bus); + get_disk_properties(vol_h, disk, &local_err); + if (local_err) { + error_propagate(errp, local_err); + goto err_close; + } + g_debug("bus type %d", disk->bus_type); /* always set pci_controller as required by schema. get_pci_info() should * report -1 values for non-PCI buses rather than fail. fail the command @@ -675,12 +676,14 @@ static GuestDiskAddressList *build_guest_disk_info(char *guid, Error **errp) disk->pci_controller = get_pci_info(name, &local_err); if (local_err) { error_propagate(errp, local_err); - goto out_close; + goto err_close; } - if (bus == BusTypeScsi || bus == BusTypeAta || bus == BusTypeRAID + if (disk->bus_type == GUEST_DISK_BUS_TYPE_SCSI + || disk->bus_type == GUEST_DISK_BUS_TYPE_IDE + || disk->bus_type == GUEST_DISK_BUS_TYPE_RAID #if (_WIN32_WINNT >= 0x0600) /* This bus type is not supported before Windows Server 2003 SP1 */ - || bus == BusTypeSas + || disk->bus_type == GUEST_DISK_BUS_TYPE_SAS #endif ) { /* We are able to use the same ioctls for different bus types @@ -700,11 +703,17 @@ static GuestDiskAddressList *build_guest_disk_info(char *guid, Error **errp) list = g_malloc0(sizeof(*list)); list->value = disk; list->next = NULL; -out_close: CloseHandle(vol_h); -out_free: g_free(name); return list; + +err_close: + g_free(disk); + CloseHandle(vol_h); +err: + g_free(name); + + return NULL; } #else From fb08aa703f4c601155aa65943eb29c1ad3972386 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Golembiovsk=C3=BD?= Date: Tue, 23 Oct 2018 13:23:19 +0200 Subject: [PATCH 12/24] qga-win: report disk serial number MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Tomáš Golembiovský *coding style fix-ups (declarations at beginning of block) *improve readability for user-visible errors *cover additional edge-cases with debug statements Signed-off-by: Michael Roth --- qga/commands-win32.c | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/qga/commands-win32.c b/qga/commands-win32.c index 1e91aa2343..2d7b56d538 100644 --- a/qga/commands-win32.c +++ b/qga/commands-win32.c @@ -634,6 +634,36 @@ static void get_disk_properties(HANDLE vol_h, GuestDiskAddress *disk, disk->bus_type = find_bus_type(dev_desc->BusType); g_debug("bus type %d", disk->bus_type); + /* Query once more. Now with long enough buffer. */ + size = dev_desc->Size; + dev_desc = g_malloc0(size); + if (!DeviceIoControl(vol_h, IOCTL_STORAGE_QUERY_PROPERTY, &query, + sizeof(STORAGE_PROPERTY_QUERY), dev_desc, + size, &received, NULL)) { + error_setg_win32(errp, GetLastError(), "failed to get serial number"); + g_debug("failed to get serial number"); + goto out_free; + } + if (dev_desc->SerialNumberOffset > 0) { + const char *serial; + size_t len; + + if (dev_desc->SerialNumberOffset >= received) { + error_setg(errp, "failed to get serial number: offset outside the buffer"); + g_debug("serial number offset outside the buffer"); + goto out_free; + } + serial = (char *)dev_desc + dev_desc->SerialNumberOffset; + len = received - dev_desc->SerialNumberOffset; + g_debug("serial number \"%s\"", serial); + if (*serial != 0) { + disk->serial = g_strndup(serial, len); + disk->has_serial = true; + } + } +out_free: + g_free(dev_desc); + return; } From 9e65fd659e1557c294d26a4daa3ae313312bcf68 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Golembiovsk=C3=BD?= Date: Tue, 23 Oct 2018 13:23:20 +0200 Subject: [PATCH 13/24] qga-win: refactor disk info MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Refactor building of disk info into a function that builds the list and a function that returns infor for single disk. This will be used in future commit that will handle multi-disk volumes. Signed-off-by: Tomáš Golembiovský Signed-off-by: Michael Roth --- qga/commands-win32.c | 52 +++++++++++++++++++++++++++++--------------- 1 file changed, 35 insertions(+), 17 deletions(-) diff --git a/qga/commands-win32.c b/qga/commands-win32.c index 2d7b56d538..21a88d31e1 100644 --- a/qga/commands-win32.c +++ b/qga/commands-win32.c @@ -667,20 +667,15 @@ out_free: return; } -/* VSS provider works with volumes, thus there is no difference if - * the volume consist of spanned disks. Info about the first disk in the - * volume is returned for the spanned disk group (LVM) */ -static GuestDiskAddressList *build_guest_disk_info(char *guid, Error **errp) +static void get_single_disk_info(char *name, GuestDiskAddress *disk, + Error **errp) { - GuestDiskAddressList *list = NULL; - GuestDiskAddress *disk; SCSI_ADDRESS addr, *scsi_ad; DWORD len; HANDLE vol_h; Error *local_err = NULL; scsi_ad = &addr; - char *name = g_strndup(guid, strlen(guid)-1); g_debug("getting disk info for: %s", name); vol_h = CreateFile(name, 0, FILE_SHARE_READ, NULL, OPEN_EXISTING, @@ -690,7 +685,6 @@ static GuestDiskAddressList *build_guest_disk_info(char *guid, Error **errp) goto err; } - disk = g_malloc0(sizeof(*disk)); get_disk_properties(vol_h, disk, &local_err); if (local_err) { error_propagate(errp, local_err); @@ -730,20 +724,44 @@ static GuestDiskAddressList *build_guest_disk_info(char *guid, Error **errp) * information about volume. */ } - list = g_malloc0(sizeof(*list)); - list->value = disk; - list->next = NULL; - CloseHandle(vol_h); - g_free(name); - return list; - err_close: - g_free(disk); CloseHandle(vol_h); err: + return; +} + +/* VSS provider works with volumes, thus there is no difference if + * the volume consist of spanned disks. Info about the first disk in the + * volume is returned for the spanned disk group (LVM) */ +static GuestDiskAddressList *build_guest_disk_info(char *guid, Error **errp) +{ + Error *local_err = NULL; + GuestDiskAddressList *list = NULL, *cur_item = NULL; + GuestDiskAddress *disk = NULL; + + /* strip final backslash */ + char *name = g_strdup(guid); + if (g_str_has_suffix(name, "\\")) { + name[strlen(name) - 1] = 0; + } + + disk = g_malloc0(sizeof(GuestDiskAddress)); + get_single_disk_info(name, disk, &local_err); + if (local_err) { + error_propagate(errp, local_err); + goto out; + } + + cur_item = g_malloc0(sizeof(*list)); + cur_item->value = disk; + disk = NULL; + list = cur_item; + +out: + qapi_free_GuestDiskAddress(disk); g_free(name); - return NULL; + return list; } #else From b1ba8890e63ce9432c41c5c3fc229f54c87c9c99 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Golembiovsk=C3=BD?= Date: Tue, 23 Oct 2018 13:23:21 +0200 Subject: [PATCH 14/24] qga-win: handle multi-disk volumes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Probe the volume for disk extents and return list of all disks. Originally only first disk of composite volume was returned. Note that the patch changes get_pci_info() from one state of brokenness into a different state of brokenness. In other words it still does not do what it's supposed to do (see comment in code). If anyone knows how to fix it, please step in. Signed-off-by: Tomáš Golembiovský Signed-off-by: Michael Roth --- qga/commands-win32.c | 126 ++++++++++++++++++++++++++++++++++++------- 1 file changed, 108 insertions(+), 18 deletions(-) diff --git a/qga/commands-win32.c b/qga/commands-win32.c index 21a88d31e1..90432bbfce 100644 --- a/qga/commands-win32.c +++ b/qga/commands-win32.c @@ -491,9 +491,26 @@ static GuestDiskBusType find_bus_type(STORAGE_BUS_TYPE bus) return win2qemu[(int)bus]; } +/* XXX: The following function is BROKEN! + * + * It does not work and probably has never worked. When we query for list of + * disks we get cryptic names like "\Device\0000001d" instead of + * "\PhysicalDriveX" or "\HarddiskX". Whether the names can be translated one + * way or the other for comparison is an open question. + * + * When we query volume names (the original version) we are able to match those + * but then the property queries report error "Invalid function". (duh!) + */ + +/* DEFINE_GUID(GUID_DEVINTERFACE_VOLUME, 0x53f5630dL, 0xb6bf, 0x11d0, 0x94, 0xf2, 0x00, 0xa0, 0xc9, 0x1e, 0xfb, 0x8b); +*/ +DEFINE_GUID(GUID_DEVINTERFACE_DISK, + 0x53f56307L, 0xb6bf, 0x11d0, 0x94, 0xf2, + 0x00, 0xa0, 0xc9, 0x1e, 0xfb, 0x8b); + static GuestPCIAddress *get_pci_info(char *guid, Error **errp) { @@ -517,7 +534,7 @@ static GuestPCIAddress *get_pci_info(char *guid, Error **errp) goto out; } - dev_info = SetupDiGetClassDevs(&GUID_DEVINTERFACE_VOLUME, 0, 0, + dev_info = SetupDiGetClassDevs(&GUID_DEVINTERFACE_DISK, 0, 0, DIGCF_PRESENT | DIGCF_DEVICEINTERFACE); if (dev_info == INVALID_HANDLE_VALUE) { error_setg_win32(errp, GetLastError(), "failed to get devices tree"); @@ -672,20 +689,20 @@ static void get_single_disk_info(char *name, GuestDiskAddress *disk, { SCSI_ADDRESS addr, *scsi_ad; DWORD len; - HANDLE vol_h; + HANDLE disk_h; Error *local_err = NULL; scsi_ad = &addr; g_debug("getting disk info for: %s", name); - vol_h = CreateFile(name, 0, FILE_SHARE_READ, NULL, OPEN_EXISTING, + disk_h = CreateFile(name, 0, FILE_SHARE_READ, NULL, OPEN_EXISTING, 0, NULL); - if (vol_h == INVALID_HANDLE_VALUE) { - error_setg_win32(errp, GetLastError(), "failed to open volume"); - goto err; + if (disk_h == INVALID_HANDLE_VALUE) { + error_setg_win32(errp, GetLastError(), "failed to open disk"); + return; } - get_disk_properties(vol_h, disk, &local_err); + get_disk_properties(disk_h, disk, &local_err); if (local_err) { error_propagate(errp, local_err); goto err_close; @@ -714,7 +731,7 @@ static void get_single_disk_info(char *name, GuestDiskAddress *disk, * according to Microsoft docs * https://technet.microsoft.com/en-us/library/ee851589(v=ws.10).aspx */ g_debug("getting pci-controller info"); - if (DeviceIoControl(vol_h, IOCTL_SCSI_GET_ADDRESS, NULL, 0, scsi_ad, + if (DeviceIoControl(disk_h, IOCTL_SCSI_GET_ADDRESS, NULL, 0, scsi_ad, sizeof(SCSI_ADDRESS), &len, NULL)) { disk->unit = addr.Lun; disk->target = addr.TargetId; @@ -725,8 +742,7 @@ static void get_single_disk_info(char *name, GuestDiskAddress *disk, } err_close: - CloseHandle(vol_h); -err: + CloseHandle(disk_h); return; } @@ -738,6 +754,10 @@ static GuestDiskAddressList *build_guest_disk_info(char *guid, Error **errp) Error *local_err = NULL; GuestDiskAddressList *list = NULL, *cur_item = NULL; GuestDiskAddress *disk = NULL; + int i; + HANDLE vol_h; + DWORD size; + PVOLUME_DISK_EXTENTS extents = NULL; /* strip final backslash */ char *name = g_strdup(guid); @@ -745,20 +765,90 @@ static GuestDiskAddressList *build_guest_disk_info(char *guid, Error **errp) name[strlen(name) - 1] = 0; } - disk = g_malloc0(sizeof(GuestDiskAddress)); - get_single_disk_info(name, disk, &local_err); - if (local_err) { - error_propagate(errp, local_err); + g_debug("opening %s", name); + vol_h = CreateFile(name, 0, FILE_SHARE_READ, NULL, OPEN_EXISTING, + 0, NULL); + if (vol_h == INVALID_HANDLE_VALUE) { + error_setg_win32(errp, GetLastError(), "failed to open volume"); goto out; } - cur_item = g_malloc0(sizeof(*list)); - cur_item->value = disk; - disk = NULL; - list = cur_item; + /* Get list of extents */ + g_debug("getting disk extents"); + size = sizeof(VOLUME_DISK_EXTENTS); + extents = g_malloc0(size); + if (!DeviceIoControl(vol_h, IOCTL_VOLUME_GET_VOLUME_DISK_EXTENTS, NULL, + 0, extents, size, NULL, NULL)) { + DWORD last_err = GetLastError(); + if (last_err == ERROR_MORE_DATA) { + /* Try once more with big enough buffer */ + size = sizeof(VOLUME_DISK_EXTENTS) + + extents->NumberOfDiskExtents*sizeof(DISK_EXTENT); + g_free(extents); + extents = g_malloc0(size); + if (!DeviceIoControl( + vol_h, IOCTL_VOLUME_GET_VOLUME_DISK_EXTENTS, NULL, + 0, extents, size, NULL, NULL)) { + error_setg_win32(errp, GetLastError(), + "failed to get disk extents"); + return NULL; + } + } else if (last_err == ERROR_INVALID_FUNCTION) { + /* Possibly CD-ROM or a shared drive. Try to pass the volume */ + g_debug("volume not on disk"); + disk = g_malloc0(sizeof(GuestDiskAddress)); + get_single_disk_info(name, disk, &local_err); + if (local_err) { + g_debug("failed to get disk info, ignoring error: %s", + error_get_pretty(local_err)); + error_free(local_err); + goto out; + } + list = g_malloc0(sizeof(*list)); + list->value = disk; + disk = NULL; + list->next = NULL; + goto out; + } else { + error_setg_win32(errp, GetLastError(), + "failed to get disk extents"); + goto out; + } + } + g_debug("Number of extents: %lu", extents->NumberOfDiskExtents); + + /* Go through each extent */ + for (i = 0; i < extents->NumberOfDiskExtents; i++) { + char *disk_name = NULL; + disk = g_malloc0(sizeof(GuestDiskAddress)); + + /* Disk numbers directly correspond to numbers used in UNCs + * + * See documentation for DISK_EXTENT: + * https://docs.microsoft.com/en-us/windows/desktop/api/winioctl/ns-winioctl-_disk_extent + * + * See also Naming Files, Paths and Namespaces: + * https://docs.microsoft.com/en-us/windows/desktop/FileIO/naming-a-file#win32-device-namespaces + */ + disk_name = g_strdup_printf("\\\\.\\PhysicalDrive%lu", + extents->Extents[i].DiskNumber); + get_single_disk_info(disk_name, disk, &local_err); + g_free(disk_name); + if (local_err) { + error_propagate(errp, local_err); + goto out; + } + cur_item = g_malloc0(sizeof(*list)); + cur_item->value = disk; + disk = NULL; + cur_item->next = list; + list = cur_item; + } + out: qapi_free_GuestDiskAddress(disk); + g_free(extents); g_free(name); return list; From 4550dee80fc1bba37bce141a3d6f83c3e390e719 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Golembiovsk=C3=BD?= Date: Tue, 23 Oct 2018 13:23:22 +0200 Subject: [PATCH 15/24] qga-win: return disk device in guest-get-fsinfo MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Report device UNC of the disk. It is reported as "\\.\PhysicalDriveX". Signed-off-by: Tomáš Golembiovský Signed-off-by: Michael Roth --- qga/commands-win32.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/qga/commands-win32.c b/qga/commands-win32.c index 90432bbfce..30d6c639c3 100644 --- a/qga/commands-win32.c +++ b/qga/commands-win32.c @@ -684,8 +684,7 @@ out_free: return; } -static void get_single_disk_info(char *name, GuestDiskAddress *disk, - Error **errp) +static void get_single_disk_info(GuestDiskAddress *disk, Error **errp) { SCSI_ADDRESS addr, *scsi_ad; DWORD len; @@ -694,8 +693,8 @@ static void get_single_disk_info(char *name, GuestDiskAddress *disk, scsi_ad = &addr; - g_debug("getting disk info for: %s", name); - disk_h = CreateFile(name, 0, FILE_SHARE_READ, NULL, OPEN_EXISTING, + g_debug("getting disk info for: %s", disk->dev); + disk_h = CreateFile(disk->dev, 0, FILE_SHARE_READ, NULL, OPEN_EXISTING, 0, NULL); if (disk_h == INVALID_HANDLE_VALUE) { error_setg_win32(errp, GetLastError(), "failed to open disk"); @@ -714,7 +713,7 @@ static void get_single_disk_info(char *name, GuestDiskAddress *disk, * if that doesn't hold since that suggests some other unexpected * breakage */ - disk->pci_controller = get_pci_info(name, &local_err); + disk->pci_controller = get_pci_info(disk->dev, &local_err); if (local_err) { error_propagate(errp, local_err); goto err_close; @@ -797,7 +796,9 @@ static GuestDiskAddressList *build_guest_disk_info(char *guid, Error **errp) /* Possibly CD-ROM or a shared drive. Try to pass the volume */ g_debug("volume not on disk"); disk = g_malloc0(sizeof(GuestDiskAddress)); - get_single_disk_info(name, disk, &local_err); + disk->has_dev = true; + disk->dev = g_strdup(name); + get_single_disk_info(disk, &local_err); if (local_err) { g_debug("failed to get disk info, ignoring error: %s", error_get_pretty(local_err)); @@ -819,7 +820,6 @@ static GuestDiskAddressList *build_guest_disk_info(char *guid, Error **errp) /* Go through each extent */ for (i = 0; i < extents->NumberOfDiskExtents; i++) { - char *disk_name = NULL; disk = g_malloc0(sizeof(GuestDiskAddress)); /* Disk numbers directly correspond to numbers used in UNCs @@ -830,10 +830,11 @@ static GuestDiskAddressList *build_guest_disk_info(char *guid, Error **errp) * See also Naming Files, Paths and Namespaces: * https://docs.microsoft.com/en-us/windows/desktop/FileIO/naming-a-file#win32-device-namespaces */ - disk_name = g_strdup_printf("\\\\.\\PhysicalDrive%lu", + disk->has_dev = true; + disk->dev = g_strdup_printf("\\\\.\\PhysicalDrive%lu", extents->Extents[i].DiskNumber); - get_single_disk_info(disk_name, disk, &local_err); - g_free(disk_name); + + get_single_disk_info(disk, &local_err); if (local_err) { error_propagate(errp, local_err); goto out; From 7fae5184790680b39f4d627d069981816f4c3191 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Golembiovsk=C3=BD?= Date: Tue, 23 Oct 2018 13:23:23 +0200 Subject: [PATCH 16/24] qga-win: demystify namespace stripping MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It was not obvious what exactly the cryptic string copying does to the GUID. This change makes the intent clearer. Signed-off-by: Tomáš Golembiovský Signed-off-by: Michael Roth --- qga/commands-win32.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/qga/commands-win32.c b/qga/commands-win32.c index 30d6c639c3..a1b7512d46 100644 --- a/qga/commands-win32.c +++ b/qga/commands-win32.c @@ -521,7 +521,7 @@ static GuestPCIAddress *get_pci_info(char *guid, Error **errp) char dev_name[MAX_PATH]; char *buffer = NULL; GuestPCIAddress *pci = NULL; - char *name = g_strdup(&guid[4]); + char *name = NULL; bool partial_pci = false; pci = g_malloc0(sizeof(*pci)); pci->domain = -1; @@ -529,6 +529,13 @@ static GuestPCIAddress *get_pci_info(char *guid, Error **errp) pci->function = -1; pci->bus = -1; + if (g_str_has_prefix(guid, "\\\\.\\") || + g_str_has_prefix(guid, "\\\\?\\")) { + name = g_strdup(guid + 4); + } else { + name = g_strdup(guid); + } + if (!QueryDosDevice(name, dev_name, ARRAY_SIZE(dev_name))) { error_setg_win32(errp, GetLastError(), "failed to get dos device name"); goto out; From d9c85b6cc5a2597e72299bc6644c966236378e50 Mon Sep 17 00:00:00 2001 From: Li Qiang Date: Wed, 17 Oct 2018 19:10:37 -0700 Subject: [PATCH 17/24] qga: fix an off-by-one issue MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Li Qiang Reviewed-by: Philippe Mathieu-Daudé Signed-off-by: Michael Roth --- qga/commands-win32.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/qga/commands-win32.c b/qga/commands-win32.c index a1b7512d46..ef1d7d48d2 100644 --- a/qga/commands-win32.c +++ b/qga/commands-win32.c @@ -485,7 +485,7 @@ static STORAGE_BUS_TYPE win2qemu[] = { static GuestDiskBusType find_bus_type(STORAGE_BUS_TYPE bus) { - if (bus > ARRAY_SIZE(win2qemu) || (int)bus < 0) { + if (bus >= ARRAY_SIZE(win2qemu) || (int)bus < 0) { return GUEST_DISK_BUS_TYPE_UNKNOWN; } return win2qemu[(int)bus]; From 50d5b3c465ffafbd6973acb8afde1f1e4ca5a2da Mon Sep 17 00:00:00 2001 From: Michael Roth Date: Sun, 7 Oct 2018 14:02:17 +0300 Subject: [PATCH 18/24] qga: group agent init/cleanup init separate routines MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Marc-André Lureau Signed-off-by: Michael Roth --- qga/main.c | 82 +++++++++++++++++++++++++++++++++--------------------- 1 file changed, 50 insertions(+), 32 deletions(-) diff --git a/qga/main.c b/qga/main.c index afcd268ee3..ab3eaa5163 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; } From 0f4d3a4912d1ca6585409b92d1d7fefb8baf60fa Mon Sep 17 00:00:00 2001 From: Michael Roth Date: Sun, 7 Oct 2018 14:02:18 +0300 Subject: [PATCH 19/24] qga: hang GAConfig/socket_activation off of GAState global 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 Signed-off-by: Sameeh Jubran *dont move GAConfig struct, just the typedef *fix build bisect for w32 Signed-off-by: Michael Roth --- qga/main.c | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/qga/main.c b/qga/main.c index ab3eaa5163..eb31f99b38 100644 --- a/qga/main.c +++ b/qga/main.c @@ -69,6 +69,8 @@ typedef struct GAPersistentState { int64_t fd_counter; } GAPersistentState; +typedef struct GAConfig GAConfig; + struct GAState { JSONMessageParser parser; GMainLoop *main_loop; @@ -94,6 +96,8 @@ struct GAState { #endif gchar *pstate_filepath; GAPersistentState pstate; + GAConfig *config; + int socket_activation; }; struct GAState *ga_state; @@ -905,7 +909,7 @@ static GList *split_list(const gchar *str, const gchar *delim) return list; } -typedef struct GAConfig { +struct GAConfig { char *channel_path; char *method; char *log_filepath; @@ -922,7 +926,7 @@ typedef struct GAConfig { int daemonize; GLogLevelFlags log_level; int dumpconf; -} GAConfig; +}; static void config_load(GAConfig *config) { @@ -1211,7 +1215,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 +1308,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,17 +1333,17 @@ 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; } #ifndef _WIN32 g_main_loop_run(ga_state->main_loop); #else - if (config->daemonize) { + if (s->config->daemonize) { SERVICE_TABLE_ENTRY service_table[] = { { (char *)QGA_SERVICE_NAME, service_main }, { NULL, NULL } }; StartServiceCtrlDispatcher(service_table); @@ -1425,12 +1431,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: From d88495a864011139683e62fb7f84db93c7b51881 Mon Sep 17 00:00:00 2001 From: Michael Roth Date: Sun, 7 Oct 2018 14:02:19 +0300 Subject: [PATCH 20/24] qga: move w32 service handling out of run_agent() 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 Signed-off-by: Bishara AbuHattoum Signed-off-by: Michael Roth --- qga/main.c | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/qga/main.c b/qga/main.c index eb31f99b38..761007deb4 100644 --- a/qga/main.c +++ b/qga/main.c @@ -119,6 +119,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) @@ -712,7 +713,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); @@ -1340,17 +1341,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 (s->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; } @@ -1436,7 +1428,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: From d951fadad6cce10d83f736de691fc2003b89bac4 Mon Sep 17 00:00:00 2001 From: Michael Roth Date: Sun, 7 Oct 2018 14:02:20 +0300 Subject: [PATCH 21/24] qga: add --retry-path option for re-initializing channel on failure 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 --- qga/main.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 54 insertions(+), 8 deletions(-) diff --git a/qga/main.c b/qga/main.c index 761007deb4..506a314140 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; @@ -98,6 +99,7 @@ struct GAState { GAPersistentState pstate; GAConfig *config; int socket_activation; + bool force_exit; }; struct GAState *ga_state; @@ -120,6 +122,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) @@ -168,9 +171,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 @@ -255,6 +256,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" @@ -614,6 +619,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; @@ -927,6 +933,7 @@ struct GAConfig { int daemonize; GLogLevelFlags log_level; int dumpconf; + bool retry_path; }; static void config_load(GAConfig *config) @@ -976,6 +983,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); @@ -1037,6 +1048,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); @@ -1055,7 +1068,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' }, @@ -1075,6 +1088,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 } }; @@ -1119,6 +1133,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); @@ -1322,9 +1339,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) { @@ -1334,7 +1348,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)) { @@ -1344,9 +1358,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; From ebc88c0ecf1ab8cb64cea9cf7973d05a12ed45e6 Mon Sep 17 00:00:00 2001 From: Michael Roth Date: Sun, 7 Oct 2018 14:02:21 +0300 Subject: [PATCH 22/24] qga-win: install service with --retry-path set by default 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 --- 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" > From a2c1ac4e22da848e0fef262086e0f29fbc6ff83a Mon Sep 17 00:00:00 2001 From: Michael Roth Date: Sun, 7 Oct 2018 14:02:22 +0300 Subject: [PATCH 23/24] qga-win: report specific error when failing to open channel MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Useful in general, but especially now that errors might occur more frequently with --retry-path set. Signed-off-by: Michael Roth Reviewed-by: Marc-André Lureau Signed-off-by: Michael Roth --- 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; } From b70d6afe4d7f252259d3344e12daa2d6baf006e2 Mon Sep 17 00:00:00 2001 From: Bishara AbuHattoum Date: Sun, 7 Oct 2018 14:02:23 +0300 Subject: [PATCH 24/24] qga-win: changing --retry-path option behavior 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 Signed-off-by: Sameeh Jubran Signed-off-by: Michael Roth --- 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 506a314140..87a0711c14 100644 --- a/qga/main.c +++ b/qga/main.c @@ -34,6 +34,7 @@ #include "qemu/systemd.h" #include "qemu-version.h" #ifdef _WIN32 +#include #include "qga/service-win32.h" #include "qga/vss-win32.h" #endif @@ -83,6 +84,7 @@ struct GAState { bool logging_enabled; #ifdef _WIN32 GAService service; + HANDLE wakeup_event; #endif bool delimit_response; bool frozen; @@ -119,6 +121,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); @@ -677,6 +680,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) { @@ -688,9 +721,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; @@ -717,10 +754,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); run_agent(ga_state); + UnregisterDeviceNotification(service->device_notification_handle); service->status.dwCurrentState = SERVICE_STOPPED; SetServiceStatus(service->status_handle, &service->status); } @@ -1328,12 +1379,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); @@ -1365,6 +1428,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; @@ -1375,7 +1459,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,