[Qemu-devel] [PATCH v4 00/11] qga: report serial number and disk node

All of lore.kernel.org
 help / color / mirror / Atom feed

* [Qemu-devel] [PATCH v4 00/11] qga: report serial number and disk node
@ 2018-10-04 11:22 Tomáš Golembiovský
  2018-10-04 11:22 ` [Qemu-devel] [PATCH v4 01/11] qga-win: fix crashes when PCI info cannot be retrived Tomáš Golembiovský
                   ` (10 more replies)
  0 siblings, 11 replies; 31+ messages in thread
From: Tomáš Golembiovský @ 2018-10-04 11:22 UTC (permalink / raw)
  To: qemu-devel
  Cc: Eric Blake, Sameeh Jubran, Marc-André Lureau, Olga Krishtal,
	Michael Roth, 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.

v4:
  - split changes into more patches
  - fixed UNC for physical drive to use device namespace
  - renamed g_debug_err() to debug_error()
  - fixed build without libudev

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

Tomáš Golembiovský (11):
  qga-win: fix crashes when PCI info cannot be retrived
  qga-win: handle NULL values
  build: rename CONFIG_QGA_NTDDDISK to CONFIG_QGA_NTDDSCSI
  qga-win: add debugging information
  qga-win: refactor disk properties (bus)
  configure: add test for libudev
  qga: report disk serial number
  qga-win: refactor disk info
  qga-win: handle multi-disk volumes
  qga: return disk device in guest-get-fsinfo
  qga-win: demystify namespace striping

 configure            |  24 +++-
 qga/Makefile.objs    |   1 +
 qga/commands-posix.c |  32 +++++
 qga/commands-win32.c | 269 ++++++++++++++++++++++++++++++++++++-------
 qga/qapi-schema.json |   5 +-
 5 files changed, 289 insertions(+), 42 deletions(-)

-- 
2.19.0

^ permalink raw reply	[flat|nested] 31+ messages in thread

* [Qemu-devel] [PATCH v4 01/11] qga-win: fix crashes when PCI info cannot be retrived
  2018-10-04 11:22 [Qemu-devel] [PATCH v4 00/11] qga: report serial number and disk node Tomáš Golembiovský
@ 2018-10-04 11:22 ` Tomáš Golembiovský
  2018-10-04 13:21   ` Marc-André Lureau
                     ` (2 more replies)
  2018-10-04 11:22 ` [Qemu-devel] [PATCH v4 02/11] qga-win: handle NULL values Tomáš Golembiovský
                   ` (9 subsequent siblings)
  10 siblings, 3 replies; 31+ messages in thread
From: Tomáš Golembiovský @ 2018-10-04 11:22 UTC (permalink / raw)
  To: qemu-devel
  Cc: Eric Blake, Sameeh Jubran, Marc-André Lureau, Olga Krishtal,
	Michael Roth, 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.19.0

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [Qemu-devel] [PATCH v4 02/11] qga-win: handle NULL values
  2018-10-04 11:22 [Qemu-devel] [PATCH v4 00/11] qga: report serial number and disk node Tomáš Golembiovský
  2018-10-04 11:22 ` [Qemu-devel] [PATCH v4 01/11] qga-win: fix crashes when PCI info cannot be retrived Tomáš Golembiovský
@ 2018-10-04 11:22 ` Tomáš Golembiovský
  2018-10-04 13:21   ` Marc-André Lureau
  2018-10-10 23:08   ` Michael Roth
  2018-10-04 11:22 ` [Qemu-devel] [PATCH v4 03/11] build: rename CONFIG_QGA_NTDDDISK to CONFIG_QGA_NTDDSCSI Tomáš Golembiovský
                   ` (8 subsequent siblings)
  10 siblings, 2 replies; 31+ messages in thread
From: Tomáš Golembiovský @ 2018-10-04 11:22 UTC (permalink / raw)
  To: qemu-devel
  Cc: Eric Blake, Sameeh Jubran, Marc-André Lureau, Olga Krishtal,
	Michael Roth, Tomáš Golembiovský

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

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

diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 9c959122d9..49fc747298 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -735,6 +735,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;
@@ -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

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [Qemu-devel] [PATCH v4 03/11] build: rename CONFIG_QGA_NTDDDISK to CONFIG_QGA_NTDDSCSI
  2018-10-04 11:22 [Qemu-devel] [PATCH v4 00/11] qga: report serial number and disk node Tomáš Golembiovský
  2018-10-04 11:22 ` [Qemu-devel] [PATCH v4 01/11] qga-win: fix crashes when PCI info cannot be retrived Tomáš Golembiovský
  2018-10-04 11:22 ` [Qemu-devel] [PATCH v4 02/11] qga-win: handle NULL values Tomáš Golembiovský
@ 2018-10-04 11:22 ` Tomáš Golembiovský
  2018-10-04 13:21   ` Marc-André Lureau
  2018-10-04 11:22 ` [Qemu-devel] [PATCH v4 04/11] qga-win: add debugging information Tomáš Golembiovský
                   ` (7 subsequent siblings)
  10 siblings, 1 reply; 31+ messages in thread
From: Tomáš Golembiovský @ 2018-10-04 11:22 UTC (permalink / raw)
  To: qemu-devel
  Cc: Eric Blake, Sameeh Jubran, Marc-André Lureau, Olga Krishtal,
	Michael Roth, 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.19.0

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [Qemu-devel] [PATCH v4 04/11] qga-win: add debugging information
  2018-10-04 11:22 [Qemu-devel] [PATCH v4 00/11] qga: report serial number and disk node Tomáš Golembiovský
                   ` (2 preceding siblings ...)
  2018-10-04 11:22 ` [Qemu-devel] [PATCH v4 03/11] build: rename CONFIG_QGA_NTDDDISK to CONFIG_QGA_NTDDSCSI Tomáš Golembiovský
@ 2018-10-04 11:22 ` Tomáš Golembiovský
  2018-10-04 13:21   ` Marc-André Lureau
  2018-10-04 11:22 ` [Qemu-devel] [PATCH v4 05/11] qga-win: refactor disk properties (bus) Tomáš Golembiovský
                   ` (6 subsequent siblings)
  10 siblings, 1 reply; 31+ messages in thread
From: Tomáš Golembiovský @ 2018-10-04 11:22 UTC (permalink / raw)
  To: qemu-devel
  Cc: Eric Blake, Sameeh Jubran, Marc-André Lureau, Olga Krishtal,
	Michael Roth, 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 49fc747298..2a7a3af614 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -89,6 +89,12 @@ static OpenFlags guest_file_open_modes[] = {
     {"a+b", FILE_GENERIC_APPEND|GENERIC_READ, OPEN_ALWAYS  }
 };
 
+#define debug_error(msg) do { \
+    char *suffix = g_win32_error_message(GetLastError()); \
+    g_debug("%s: %s", (msg), suffix); \
+    g_free(suffix); \
+} while (0)
+
 static OpenFlags *find_open_flag(const char *mode_str)
 {
     int mode;
@@ -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

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [Qemu-devel] [PATCH v4 05/11] qga-win: refactor disk properties (bus)
  2018-10-04 11:22 [Qemu-devel] [PATCH v4 00/11] qga: report serial number and disk node Tomáš Golembiovský
                   ` (3 preceding siblings ...)
  2018-10-04 11:22 ` [Qemu-devel] [PATCH v4 04/11] qga-win: add debugging information Tomáš Golembiovský
@ 2018-10-04 11:22 ` Tomáš Golembiovský
  2018-10-04 13:21   ` Marc-André Lureau
  2018-10-04 11:22 ` [Qemu-devel] [PATCH v4 06/11] configure: add test for libudev Tomáš Golembiovský
                   ` (5 subsequent siblings)
  10 siblings, 1 reply; 31+ messages in thread
From: Tomáš Golembiovský @ 2018-10-04 11:22 UTC (permalink / raw)
  To: qemu-devel
  Cc: Eric Blake, Sameeh Jubran, Marc-André Lureau, Olga Krishtal,
	Michael Roth, Tomáš Golembiovský

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

diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 2a7a3af614..d7864fc65a 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -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,
                          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);
+    g_free(dev_desc);
 
-    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

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [Qemu-devel] [PATCH v4 06/11] configure: add test for libudev
  2018-10-04 11:22 [Qemu-devel] [PATCH v4 00/11] qga: report serial number and disk node Tomáš Golembiovský
                   ` (4 preceding siblings ...)
  2018-10-04 11:22 ` [Qemu-devel] [PATCH v4 05/11] qga-win: refactor disk properties (bus) Tomáš Golembiovský
@ 2018-10-04 11:22 ` Tomáš Golembiovský
  2018-10-04 13:21   ` Marc-André Lureau
  2018-10-04 11:22 ` [Qemu-devel] [PATCH v4 07/11] qga: report disk serial number Tomáš Golembiovský
                   ` (4 subsequent siblings)
  10 siblings, 1 reply; 31+ messages in thread
From: Tomáš Golembiovský @ 2018-10-04 11:22 UTC (permalink / raw)
  To: qemu-devel
  Cc: Eric Blake, Sameeh Jubran, Marc-André Lureau, Olga Krishtal,
	Michael Roth, Tomáš Golembiovský

Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
---
 configure | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

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

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [Qemu-devel] [PATCH v4 07/11] qga: report disk serial number
  2018-10-04 11:22 [Qemu-devel] [PATCH v4 00/11] qga: report serial number and disk node Tomáš Golembiovský
                   ` (5 preceding siblings ...)
  2018-10-04 11:22 ` [Qemu-devel] [PATCH v4 06/11] configure: add test for libudev Tomáš Golembiovský
@ 2018-10-04 11:22 ` Tomáš Golembiovský
  2018-10-04 13:21   ` Marc-André Lureau
  2018-10-04 11:22 ` [Qemu-devel] [PATCH v4 08/11] qga-win: refactor disk info Tomáš Golembiovský
                   ` (3 subsequent siblings)
  10 siblings, 1 reply; 31+ messages in thread
From: Tomáš Golembiovský @ 2018-10-04 11:22 UTC (permalink / raw)
  To: qemu-devel
  Cc: Eric Blake, Sameeh Jubran, Marc-André Lureau, Olga Krishtal,
	Michael Roth, 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>
---
 qga/Makefile.objs    |  1 +
 qga/commands-posix.c | 27 +++++++++++++++++++++++++++
 qga/commands-win32.c | 24 ++++++++++++++++++++++++
 qga/qapi-schema.json |  4 +++-
 4 files changed, 55 insertions(+), 1 deletion(-)

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..4d324178f2 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -48,6 +48,10 @@ extern char **environ;
 #include <net/if.h>
 #include <sys/statvfs.h>
 
+#ifdef CONFIG_LIBUDEV
+#include <libudev.h>
+#endif
+
 #ifdef FIFREEZE
 #define CONFIG_FSFREEZE
 #endif
@@ -872,6 +876,10 @@ static void build_guest_fsinfo_for_real_device(char const *syspath,
     GuestDiskAddressList *list = NULL;
     bool has_ata = false, has_host = false, has_tgt = false;
     char *p, *q, *driver = NULL;
+#ifdef CONFIG_LIBUDEV
+    struct udev *udev = NULL;
+    struct udev_device *udevice = NULL;
+#endif
 
     p = strstr(syspath, "/devices/pci");
     if (!p || sscanf(p + 12, "%*x:%*x/%x:%x:%x.%x%n",
@@ -936,6 +944,21 @@ static void build_guest_fsinfo_for_real_device(char const *syspath,
     list = g_malloc0(sizeof(*list));
     list->value = disk;
 
+#ifdef CONFIG_LIBUDEV
+    udev = udev_new();
+    udevice = udev_device_new_from_syspath(udev, syspath);
+    if (udev == NULL || udevice == NULL) {
+        g_debug("failed to query udev");
+    } else {
+        const char *serial;
+        serial = udev_device_get_property_value(udevice, "ID_SERIAL");
+        if (serial != NULL && *serial != 0) {
+            disk->serial = g_strdup(serial);
+            disk->has_serial = true;
+        }
+    }
+#endif
+
     if (strcmp(driver, "ata_piix") == 0) {
         /* a host per ide bus, target*:0:<unit>:0 */
         if (!has_host || !has_tgt) {
@@ -1003,6 +1026,10 @@ cleanup:
         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,
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index d7864fc65a..376ca1e288 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -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;
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.19.0

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [Qemu-devel] [PATCH v4 08/11] qga-win: refactor disk info
  2018-10-04 11:22 [Qemu-devel] [PATCH v4 00/11] qga: report serial number and disk node Tomáš Golembiovský
                   ` (6 preceding siblings ...)
  2018-10-04 11:22 ` [Qemu-devel] [PATCH v4 07/11] qga: report disk serial number Tomáš Golembiovský
@ 2018-10-04 11:22 ` Tomáš Golembiovský
  2018-10-04 11:22 ` [Qemu-devel] [PATCH v4 09/11] qga-win: handle multi-disk volumes Tomáš Golembiovský
                   ` (2 subsequent siblings)
  10 siblings, 0 replies; 31+ messages in thread
From: Tomáš Golembiovský @ 2018-10-04 11:22 UTC (permalink / raw)
  To: qemu-devel
  Cc: Eric Blake, Sameeh Jubran, Marc-André Lureau, Olga Krishtal,
	Michael Roth, Tomáš Golembiovský

Refactor building of disk info into a function that builds the list and
a function that returns infor for single disk. This will be used in
future commit that will handle multi-disk volumes.

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

diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 376ca1e288..1e64642b8a 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -632,20 +632,15 @@ out_free:
     return;
 }
 
-/* VSS provider works with volumes, thus there is no difference if
- * the volume consist of spanned disks. Info about the first disk in the
- * volume is returned for the spanned disk group (LVM) */
-static GuestDiskAddressList *build_guest_disk_info(char *guid, Error **errp)
+static void get_single_disk_info(char *name, GuestDiskAddress *disk,
+    Error **errp)
 {
-    GuestDiskAddressList *list = NULL;
-    GuestDiskAddress *disk;
     SCSI_ADDRESS addr, *scsi_ad;
     DWORD len;
     HANDLE vol_h;
     Error *local_err = NULL;
 
     scsi_ad = &addr;
-    char *name = g_strndup(guid, strlen(guid)-1);
 
     g_debug("getting disk info for: %s", name);
     vol_h = CreateFile(name, 0, FILE_SHARE_READ, NULL, OPEN_EXISTING,
@@ -655,7 +650,6 @@ static GuestDiskAddressList *build_guest_disk_info(char *guid, Error **errp)
         goto err;
     }
 
-    disk = g_malloc0(sizeof(*disk));
     get_disk_properties(vol_h, disk, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
@@ -676,7 +670,6 @@ 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;
@@ -688,6 +681,7 @@ static GuestDiskAddressList *build_guest_disk_info(char *guid, Error **errp)
                 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,20 +698,44 @@ 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);
-    g_free(name);
-    return list;
-
 err_close:
-    g_free(disk);
     CloseHandle(vol_h);
 err:
+    return;
+}
+
+/* VSS provider works with volumes, thus there is no difference if
+ * the volume consist of spanned disks. Info about the first disk in the
+ * volume is returned for the spanned disk group (LVM) */
+static GuestDiskAddressList *build_guest_disk_info(char *guid, Error **errp)
+{
+    Error *local_err = NULL;
+    GuestDiskAddressList *list = NULL, *cur_item = NULL;
+    GuestDiskAddress *disk = NULL;
+
+    /* strip final backslash */
+    char *name = g_strdup(guid);
+    if (g_str_has_suffix(name, "\\") == TRUE) {
+        name[strlen(name) - 1] = 0;
+    }
+
+    disk = g_malloc0(sizeof(GuestDiskAddress));
+    get_single_disk_info(name, disk, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        goto out;
+    }
+
+    cur_item = g_malloc0(sizeof(*list));
+    cur_item->value = disk;
+    disk = NULL;
+    list = cur_item;
+
+out:
+    g_free(disk);
     g_free(name);
 
-    return NULL;
+    return list;
 }
 
 #else
-- 
2.19.0

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [Qemu-devel] [PATCH v4 09/11] qga-win: handle multi-disk volumes
  2018-10-04 11:22 [Qemu-devel] [PATCH v4 00/11] qga: report serial number and disk node Tomáš Golembiovský
                   ` (7 preceding siblings ...)
  2018-10-04 11:22 ` [Qemu-devel] [PATCH v4 08/11] qga-win: refactor disk info Tomáš Golembiovský
@ 2018-10-04 11:22 ` Tomáš Golembiovský
  2018-10-07 12:13   ` Sameeh Jubran
  2018-10-04 11:22 ` [Qemu-devel] [PATCH v4 10/11] qga: return disk device in guest-get-fsinfo Tomáš Golembiovský
  2018-10-04 11:22 ` [Qemu-devel] [PATCH v4 11/11] qga-win: demystify namespace striping Tomáš Golembiovský
  10 siblings, 1 reply; 31+ messages in thread
From: Tomáš Golembiovský @ 2018-10-04 11:22 UTC (permalink / raw)
  To: qemu-devel
  Cc: Eric Blake, Sameeh Jubran, Marc-André Lureau, Olga Krishtal,
	Michael Roth, 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ý <tgolembi@redhat.com>
---
 qga/commands-win32.c | 126 ++++++++++++++++++++++++++++++++++++-------
 1 file changed, 108 insertions(+), 18 deletions(-)

diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 1e64642b8a..a591d8221d 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");
@@ -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;
@@ -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;
@@ -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);
@@ -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
+         *
+         * See also Naming Files, Paths and Namespaces:
+         * https://docs.microsoft.com/en-us/windows/desktop/FileIO/naming-a-file#win32-device-namespaces
+         */
+        disk_name = g_strdup_printf("\\\\.\\PhysicalDrive%lu",
+            extents->Extents[i].DiskNumber);
+        get_single_disk_info(disk_name, disk, &local_err);
+        g_free(disk_name);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            goto out;
+        }
+        cur_item = g_malloc0(sizeof(*list));
+        cur_item->value = disk;
+        disk = NULL;
+        cur_item->next = list;
+        list = cur_item;
+    }
+
 
 out:
+    g_free(extents);
     g_free(disk);
     g_free(name);
 
-- 
2.19.0

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [Qemu-devel] [PATCH v4 10/11] qga: return disk device in guest-get-fsinfo
  2018-10-04 11:22 [Qemu-devel] [PATCH v4 00/11] qga: report serial number and disk node Tomáš Golembiovský
                   ` (8 preceding siblings ...)
  2018-10-04 11:22 ` [Qemu-devel] [PATCH v4 09/11] qga-win: handle multi-disk volumes Tomáš Golembiovský
@ 2018-10-04 11:22 ` Tomáš Golembiovský
  2018-10-04 13:20   ` Marc-André Lureau
  2018-10-04 11:22 ` [Qemu-devel] [PATCH v4 11/11] qga-win: demystify namespace striping Tomáš Golembiovský
  10 siblings, 1 reply; 31+ messages in thread
From: Tomáš Golembiovský @ 2018-10-04 11:22 UTC (permalink / raw)
  To: qemu-devel
  Cc: Eric Blake, Sameeh Jubran, Marc-André Lureau, Olga Krishtal,
	Michael Roth, Tomáš Golembiovský

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ý <tgolembi@redhat.com>
---
 qga/commands-posix.c |  7 ++++++-
 qga/commands-win32.c | 21 +++++++++++----------
 qga/qapi-schema.json |  3 ++-
 3 files changed, 19 insertions(+), 12 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 4d324178f2..31952b07f4 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -950,7 +950,12 @@ static void build_guest_fsinfo_for_real_device(char const *syspath,
     if (udev == NULL || udevice == NULL) {
         g_debug("failed to query udev");
     } else {
-        const char *serial;
+        const char *devnode, *serial;
+        devnode = udev_device_get_devnode(udevice);
+        if (devnode != NULL) {
+            disk->dev = g_strdup(devnode);
+            disk->has_dev = true;
+        }
         serial = udev_device_get_property_value(udevice, "ID_SERIAL");
         if (serial != NULL && *serial != 0) {
             disk->serial = g_strdup(serial);
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index a591d8221d..d0d969d0ce 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -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;
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.19.0

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [Qemu-devel] [PATCH v4 11/11] qga-win: demystify namespace striping
  2018-10-04 11:22 [Qemu-devel] [PATCH v4 00/11] qga: report serial number and disk node Tomáš Golembiovský
                   ` (9 preceding siblings ...)
  2018-10-04 11:22 ` [Qemu-devel] [PATCH v4 10/11] qga: return disk device in guest-get-fsinfo Tomáš Golembiovský
@ 2018-10-04 11:22 ` Tomáš Golembiovský
  2018-10-04 13:20   ` Marc-André Lureau
  10 siblings, 1 reply; 31+ messages in thread
From: Tomáš Golembiovský @ 2018-10-04 11:22 UTC (permalink / raw)
  To: qemu-devel
  Cc: Eric Blake, Sameeh Jubran, Marc-André Lureau, Olga Krishtal,
	Michael Roth, 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ý <tgolembi@redhat.com>
---
 qga/commands-win32.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index d0d969d0ce..82881aa749 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -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]);
+    } 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

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* Re: [Qemu-devel] [PATCH v4 11/11] qga-win: demystify namespace striping
  2018-10-04 11:22 ` [Qemu-devel] [PATCH v4 11/11] qga-win: demystify namespace striping Tomáš Golembiovský
@ 2018-10-04 13:20   ` Marc-André Lureau
  2018-10-07 11:03     ` Sameeh Jubran
                       ` (2 more replies)
  0 siblings, 3 replies; 31+ messages in thread
From: Marc-André Lureau @ 2018-10-04 13:20 UTC (permalink / raw)
  To: Tomas Golembiovsky
  Cc: qemu-devel, Blake, Eric, sjubran, okrishtal, Michael Roth

Hi

On Thu, Oct 4, 2018 at 3:22 PM Tomáš Golembiovský <tgolembi@redhat.com> wrote:
>
> 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ý <tgolembi@redhat.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
>  qga/commands-win32.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index d0d969d0ce..82881aa749 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -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

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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [Qemu-devel] [PATCH v4 10/11] qga: return disk device in guest-get-fsinfo
  2018-10-04 11:22 ` [Qemu-devel] [PATCH v4 10/11] qga: return disk device in guest-get-fsinfo Tomáš Golembiovský
@ 2018-10-04 13:20   ` Marc-André Lureau
  0 siblings, 0 replies; 31+ messages in thread
From: Marc-André Lureau @ 2018-10-04 13:20 UTC (permalink / raw)
  To: Tomas Golembiovsky
  Cc: qemu-devel, Blake, Eric, sjubran, okrishtal, Michael Roth

Hi

On Thu, Oct 4, 2018 at 3:22 PM Tomáš Golembiovský <tgolembi@redhat.com> wrote:
>
> 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ý <tgolembi@redhat.com>
> ---
>  qga/commands-posix.c |  7 ++++++-
>  qga/commands-win32.c | 21 +++++++++++----------
>  qga/qapi-schema.json |  3 ++-
>  3 files changed, 19 insertions(+), 12 deletions(-)
>
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 4d324178f2..31952b07f4 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -950,7 +950,12 @@ static void build_guest_fsinfo_for_real_device(char const *syspath,
>      if (udev == NULL || udevice == NULL) {
>          g_debug("failed to query udev");
>      } else {
> -        const char *serial;
> +        const char *devnode, *serial;
> +        devnode = udev_device_get_devnode(udevice);
> +        if (devnode != NULL) {
> +            disk->dev = g_strdup(devnode);
> +            disk->has_dev = true;
> +        }

ack the posix/linux part.

You may want to split the Windows part..

>          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 a591d8221d..d0d969d0ce 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -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;
> 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.19.0
>

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [Qemu-devel] [PATCH v4 07/11] qga: report disk serial number
  2018-10-04 11:22 ` [Qemu-devel] [PATCH v4 07/11] qga: report disk serial number Tomáš Golembiovský
@ 2018-10-04 13:21   ` Marc-André Lureau
  0 siblings, 0 replies; 31+ messages in thread
From: Marc-André Lureau @ 2018-10-04 13:21 UTC (permalink / raw)
  To: Tomas Golembiovsky
  Cc: qemu-devel, Blake, Eric, sjubran, okrishtal, Michael Roth

Hi

On Thu, Oct 4, 2018 at 3:22 PM Tomáš Golembiovský <tgolembi@redhat.com> wrote:
>
> 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>
> ---
>  qga/Makefile.objs    |  1 +
>  qga/commands-posix.c | 27 +++++++++++++++++++++++++++
>  qga/commands-win32.c | 24 ++++++++++++++++++++++++
>  qga/qapi-schema.json |  4 +++-
>  4 files changed, 55 insertions(+), 1 deletion(-)
>
> 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..4d324178f2 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -48,6 +48,10 @@ extern char **environ;
>  #include <net/if.h>
>  #include <sys/statvfs.h>
>
> +#ifdef CONFIG_LIBUDEV
> +#include <libudev.h>
> +#endif
> +
>  #ifdef FIFREEZE
>  #define CONFIG_FSFREEZE
>  #endif
> @@ -872,6 +876,10 @@ static void build_guest_fsinfo_for_real_device(char const *syspath,
>      GuestDiskAddressList *list = NULL;
>      bool has_ata = false, has_host = false, has_tgt = false;
>      char *p, *q, *driver = NULL;
> +#ifdef CONFIG_LIBUDEV
> +    struct udev *udev = NULL;
> +    struct udev_device *udevice = NULL;
> +#endif
>
>      p = strstr(syspath, "/devices/pci");
>      if (!p || sscanf(p + 12, "%*x:%*x/%x:%x:%x.%x%n",
> @@ -936,6 +944,21 @@ static void build_guest_fsinfo_for_real_device(char const *syspath,
>      list = g_malloc0(sizeof(*list));
>      list->value = disk;
>
> +#ifdef CONFIG_LIBUDEV
> +    udev = udev_new();
> +    udevice = udev_device_new_from_syspath(udev, syspath);
> +    if (udev == NULL || udevice == NULL) {
> +        g_debug("failed to query udev");
> +    } else {
> +        const char *serial;
> +        serial = udev_device_get_property_value(udevice, "ID_SERIAL");
> +        if (serial != NULL && *serial != 0) {
> +            disk->serial = g_strdup(serial);
> +            disk->has_serial = true;
> +        }
> +    }
> +#endif
> +
>      if (strcmp(driver, "ata_piix") == 0) {
>          /* a host per ide bus, target*:0:<unit>:0 */
>          if (!has_host || !has_tgt) {
> @@ -1003,6 +1026,10 @@ cleanup:

You correcly free udev context on error, but on normal return, it's leaking

You may want to change the return/cleanup path to have a common path.

(btw the leak is spotted by test/test-qga if you build with --enable-sanitizers)

>          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,
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index d7864fc65a..376ca1e288 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -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;
> 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.19.0

looks good otherwise

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [Qemu-devel] [PATCH v4 06/11] configure: add test for libudev
  2018-10-04 11:22 ` [Qemu-devel] [PATCH v4 06/11] configure: add test for libudev Tomáš Golembiovský
@ 2018-10-04 13:21   ` Marc-André Lureau
  0 siblings, 0 replies; 31+ messages in thread
From: Marc-André Lureau @ 2018-10-04 13:21 UTC (permalink / raw)
  To: Tomas Golembiovsky
  Cc: qemu-devel, Blake, Eric, sjubran, okrishtal, Michael Roth

On Thu, Oct 4, 2018 at 3:22 PM Tomáš Golembiovský <tgolembi@redhat.com> wrote:
>
> Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
>  configure | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
>
> 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
> --
> 2.19.0
>

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [Qemu-devel] [PATCH v4 05/11] qga-win: refactor disk properties (bus)
  2018-10-04 11:22 ` [Qemu-devel] [PATCH v4 05/11] qga-win: refactor disk properties (bus) Tomáš Golembiovský
@ 2018-10-04 13:21   ` Marc-André Lureau
  0 siblings, 0 replies; 31+ messages in thread
From: Marc-André Lureau @ 2018-10-04 13:21 UTC (permalink / raw)
  To: Tomas Golembiovsky
  Cc: qemu-devel, Blake, Eric, sjubran, okrishtal, Michael Roth

Hi

On Thu, Oct 4, 2018 at 3:22 PM Tomáš Golembiovský <tgolembi@redhat.com> wrote:
>
> 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ý <tgolembi@redhat.com>
> ---
>  qga/commands-win32.c | 46 +++++++++++++++++++++++++++-----------------
>  1 file changed, 28 insertions(+), 18 deletions(-)
>
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index 2a7a3af614..d7864fc65a 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -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,
>                           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);
> +    g_free(dev_desc);

bad free(), dev_desc = &buf.

looks good otherwise

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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [Qemu-devel] [PATCH v4 04/11] qga-win: add debugging information
  2018-10-04 11:22 ` [Qemu-devel] [PATCH v4 04/11] qga-win: add debugging information Tomáš Golembiovský
@ 2018-10-04 13:21   ` Marc-André Lureau
  0 siblings, 0 replies; 31+ messages in thread
From: Marc-André Lureau @ 2018-10-04 13:21 UTC (permalink / raw)
  To: Tomas Golembiovsky
  Cc: qemu-devel, Blake, Eric, sjubran, okrishtal, Michael Roth

Hi

On Thu, Oct 4, 2018 at 3:22 PM Tomáš Golembiovský <tgolembi@redhat.com> wrote:
>
> 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>

Reviewed-by: Marc-André Lureau <marcandre.lureau@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 49fc747298..2a7a3af614 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -89,6 +89,12 @@ static OpenFlags guest_file_open_modes[] = {
>      {"a+b", FILE_GENERIC_APPEND|GENERIC_READ, OPEN_ALWAYS  }
>  };
>
> +#define debug_error(msg) do { \
> +    char *suffix = g_win32_error_message(GetLastError()); \
> +    g_debug("%s: %s", (msg), suffix); \
> +    g_free(suffix); \
> +} while (0)
> +
>  static OpenFlags *find_open_flag(const char *mode_str)
>  {
>      int mode;
> @@ -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
>

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [Qemu-devel] [PATCH v4 03/11] build: rename CONFIG_QGA_NTDDDISK to CONFIG_QGA_NTDDSCSI
  2018-10-04 11:22 ` [Qemu-devel] [PATCH v4 03/11] build: rename CONFIG_QGA_NTDDDISK to CONFIG_QGA_NTDDSCSI Tomáš Golembiovský
@ 2018-10-04 13:21   ` Marc-André Lureau
  2018-10-07  9:29     ` Sameeh Jubran
  0 siblings, 1 reply; 31+ messages in thread
From: Marc-André Lureau @ 2018-10-04 13:21 UTC (permalink / raw)
  To: Tomas Golembiovsky
  Cc: qemu-devel, Blake, Eric, sjubran, okrishtal, Michael Roth

Hi

On Thu, Oct 4, 2018 at 3:22 PM Tomáš Golembiovský <tgolembi@redhat.com> wrote:
>
> 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>

Reviewed-by: Marc-André Lureau <marcandre.lureau@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.19.0
>

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [Qemu-devel] [PATCH v4 02/11] qga-win: handle NULL values
  2018-10-04 11:22 ` [Qemu-devel] [PATCH v4 02/11] qga-win: handle NULL values Tomáš Golembiovský
@ 2018-10-04 13:21   ` Marc-André Lureau
  2018-10-10 23:08   ` Michael Roth
  1 sibling, 0 replies; 31+ messages in thread
From: Marc-André Lureau @ 2018-10-04 13:21 UTC (permalink / raw)
  To: Tomas Golembiovsky
  Cc: qemu-devel, Blake, Eric, sjubran, okrishtal, Michael Roth

Hi
On Thu, Oct 4, 2018 at 3:22 PM Tomáš Golembiovský <tgolembi@redhat.com> wrote:
>
> Handle returned NULLs properly to:
> - avoid crashes in serialization.
> - properly report errors to the caller
>
> Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
> ---
>  qga/commands-win32.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index 9c959122d9..49fc747298 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -735,6 +735,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);

I'd use qapi_free_GuestFilesystemInfo()

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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [Qemu-devel] [PATCH v4 01/11] qga-win: fix crashes when PCI info cannot be retrived
  2018-10-04 11:22 ` [Qemu-devel] [PATCH v4 01/11] qga-win: fix crashes when PCI info cannot be retrived Tomáš Golembiovský
@ 2018-10-04 13:21   ` Marc-André Lureau
  2018-10-09 11:07     ` Tomáš Golembiovský
  2018-10-10 23:35   ` Michael Roth
  2018-10-11  1:04   ` Eric Blake
  2 siblings, 1 reply; 31+ messages in thread
From: Marc-André Lureau @ 2018-10-04 13:21 UTC (permalink / raw)
  To: Tomas Golembiovsky
  Cc: qemu-devel, Blake, Eric, sjubran, okrishtal, Michael Roth

Hi
On Thu, Oct 4, 2018 at 3:22 PM Tomáš Golembiovský <tgolembi@redhat.com> wrote:
>
> 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));
>      }

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

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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [Qemu-devel] [PATCH v4 03/11] build: rename CONFIG_QGA_NTDDDISK to CONFIG_QGA_NTDDSCSI
  2018-10-04 13:21   ` Marc-André Lureau
@ 2018-10-07  9:29     ` Sameeh Jubran
  0 siblings, 0 replies; 31+ messages in thread
From: Sameeh Jubran @ 2018-10-07  9:29 UTC (permalink / raw)
  To: marcandre.lureau
  Cc: tgolembi, Sameeh Jubran, okrishtal, QEMU Developers, Michael Roth

Reviewed-by: Sameeh Jubran <sjubran@redhat.com>
On Thu, Oct 4, 2018 at 4:23 PM Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
>
> Hi
>
> On Thu, Oct 4, 2018 at 3:22 PM Tomáš Golembiovský <tgolembi@redhat.com> wrote:
> >
> > 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>
>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@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.19.0
> >
>


-- 
Respectfully,
Sameeh Jubran
Linkedin
Software Engineer @ Daynix.

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [Qemu-devel] [PATCH v4 11/11] qga-win: demystify namespace striping
  2018-10-04 13:20   ` Marc-André Lureau
@ 2018-10-07 11:03     ` Sameeh Jubran
  2018-10-09 12:38     ` Tomáš Golembiovský
  2018-10-10 17:19     ` Eric Blake
  2 siblings, 0 replies; 31+ messages in thread
From: Sameeh Jubran @ 2018-10-07 11:03 UTC (permalink / raw)
  To: marcandre.lureau
  Cc: tgolembi, Sameeh Jubran, okrishtal, QEMU Developers, Michael Roth

+1, this is much clearer.
On Thu, Oct 4, 2018 at 4:34 PM Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
>
> Hi
>
> On Thu, Oct 4, 2018 at 3:22 PM Tomáš Golembiovský <tgolembi@redhat.com> wrote:
> >
> > 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ý <tgolembi@redhat.com>
>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> > ---
> >  qga/commands-win32.c | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> > index d0d969d0ce..82881aa749 100644
> > --- a/qga/commands-win32.c
> > +++ b/qga/commands-win32.c
> > @@ -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
>
> > +    } 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
> >
>


-- 
Respectfully,
Sameeh Jubran
Linkedin
Software Engineer @ Daynix.

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [Qemu-devel] [PATCH v4 09/11] qga-win: handle multi-disk volumes
  2018-10-04 11:22 ` [Qemu-devel] [PATCH v4 09/11] qga-win: handle multi-disk volumes Tomáš Golembiovský
@ 2018-10-07 12:13   ` Sameeh Jubran
  2018-10-10 14:57     ` Tomáš Golembiovský
  0 siblings, 1 reply; 31+ messages in thread
From: Sameeh Jubran @ 2018-10-07 12:13 UTC (permalink / raw)
  To: tgolembi
  Cc: QEMU Developers, Michael Roth, okrishtal, Sameeh Jubran,
	marcandre.lureau

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


On Thu, Oct 4, 2018 at 2:43 PM Tomáš Golembiovský <tgolembi@redhat.com>
wrote:

> 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ý <tgolembi@redhat.com>
> ---
>  qga/commands-win32.c | 126 ++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 108 insertions(+), 18 deletions(-)
>
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index 1e64642b8a..a591d8221d 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");
> @@ -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;
> @@ -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;
> @@ -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);
> @@ -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
> +         *
> +         * See also Naming Files, Paths and Namespaces:
> +         *
> https://docs.microsoft.com/en-us/windows/desktop/FileIO/naming-a-file#win32-device-namespaces
> +         */
> +        disk_name = g_strdup_printf("\\\\.\\PhysicalDrive%lu",
> +            extents->Extents[i].DiskNumber);
> +        get_single_disk_info(disk_name, disk, &local_err);
> +        g_free(disk_name);
> +        if (local_err) {
> +            error_propagate(errp, local_err);
> +            goto out;
> +        }
> +        cur_item = g_malloc0(sizeof(*list));
> +        cur_item->value = disk;
> +        disk = NULL;
> +        cur_item->next = list;
> +        list = cur_item;
> +    }
> +
>
>  out:
> +    g_free(extents);
>      g_free(disk);
>      g_free(name);
>
> --
> 2.19.0
>
>
>

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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [Qemu-devel] [PATCH v4 01/11] qga-win: fix crashes when PCI info cannot be retrived
  2018-10-04 13:21   ` Marc-André Lureau
@ 2018-10-09 11:07     ` Tomáš Golembiovský
  0 siblings, 0 replies; 31+ messages in thread
From: Tomáš Golembiovský @ 2018-10-09 11:07 UTC (permalink / raw)
  To: Marc-André Lureau
  Cc: qemu-devel, Blake, Eric, sjubran, okrishtal, Michael Roth

On Thu, 4 Oct 2018 17:21:32 +0400
Marc-André Lureau <marcandre.lureau@redhat.com> wrote:

> Hi
> On Thu, Oct 4, 2018 at 3:22 PM Tomáš Golembiovský <tgolembi@redhat.com> wrote:
> >
> > 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));
> >      }  
> 
> Shouldn't pci-controller be made optional in the schema instead?

It should, but that requires API change. 
Eric suggested that previously too:

https://lists.gnu.org/archive/html/qemu-devel/2018-06/msg08599.html

I will do that in next version.

    Tomas

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


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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [Qemu-devel] [PATCH v4 11/11] qga-win: demystify namespace striping
  2018-10-04 13:20   ` Marc-André Lureau
  2018-10-07 11:03     ` Sameeh Jubran
@ 2018-10-09 12:38     ` Tomáš Golembiovský
  2018-10-10 17:19     ` Eric Blake
  2 siblings, 0 replies; 31+ messages in thread
From: Tomáš Golembiovský @ 2018-10-09 12:38 UTC (permalink / raw)
  To: Marc-André Lureau
  Cc: qemu-devel, Blake, Eric, sjubran, okrishtal, Michael Roth

On Thu, 4 Oct 2018 17:20:50 +0400
Marc-André Lureau <marcandre.lureau@redhat.com> wrote:

> Hi
> 
> On Thu, Oct 4, 2018 at 3:22 PM Tomáš Golembiovský <tgolembi@redhat.com> wrote:
> >
> > 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ý <tgolembi@redhat.com>  
> 
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> > ---
> >  qga/commands-win32.c | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> > index d0d969d0ce..82881aa749 100644
> > --- a/qga/commands-win32.c
> > +++ b/qga/commands-win32.c
> > @@ -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

I will change that, no problem.

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


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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [Qemu-devel] [PATCH v4 09/11] qga-win: handle multi-disk volumes
  2018-10-07 12:13   ` Sameeh Jubran
@ 2018-10-10 14:57     ` Tomáš Golembiovský
  0 siblings, 0 replies; 31+ messages in thread
From: Tomáš Golembiovský @ 2018-10-10 14:57 UTC (permalink / raw)
  To: Sameeh Jubran
  Cc: QEMU Developers, Michael Roth, okrishtal, Sameeh Jubran,
	marcandre.lureau

On Sun, 7 Oct 2018 15:13:26 +0300
Sameeh Jubran <sameeh@daynix.com> wrote:

> 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

Unfortunately the function QueryDosDevice does not help much with our
situation. Yes, it can tell you that "HarddiskVolume2" is symbolic link
to "\Device\HarddiskVolume2" or that "PhysicalDrive1" is symbolic link
to "\Device\Harddisk1\DR1".

What it does not tell you is that "\Device\Harddisk1\DR1" and
"\Device\0000001c" are two different names for one device.

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

Yes, this is a nice tool for listing the devices.

    Tomas
> 
> 
> On Thu, Oct 4, 2018 at 2:43 PM Tomáš Golembiovský <tgolembi@redhat.com>
> wrote:
> 
> > 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ý <tgolembi@redhat.com>
> > ---
> >  qga/commands-win32.c | 126 ++++++++++++++++++++++++++++++++++++-------
> >  1 file changed, 108 insertions(+), 18 deletions(-)
> >
> > diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> > index 1e64642b8a..a591d8221d 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");
> > @@ -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;
> > @@ -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;
> > @@ -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);
> > @@ -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
> > +         *
> > +         * See also Naming Files, Paths and Namespaces:
> > +         *
> > https://docs.microsoft.com/en-us/windows/desktop/FileIO/naming-a-file#win32-device-namespaces
> > +         */
> > +        disk_name = g_strdup_printf("\\\\.\\PhysicalDrive%lu",
> > +            extents->Extents[i].DiskNumber);
> > +        get_single_disk_info(disk_name, disk, &local_err);
> > +        g_free(disk_name);
> > +        if (local_err) {
> > +            error_propagate(errp, local_err);
> > +            goto out;
> > +        }
> > +        cur_item = g_malloc0(sizeof(*list));
> > +        cur_item->value = disk;
> > +        disk = NULL;
> > +        cur_item->next = list;
> > +        list = cur_item;
> > +    }
> > +
> >
> >  out:
> > +    g_free(extents);
> >      g_free(disk);
> >      g_free(name);
> >
> > --
> > 2.19.0
> >
> >
> >  
> 
> -- 
> Respectfully,
> *Sameeh Jubran*
> *Linkedin <https://il.linkedin.com/pub/sameeh-jubran/87/747/a8a>*
> *Software Engineer @ Daynix <http://www.daynix.com>.*


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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [Qemu-devel] [PATCH v4 11/11] qga-win: demystify namespace striping
  2018-10-04 13:20   ` Marc-André Lureau
  2018-10-07 11:03     ` Sameeh Jubran
  2018-10-09 12:38     ` Tomáš Golembiovský
@ 2018-10-10 17:19     ` Eric Blake
  2 siblings, 0 replies; 31+ messages in thread
From: Eric Blake @ 2018-10-10 17:19 UTC (permalink / raw)
  To: Marc-André Lureau, Tomas Golembiovsky
  Cc: qemu-devel, sjubran, okrishtal, Michael Roth

On 10/4/18 8:20 AM, Marc-André Lureau wrote:
> Hi
> 
> On Thu, Oct 4, 2018 at 3:22 PM Tomáš Golembiovský <tgolembi@redhat.com> wrote:
>>
>> It was not obvious what exactly the cryptic string copying does to the
>> GUID. This change makes the intent clearer.

In the subject line, s/striping/stripping/ (this is about performing a 
'strip' operation on a prefix, but I read the subject as an instance of 
'stripe' as in drawing a line or fragmenting data in a RAID).

>> +++ b/qga/commands-win32.c
>> @@ -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)) {

I find that 'cond == true' is redundant to just writing 'cond'. And that 
sentiment applies to both the <stdbool.h> 'bool' and to the glib 
abomination TRUE (why they had to invent their own boolean names, worse 
in every way compared to <stdbool.h>, is beyond me).

>> +        name = g_strdup(&guid[4]);
> 
> I find "guid + 4" easier to read though

Concur.

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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [Qemu-devel] [PATCH v4 02/11] qga-win: handle NULL values
  2018-10-04 11:22 ` [Qemu-devel] [PATCH v4 02/11] qga-win: handle NULL values Tomáš Golembiovský
  2018-10-04 13:21   ` Marc-André Lureau
@ 2018-10-10 23:08   ` Michael Roth
  1 sibling, 0 replies; 31+ messages in thread
From: Michael Roth @ 2018-10-10 23:08 UTC (permalink / raw)
  To: Tomáš Golembiovský, qemu-devel
  Cc: Eric Blake, Sameeh Jubran, Marc-André Lureau, Olga Krishtal

Quoting Tomáš Golembiovský (2018-10-04 06:22:29)
> Handle returned NULLs properly to:
> - avoid crashes in serialization.
> - properly report errors to the caller
> 
> Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
> ---
>  qga/commands-win32.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index 9c959122d9..49fc747298 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -735,6 +735,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;
> +    }
> +

The QAPI schema defines fs->disk to be a list. In the current upstream
code (where CONFIG_NTDDSCSI is unset) we always set fs->disk to NULL
and that just results in an empty list, which works and doesn't violate
the schema, so I don't understand why that's needed here.

>  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;
>          }

This fails the whole guest_get_fsinfo command for any case where we
can't retrieve the 'disk' list for a particular volume. I would consider
that a regression in functionality.

Can you confirm this is for fixing the current code? Or is it just
something you need for something later in this series? If the latter,
I suspect this is the wrong place to address it.

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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [Qemu-devel] [PATCH v4 01/11] qga-win: fix crashes when PCI info cannot be retrived
  2018-10-04 11:22 ` [Qemu-devel] [PATCH v4 01/11] qga-win: fix crashes when PCI info cannot be retrived Tomáš Golembiovský
  2018-10-04 13:21   ` Marc-André Lureau
@ 2018-10-10 23:35   ` Michael Roth
  2018-10-11  1:04   ` Eric Blake
  2 siblings, 0 replies; 31+ messages in thread
From: Michael Roth @ 2018-10-10 23:35 UTC (permalink / raw)
  To: Tomáš Golembiovský, qemu-devel
  Cc: Eric Blake, Sameeh Jubran, Marc-André Lureau, Olga Krishtal

Quoting Tomáš Golembiovský (2018-10-04 06:22:28)
> 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));

Initializing to 0 would be wrong. I pointed out a patch from Sameeh in
v3 that initializes to -1. I'd recommend either picking up his patch,
or perhaps the schema change. But if we do go to the extent of a
non-backward-compatible schema change, I think we should also consider
just deprecating the current GuestDiskAddress list completely:

{ 'struct': 'GuestDiskAddress',
  'data': {'pci-controller': 'GuestPCIAddress',
           'bus-type': 'GuestDiskBusType',
           'bus': 'int', 'target': 'int', 'unit': 'int'} }

and defining something more modular. Some these there don't make a lot
of sense, like how GuestDiskBusType varies between scsi, ide, usb, etc,
but we still have the same bus/target/unit fields. I think each bus type 
should have it's own addressing units associated with it. The original
code made use of the fact that IDE/SATA/SCSI/SAS/etc could all be
retrieved via IOCTL_SCSI_GET_ADDRESS with those units but making sense
of them is sort of Windows magic that isn't good from an API perspective
and then there's all the other bus types where those units may or may
not be sensible. And on POSIX you basically have to look at the code
to figure out where each unit is/isn't being plucked from...

So for now I'd recommend just hard-setting the PCI fields to -1 like
in Sameeh's patch, and I'll do some testing and send a follow-up patch
to do the same for bus-type if that seems needed. We can explore better
options after 3.1.

>      }
> 
>      list = g_malloc0(sizeof(*list));
> -- 
> 2.19.0
> 

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [Qemu-devel] [PATCH v4 01/11] qga-win: fix crashes when PCI info cannot be retrived
  2018-10-04 11:22 ` [Qemu-devel] [PATCH v4 01/11] qga-win: fix crashes when PCI info cannot be retrived Tomáš Golembiovský
  2018-10-04 13:21   ` Marc-André Lureau
  2018-10-10 23:35   ` Michael Roth
@ 2018-10-11  1:04   ` Eric Blake
  2 siblings, 0 replies; 31+ messages in thread
From: Eric Blake @ 2018-10-11  1:04 UTC (permalink / raw)
  To: Tomáš Golembiovský, qemu-devel
  Cc: Sameeh Jubran, Marc-André Lureau, Olga Krishtal, Michael Roth

On 10/4/18 6:22 AM, Tomáš Golembiovský wrote:

In the subject: s/retrived/retrieved/

> 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(-)
> 

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

^ permalink raw reply	[flat|nested] 31+ messages in thread

end of thread, other threads:[~2018-10-11  1:04 UTC | newest]

Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-04 11:22 [Qemu-devel] [PATCH v4 00/11] qga: report serial number and disk node Tomáš Golembiovský
2018-10-04 11:22 ` [Qemu-devel] [PATCH v4 01/11] qga-win: fix crashes when PCI info cannot be retrived Tomáš Golembiovský
2018-10-04 13:21   ` Marc-André Lureau
2018-10-09 11:07     ` Tomáš Golembiovský
2018-10-10 23:35   ` Michael Roth
2018-10-11  1:04   ` Eric Blake
2018-10-04 11:22 ` [Qemu-devel] [PATCH v4 02/11] qga-win: handle NULL values Tomáš Golembiovský
2018-10-04 13:21   ` Marc-André Lureau
2018-10-10 23:08   ` Michael Roth
2018-10-04 11:22 ` [Qemu-devel] [PATCH v4 03/11] build: rename CONFIG_QGA_NTDDDISK to CONFIG_QGA_NTDDSCSI Tomáš Golembiovský
2018-10-04 13:21   ` Marc-André Lureau
2018-10-07  9:29     ` Sameeh Jubran
2018-10-04 11:22 ` [Qemu-devel] [PATCH v4 04/11] qga-win: add debugging information Tomáš Golembiovský
2018-10-04 13:21   ` Marc-André Lureau
2018-10-04 11:22 ` [Qemu-devel] [PATCH v4 05/11] qga-win: refactor disk properties (bus) Tomáš Golembiovský
2018-10-04 13:21   ` Marc-André Lureau
2018-10-04 11:22 ` [Qemu-devel] [PATCH v4 06/11] configure: add test for libudev Tomáš Golembiovský
2018-10-04 13:21   ` Marc-André Lureau
2018-10-04 11:22 ` [Qemu-devel] [PATCH v4 07/11] qga: report disk serial number Tomáš Golembiovský
2018-10-04 13:21   ` Marc-André Lureau
2018-10-04 11:22 ` [Qemu-devel] [PATCH v4 08/11] qga-win: refactor disk info Tomáš Golembiovský
2018-10-04 11:22 ` [Qemu-devel] [PATCH v4 09/11] qga-win: handle multi-disk volumes Tomáš Golembiovský
2018-10-07 12:13   ` Sameeh Jubran
2018-10-10 14:57     ` Tomáš Golembiovský
2018-10-04 11:22 ` [Qemu-devel] [PATCH v4 10/11] qga: return disk device in guest-get-fsinfo Tomáš Golembiovský
2018-10-04 13:20   ` Marc-André Lureau
2018-10-04 11:22 ` [Qemu-devel] [PATCH v4 11/11] qga-win: demystify namespace striping Tomáš Golembiovský
2018-10-04 13:20   ` Marc-André Lureau
2018-10-07 11:03     ` Sameeh Jubran
2018-10-09 12:38     ` Tomáš Golembiovský
2018-10-10 17:19     ` Eric Blake

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.