qga: report serial number and disk node

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ý <[email protected]>

---
 qga/commands-win32.c | 27 ++++++++++++++++++++++-----
 1 file changed, 22 insertions(+), 5 deletions(-)

qga/commands-win32.c +16 -2

index 98d9735389..9c959122d9 100644
@@ -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

qga/commands-win32.c +0 -1

     list = g_malloc0(sizeof(*list));
--

2.19.0

Handle returned NULLs properly to:
- avoid crashes in serialization.
- properly report errors to the caller

Signed-off-by Tomáš Golembiovský <[email protected]>

---
 qga/commands-win32.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

qga/commands-win32.c +0 -0

index 9c959122d9..49fc747298 100644

qga/commands-win32.c +6 -2

+        fs = NULL;
+        goto free;
+    }
+
 free:
     g_free(mnt_point);
     return fs;

@@ -755,7 +761,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;

@@ -767,6 +773,7 @@ GuestFilesystemInfoList *qmp_guest_get_fsinfo(Error **errp)
         error_setg_win32(errp, GetLastError(), "failed to find next volume");
     }
 
+out:
     FindVolumeClose(vol_h);
     return ret;
 }
--

2.19.0

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ý <[email protected]>

---
 configure | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

configure +1 -2

index 58862d2ae8..0f168607e8 100755
@@ -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.19.0

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ý <[email protected]>

---
 qga/commands-win32.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

qga/commands-win32.c +15 -1

index 49fc747298..2a7a3af614 100644
@@ -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;

@@ -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)) {
+            debug_error("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)) {
+            debug_error("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)) {
+            debug_error("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.19.0

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ý <[email protected]>

---
 qga/commands-win32.c | 46 +++++++++++++++++++++++++++-----------------
 1 file changed, 28 insertions(+), 18 deletions(-)

qga/commands-win32.c +3 -2

index 2a7a3af614..d7864fc65a 100644
@@ -583,25 +583,29 @@ 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,

qga/commands-win32.c +20 -15

-    return dev_desc->BusType;
+    return;
 }
 
 /* VSS provider works with volumes, thus there is no difference if

@@ -613,8 +617,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 +628,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 +683,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
--

2.19.0

Signed-off-by Tomáš Golembiovský <[email protected]>

---
 configure | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

configure +22 -1

index 0f168607e8..ac24cb3975 100755
@@ -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
--

2.19.0

qga/commands-posix.c +4 -0

         qapi_free_GuestDiskAddressList(list);
     }
     g_free(driver);
+#ifdef CONFIG_LIBUDEV
+    udev_unref(udev);
+    udev_device_unref(udevice);
+#endif
 }
 
 static void build_guest_fsinfo_for_device(char const *devpath,

qga/commands-win32.c +24 -0

index d7864fc65a..376ca1e288 100644
@@ -603,6 +603,30 @@ 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");
+        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;

qga/qapi-schema.json +3 -2

index dfbc4a5e32..3bcda6257e 100644
@@ -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.19.0

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

Signed-off-by Tomáš Golembiovský <[email protected]>

---
 qga/commands-posix.c |  7 ++++++-
 qga/commands-win32.c | 21 +++++++++++----------
 qga/qapi-schema.json |  3 ++-
 3 files changed, 19 insertions(+), 12 deletions(-)

qga/commands-posix.c +0 -0

index 4d324178f2..31952b07f4 100644

qga/commands-posix.c +0 -0

         serial = udev_device_get_property_value(udevice, "ID_SERIAL");
         if (serial != NULL && *serial != 0) {
             disk->serial = g_strdup(serial);

qga/commands-win32.c +11 -10

index a591d8221d..d0d969d0ce 100644
@@ -649,8 +649,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;

@@ -659,8 +658,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");

@@ -692,7 +691,7 @@ static void get_single_disk_info(char *name, GuestDiskAddress *disk,
             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",

@@ -771,7 +770,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));

@@ -793,7 +794,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

@@ -804,10 +804,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;

qga/qapi-schema.json +2 -2

index 3bcda6257e..c6725b3ec8 100644
@@ -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.19.0

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ý <[email protected]>

---
 qga/commands-win32.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

qga/commands-win32.c +0 -0

index d0d969d0ce..82881aa749 100644

qga/commands-win32.c +3 -1

+    } 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");
--

2.19.0

Hi

Quoted 6 lines · Tomáš Golembiovský

There was inconsistency between commits:

50cbebb9a3 configure: add configure check for ntdddisk.h a3ef3b2272 qga: added bus type and disk location path

Quoted 8 lines

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ý <[email protected]>

Reviewed-by Marc-André Lureau <[email protected]>

Quoted 20 lines

---
 configure | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

configure +1 -2

index 58862d2ae8..0f168607e8 100755
@@ -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.19.0

Hi

Quoted 5 lines · Tomáš Golembiovský

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ý <[email protected]>

Reviewed-by Marc-André Lureau <[email protected]>

Quoted 9 lines

---
 qga/commands-win32.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

qga/commands-win32.c +0 -0

index d0d969d0ce..82881aa749 100644

qga/commands-win32.c static GuestPCIAddress *get_pci_info(char *guid, Error **errp) · line 514

Quoted 9 lines

qga/commands-win32.c +5 -1

@@ -507,7 +507,14 @@ 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;
+
+    if ((g_str_has_prefix(guid, "\\\\.\\") == TRUE) ||
+        (g_str_has_prefix(guid, "\\\\?\\") == TRUE)) {
+        name = g_strdup(&guid[4]);

I find "guid + 4" easier to read though

Quoted 9 lines

qga/commands-win32.c +3 -1

+    } 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");
--

2.19.0

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ý <[email protected]>

---
 qga/commands-win32.c | 126 ++++++++++++++++++++++++++++++++++++-------
 1 file changed, 108 insertions(+), 18 deletions(-)

qga/commands-win32.c +3 -0

index 1e64642b8a..a591d8221d 100644
@@ -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!)
+ */

qga/commands-win32.c +8 -1

+
+/*
 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");

qga/commands-win32.c +6 -6

@@ -637,20 +654,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;

qga/commands-win32.c +0 -0

@@ -668,7 +685,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;

qga/commands-win32.c +5 -2

@@ -699,8 +716,7 @@ static void get_single_disk_info(char *name,
 GuestDiskAddress *disk,
     }
 
 err_close:
-    CloseHandle(vol_h);
-err:
+    CloseHandle(disk_h);
     return;
 }

@@ -712,6 +728,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);

qga/commands-win32.c +58 -8

@@ -719,19 +739,89 @@ 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

qga/commands-win32.c +3 -0

+         *
+         * See also Naming Files, Paths and Namespaces:
+         *

https://docs.microsoft.com/en-us/windows/desktop/FileIO/naming-a-file#win32-device-namespaces

qga/commands-win32.c +17 -1

+         */
+        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:
+    g_free(extents);
     g_free(disk);
     g_free(name);
 
--

2.19.0

Hi

Quoted 5 lines · Tomáš Golembiovský

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ý <[email protected]>

Reviewed-by Marc-André Lureau <[email protected]>

Quoted 4 lines

---
 qga/commands-win32.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Quoted 5 lines

qga/commands-win32.c +0 -0

index d0d969d0ce..82881aa749 100644

qga/commands-win32.c static GuestPCIAddress *get_pci_info(char *guid, Error **errp) · line 514

Quoted 9 lines

qga/commands-win32.c +5 -1

@@ -507,7 +507,14 @@ 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;
+
+    if ((g_str_has_prefix(guid, "\\\\.\\") == TRUE) ||
+        (g_str_has_prefix(guid, "\\\\?\\") == TRUE)) {
+        name = g_strdup(&guid[4]);  

I find "guid + 4" easier to read though

Quoted 10 lines

qga/commands-win32.c +3 -1

+    } 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");
--

2.19.0

Hi

Quoted 46 lines · 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ý <[email protected]>

---
 qga/commands-win32.c | 27 ++++++++++++++++++++++-----
 1 file changed, 22 insertions(+), 5 deletions(-)

qga/commands-win32.c +17 -5

index 98d9735389..9c959122d9 100644
@@ -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;
+    }

qga/commands-win32.c static GuestDiskAddressList *build_guest_disk_info(char *guid, Error **errp) · line 662

Quoted 6 lines

qga/commands-win32.c +5 -0

+    /* 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));
     }  

Shouldn't pci-controller be made optional in the schema instead?

Quoted 6 lines

qga/commands-win32.c +0 -1

     list = g_malloc0(sizeof(*list));
--

2.19.0

I did a quick scan for the documentation and the code and it seems that the name format that you're looking for is provided by the "QueryDosDeviceW" function. The function returns multiple names and in the current code we only check the first one. I believe that one of these names provided should be the win32 device name (dos name).

Plus I did some googling and found out this similar question:
https://stackoverflow.com/questions/36199097/list-the-content-of-the-win32-device-namespace

Which suggested using the following tool:
https://docs.microsoft.com/en-us/sysinternals/downloads/winobj

Quoted 231 lines · Tomáš Golembiovský

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ý <[email protected]>

---
 qga/commands-win32.c | 126 ++++++++++++++++++++++++++++++++++++-------
 1 file changed, 108 insertions(+), 18 deletions(-)

qga/commands-win32.c +3 -0

index 1e64642b8a..a591d8221d 100644
@@ -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!)
+ */

qga/commands-win32.c +8 -1

+
+/*
 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");

qga/commands-win32.c +6 -6

@@ -637,20 +654,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;

qga/commands-win32.c +0 -0

@@ -668,7 +685,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;

qga/commands-win32.c +5 -2

@@ -699,8 +716,7 @@ static void get_single_disk_info(char *name,
 GuestDiskAddress *disk,
     }
 
 err_close:
-    CloseHandle(vol_h);
-err:
+    CloseHandle(disk_h);
     return;
 }

@@ -712,6 +728,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);

qga/commands-win32.c +58 -8

@@ -719,19 +739,89 @@ 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

qga/commands-win32.c +3 -0

+         *
+         * See also Naming Files, Paths and Namespaces:
+         *

https://docs.microsoft.com/en-us/windows/desktop/FileIO/naming-a-file#win32-device-namespaces

qga/commands-win32.c +17 -1

+         */
+        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:
+    g_free(extents);
     g_free(disk);
     g_free(name);
 
--

2.19.0

Signature · 4 lines

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

Hi

Quoted 3 lines · Tomáš Golembiovský

It was not obvious what exactly the cryptic string copying does to the GUID. This change makes the intent clearer.

qga/commands-win32.c static GuestPCIAddress *get_pci_info(char *guid, Error **errp) · line 513

Quoted 10 lines

+++ b/qga/commands-win32.c

qga/commands-win32.c +4 -1

@@ -507,7 +507,14 @@ 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;
+
+    if ((g_str_has_prefix(guid, "\\\\.\\") == TRUE) ||
+        (g_str_has_prefix(guid, "\\\\?\\") == TRUE)) {

qga/commands-win32.c static GuestPCIAddress *get_pci_info(char *guid, Error **errp) · line 514

Quoted 1 line

qga/commands-win32.c +1 -0

+        name = g_strdup(&guid[4]);

I find "guid + 4" easier to read though

Handle returned NULLs properly to:
- avoid crashes in serialization.
- properly report errors to the caller

Signed-off-by Tomáš Golembiovský <[email protected]>

---
 qga/commands-win32.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

qga/commands-win32.c +0 -0

index 9c959122d9..49fc747298 100644

qga/commands-win32.c +1 -0

         new = g_malloc(sizeof(*ret));
         new->value = info;

@@ -767,6 +773,7 @@ GuestFilesystemInfoList *qmp_guest_get_fsinfo(Error **errp)
         error_setg_win32(errp, GetLastError(), "failed to find next volume");
     }
 
+out:
     FindVolumeClose(vol_h);
     return ret;
 }

Signature · 1 line

2.19.0

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ý <[email protected]>

---
 qga/commands-win32.c | 27 ++++++++++++++++++++++-----
 1 file changed, 22 insertions(+), 5 deletions(-)

qga/commands-win32.c +16 -1

index 98d9735389..9c959122d9 100644
@@ -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);
+            }
         }

qga/commands-win32.c +0 -0

     }
 
     list = g_malloc0(sizeof(*list));

Signature · 1 line

2.19.0

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ý <[email protected]>

---
  qga/commands-win32.c | 27 ++++++++++++++++++++++-----
  1 file changed, 22 insertions(+), 5 deletions(-)