From bb6c8d407e49d7b805ac52fe46abf4d8d5262046 Mon Sep 17 00:00:00 2001 From: Daniel Henrique Barboza Date: Tue, 13 Nov 2018 14:55:39 -0200 Subject: [PATCH 1/7] qga: update docs with systemd suspend support info Commit 067927d62e ("qga: systemd hibernate/suspend/hybrid-sleep support") failed to update qapi-schema.json after adding systemd hibernate/suspend/hybrid-sleep capabilities to guest-suspend-* QGA commands. Signed-off-by: Daniel Henrique Barboza Reviewed-by: Eric Blake Signed-off-by: Michael Roth --- qga/qapi-schema.json | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json index a4ff5b8fc9..fb4605cc19 100644 --- a/qga/qapi-schema.json +++ b/qga/qapi-schema.json @@ -532,12 +532,12 @@ # # Suspend guest to disk. # -# This command tries to execute the scripts provided by the pm-utils package. -# If it's not available, the suspend operation will be performed by manually -# writing to a sysfs file. +# This command attempts to suspend the guest using three strategies, in this +# order: # -# For the best results it's strongly recommended to have the pm-utils -# package installed in the guest. +# - systemd hibernate +# - pm-utils (via pm-hibernate) +# - manual write into sysfs # # This command does NOT return a response on success. There is a high chance # the command succeeded if the VM exits with a zero exit status or, when @@ -560,12 +560,12 @@ # # Suspend guest to ram. # -# This command tries to execute the scripts provided by the pm-utils package. -# If it's not available, the suspend operation will be performed by manually -# writing to a sysfs file. +# This command attempts to suspend the guest using three strategies, in this +# order: # -# For the best results it's strongly recommended to have the pm-utils -# package installed in the guest. +# - systemd suspend +# - pm-utils (via pm-suspend) +# - manual write into sysfs # # IMPORTANT: guest-suspend-ram requires working wakeup support in # QEMU. You should check QMP command query-current-machine returns @@ -594,7 +594,10 @@ # # Save guest state to disk and suspend to ram. # -# This command requires the pm-utils package to be installed in the guest. +# This command attempts to suspend the guest by executing, in this order: +# +# - systemd hybrid-sleep +# - pm-utils (via pm-suspend-hybrid) # # IMPORTANT: guest-suspend-hybrid requires working wakeup support in # QEMU. You should check QMP command query-current-machine returns From bd586a91338a653082fae7886e0c2dd2f65ca099 Mon Sep 17 00:00:00 2001 From: Bishara AbuHattoum Date: Wed, 19 Dec 2018 10:40:51 +0200 Subject: [PATCH 2/7] qga-win: Adding support for Windows Server 2019 get-osinfo command Since Windows Server 2016, Microsoft stopped upgrading the major and minor versions of their new Windows Server product, so, the current functionality of checking major and minor version numbers to determine the Windows Server version wont work as expected. The implemented solution here is to use the build number in addition to the major and minor version numbers of the product to determine the Windows Server product version. The final build number of Windows Server 2016 is 14939, and the final build number of Windows Server 2019 is 17764, so any Windows Server product that has the major version of 10 and minor version of 0 with a build number lower or equal to 14939 will resemble 2016 and if the build number is lower or equal to 17763 will resemble 2019. Reference: https://techcommunity.microsoft.com/t5/Windows-Server-Insiders/Windows-Server-2019-version-info/m-p/293112/highlight/true#M859 Signed-off-by: Bishara AbuHattoum Signed-off-by: Michael Roth --- qga/commands-win32.c | 29 +++++++++++++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-) diff --git a/qga/commands-win32.c b/qga/commands-win32.c index 989b93e702..fb48463885 100644 --- a/qga/commands-win32.c +++ b/qga/commands-win32.c @@ -1941,12 +1941,24 @@ static ga_matrix_lookup_t const WIN_VERSION_MATRIX[2][8] = { { 6, 1, "Microsoft Windows Server 2008 R2", "2008r2"}, { 6, 2, "Microsoft Windows Server 2012", "2012"}, { 6, 3, "Microsoft Windows Server 2012 R2", "2012r2"}, - {10, 0, "Microsoft Windows Server 2016", "2016"}, + { 0, 0, 0}, { 0, 0, 0}, { 0, 0, 0} } }; +typedef struct _ga_win_10_0_server_t { + int final_build; + char const *version; + char const *version_id; +} ga_win_10_0_server_t; + +static ga_win_10_0_server_t const WIN_10_0_SERVER_VERSION_MATRIX[3] = { + {14393, "Microsoft Windows Server 2016", "2016"}, + {17763, "Microsoft Windows Server 2019", "2019"}, + {0, 0} +}; + static void ga_get_win_version(RTL_OSVERSIONINFOEXW *info, Error **errp) { typedef NTSTATUS(WINAPI * rtl_get_version_t)( @@ -1971,10 +1983,23 @@ static char *ga_get_win_name(OSVERSIONINFOEXW const *os_version, bool id) { DWORD major = os_version->dwMajorVersion; DWORD minor = os_version->dwMinorVersion; + DWORD build = os_version->dwBuildNumber; int tbl_idx = (os_version->wProductType != VER_NT_WORKSTATION); ga_matrix_lookup_t const *table = WIN_VERSION_MATRIX[tbl_idx]; + ga_win_10_0_server_t const *win_10_0_table = WIN_10_0_SERVER_VERSION_MATRIX; while (table->version != NULL) { - if (major == table->major && minor == table->minor) { + if (major == 10 && minor == 0 && tbl_idx) { + while (win_10_0_table->version != NULL) { + if (build <= win_10_0_table->final_build) { + if (id) { + return g_strdup(win_10_0_table->version_id); + } else { + return g_strdup(win_10_0_table->version); + } + } + win_10_0_table++; + } + } else if (major == table->major && minor == table->minor) { if (id) { return g_strdup(table->version_id); } else { From 82a58d270c6fbbe2f2381224946340fd3814a273 Mon Sep 17 00:00:00 2001 From: Michael Roth Date: Tue, 12 Feb 2019 15:29:16 -0600 Subject: [PATCH 3/7] qga-win: include glib when building VSS DLL MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Commit 3ebee3b191e defined assert() as g_assert(), but when we build the VSS DLL component of QGA (to handle fsfreeze) we do not include glib, which results in breakage when building with VSS support enabled. Fix this by including glib (along with the -lintl and -lws2_32 dependencies it brings). Since the VSS DLL is built statically, this introduces an additional dependency on static glib and supporting libs for the mingw environment (possibly why we didn't include glib originally), but VSS support already has very specific prerequisites so it shouldn't affect too many build environments. Since the VSS DLL code does use qemu/osdep.h, this should also help avoid future breakages and possibly allow for some clean ups in current VSS code. Suggested-by: Daniel P. Berrangé Cc: Daniel P. Berrangé Cc: qemu-stable@nongnu.org Signed-off-by: Michael Roth --- qga/vss-win32/Makefile.objs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/qga/vss-win32/Makefile.objs b/qga/vss-win32/Makefile.objs index 23d08da225..dad9d1b0ba 100644 --- a/qga/vss-win32/Makefile.objs +++ b/qga/vss-win32/Makefile.objs @@ -5,7 +5,7 @@ qga-vss-dll-obj-y += requester.o provider.o install.o obj-qga-vss-dll-obj-y = $(addprefix $(obj)/, $(qga-vss-dll-obj-y)) $(obj-qga-vss-dll-obj-y): QEMU_CXXFLAGS = $(filter-out -Wstrict-prototypes -Wmissing-prototypes -Wnested-externs -Wold-style-declaration -Wold-style-definition -Wredundant-decls -fstack-protector-all -fstack-protector-strong, $(QEMU_CFLAGS)) -Wno-unknown-pragmas -Wno-delete-non-virtual-dtor -$(obj)/qga-vss.dll: LDFLAGS = -shared -Wl,--add-stdcall-alias,--enable-stdcall-fixup -lole32 -loleaut32 -lshlwapi -luuid -static +$(obj)/qga-vss.dll: LDFLAGS = -shared -Wl,--add-stdcall-alias,--enable-stdcall-fixup -lglib-2.0 -lole32 -loleaut32 -lshlwapi -luuid -lintl -lws2_32 -static $(obj)/qga-vss.dll: $(obj-qga-vss-dll-obj-y) $(SRC_PATH)/$(obj)/qga-vss.def $(call quiet-command,$(CXX) -o $@ $(qga-vss-dll-obj-y) $(SRC_PATH)/qga/vss-win32/qga-vss.def $(CXXFLAGS) $(LDFLAGS),"LINK","$(TARGET_DIR)$@") From 40cebc58117dbc6275a5d7c4e3ba6611964d9e6e Mon Sep 17 00:00:00 2001 From: Michael Roth Date: Fri, 15 Mar 2019 20:24:30 -0500 Subject: [PATCH 4/7] qga-win: fix VSS build breakage due to unintended gnu99 C++ flag MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Commit 7be41675f7c set -std=gnu99 for C code via QEMU_CFLAGS. Currently we generate a "custom" QEMU_CXXFLAGS for VSS DLL C++ build by filtering out some options from QEMU_CFLAGS and adding some others. Since we don't filter out -std=gnu99 currently this breaks builds when VSS support is enabled. We could keep the existing approach, filter out -std=gnu99 from QEMU_CFLAGS, and add -std=gnu++98, like configure currently does for QEMU_CXXFLAGS, but as it turns out our resulting QEMU_CXXFLAGS would be exactly what configure already generates, just with these filtered out: -fstack-protector-all -fstack-protector-strong and these added: -Wno-unknown-pragmas -Wno-delete-non-virtual-dtor So fix the issue by re-using configure-generated QEMU_CXXFLAGS and just handling these specific changes. Signed-off-by: Michael Roth Reviewed-by: Daniel P. Berrangé --- qga/vss-win32/Makefile.objs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/qga/vss-win32/Makefile.objs b/qga/vss-win32/Makefile.objs index dad9d1b0ba..fd3ba1896b 100644 --- a/qga/vss-win32/Makefile.objs +++ b/qga/vss-win32/Makefile.objs @@ -3,7 +3,7 @@ qga-vss-dll-obj-y += requester.o provider.o install.o obj-qga-vss-dll-obj-y = $(addprefix $(obj)/, $(qga-vss-dll-obj-y)) -$(obj-qga-vss-dll-obj-y): QEMU_CXXFLAGS = $(filter-out -Wstrict-prototypes -Wmissing-prototypes -Wnested-externs -Wold-style-declaration -Wold-style-definition -Wredundant-decls -fstack-protector-all -fstack-protector-strong, $(QEMU_CFLAGS)) -Wno-unknown-pragmas -Wno-delete-non-virtual-dtor +$(obj-qga-vss-dll-obj-y): QEMU_CXXFLAGS := $(filter-out -fstack-protector-all -fstack-protector-strong, $(QEMU_CXXFLAGS)) -Wno-unknown-pragmas -Wno-delete-non-virtual-dtor $(obj)/qga-vss.dll: LDFLAGS = -shared -Wl,--add-stdcall-alias,--enable-stdcall-fixup -lglib-2.0 -lole32 -loleaut32 -lshlwapi -luuid -lintl -lws2_32 -static $(obj)/qga-vss.dll: $(obj-qga-vss-dll-obj-y) $(SRC_PATH)/$(obj)/qga-vss.def From 996b9cdc2f190a173e48f4c8d64de3d50e570b7b Mon Sep 17 00:00:00 2001 From: Matt Hines Date: Mon, 28 Jan 2019 14:30:56 -0800 Subject: [PATCH 5/7] qga: Fix guest-get-fsinfo PCI address collection in Windows The Windows QEMU guest agent erroneously tries to collect PCI information directly from the physical drive. However, windows stores SCSI/IDE information with the drive and PCI information with the underlying storage controller This changes get_pci_info to use the physical drive's underlying storage controller to get PCI information. * Additionally Fixes incorrect size being passed to DeviceIoControl when getting volume extents. Can occasionally crash the guest agent Signed-off-by: Matt Hines *fix up some checkpatch warnings *fix domain reporting and add some sanity checks for debug Signed-off-by: Michael Roth --- configure | 2 +- qga/commands-win32.c | 317 ++++++++++++++++++++++++++++--------------- 2 files changed, 211 insertions(+), 108 deletions(-) diff --git a/configure b/configure index 7071f52584..02ea221272 100755 --- a/configure +++ b/configure @@ -4934,7 +4934,7 @@ int main(void) { EOF if compile_prog "" "" ; then guest_agent_ntddscsi=yes - libs_qga="-lsetupapi $libs_qga" + libs_qga="-lsetupapi -lcfgmgr32 $libs_qga" fi fi diff --git a/qga/commands-win32.c b/qga/commands-win32.c index fb48463885..d40d61f605 100644 --- a/qga/commands-win32.c +++ b/qga/commands-win32.c @@ -22,6 +22,7 @@ #include #include #include +#include #include #endif #include @@ -485,56 +486,29 @@ 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); +DEFINE_GUID(GUID_DEVINTERFACE_STORAGEPORT, + 0x2accfe60L, 0xc130, 0x11d2, 0xb0, 0x82, + 0x00, 0xa0, 0xc9, 0x1e, 0xfb, 0x8b); - -static GuestPCIAddress *get_pci_info(char *guid, Error **errp) +static GuestPCIAddress *get_pci_info(int number, Error **errp) { HDEVINFO dev_info; SP_DEVINFO_DATA dev_info_data; - DWORD size = 0; + SP_DEVICE_INTERFACE_DATA dev_iface_data; + HANDLE dev_file; int i; - char dev_name[MAX_PATH]; - char *buffer = NULL; GuestPCIAddress *pci = NULL; - char *name = NULL; bool partial_pci = false; + pci = g_malloc0(sizeof(*pci)); pci->domain = -1; pci->slot = -1; 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; - } - dev_info = SetupDiGetClassDevs(&GUID_DEVINTERFACE_DISK, 0, 0, DIGCF_PRESENT | DIGCF_DEVICEINTERFACE); if (dev_info == INVALID_HANDLE_VALUE) { @@ -544,90 +518,220 @@ static GuestPCIAddress *get_pci_info(char *guid, Error **errp) g_debug("enumerating devices"); dev_info_data.cbSize = sizeof(SP_DEVINFO_DATA); + dev_iface_data.cbSize = sizeof(SP_DEVICE_INTERFACE_DATA); for (i = 0; SetupDiEnumDeviceInfo(dev_info, i, &dev_info_data); i++) { - 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, - &size2)) { - size = MAX(size, size2); - if (GetLastError() == ERROR_INSUFFICIENT_BUFFER) { - g_free(buffer); - /* Double the size to avoid problems on - * W2k MBCS systems per KB 888609. - * https://support.microsoft.com/en-us/kb/259695 */ - buffer = g_malloc(size * 2); - } else { + PSP_DEVICE_INTERFACE_DETAIL_DATA pdev_iface_detail_data = NULL; + STORAGE_DEVICE_NUMBER sdn; + char *parent_dev_id = NULL; + HDEVINFO parent_dev_info; + SP_DEVINFO_DATA parent_dev_info_data; + DWORD j; + DWORD size = 0; + + g_debug("getting device path"); + if (SetupDiEnumDeviceInterfaces(dev_info, &dev_info_data, + &GUID_DEVINTERFACE_DISK, 0, + &dev_iface_data)) { + while (!SetupDiGetDeviceInterfaceDetail(dev_info, &dev_iface_data, + pdev_iface_detail_data, + size, &size, + &dev_info_data)) { + if (GetLastError() == ERROR_INSUFFICIENT_BUFFER) { + pdev_iface_detail_data = g_malloc(size); + pdev_iface_detail_data->cbSize = + sizeof(*pdev_iface_detail_data); + } else { + error_setg_win32(errp, GetLastError(), + "failed to get device interfaces"); + goto free_dev_info; + } + } + + dev_file = CreateFile(pdev_iface_detail_data->DevicePath, 0, + FILE_SHARE_READ, NULL, OPEN_EXISTING, 0, + NULL); + g_free(pdev_iface_detail_data); + + if (!DeviceIoControl(dev_file, IOCTL_STORAGE_GET_DEVICE_NUMBER, + NULL, 0, &sdn, sizeof(sdn), &size, NULL)) { + CloseHandle(dev_file); error_setg_win32(errp, GetLastError(), - "failed to get device name"); + "failed to get device slot number"); goto free_dev_info; } + + CloseHandle(dev_file); + if (sdn.DeviceNumber != number) { + continue; + } + } else { + error_setg_win32(errp, GetLastError(), + "failed to get device interfaces"); + goto free_dev_info; + } + + g_debug("found device slot %d. Getting storage controller", number); + { + CONFIGRET cr; + DEVINST dev_inst, parent_dev_inst; + ULONG dev_id_size = 0; + + size = 0; + while (!SetupDiGetDeviceInstanceId(dev_info, &dev_info_data, + parent_dev_id, size, &size)) { + if (GetLastError() == ERROR_INSUFFICIENT_BUFFER) { + parent_dev_id = g_malloc(size); + } else { + error_setg_win32(errp, GetLastError(), + "failed to get device instance ID"); + goto out; + } + } + + /* + * CM API used here as opposed to + * SetupDiGetDeviceProperty(..., DEVPKEY_Device_Parent, ...) + * which exports are only available in mingw-w64 6+ + */ + cr = CM_Locate_DevInst(&dev_inst, parent_dev_id, 0); + if (cr != CR_SUCCESS) { + g_error("CM_Locate_DevInst failed with code %lx", cr); + error_setg_win32(errp, GetLastError(), + "failed to get device instance"); + goto out; + } + cr = CM_Get_Parent(&parent_dev_inst, dev_inst, 0); + if (cr != CR_SUCCESS) { + g_error("CM_Get_Parent failed with code %lx", cr); + error_setg_win32(errp, GetLastError(), + "failed to get parent device instance"); + goto out; + } + + cr = CM_Get_Device_ID_Size(&dev_id_size, parent_dev_inst, 0); + if (cr != CR_SUCCESS) { + g_error("CM_Get_Device_ID_Size failed with code %lx", cr); + error_setg_win32(errp, GetLastError(), + "failed to get parent device ID length"); + goto out; + } + + ++dev_id_size; + if (dev_id_size > size) { + g_free(parent_dev_id); + parent_dev_id = g_malloc(dev_id_size); + } + + cr = CM_Get_Device_ID(parent_dev_inst, parent_dev_id, dev_id_size, + 0); + if (cr != CR_SUCCESS) { + g_error("CM_Get_Device_ID failed with code %lx", cr); + error_setg_win32(errp, GetLastError(), + "failed to get parent device ID"); + goto out; + } } - if (g_strcmp0(buffer, dev_name)) { - continue; - } - g_debug("found device %s", dev_name); + g_debug("querying storage controller %s for PCI information", + parent_dev_id); + parent_dev_info = + SetupDiGetClassDevs(&GUID_DEVINTERFACE_STORAGEPORT, parent_dev_id, + NULL, DIGCF_PRESENT | DIGCF_DEVICEINTERFACE); + g_free(parent_dev_id); - /* There is no need to allocate buffer in the next functions. The size - * is known and ULONG according to - * https://support.microsoft.com/en-us/kb/253232 - * https://msdn.microsoft.com/en-us/library/windows/hardware/ff543095(v=vs.85).aspx - */ - 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; + if (parent_dev_info == INVALID_HANDLE_VALUE) { + error_setg_win32(errp, GetLastError(), + "failed to get parent device"); + goto out; } - /* 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)) { - debug_error("failed to get address"); - addr = -1; - partial_pci = true; + parent_dev_info_data.cbSize = sizeof(SP_DEVINFO_DATA); + if (!SetupDiEnumDeviceInfo(parent_dev_info, 0, &parent_dev_info_data)) { + error_setg_win32(errp, GetLastError(), + "failed to get parent device data"); + goto out; } - /* 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)) { - debug_error("failed to get slot"); - slot = -1; - partial_pci = true; - } + for (j = 0; + SetupDiEnumDeviceInfo(parent_dev_info, j, &parent_dev_info_data); + j++) { + DWORD addr, bus, ui_slot, type; + int func, slot; - /* SetupApi gives us the same information as driver with - * IoGetDeviceProperty. According to Microsoft - * https://support.microsoft.com/en-us/kb/253232 - * FunctionNumber = (USHORT)((propertyAddress) & 0x0000FFFF); - * DeviceNumber = (USHORT)(((propertyAddress) >> 16) & 0x0000FFFF); - * SPDRP_ADDRESS is propertyAddress, so we do the same.*/ + /* + * There is no need to allocate buffer in the next functions. The + * size is known and ULONG according to + * https://msdn.microsoft.com/en-us/library/windows/hardware/ff543095(v=vs.85).aspx + */ + if (!SetupDiGetDeviceRegistryProperty( + parent_dev_info, &parent_dev_info_data, SPDRP_BUSNUMBER, + &type, (PBYTE)&bus, size, NULL)) { + debug_error("failed to get PCI bus"); + bus = -1; + partial_pci = true; + } - 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; + /* + * The function retrieves the device's address. This value will be + * transformed into device function and number + */ + if (!SetupDiGetDeviceRegistryProperty( + parent_dev_info, &parent_dev_info_data, SPDRP_ADDRESS, + &type, (PBYTE)&addr, size, NULL)) { + debug_error("failed to get PCI address"); + addr = -1; + partial_pci = true; + } + + /* + * This call returns UINumber of DEVICE_CAPABILITIES structure. + * This number is typically a user-perceived slot number. + */ + if (!SetupDiGetDeviceRegistryProperty( + parent_dev_info, &parent_dev_info_data, SPDRP_UI_NUMBER, + &type, (PBYTE)&ui_slot, size, NULL)) { + debug_error("failed to get PCI slot"); + ui_slot = -1; + partial_pci = true; + } + + /* + * SetupApi gives us the same information as driver with + * IoGetDeviceProperty. According to Microsoft: + * + * FunctionNumber = (USHORT)((propertyAddress) & 0x0000FFFF) + * DeviceNumber = (USHORT)(((propertyAddress) >> 16) & 0x0000FFFF) + * SPDRP_ADDRESS is propertyAddress, so we do the same. + * + * https://docs.microsoft.com/en-us/windows/desktop/api/setupapi/nf-setupapi-setupdigetdeviceregistrypropertya + */ + if (partial_pci) { + pci->domain = -1; + pci->slot = -1; + pci->function = -1; + pci->bus = -1; + continue; + } else { + func = ((int)addr == -1) ? -1 : addr & 0x0000FFFF; + slot = ((int)addr == -1) ? -1 : (addr >> 16) & 0x0000FFFF; + if ((int)ui_slot != slot) { + g_debug("mismatch with reported slot values: %d vs %d", + (int)ui_slot, slot); + } + pci->domain = 0; + pci->slot = (int)ui_slot; + pci->function = func; + pci->bus = (int)bus; + break; + } } + SetupDiDestroyDeviceInfoList(parent_dev_info); break; } free_dev_info: SetupDiDestroyDeviceInfoList(dev_info); out: - g_free(buffer); - g_free(name); return pci; } @@ -685,7 +789,8 @@ out_free: return; } -static void get_single_disk_info(GuestDiskAddress *disk, Error **errp) +static void get_single_disk_info(int disk_number, + GuestDiskAddress *disk, Error **errp) { SCSI_ADDRESS addr, *scsi_ad; DWORD len; @@ -714,7 +819,7 @@ static void get_single_disk_info(GuestDiskAddress *disk, Error **errp) * if that doesn't hold since that suggests some other unexpected * breakage */ - disk->pci_controller = get_pci_info(disk->dev, &local_err); + disk->pci_controller = get_pci_info(disk_number, &local_err); if (local_err) { error_propagate(errp, local_err); goto err_close; @@ -728,7 +833,7 @@ static void get_single_disk_info(GuestDiskAddress *disk, 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"); + g_debug("getting SCSI info"); if (DeviceIoControl(disk_h, IOCTL_SCSI_GET_ADDRESS, NULL, 0, scsi_ad, sizeof(SCSI_ADDRESS), &len, NULL)) { disk->unit = addr.Lun; @@ -776,12 +881,10 @@ static GuestDiskAddressList *build_guest_disk_info(char *guid, Error **errp) 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)) { + 0, extents, size, &size, 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( @@ -797,7 +900,7 @@ static GuestDiskAddressList *build_guest_disk_info(char *guid, Error **errp) disk = g_malloc0(sizeof(GuestDiskAddress)); disk->has_dev = true; disk->dev = g_strdup(name); - get_single_disk_info(disk, &local_err); + get_single_disk_info(0xffffffff, disk, &local_err); if (local_err) { g_debug("failed to get disk info, ignoring error: %s", error_get_pretty(local_err)); @@ -831,9 +934,9 @@ static GuestDiskAddressList *build_guest_disk_info(char *guid, Error **errp) */ disk->has_dev = true; disk->dev = g_strdup_printf("\\\\.\\PhysicalDrive%lu", - extents->Extents[i].DiskNumber); + extents->Extents[i].DiskNumber); - get_single_disk_info(disk, &local_err); + get_single_disk_info(extents->Extents[i].DiskNumber, disk, &local_err); if (local_err) { error_propagate(errp, local_err); goto out; From 781f2b3d1e5ef389b44016a897fd55e7a780bf35 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Lureau?= Date: Wed, 20 Feb 2019 16:42:52 +0100 Subject: [PATCH 6/7] qga: process_event() simplification MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Simplify the code around qmp_dispatch(): - rely on qmp_dispatch/check_obj() for message checking - have a single send_response() point - constify send_response() argument It changes a couple of error messages: * When @req isn't a dictionary, from Invalid JSON syntax to QMP input must be a JSON object * When @req lacks member "execute", from this feature or command is not currently supported to QMP input lacks member 'execute' CC: Michael Roth Signed-off-by: Marc-André Lureau Signed-off-by: Michael Roth Reviewed-by: Eric Blake --- qga/main.c | 47 +++++++++-------------------------------------- 1 file changed, 9 insertions(+), 38 deletions(-) diff --git a/qga/main.c b/qga/main.c index 87a0711c14..c0d77c79c4 100644 --- a/qga/main.c +++ b/qga/main.c @@ -523,15 +523,15 @@ fail: #endif } -static int send_response(GAState *s, QDict *payload) +static int send_response(GAState *s, const QDict *rsp) { const char *buf; QString *payload_qstr, *response_qstr; GIOStatus status; - g_assert(payload && s->channel); + g_assert(rsp && s->channel); - payload_qstr = qobject_to_json(QOBJECT(payload)); + payload_qstr = qobject_to_json(QOBJECT(rsp)); if (!payload_qstr) { return -EINVAL; } @@ -557,53 +557,24 @@ static int send_response(GAState *s, QDict *payload) return 0; } -static void process_command(GAState *s, QDict *req) -{ - QDict *rsp; - int ret; - - g_assert(req); - g_debug("processing command"); - rsp = qmp_dispatch(&ga_commands, QOBJECT(req), false); - if (rsp) { - ret = send_response(s, rsp); - if (ret < 0) { - g_warning("error sending response: %s", strerror(-ret)); - } - qobject_unref(rsp); - } -} - /* handle requests/control events coming in over the channel */ static void process_event(void *opaque, QObject *obj, Error *err) { GAState *s = opaque; - QDict *req, *rsp; + QDict *rsp; int ret; g_debug("process_event: called"); assert(!obj != !err); if (err) { - goto err; - } - req = qobject_to(QDict, obj); - if (!req) { - error_setg(&err, "Input must be a JSON object"); - goto err; - } - if (!qdict_haskey(req, "execute")) { - g_warning("unrecognized payload format"); - error_setg(&err, QERR_UNSUPPORTED); - goto err; + rsp = qmp_error_response(err); + goto end; } - process_command(s, req); - qobject_unref(obj); - return; + g_debug("processing command"); + rsp = qmp_dispatch(&ga_commands, obj, false); -err: - g_warning("failed to parse event: %s", error_get_pretty(err)); - rsp = qmp_error_response(err); +end: ret = send_response(s, rsp); if (ret < 0) { g_warning("error sending error response: %s", strerror(-ret)); From 4eaca8de268d74ac5daaf8938abcb69d37ba2889 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Lureau?= Date: Wed, 20 Feb 2019 16:42:53 +0100 Subject: [PATCH 7/7] qmp: common 'id' handling & make QGA conform to QMP spec MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Let qmp_dispatch() copy the 'id' field. That way any qmp client will conform to the specification, including QGA. Furthermore, it simplifies the work for qemu monitor. CC: Michael Roth Signed-off-by: Marc-André Lureau Reviewed-by: Markus Armbruster Signed-off-by: Michael Roth --- monitor.c | 33 ++++++++++++--------------------- qapi/qmp-dispatch.c | 10 ++++++++-- tests/test-qga.c | 13 +++++-------- 3 files changed, 25 insertions(+), 31 deletions(-) diff --git a/monitor.c b/monitor.c index 72061d5bae..4807bbe811 100644 --- a/monitor.c +++ b/monitor.c @@ -250,8 +250,6 @@ QEMUBH *qmp_dispatcher_bh; struct QMPRequest { /* Owner of the request */ Monitor *mon; - /* "id" field of the request */ - QObject *id; /* * Request object to be handled or Error to be reported * (exactly one of them is non-null) @@ -353,7 +351,6 @@ int monitor_read_password(Monitor *mon, ReadLineFunc *readline_func, static void qmp_request_free(QMPRequest *req) { - qobject_unref(req->id); qobject_unref(req->req); error_free(req->err); g_free(req); @@ -4108,18 +4105,14 @@ static int monitor_can_read(void *opaque) * Null @rsp can only happen for commands with QCO_NO_SUCCESS_RESP. * Nothing is emitted then. */ -static void monitor_qmp_respond(Monitor *mon, QDict *rsp, QObject *id) +static void monitor_qmp_respond(Monitor *mon, QDict *rsp) { if (rsp) { - if (id) { - qdict_put_obj(rsp, "id", qobject_ref(id)); - } - qmp_send_response(mon, rsp); } } -static void monitor_qmp_dispatch(Monitor *mon, QObject *req, QObject *id) +static void monitor_qmp_dispatch(Monitor *mon, QObject *req) { Monitor *old_mon; QDict *rsp; @@ -4144,7 +4137,7 @@ static void monitor_qmp_dispatch(Monitor *mon, QObject *req, QObject *id) } } - monitor_qmp_respond(mon, rsp, id); + monitor_qmp_respond(mon, rsp); qobject_unref(rsp); } @@ -4208,13 +4201,15 @@ static void monitor_qmp_bh_dispatcher(void *data) mon->qmp.qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1; qemu_mutex_unlock(&mon->qmp.qmp_queue_lock); if (req_obj->req) { - trace_monitor_qmp_cmd_in_band(qobject_get_try_str(req_obj->id) ?: ""); - monitor_qmp_dispatch(mon, req_obj->req, req_obj->id); + QDict *qdict = qobject_to(QDict, req_obj->req); + QObject *id = qdict ? qdict_get(qdict, "id") : NULL; + trace_monitor_qmp_cmd_in_band(qobject_get_try_str(id) ?: ""); + monitor_qmp_dispatch(mon, req_obj->req); } else { assert(req_obj->err); rsp = qmp_error_response(req_obj->err); req_obj->err = NULL; - monitor_qmp_respond(mon, rsp, NULL); + monitor_qmp_respond(mon, rsp); qobject_unref(rsp); } @@ -4239,8 +4234,7 @@ static void handle_qmp_command(void *opaque, QObject *req, Error *err) qdict = qobject_to(QDict, req); if (qdict) { - id = qobject_ref(qdict_get(qdict, "id")); - qdict_del(qdict, "id"); + id = qdict_get(qdict, "id"); } /* else will fail qmp_dispatch() */ if (req && trace_event_get_state_backends(TRACE_HANDLE_QMP_COMMAND)) { @@ -4251,17 +4245,14 @@ static void handle_qmp_command(void *opaque, QObject *req, Error *err) if (qdict && qmp_is_oob(qdict)) { /* OOB commands are executed immediately */ - trace_monitor_qmp_cmd_out_of_band(qobject_get_try_str(id) - ?: ""); - monitor_qmp_dispatch(mon, req, id); + trace_monitor_qmp_cmd_out_of_band(qobject_get_try_str(id) ?: ""); + monitor_qmp_dispatch(mon, req); qobject_unref(req); - qobject_unref(id); return; } req_obj = g_new0(QMPRequest, 1); req_obj->mon = mon; - req_obj->id = id; req_obj->req = req; req_obj->err = err; @@ -4281,7 +4272,7 @@ static void handle_qmp_command(void *opaque, QObject *req, Error *err) /* * Put the request to the end of queue so that requests will be - * handled in time order. Ownership for req_obj, req, id, + * handled in time order. Ownership for req_obj, req, * etc. will be delivered to the handler side. */ assert(mon->qmp.qmp_requests->length < QMP_REQ_QUEUE_LEN_MAX); diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c index 1d922e04f7..5f812bb9f2 100644 --- a/qapi/qmp-dispatch.c +++ b/qapi/qmp-dispatch.c @@ -58,6 +58,8 @@ static QDict *qmp_dispatch_check_obj(const QObject *request, bool allow_oob, "QMP input member 'arguments' must be an object"); return NULL; } + } else if (!strcmp(arg_name, "id")) { + continue; } else { error_setg(errp, "QMP input member '%s' is unexpected", arg_name); @@ -165,11 +167,11 @@ QDict *qmp_dispatch(QmpCommandList *cmds, QObject *request, bool allow_oob) { Error *err = NULL; - QObject *ret; + QDict *dict = qobject_to(QDict, request); + QObject *ret, *id = dict ? qdict_get(dict, "id") : NULL; QDict *rsp; ret = do_qmp_dispatch(cmds, request, allow_oob, &err); - if (err) { rsp = qmp_error_response(err); } else if (ret) { @@ -180,5 +182,9 @@ QDict *qmp_dispatch(QmpCommandList *cmds, QObject *request, rsp = NULL; } + if (rsp && id) { + qdict_put_obj(rsp, "id", qobject_ref(id)); + } + return rsp; } diff --git a/tests/test-qga.c b/tests/test-qga.c index 3d6377436a..891aa3d322 100644 --- a/tests/test-qga.c +++ b/tests/test-qga.c @@ -225,18 +225,15 @@ static void test_qga_ping(gconstpointer fix) qobject_unref(ret); } -static void test_qga_invalid_id(gconstpointer fix) +static void test_qga_id(gconstpointer fix) { const TestFixture *fixture = fix; - QDict *ret, *error; - const char *class; + QDict *ret; ret = qmp_fd(fixture->fd, "{'execute': 'guest-ping', 'id': 1}"); g_assert_nonnull(ret); - - error = qdict_get_qdict(ret, "error"); - class = qdict_get_try_str(error, "class"); - g_assert_cmpstr(class, ==, "GenericError"); + qmp_assert_no_error(ret); + g_assert_cmpint(qdict_get_int(ret, "id"), ==, 1); qobject_unref(ret); } @@ -992,7 +989,7 @@ int main(int argc, char **argv) g_test_add_data_func("/qga/file-ops", &fix, test_qga_file_ops); g_test_add_data_func("/qga/file-write-read", &fix, test_qga_file_write_read); g_test_add_data_func("/qga/get-time", &fix, test_qga_get_time); - g_test_add_data_func("/qga/invalid-id", &fix, test_qga_invalid_id); + g_test_add_data_func("/qga/id", &fix, test_qga_id); g_test_add_data_func("/qga/invalid-oob", &fix, test_qga_invalid_oob); g_test_add_data_func("/qga/invalid-cmd", &fix, test_qga_invalid_cmd); g_test_add_data_func("/qga/invalid-args", &fix, test_qga_invalid_args);