[Qemu-devel] [PATCH v3 0/5] qga: report serial number and disk node

[Qemu-devel] [PATCH v3 0/5] qga: report serial number and disk node

[Tomáš Golembiovský]

Note that PCI controller reporting on Windows was and still is broken.
Unfortunately I don't know how to fix it at the momemnt. See commit message and
code comment. If anyone has environment where the original code works let me
know. CCing author of the code In case I missed something obvious.

v3:
  - fix typos
  - add configure test for libudev
  - change order of patches fixing PCI controller info and build fix to avoid
    exposing broken code
  - split reporting of serial number and device node into two separate patches

v2:
  - fix checkpatch error

Patches 1 and 3 are bug-fixes of existing code
Patch 2 is optional and contains debug messages -- the windows QGA seriously
lacks debug messages...
Patch 4 adds reporting of disk serial number and device node of the disk.

Tomáš Golembiovský (5):
  qga: win32: fix crashes when PCI info cannot be retrived
  build: rename CONFIG_QGA_NTDDDISK to CONFIG_QGA_NTDDSCSI
  qga: win32: add debugging information
  qga: report disk serial number
  qga: return disk device in guest-get-fsinfo

 configure            |  24 ++++-
 qga/Makefile.objs    |   1 +
 qga/commands-posix.c |  27 +++++
 qga/commands-win32.c | 248 ++++++++++++++++++++++++++++++++++++-------
 qga/qapi-schema.json |   5 +-
 5 files changed, 264 insertions(+), 41 deletions(-)

-- 
2.18.0

[Qemu-devel] [PATCH v3 1/5] qga: win32: fix crashes when PCI info cannot be retrived

[Tomáš Golembiovský]

The guest-get-fsinfo command collects also information about PCI
controller where the disk is attached. When this fails for some reasons
it tries to return just the partial information. However in certain
cases the pointer to the structure was not initialized and was set to
NULL. This breaks the serializer and leads to a crash of the guest agent.

Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
---
 qga/commands-win32.c | 27 ++++++++++++++++++++++-----
 1 file changed, 22 insertions(+), 5 deletions(-)

diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 98d9735389..9c959122d9 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -633,15 +633,32 @@ static GuestDiskAddressList *build_guest_disk_info(char *guid, Error **errp)
          * https://technet.microsoft.com/en-us/library/ee851589(v=ws.10).aspx */
         if (DeviceIoControl(vol_h, IOCTL_SCSI_GET_ADDRESS, NULL, 0, scsi_ad,
                             sizeof(SCSI_ADDRESS), &len, NULL)) {
+            Error *local_err = NULL;
             disk->unit = addr.Lun;
             disk->target = addr.TargetId;
             disk->bus = addr.PathId;
-            disk->pci_controller = get_pci_info(name, errp);
+            g_debug("unit=%lld target=%lld bus=%lld",
+                disk->unit, disk->target, disk->bus);
+            disk->pci_controller = get_pci_info(name, &local_err);
+
+            if (local_err) {
+                g_debug("failed to get PCI controller info: %s",
+                    error_get_pretty(local_err));
+                error_free(local_err);
+            } else if (disk->pci_controller != NULL) {
+                g_debug("pci: domain=%lld bus=%lld slot=%lld function=%lld",
+                    disk->pci_controller->domain,
+                    disk->pci_controller->bus,
+                    disk->pci_controller->slot,
+                    disk->pci_controller->function);
+            }
         }
-        /* We do not set error in this case, because we still have enough
-         * information about volume. */
-    } else {
-         disk->pci_controller = NULL;
+    }
+    /* We do not set error in case pci_controller is NULL, because we still
+     * have enough information about volume. */
+    if (disk->pci_controller == NULL) {
+        g_debug("no PCI controller info");
+        disk->pci_controller = g_malloc0(sizeof(GuestPCIAddress));
     }
 
     list = g_malloc0(sizeof(*list));
-- 
2.18.0

[Qemu-devel] [PATCH v3 2/5] build: rename CONFIG_QGA_NTDDDISK to CONFIG_QGA_NTDDSCSI

[Tomáš Golembiovský]

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ý <tgolembi@redhat.com>
---
 configure | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/configure b/configure
index 58862d2ae8..0f168607e8 100755
--- a/configure
+++ b/configure
@@ -6201,7 +6201,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
-- 
2.18.0

[Qemu-devel] [PATCH v3 3/5] qga: win32: add debugging information

[Tomáš Golembiovský]

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ý <tgolembi@redhat.com>
---
 qga/commands-win32.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 9c959122d9..e16c58275e 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 g_debug_err(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;
@@ -498,6 +504,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, func, dev, data, size2;
@@ -522,6 +529,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
@@ -530,6 +538,7 @@ static GuestPCIAddress *get_pci_info(char *guid, Error **errp)
          */
         if (!SetupDiGetDeviceRegistryProperty(dev_info, &dev_info_data,
                    SPDRP_BUSNUMBER, &data, (PBYTE)&bus, size, NULL)) {
+            g_debug_err("failed to get bus");
             break;
         }
 
@@ -537,6 +546,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)) {
+            g_debug_err("failed to get address");
             break;
         }
 
@@ -544,6 +554,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)) {
+            g_debug_err("failed to get slot");
             break;
         }
 
@@ -608,6 +619,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) {
@@ -615,6 +627,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;
@@ -622,6 +635,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);
     if (bus == BusTypeScsi || bus == BusTypeAta || bus == BusTypeRAID
 #if (_WIN32_WINNT >= 0x0600)
             /* This bus type is not supported before Windows Server 2003 SP1 */
@@ -631,6 +645,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)) {
             Error *local_err = NULL;
-- 
2.18.0

[Qemu-devel] [PATCH v3 4/5] qga: report disk serial number

[Tomáš Golembiovský]

The feature is implemented for Windows and Linux. Reporting of serial
number on Linux depends on libudev.

Example from Linux:

    {
      "name": "dm-2",
      "mountpoint": "/",
      ...
      "disk": [
        {
          "serial": "SAMSUNG_MZ7LN512HCHP-000L1_S1ZKNXAG822493",
          ...
        }
      ],
    }

Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
---
 configure            | 22 ++++++++++++++
 qga/Makefile.objs    |  1 +
 qga/commands-posix.c | 22 ++++++++++++++
 qga/commands-win32.c | 71 ++++++++++++++++++++++++++++++++------------
 qga/qapi-schema.json |  4 ++-
 5 files changed, 100 insertions(+), 20 deletions(-)

diff --git a/configure b/configure
index 0f168607e8..ac24cb3975 100755
--- a/configure
+++ b/configure
@@ -477,6 +477,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"
@@ -873,6 +874,7 @@ Linux)
   vhost_vsock="yes"
   QEMU_INCLUDES="-I\$(SRC_PATH)/linux-headers -I$(pwd)/linux-headers $QEMU_INCLUDES"
   supported_os="yes"
+  libudev="yes"
 ;;
 esac
 
@@ -5676,6 +5678,20 @@ if test "$libnfs" != "no" ; then
   fi
 fi
 
+##########################################
+# Do we have libudev
+if test "$libudev" != "no" ; then
+  if $pkg_config libudev; then
+    libudev="yes"
+    libudev_libs=$($pkg_config --libs libudev)
+  else
+    if test "$libudev" = "yes" ; then
+      feature_not_found "libudev" "Install systemd development files"
+    fi
+    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"
@@ -6100,6 +6116,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"
@@ -6944,6 +6961,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
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 37e8a2d791..37fedd123b 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -47,6 +47,7 @@ extern char **environ;
 #include <sys/socket.h>
 #include <net/if.h>
 #include <sys/statvfs.h>
+#include <libudev.h>
 
 #ifdef FIFREEZE
 #define CONFIG_FSFREEZE
@@ -872,6 +873,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 +941,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:<unit>:0 */
         if (!has_host || !has_tgt) {
@@ -1003,6 +1023,8 @@ cleanup:
         qapi_free_GuestDiskAddressList(list);
     }
     g_free(driver);
+    udev_unref(udev);
+    udev_device_unref(udevice);
 }
 
 static void build_guest_fsinfo_for_device(char const *devpath,
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index e16c58275e..fa186154a8 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -583,25 +583,53 @@ 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);
+
+    /* 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");
+        goto out_free;
+    }
+    if (dev_desc->SerialNumberOffset > 0) {
+        if (dev_desc->SerialNumberOffset >= received) {
+            error_setg(errp, "offset outside the buffer");
+            goto out_free;
+        }
+        const char *serial = (char *)dev_desc + dev_desc->SerialNumberOffset;
+        size_t len = received - dev_desc->SerialNumberOffset;
+        if (*serial != 0) {
+            disk->serial = g_strndup(serial, len);
+            disk->has_serial = true;
+            g_debug("serial number %s", disk->serial);
+        }
     }
+out_free:
+    g_free(dev_desc);
 
-    return dev_desc->BusType;
+    return;
 }
 
 /* VSS provider works with volumes, thus there is no difference if
@@ -613,8 +641,8 @@ 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;
 
     scsi_ad = &addr;
     char *name = g_strndup(guid, strlen(guid)-1);
@@ -624,22 +652,22 @@ 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;
+        goto err;
     }
 
-    g_debug("getting bus type");
-    bus = get_disk_bus_type(vol_h, errp);
-    if (bus < 0) {
-        goto out_close;
+    disk = g_malloc0(sizeof(*disk));
+    get_disk_properties(vol_h, disk, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        goto err_close;
     }
 
-    disk = g_malloc0(sizeof(*disk));
-    disk->bus_type = find_bus_type(bus);
-    g_debug("bus type %d", disk->bus_type);
-    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
@@ -679,11 +707,16 @@ 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
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:
-- 
2.18.0

[Qemu-devel] [PATCH v3 5/5] qga: return disk device in guest-get-fsinfo

[Tomáš Golembiovský]

Report device node of the disk. It is implemented for Linux (needs udev)
and Windows. The node is reported e.g. as "/dev/sda2" on Linux and as
"\\?\PhysicalDriveX" on Windows.

As part of this effort the Windows code was changed to probe 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ý <tgolembi@redhat.com>
---
 qga/commands-posix.c |   7 +-
 qga/commands-win32.c | 163 +++++++++++++++++++++++++++++++++++--------
 qga/qapi-schema.json |   3 +-
 3 files changed, 142 insertions(+), 31 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 37fedd123b..d0d6ba49cb 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -947,7 +947,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/commands-win32.c b/qga/commands-win32.c
index fa186154a8..2cde6bd0af 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -477,9 +477,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)
 {
@@ -497,7 +514,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");
@@ -632,31 +649,24 @@ 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(GuestDiskAddress *disk, Error **errp)
 {
-    GuestDiskAddressList *list = NULL;
-    GuestDiskAddress *disk;
     SCSI_ADDRESS addr, *scsi_ad;
     DWORD len;
-    HANDLE vol_h;
+    HANDLE disk_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,
+    g_debug("getting disk info for: %s", disk->dev);
+    disk_h = CreateFile(disk->dev, 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;
     }
 
-    disk = g_malloc0(sizeof(*disk));
-    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;
@@ -674,20 +684,20 @@ static GuestDiskAddressList *build_guest_disk_info(char *guid, Error **errp)
          * 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)) {
-            Error *local_err = NULL;
             disk->unit = addr.Lun;
             disk->target = addr.TargetId;
             disk->bus = addr.PathId;
             g_debug("unit=%lld target=%lld bus=%lld",
                 disk->unit, disk->target, disk->bus);
-            disk->pci_controller = get_pci_info(name, &local_err);
+            disk->pci_controller = get_pci_info(disk->dev, &local_err);
 
             if (local_err) {
                 g_debug("failed to get PCI controller info: %s",
                     error_get_pretty(local_err));
                 error_free(local_err);
+                local_err = NULL;
             } else if (disk->pci_controller != NULL) {
                 g_debug("pci: domain=%lld bus=%lld slot=%lld function=%lld",
                     disk->pci_controller->domain,
@@ -704,19 +714,107 @@ static GuestDiskAddressList *build_guest_disk_info(char *guid, Error **errp)
         disk->pci_controller = g_malloc0(sizeof(GuestPCIAddress));
     }
 
-    list = g_malloc0(sizeof(*list));
-    list->value = disk;
-    list->next = NULL;
-    CloseHandle(vol_h);
-    return list;
-
 err_close:
+    CloseHandle(disk_h);
+    return;
+}
+
+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_strndup(guid, strlen(guid)-1);
+
+    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;
+    }
+
+    /* 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));
+            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));
+                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++) {
+        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
+         */
+        disk->has_dev = true;
+        disk->dev = g_strdup_printf("\\\\?\\PhysicalDrive%lu",
+            extents->Extents[i].DiskNumber);
+
+        get_single_disk_info(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;
+        cur_item->next = list;
+        list = cur_item;
+    }
+
+
+out:
+    g_free(extents);
     g_free(disk);
-    CloseHandle(vol_h);
-err:
     g_free(name);
 
-    return NULL;
+    return list;
 }
 
 #else
@@ -783,6 +881,12 @@ static GuestFilesystemInfo *build_guest_fsinfo(char *guid, Error **errp)
     }
     fs->type = g_strdup(fs_name);
     fs->disk = build_guest_disk_info(guid, errp);
+    if (fs->disk == NULL) {
+        g_free(fs);
+        fs = NULL;
+        goto free;
+    }
+
 free:
     g_free(mnt_point);
     return fs;
@@ -803,7 +907,7 @@ GuestFilesystemInfoList *qmp_guest_get_fsinfo(Error **errp)
     do {
         GuestFilesystemInfo *info = build_guest_fsinfo(guid, errp);
         if (info == NULL) {
-            continue;
+            goto out;
         }
         new = g_malloc(sizeof(*ret));
         new->value = info;
@@ -815,6 +919,7 @@ GuestFilesystemInfoList *qmp_guest_get_fsinfo(Error **errp)
         error_setg_win32(errp, GetLastError(), "failed to find next volume");
     }
 
+out:
     FindVolumeClose(vol_h);
     return ret;
 }
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:
-- 
2.18.0

Re: [Qemu-devel] [PATCH v3 1/5] qga: win32: fix crashes when PCI info cannot be retrived

[Tomáš Golembiovský]

On Thu, 27 Sep 2018 18:16:04 +0300
Sameeh Jubran <sjubran@redhat.com> wrote:

> On Thu, Sep 27, 2018 at 12:06 PM Tomáš Golembiovský <tgolembi@redhat.com>
> wrote:
> 
> > Hi Michael,
> >
> > thanks for looking into this. My comments are below.
> >
> > Adding Sameeh...
> >
> >
> > On Wed, 26 Sep 2018 12:15:48 -0500
> > Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> >  
> > > Quoting Tomáš Golembiovský (2018-09-07 06:42:09)  
> > > > The guest-get-fsinfo command collects also information about PCI
> > > > controller where the disk is attached. When this fails for some reasons
> > > > it tries to return just the partial information. However in certain
> > > > cases the pointer to the structure was not initialized and was set to
> > > > NULL. This breaks the serializer and leads to a crash of the guest  
> > agent.  
> > > >
> > > > Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>  
> > >
> > > For a win10 guest started with:
> > >
> > > qemu-system-x86_64 -m 2G -smp 2,cores=2,sockets=1 -drive  
> > file=/home/mdroth/vm/win10_pro_n_snap0.qcow2,if=virtio -drive
> > file=/home/mdroth/vm/virtio-win-0.1.102.iso,if=ide,media=cdrom -rtc
> > base=localtime,driftfix=slew -vga std -boot d -name vm4 -netdev
> > tap,script=/etc/qemu-ifup,vhost=on,id=vnet0 -device
> > virtio-net-pci,mac=52:54:00:12:34:04,id=vnic0,netdev=vnet0,disable-modern=true
> > -vnc :4 -device virtio-serial -balloon virtio -mon chardev=hmp0 -chardev
> > socket,path=/tmp/vm4-hmp0.sock,server,nowait,id=hmp0 -mon
> > chardev=qmp0,mode=control -chardev
> > socket,path=/tmp/vm4-qmp0.sock,server,nowait,id=qmp0 -device
> > virtserialport,chardev=vs0,name=vs0,id=vs_vs0 -chardev
> > socket,path=/tmp/vm4-vs0.sock,server,nowait,id=vs0 -device
> > virtserialport,chardev=vs1,name=vs1,id=vs_vs1 -chardev
> > socket,path=/tmp/vm4-vs1.sock,server,nowait,id=vs1 -device
> > virtserialport,chardev=qga,name=org.qemu.guest_agent.0,id=vs_qga -chardev
> > socket,path=/tmp/vm4-qga.sock,server,nowait,id=qga -device
> > isa-serial,chardev=serial0,id=serial_serial0 -chardev
> > socket,path=/tmp/vm4-serial0.sock,server,nowait,id=serial0 -L
> > /home/mdroth/w/build/qemu-2.11.2-build/pc-bios --enable-kvm  
> > >
> > > this yields the following:
> > >
> > > {'execute':'guest-get-fsinfo'}
> > > {"return": [{"name":  
> > "\\\\?\\Volume{83835b2d-0032-11e6-a84f-806e6f6e6963}\\", "total-bytes":
> > 160755712, "mountpoint": "D:\\", "disk": [{"bus-type": "ide", "bus": 0,
> > "unit": 0, "pci-controller": {"bus": 0, "slot": 0, "domain": 0, "function":
> > 0}, "target": 0}], "used-bytes": 160755712, "type": "CDFS"}, {"name":
> > "\\\\?\\Volume{2ea839c6-0000-0000-0000-80620c000000}\\", "mountpoint":
> > "System Reserved", "disk": [{"bus-type": "scsi", "bus": 0, "unit": 0,
> > "pci-controller": {"bus": 0, "slot": 0, "domain": 0, "function": 0},
> > "target": 0}], "type": "NTFS"}, {"name":
> > "\\\\?\\Volume{2ea839c6-0000-0000-0000-501f00000000}\\", "total-bytes":
> > 52665839616, "mountpoint": "C:\\", "disk": [{"bus-type": "scsi", "bus": 0,
> > "unit": 0, "pci-controller": {"bus": 0, "slot": 0, "domain": 0, "function":
> > 0}, "target": 0}], "used-bytes": 25265487872, "type": "NTFS"}, {"name":
> > "\\\\?\\Volume{2ea839c6-0000-0000-0000-100000000000}\\", "mountpoint":
> > "System Reserved", "disk": [{"bus-type": "scsi", "bus": 0, "unit": 0,
> > "pci-controller": {"bus": 0, "slot": 0, "domain": 0, "function": 0},
> > "target": 0}], "type": "NTFS"}]}  
> > >
> > > domain/bus/slot/function=0 are valid PCI addresses so initializing to 0  
> > is  
> > > wrong. Sameeh had a previous series that initializes to -1 that I think
> > > is more appropriate (it hasn't gone in yet since we opted not to enable
> > > CONFIG_QGA_NTDDSCSI for 3.0 since the PCI stuff seems be generally
> > > broken for Windows, also because the 2nd patch needs some fixups:  
> >  
> Can you please elaborate? What kind of fixups? I can see no comments of
> this in the 2nd patch
> 

I think he refers to this fix:

https://github.com/mdroth/qemu/commit/201db36b56d7d1ba5ff720eedcb3b62b75306fde

Plus there seems to be an issue when "calculating" function number as
described below. (Maybe related to casting addr from -1 to uint and
back?)

> > >
> > >   https://lists.gnu.org/archive/html/qemu-devel/2018-06/msg07500.html  
> >
> > I wasn't aware of that. I is trying to "fix" the same issue.
> >
> > I've been also thinking about using -1, but I didn't know what is/isn't
> > correct PCI address.
> >  
> > >
> > > With that series (and some fixups I have on top at
> > > https://github.com/mdroth/qemu/commits/qga-test), we get the following
> > > output:  
> >
> > Should I rebase on that and drop my patch?
> >  
> > >
> > > {'execute':'guest-get-fsinfo'}
> > > {"return": [{"name":  
> > "\\\\?\\Volume{83835b2d-0032-11e6-a84f-806e6f6e6963}\\", "total-bytes":
> > 160755712, "mountpoint": "D:\\", "disk": [{"bus-type": "ide", "bus": 0,
> > "unit": 0, "pci-controller": {"bus": -1, "slot": -1, "domain": -1,
> > "function": -1}, "target": 0}], "used-bytes": 160755712, "type": "CDFS"},
> > {"name": "\\\\?\\Volume{2ea839c6-0000-0000-0000-80620c000000}\\",
> > "mountpoint": "System Reserved", "disk": [{"bus-type": "scsi", "bus": 0,
> > "unit": 0, "pci-controller": {"bus": -1, "slot": -1, "domain": 0,
> > "function": 3}, "target": 0}], "type": "NTFS"}, {"name":
> > "\\\\?\\Volume{2ea839c6-0000-0000-0000-501f00000000}\\", "total-bytes":
> > 52665839616, "mountpoint": "C:\\", "disk": [{"bus-type": "scsi", "bus": 0,
> > "unit": 0, "pci-controller": {"bus": -1, "slot": -1, "domain": 0,
> > "function": 2}, "target": 0}], "used-bytes": 25267560448, "type": "NTFS"},
> > {"name": "\\\\?\\Volume{2ea839c6-0000-0000-0000-100000000000}\\",
> > "mountpoint": "System Reserved", "disk": [{"bus-type": "scsi", "bus": 0,
> > "unit": 0, "pci-controller": {"bus": -1, "slot": -1, "domain": 0,
> > "function": 1}, "target": 0}], "type": "NTFS"}]}  
> > >
> > > Here we see the non-sensical PCI topology info I mentioned previously.
> > > There are values like '"function": 3' even though there are no
> > > multifunction devices present. This will be exposed to users if we
> > > enable CONFIG_QGA_NTDDSCSI with things as they stand.  
> >
> > Sadly, I have no idea how to properly fix this code. But patch 5 of the
> > series IMHO brings it one step closer to proper solution. The original
> > code tries to fetch the addr/bus/domain/slot for volume handle (which
> > fails on missing properties) instead of disk handle.  
> 
> 
> > The remaining issue is that when listing the disks the entries are
> > cryptic names like "\Device\0000001d" instead of "\PhysicalDriveX" or
> > "\HarddiskX". And I don't know how to map one name to the other.
> >  
>     char *name = g_strdup(&guid[4]);
> This needs some investigation...
> 

This is to strip first 4 characters from UNC names.

    "\\?\abcdef" -> "\abcdef"


> >  
> > > Currently we
> > > just get an empty array for "disk" field of GuestFilesystemInfo
> > > for w32, which fortunately aligns with the current QAPI schema (it's
> > > an array since the volume can span multiple disks).  
> >
> > No. You get array with one item and useless data. Unless I missed
> > something.
> >
> >  
> > > I'm not about the
> > > SCSI bus/unit/target data either. It's a real mess (maybe things
> > > work a bit better when an actual SCSI controller is used?) and I'm
> > > not sure why/how I didn't notice during initial testing.  
> >
> > I think unit/target should work. At least with all patches from my
> > series (well notably the last patch). But please, do verify that if you
> > can.
> >
> >  
> > > I think all this needs to be addressed before we enable
> > > CONFIG_QGA_NTDDSCSI (in terms of what that's used for in the current
> > > code at least, i.e. enabling everything reported by
> > > GuestFilesystemInfo.disk).
> > >
> > > What is the oVirt use-case? It doesn't seem like you need PCI topology,
> > > but what about SCSI topology (and if so, does that information seem
> > > correct to you)? Or is just a list a serials/dev paths by themselves
> > > all that's needed? Trying to see how we might decouple things from the
> > > broken PCI topology stuff. May need to deprecate
> > > GuestFilesystemInfo.disk in favor of something less monolithic if all
> > > of this isn't fixable in a reasonable-enough timeframe.  
> >
> > We don't need the PCI info. I've been only trying to fix this to some
> > sort along the way. What we need is to get disk serial number (patch 4)
> > and "name" of the disk of some sort (patch 5). For that I use device
> > node (e.g. /dev/sda) on Linux and UNC for the disk on Windows (e.g.
> > \\?\PhysicalDrive1). But the code relies on CONFIG_QGA_NTDDSCSI being
> > enabled.
> >  
> This is already enabled in downstream version of qga-win, I don't think it
> should be
> enabled upstream if it's not necessary. ( Actually I'm not sure why it is
> disabled in the first place? I supposed because it is not stable)

So can we enable CONFIG_QGA_NTDDSCSI and disable PCI controller info by
different means?

> 
> >
> >     Tomas
> >  
> > >
> > >  
> > > > ---
> > > >  qga/commands-win32.c | 27 ++++++++++++++++++++++-----
> > > >  1 file changed, 22 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> > > > index 98d9735389..9c959122d9 100644
> > > > --- a/qga/commands-win32.c
> > > > +++ b/qga/commands-win32.c
> > > > @@ -633,15 +633,32 @@ static GuestDiskAddressList  
> > *build_guest_disk_info(char *guid, Error **errp)  
> > > >           *  
> > https://technet.microsoft.com/en-us/library/ee851589(v=ws.10).aspx */  
> > > >          if (DeviceIoControl(vol_h, IOCTL_SCSI_GET_ADDRESS, NULL, 0,  
> > scsi_ad,  
> > > >                              sizeof(SCSI_ADDRESS), &len, NULL)) {
> > > > +            Error *local_err = NULL;
> > > >              disk->unit = addr.Lun;
> > > >              disk->target = addr.TargetId;
> > > >              disk->bus = addr.PathId;
> > > > -            disk->pci_controller = get_pci_info(name, errp);
> > > > +            g_debug("unit=%lld target=%lld bus=%lld",
> > > > +                disk->unit, disk->target, disk->bus);
> > > > +            disk->pci_controller = get_pci_info(name, &local_err);
> > > > +
> > > > +            if (local_err) {
> > > > +                g_debug("failed to get PCI controller info: %s",
> > > > +                    error_get_pretty(local_err));
> > > > +                error_free(local_err);
> > > > +            } else if (disk->pci_controller != NULL) {
> > > > +                g_debug("pci: domain=%lld bus=%lld slot=%lld  
> > function=%lld",  
> > > > +                    disk->pci_controller->domain,
> > > > +                    disk->pci_controller->bus,
> > > > +                    disk->pci_controller->slot,
> > > > +                    disk->pci_controller->function);
> > > > +            }
> > > >          }
> > > > -        /* We do not set error in this case, because we still have  
> > enough  
> > > > -         * information about volume. */
> > > > -    } else {
> > > > -         disk->pci_controller = NULL;
> > > > +    }
> > > > +    /* We do not set error in case pci_controller is NULL, because we  
> > still  
> > > > +     * have enough information about volume. */
> > > > +    if (disk->pci_controller == NULL) {
> > > > +        g_debug("no PCI controller info");
> > > > +        disk->pci_controller = g_malloc0(sizeof(GuestPCIAddress));
> > > >      }
> > > >
> > > >      list = g_malloc0(sizeof(*list));
> > > > --
> > > > 2.18.0
> > > >  
> >
> >
> > --
> > Tomáš Golembiovský <tgolembi@redhat.com>
> >  


-- 
Tomáš Golembiovský <tgolembi@redhat.com>