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

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

[Tomáš Golembiovský]

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ý (4):
  build: rename CONFIG_QGA_NTDDDISK to CONFIG_QGA_NTDDSCSI
  qga: win32: add debugging information
  qga: win32: fix crashes when PCI info cannot be retrived
  qga: report serial ID and device node

 configure            |   3 +-
 qga/commands-posix.c |  23 ++++++++++
 qga/commands-win32.c | 105 ++++++++++++++++++++++++++++++++++---------
 qga/qapi-schema.json |   4 +-
 4 files changed, 111 insertions(+), 24 deletions(-)

-- 
2.18.0

[Qemu-devel] [PATCH v2 1/4] 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 2a7796ea80..b31f37ed50 100755
--- a/configure
+++ b/configure
@@ -6111,7 +6111,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 v2 2/4] 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 | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 318d760a74..36d76c22c0 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -498,6 +498,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 +523,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 +532,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("failed to get bus");
             break;
         }
 
@@ -537,6 +540,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("failed to get address");
             break;
         }
 
@@ -544,6 +548,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("failed to get slot");
             break;
         }
 
@@ -608,6 +613,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 +621,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 +629,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 +639,7 @@ static GuestDiskAddressList *build_guest_disk_info(char *guid, Error **errp)
         /* We are able to use the same ioctls for different bus types
          * according to Microsoft docs
          * https://technet.microsoft.com/en-us/library/ee851589(v=ws.10).aspx */
+        g_debug("getting pci-controller info");
         if (DeviceIoControl(vol_h, IOCTL_SCSI_GET_ADDRESS, NULL, 0, scsi_ad,
                             sizeof(SCSI_ADDRESS), &len, NULL)) {
             disk->unit = addr.Lun;
-- 
2.18.0

[Qemu-devel] [PATCH v2 3/4] 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 lead to crasehs 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 36d76c22c0..995f62c2e4 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -642,15 +642,32 @@ static GuestDiskAddressList *build_guest_disk_info(char *guid, Error **errp)
         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;
             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) {
+                slog("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 v2 4/4] qga: report serial ID and device node

[Tomáš Golembiovský]

On Linux the functionality depends on libudev.

Example from Linux:

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

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

diff --git a/configure b/configure
index b31f37ed50..d73d929b6c 100755
--- a/configure
+++ b/configure
@@ -871,6 +871,7 @@ Linux)
   vhost_vsock="yes"
   QEMU_INCLUDES="-I\$(SRC_PATH)/linux-headers -I$(pwd)/linux-headers $QEMU_INCLUDES"
   supported_os="yes"
+  libs_qga="$($pkg_config --libs libudev) $libs_qga"
 ;;
 esac
 
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 233f78a406..4270f85aae 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,8 @@ 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;
+    struct udev *udev = NULL;
+    struct udev_device *udevice = NULL;
 
     p = strstr(syspath, "/devices/pci");
     if (!p || sscanf(p + 12, "%*x:%*x/%x:%x:%x.%x%n",
@@ -935,6 +938,24 @@ static void build_guest_fsinfo_for_real_device(char const *syspath,
     list = g_malloc0(sizeof(*list));
     list->value = disk;
 
+    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 *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);
+            disk->has_serial = true;
+        }
+    }
+
     if (strcmp(driver, "ata_piix") == 0) {
         /* a host per ide bus, target*:0:<unit>:0 */
         if (!has_host || !has_tgt) {
@@ -1002,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 995f62c2e4..666443dec2 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -577,25 +577,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
@@ -607,8 +635,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);
@@ -618,22 +646,24 @@ 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));
+    disk->dev = name;
+    disk->has_dev = true;
+    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
@@ -673,11 +703,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..dea995181e 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: 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', '*dev': 'str'} }
 
 ##
 # @GuestFilesystemInfo:
-- 
2.18.0

Re: [Qemu-devel] [PATCH v2 4/4] qga: report serial ID and device node

[Eric Blake]

On 08/07/2018 05:51 AM, Tomáš Golembiovský wrote:
> On Linux the functionality depends on libudev.
> 
> Example from Linux:
> 
>      {
>        "name": "dm-2",
>        "mountpoint": "/",
>        ...
>        "disk": [
>          {
>            "serial": "SAMSUNG_MZ7LN512HCHP-000L1_S1ZKNXAG822493",
>            "dev": "/dev/sda2",
>            ...
>          }
>        ],
>      }
> 
> Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
> ---

> +++ 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>

Is libudev universally available on BSD systems and always preinstalled 
on Linux systems, or does this need a configure probe?

> +++ b/qga/qapi-schema.json
> @@ -834,13 +834,15 @@
>   # @bus: bus id
>   # @target: target id
>   # @unit: unit id
> +# @serial: serial number

Mails crossed; you posted v2 before my v1 comment that this needs a 
'since 3.1' tag.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH v2 4/4] qga: report serial ID and device node

[Tomáš Golembiovský]

On Tue, 7 Aug 2018 08:52:37 -0500
Eric Blake <eblake@redhat.com> wrote:

> On 08/07/2018 05:51 AM, Tomáš Golembiovský wrote:
> > On Linux the functionality depends on libudev.
> > 
> > Example from Linux:
> > 
> >      {
> >        "name": "dm-2",
> >        "mountpoint": "/",
> >        ...
> >        "disk": [
> >          {
> >            "serial": "SAMSUNG_MZ7LN512HCHP-000L1_S1ZKNXAG822493",
> >            "dev": "/dev/sda2",
> >            ...
> >          }
> >        ],
> >      }
> > 
> > Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
> > ---  
> 
> > +++ 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>  
> 
> Is libudev universally available on BSD systems and always preinstalled 
> on Linux systems, or does this need a configure probe?

The code is specific to Linux. I couldn't find portable way of querying
the serial number.

libudev is part of systemd, so it should generally be there. But I
suppose we should not break on systems that don't use it. I'll add
configure check for it.

    Tomas

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

Re: [Qemu-devel] [PATCH v2 3/4] qga: win32: fix crashes when PCI info cannot be retrived

[Michael Roth]

Quoting Tomáš Golembiovský (2018-08-07 05:51:37)
> 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 lead to crasehs 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 36d76c22c0..995f62c2e4 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -642,15 +642,32 @@ static GuestDiskAddressList *build_guest_disk_info(char *guid, Error **errp)
>          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;
>              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) {
> +                slog("failed to get PCI controller info: %s",
> +                    error_get_pretty(local_err));

slog() is more for logging/auditing events that a guest administrator might
be interested in knowing about, like when qga is accessing files, freezing
filesystems, etc. General qga-side error reporting and debug logging should
go through the normal g_debug/g_warning/etc interfaces to be captured in
qga's log file.

We should also moved patch 1 after this so we don't expose a breakage
prior to the fix.

How often are you seeing failures with the pci info? And does the
information for the non-failures look valid to you? I tried to fix the
CONFIG_QGA_NTDDSCSI naming screw-up a while back and some values like
PCI func/bus/etc looked bogus, SPDRP_BUSNUMBER/SPDRP_ADDRESS/SPDRP_BUSNUMBER
didn't seem to be returning what the current code thinks they are. If that's
still the case it would be good to fix that before we final re-enable this
code.

> +                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
> 

Re: [Qemu-devel] [PATCH v2 3/4] qga: win32: fix crashes when PCI info cannot be retrived

[Tomáš Golembiovský]

On Wed, 05 Sep 2018 18:21:07 -0500
Michael Roth <mdroth@linux.vnet.ibm.com> wrote:

> Quoting Tomáš Golembiovský (2018-08-07 05:51:37)
> > 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 lead to crasehs 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 36d76c22c0..995f62c2e4 100644
> > --- a/qga/commands-win32.c
> > +++ b/qga/commands-win32.c
> > @@ -642,15 +642,32 @@ static GuestDiskAddressList *build_guest_disk_info(char *guid, Error **errp)
> >          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;
> >              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) {
> > +                slog("failed to get PCI controller info: %s",
> > +                    error_get_pretty(local_err));  
> 
> slog() is more for logging/auditing events that a guest administrator might
> be interested in knowing about, like when qga is accessing files, freezing
> filesystems, etc. General qga-side error reporting and debug logging should
> go through the normal g_debug/g_warning/etc interfaces to be captured in
> qga's log file.

ok

> 
> We should also moved patch 1 after this so we don't expose a breakage
> prior to the fix.

ok

> 
> How often are you seeing failures with the pci info?

On Windows 10 Enterprise every the time. On Windows 8 the original code
fails terribly much sooner.

> And does the
> information for the non-failures look valid to you?

I'll tell you when I see that. :)


> I tried to fix the
> CONFIG_QGA_NTDDSCSI naming screw-up a while back and some values like
> PCI func/bus/etc looked bogus, SPDRP_BUSNUMBER/SPDRP_ADDRESS/SPDRP_BUSNUMBER
> didn't seem to be returning what the current code thinks they are. If that's
> still the case it would be good to fix that before we final re-enable this
> code.

Does that mean the queries for SPDRP_* properties work for you?
The issue is that it fails every time as the request is for a
volume not a disk.

Unfortunately I don't know how to fix that at the moment. See my comment
in the followup version of the series that I will send shortly.

    Tomas

> 
> > +                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>