qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 0/3] qga: add command guest-get-disks
@ 2020-10-07  7:46 Tomáš Golembiovský
  2020-10-07  7:46 ` [PATCH v3 1/3] " Tomáš Golembiovský
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Tomáš Golembiovský @ 2020-10-07  7:46 UTC (permalink / raw)
  To: qemu-devel
  Cc: Daniel P. Berrangé, Michael Roth, Yonggang Luo,
	Tomáš Golembiovský, Marc-André Lureau,
	Philippe Mathieu-Daudé

Adds command to list disks of the VM.

The patch does compile on master but to work properly it requires changes to
qemu-ga by Thomas Huth in series: Allow guest-get-fsinfo also for non-PCI
devices.

v3:
- renamed "slaves" field to "dependents"
- comments from Marc and Daniel
- 2/3: factored out pieces of code into separate functions

v2:
- added new struct in API to handle the information
- split changes into several patches
- for Linux list also slaves, partitions and virtual disks so that the disk
  hierarchy is preserved
- fixed issues pointed out by Michael

Tomáš Golembiovský (3):
  qga: add command guest-get-disks
  qga: add implementation of guest-get-disks for Linux
  qga: add implementation of guest-get-disks for Windows

 qga/commands-posix.c | 287 ++++++++++++++++++++++++++++++++++++++++++-
 qga/commands-win32.c |  92 ++++++++++++++
 qga/qapi-schema.json |  31 +++++
 3 files changed, 407 insertions(+), 3 deletions(-)

-- 
2.28.0



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

* [PATCH v3 1/3] qga: add command guest-get-disks
  2020-10-07  7:46 [PATCH v3 0/3] qga: add command guest-get-disks Tomáš Golembiovský
@ 2020-10-07  7:46 ` Tomáš Golembiovský
  2020-10-07  7:46 ` [PATCH v3 2/3] qga: add implementation of guest-get-disks for Linux Tomáš Golembiovský
  2020-10-07  7:46 ` [PATCH v3 3/3] qga: add implementation of guest-get-disks for Windows Tomáš Golembiovský
  2 siblings, 0 replies; 7+ messages in thread
From: Tomáš Golembiovský @ 2020-10-07  7:46 UTC (permalink / raw)
  To: qemu-devel
  Cc: Daniel P. Berrangé, Michael Roth, Yonggang Luo,
	Tomáš Golembiovský, Marc-André Lureau,
	Marc-André Lureau, Philippe Mathieu-Daudé

Add API and stubs for new guest-get-disks command.

The command guest-get-fsinfo can be used to list information about disks
and partitions but it is limited only to mounted disks with filesystem.
This new command should allow listing information about disks of the VM
regardles whether they are mounted or not. This can be usefull for
management applications for mapping virtualized devices or pass-through
devices to device names in the guest OS.

Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 qga/commands-posix.c |  6 ++++++
 qga/commands-win32.c |  6 ++++++
 qga/qapi-schema.json | 31 +++++++++++++++++++++++++++++++
 3 files changed, 43 insertions(+)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 3bffee99d4..422144bcff 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -3051,3 +3051,9 @@ GuestDeviceInfoList *qmp_guest_get_devices(Error **errp)
 
     return NULL;
 }
+
+GuestDiskInfoList *qmp_guest_get_disks(Error **errp)
+{
+    error_setg(errp, QERR_UNSUPPORTED);
+    return NULL;
+}
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 0c3c05484f..0dd16315d7 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -2457,3 +2457,9 @@ GuestDeviceInfoList *qmp_guest_get_devices(Error **errp)
     }
     return head;
 }
+
+GuestDiskInfoList *qmp_guest_get_disks(Error **errp)
+{
+    error_setg(errp, QERR_UNSUPPORTED);
+    return NULL;
+}
diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index cec98c7e06..1ba8f19efc 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -865,6 +865,37 @@
            'bus': 'int', 'target': 'int', 'unit': 'int',
            '*serial': 'str', '*dev': 'str'} }
 
+##
+# @GuestDiskInfo:
+#
+# @name: device node (Linux) or device UNC (Windows)
+# @partition: whether this is a partition or disk
+# @dependents: list of dependent devices; e.g. for LVs of the LVM this will
+#              hold the list of PVs, for LUKS encrypted volume this will
+#              contain the disk where the volume is placed.     (Linux)
+# @address: disk address information (only for non-virtual devices)
+# @alias: optional alias assigned to the disk, on Linux this is a name assigned
+#         by device mapper
+#
+# Since 5.2
+##
+{ 'struct': 'GuestDiskInfo',
+  'data': {'name': 'str', 'partition': 'bool', 'dependents': ['str'],
+           '*address': 'GuestDiskAddress', '*alias': 'str'} }
+
+##
+# @guest-get-disks:
+#
+# Returns: The list of disks in the guest. For Windows these are only the
+#          physical disks. On Linux these are all root block devices of
+#          non-zero size including e.g. removable devices, loop devices,
+#          NBD, etc.
+#
+# Since: 5.2
+##
+{ 'command': 'guest-get-disks',
+  'returns': ['GuestDiskInfo'] }
+
 ##
 # @GuestFilesystemInfo:
 #
-- 
2.28.0



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

* [PATCH v3 2/3] qga: add implementation of guest-get-disks for Linux
  2020-10-07  7:46 [PATCH v3 0/3] qga: add command guest-get-disks Tomáš Golembiovský
  2020-10-07  7:46 ` [PATCH v3 1/3] " Tomáš Golembiovský
@ 2020-10-07  7:46 ` Tomáš Golembiovský
  2020-10-07  8:13   ` Marc-André Lureau
  2020-10-07  7:46 ` [PATCH v3 3/3] qga: add implementation of guest-get-disks for Windows Tomáš Golembiovský
  2 siblings, 1 reply; 7+ messages in thread
From: Tomáš Golembiovský @ 2020-10-07  7:46 UTC (permalink / raw)
  To: qemu-devel
  Cc: Daniel P. Berrangé, Michael Roth, Yonggang Luo,
	Tomáš Golembiovský, Marc-André Lureau,
	Philippe Mathieu-Daudé

The command lists all disks (real and virtual) as well as disk
partitions. For each disk the list of dependent disks is also listed and
/dev path is used as a handle so it can be matched with "name" field of
other returned disk entries. For disk partitions the "dependents" list
is populated with the the parent device for easier tracking of
hierarchy.

Example output:
{
  "return": [
    ...
    {
      "name": "/dev/dm-0",
      "partition": false,
      "dependents": [
        "/dev/sda2"
      ],
      "alias": "luks-7062202e-5b9b-433e-81e8-6628c40da9f7"
    },
    {
      "name": "/dev/sda2",
      "partition": true,
      "dependents": [
        "/dev/sda"
      ]
    },
    {
      "name": "/dev/sda",
      "partition": false,
      "address": {
        "serial": "SAMSUNG_MZ7LN512HCHP-000L1_S1ZKNXAG822493",
        "bus-type": "sata",
        ...
        "dev": "/dev/sda",
        "target": 0
      },
      "dependents": []
    },
    ...
  ]
}

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

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 422144bcff..b0ec1223c9 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -1150,12 +1150,25 @@ static void build_guest_fsinfo_for_virtual_device(char const *syspath,
     closedir(dir);
 }
 
+static bool is_disk_virtual(const char *devpath, Error **errp)
+{
+    g_autofree char *syspath = realpath(devpath, NULL);
+
+    if (!syspath) {
+        error_setg_errno(errp, errno, "realpath(\"%s\")", devpath);
+        return false;
+    }
+    return strstr(syspath, "/devices/virtual/block/") != NULL;
+}
+
 /* Dispatch to functions for virtual/real device */
 static void build_guest_fsinfo_for_device(char const *devpath,
                                           GuestFilesystemInfo *fs,
                                           Error **errp)
 {
+    ERRP_GUARD();
     char *syspath = realpath(devpath, NULL);
+    bool is_virtual = false;
 
     if (!syspath) {
         error_setg_errno(errp, errno, "realpath(\"%s\")", devpath);
@@ -1167,8 +1180,11 @@ static void build_guest_fsinfo_for_device(char const *devpath,
     }
 
     g_debug("  parse sysfs path '%s'", syspath);
-
-    if (strstr(syspath, "/devices/virtual/block/")) {
+    is_virtual = is_disk_virtual(syspath, errp);
+    if (*errp != NULL) {
+        return;
+    }
+    if (is_virtual) {
         build_guest_fsinfo_for_virtual_device(syspath, fs, errp);
     } else {
         build_guest_fsinfo_for_real_device(syspath, fs, errp);
@@ -1177,6 +1193,270 @@ static void build_guest_fsinfo_for_device(char const *devpath,
     free(syspath);
 }
 
+#ifdef CONFIG_LIBUDEV
+
+/*
+ * Wrapper around build_guest_fsinfo_for_device() for getting just
+ * the disk address.
+ */
+static GuestDiskAddress *get_disk_address(const char *syspath, Error **errp)
+{
+    g_autoptr(GuestFilesystemInfo) fs = NULL;
+
+    fs = g_new0(GuestFilesystemInfo, 1);
+    build_guest_fsinfo_for_device(syspath, fs, errp);
+    if (fs->disk != NULL) {
+        return g_steal_pointer(&fs->disk->value);
+    }
+    return NULL;
+}
+
+static char *get_alias_for_syspath(const char *syspath)
+{
+    struct udev *udev = NULL;
+    struct udev_device *udevice = NULL;
+    char *ret = NULL;
+
+    udev = udev_new();
+    if (udev == NULL) {
+        g_debug("failed to query udev");
+        goto out;
+    }
+    udevice = udev_device_new_from_syspath(udev, syspath);
+    if (udevice == NULL) {
+        g_debug("failed to query udev for path: %s", syspath);
+        goto out;
+    } else {
+        const char *alias = udev_device_get_property_value(
+            udevice, "DM_NAME");
+        /*
+         * NULL means there was an error and empty string means there is no
+         * alias. In case of no alias we return NULL instead of empty string.
+         */
+        if (alias == NULL) {
+            g_debug("failed to query udev for device alias for: %s",
+                syspath);
+        } else if (*alias != 0) {
+            ret = g_strdup(alias);
+        }
+    }
+
+out:
+    udev_unref(udev);
+    udev_device_unref(udevice);
+    return ret;
+}
+
+static char *get_device_for_syspath(const char *syspath)
+{
+    struct udev *udev = NULL;
+    struct udev_device *udevice = NULL;
+    char *ret = NULL;
+
+    udev = udev_new();
+    if (udev == NULL) {
+        g_debug("failed to query udev");
+        goto out;
+    }
+    udevice = udev_device_new_from_syspath(udev, syspath);
+    if (udevice == NULL) {
+        g_debug("failed to query udev for path: %s", syspath);
+        goto out;
+    } else {
+        ret = g_strdup(udev_device_get_devnode(udevice));
+    }
+
+out:
+    udev_unref(udev);
+    udev_device_unref(udevice);
+    return ret;
+}
+
+static void get_disk_deps(const char *disk_dir, GuestDiskInfo *disk)
+{
+    g_autofree char *deps_dir = NULL;
+    const gchar *dep;
+    GDir *dp_deps = NULL;
+
+    /* List dependent disks */
+    deps_dir = g_strdup_printf("%s/slaves", disk_dir);
+    g_debug("  listing entries in: %s", deps_dir);
+    dp_deps = g_dir_open(deps_dir, 0, NULL);
+    if (dp_deps == NULL) {
+        g_debug("failed to list entries in %s", deps_dir);
+        return;
+    }
+    while ((dep = g_dir_read_name(dp_deps)) != NULL) {
+        g_autofree char *dep_dir = NULL;
+        strList *dep_item = NULL;
+        char *dev_name;
+
+        /* Add dependent disks */
+        dep_dir = g_strdup_printf("%s/%s", deps_dir, dep);
+        dev_name = get_device_for_syspath(dep_dir);
+        if (dev_name != NULL) {
+            g_debug("  adding dependent device: %s", dev_name);
+            dep_item = g_new0(strList, 1);
+            dep_item->value = dev_name;
+            dep_item->next = disk->dependents;
+            disk->dependents = dep_item;
+        }
+    }
+    g_dir_close(dp_deps);
+}
+
+/*
+ * Detect partitions subdirectory, name is "<disk_name><number>" or
+ * "<disk_name>p<number>"
+ *
+ * @disk_name -- last component of /sys path (e.g. sda)
+ * @disk_dir -- sys path of the disk (e.g. /sys/block/sda)
+ * @disk_dev -- device node of the disk (e.g. /dev/sda)
+ */
+static GuestDiskInfoList *get_disk_partitions(
+    GuestDiskInfoList *list,
+    const char *disk_name, const char *disk_dir,
+    const char *disk_dev)
+{
+    GuestDiskInfoList *item, *ret = list;
+    struct dirent *de_disk;
+    DIR *dp_disk = NULL;
+    size_t len = strlen(disk_name);
+
+    dp_disk = opendir(disk_dir);
+    while ((de_disk = readdir(dp_disk)) != NULL) {
+        g_autofree char *partition_dir = NULL;
+        char *dev_name;
+        GuestDiskInfo *partition;
+
+        if (!(de_disk->d_type & DT_DIR)) {
+            continue;
+        }
+
+        if (!(strncmp(disk_name, de_disk->d_name, len) == 0 &&
+            ((*(de_disk->d_name + len) == 'p' &&
+            isdigit(*(de_disk->d_name + len + 1))) ||
+                isdigit(*(de_disk->d_name + len))))) {
+            continue;
+        }
+
+        partition_dir = g_strdup_printf("%s/%s",
+            disk_dir, de_disk->d_name);
+        dev_name = get_device_for_syspath(partition_dir);
+        if (dev_name == NULL) {
+            g_debug("Failed to get device name for syspath: %s",
+                disk_dir);
+            continue;
+        }
+        partition = g_new0(GuestDiskInfo, 1);
+        partition->name = dev_name;
+        partition->partition = true;
+        /* Add parent disk as dependent for easier tracking of hierarchy */
+        partition->dependents = g_new0(strList, 1);
+        partition->dependents->value = g_strdup(disk_dev);
+
+        item = g_new0(GuestDiskInfoList, 1);
+        item->value = partition;
+        item->next = ret;
+        ret = item;
+
+    }
+    closedir(dp_disk);
+
+    return ret;
+}
+
+GuestDiskInfoList *qmp_guest_get_disks(Error **errp)
+{
+    GuestDiskInfoList *item, *ret = NULL;
+    GuestDiskInfo *disk;
+    DIR *dp = NULL;
+    struct dirent *de = NULL;
+
+    g_debug("listing /sys/block directory");
+    dp = opendir("/sys/block");
+    if (dp == NULL) {
+        error_setg_errno(errp, errno, "Can't open directory \"/sys/block\"");
+        return NULL;
+    }
+    while ((de = readdir(dp)) != NULL) {
+        g_autofree char *disk_dir = NULL, *line = NULL,
+            *size_path = NULL, *deps_dir = NULL;
+        char *dev_name;
+        Error *local_err = NULL;
+        if (de->d_type != DT_LNK) {
+            g_debug("  skipping entry: %s", de->d_name);
+            continue;
+        }
+
+        /* Check size and skip zero-sized disks */
+        g_debug("  checking disk size");
+        size_path = g_strdup_printf("/sys/block/%s/size", de->d_name);
+        if (!g_file_get_contents(size_path, &line, NULL, NULL)) {
+            g_debug("  failed to read disk size");
+            continue;
+        }
+        if (g_strcmp0(line, "0\n") == 0) {
+            g_debug("  skipping zero-sized disk");
+            continue;
+        }
+
+        g_debug("  adding %s", de->d_name);
+        disk_dir = g_strdup_printf("/sys/block/%s", de->d_name);
+        dev_name = get_device_for_syspath(disk_dir);
+        if (dev_name == NULL) {
+            g_debug("Failed to get device name for syspath: %s",
+                disk_dir);
+            continue;
+        }
+        disk = g_new0(GuestDiskInfo, 1);
+        disk->name = dev_name;
+        disk->partition = false;
+        disk->alias = get_alias_for_syspath(disk_dir);
+        disk->has_alias = (disk->alias != NULL);
+        item = g_new0(GuestDiskInfoList, 1);
+        item->value = disk;
+        item->next = ret;
+        ret = item;
+
+        /* Get address for non-virtual devices */
+        bool is_virtual = is_disk_virtual(disk_dir, &local_err);
+        if (local_err != NULL) {
+            g_debug("  failed to check disk path, ignoring error: %s",
+                error_get_pretty(local_err));
+            error_free(local_err);
+            local_err = NULL;
+            /* Don't try to get the address */
+            is_virtual = true;
+        }
+        if (!is_virtual) {
+            disk->address = get_disk_address(disk_dir, &local_err);
+            if (local_err != NULL) {
+                g_debug("  failed to get device info, ignoring error: %s",
+                    error_get_pretty(local_err));
+                error_free(local_err);
+                local_err = NULL;
+            } else if (disk->address != NULL) {
+                disk->has_address = true;
+            }
+        }
+
+        get_disk_deps(disk_dir, disk);
+        ret = get_disk_partitions(ret, de->d_name, disk_dir, dev_name);
+    }
+    return ret;
+}
+
+#else
+
+GuestDiskInfoList *qmp_guest_get_disks(Error **errp)
+{
+    error_setg(errp, QERR_UNSUPPORTED);
+    return NULL;
+}
+
+#endif
+
 /* Return a list of the disk device(s)' info which @mount lies on */
 static GuestFilesystemInfo *build_guest_fsinfo(struct FsMount *mount,
                                                Error **errp)
@@ -2809,7 +3089,8 @@ GList *ga_command_blacklist_init(GList *blacklist)
         const char *list[] = {
             "guest-get-fsinfo", "guest-fsfreeze-status",
             "guest-fsfreeze-freeze", "guest-fsfreeze-freeze-list",
-            "guest-fsfreeze-thaw", "guest-get-fsinfo", NULL};
+            "guest-fsfreeze-thaw", "guest-get-fsinfo",
+            "guest-get-disks", NULL};
         char **p = (char **)list;
 
         while (*p) {
@@ -3051,9 +3332,3 @@ GuestDeviceInfoList *qmp_guest_get_devices(Error **errp)
 
     return NULL;
 }
-
-GuestDiskInfoList *qmp_guest_get_disks(Error **errp)
-{
-    error_setg(errp, QERR_UNSUPPORTED);
-    return NULL;
-}
-- 
2.28.0



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

* [PATCH v3 3/3] qga: add implementation of guest-get-disks for Windows
  2020-10-07  7:46 [PATCH v3 0/3] qga: add command guest-get-disks Tomáš Golembiovský
  2020-10-07  7:46 ` [PATCH v3 1/3] " Tomáš Golembiovský
  2020-10-07  7:46 ` [PATCH v3 2/3] qga: add implementation of guest-get-disks for Linux Tomáš Golembiovský
@ 2020-10-07  7:46 ` Tomáš Golembiovský
  2020-10-07  8:20   ` Marc-André Lureau
  2 siblings, 1 reply; 7+ messages in thread
From: Tomáš Golembiovský @ 2020-10-07  7:46 UTC (permalink / raw)
  To: qemu-devel
  Cc: Daniel P. Berrangé, Michael Roth, Yonggang Luo,
	Tomáš Golembiovský, Marc-André Lureau,
	Philippe Mathieu-Daudé

The command lists all the physical disk drives. Unlike for Linux
partitions and virtual volumes are not listed.

Example output:

{
  "return": [
    {
      "name": "\\\\.\\PhysicalDrive0",
      "partition": false,
      "address": {
        "serial": "QM00001",
        "bus-type": "sata",
        ...
      },
      "dependents": []
    }
  ]
}

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

diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 0dd16315d7..7fe5676785 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -979,6 +979,92 @@ out:
     return list;
 }
 
+GuestDiskInfoList *qmp_guest_get_disks(Error **errp)
+{
+    GuestDiskInfoList *new = NULL, *ret = NULL;
+    HDEVINFO dev_info;
+    SP_DEVICE_INTERFACE_DATA dev_iface_data;
+    int i;
+
+    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 device tree");
+        return NULL;
+    }
+
+    g_debug("enumerating devices");
+    dev_iface_data.cbSize = sizeof(SP_DEVICE_INTERFACE_DATA);
+    for (i = 0;
+        SetupDiEnumDeviceInterfaces(dev_info, NULL, &GUID_DEVINTERFACE_DISK,
+            i, &dev_iface_data);
+        i++) {
+        GuestDiskAddress *address = NULL;
+        GuestDiskInfo *disk = NULL;
+        Error *local_err = NULL;
+        g_autofree PSP_DEVICE_INTERFACE_DETAIL_DATA
+            pdev_iface_detail_data = NULL;
+        STORAGE_DEVICE_NUMBER sdn;
+        HANDLE dev_file;
+        DWORD size = 0;
+
+        g_debug("  getting device path");
+        while (!SetupDiGetDeviceInterfaceDetail(dev_info, &dev_iface_data,
+                pdev_iface_detail_data,
+                size, &size,
+                NULL)) {
+            if (GetLastError() == ERROR_INSUFFICIENT_BUFFER) {
+                pdev_iface_detail_data = g_realloc(pdev_iface_detail_data,
+                    size);
+                pdev_iface_detail_data->cbSize =
+                    sizeof(*pdev_iface_detail_data);
+            } else {
+                g_debug("failed to get device interface details");
+                continue;
+            }
+        }
+
+        g_debug("  device: %s", pdev_iface_detail_data->DevicePath);
+        dev_file = CreateFile(pdev_iface_detail_data->DevicePath, 0,
+            FILE_SHARE_READ, NULL, OPEN_EXISTING, 0, NULL);
+        if (!DeviceIoControl(dev_file, IOCTL_STORAGE_GET_DEVICE_NUMBER,
+                NULL, 0, &sdn, sizeof(sdn), &size, NULL)) {
+            CloseHandle(dev_file);
+            debug_error("failed to get storage device number");
+            continue;
+        }
+        CloseHandle(dev_file);
+
+        disk = g_new0(GuestDiskInfo, 1);
+        disk->name = g_strdup_printf("\\\\.\\PhysicalDrive%lu",
+            sdn.DeviceNumber);
+
+        g_debug("  number: %lu", sdn.DeviceNumber);
+        address = g_malloc0(sizeof(GuestDiskAddress));
+        address->has_dev = true;
+        address->dev = g_strdup(disk->name);
+        get_single_disk_info(sdn.DeviceNumber, address, &local_err);
+        if (local_err) {
+            g_debug("failed to get disk info: %s",
+                error_get_pretty(local_err));
+            error_free(local_err);
+            qapi_free_GuestDiskAddress(address);
+            address = NULL;
+        } else {
+            disk->address = address;
+            disk->has_address = true;
+        }
+
+        new = g_malloc0(sizeof(GuestDiskInfoList));
+        new->value = disk;
+        new->next = ret;
+        ret = new;
+    }
+
+    SetupDiDestroyDeviceInfoList(dev_info);
+    return ret;
+}
+
 #else
 
 static GuestDiskAddressList *build_guest_disk_info(char *guid, Error **errp)
@@ -986,6 +1072,12 @@ static GuestDiskAddressList *build_guest_disk_info(char *guid, Error **errp)
     return NULL;
 }
 
+GuestDiskInfoList *qmp_guest_get_disks(Error **errp)
+{
+    error_setg(errp, QERR_UNSUPPORTED);
+    return NULL;
+}
+
 #endif /* CONFIG_QGA_NTDDSCSI */
 
 static GuestFilesystemInfo *build_guest_fsinfo(char *guid, Error **errp)
@@ -2457,9 +2549,3 @@ GuestDeviceInfoList *qmp_guest_get_devices(Error **errp)
     }
     return head;
 }
-
-GuestDiskInfoList *qmp_guest_get_disks(Error **errp)
-{
-    error_setg(errp, QERR_UNSUPPORTED);
-    return NULL;
-}
-- 
2.28.0



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

* Re: [PATCH v3 2/3] qga: add implementation of guest-get-disks for Linux
  2020-10-07  7:46 ` [PATCH v3 2/3] qga: add implementation of guest-get-disks for Linux Tomáš Golembiovský
@ 2020-10-07  8:13   ` Marc-André Lureau
  0 siblings, 0 replies; 7+ messages in thread
From: Marc-André Lureau @ 2020-10-07  8:13 UTC (permalink / raw)
  To: Tomáš Golembiovský
  Cc: Daniel P. Berrangé, Yonggang Luo,
	Philippe Mathieu-Daudé, QEMU, Michael Roth

[-- Attachment #1: Type: text/plain, Size: 13108 bytes --]

On Wed, Oct 7, 2020 at 11:46 AM Tomáš Golembiovský <tgolembi@redhat.com>
wrote:

> The command lists all disks (real and virtual) as well as disk
> partitions. For each disk the list of dependent disks is also listed and
> /dev path is used as a handle so it can be matched with "name" field of
> other returned disk entries. For disk partitions the "dependents" list
> is populated with the the parent device for easier tracking of
> hierarchy.
>
> Example output:
> {
>   "return": [
>     ...
>     {
>       "name": "/dev/dm-0",
>       "partition": false,
>       "dependents": [
>         "/dev/sda2"
>       ],
>       "alias": "luks-7062202e-5b9b-433e-81e8-6628c40da9f7"
>     },
>     {
>       "name": "/dev/sda2",
>       "partition": true,
>       "dependents": [
>         "/dev/sda"
>       ]
>     },
>     {
>       "name": "/dev/sda",
>       "partition": false,
>       "address": {
>         "serial": "SAMSUNG_MZ7LN512HCHP-000L1_S1ZKNXAG822493",
>         "bus-type": "sata",
>         ...
>         "dev": "/dev/sda",
>         "target": 0
>       },
>       "dependents": []
>     },
>     ...
>   ]
> }
>
> Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
> ---
>  qga/commands-posix.c | 293 +++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 284 insertions(+), 9 deletions(-)
>
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 422144bcff..b0ec1223c9 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -1150,12 +1150,25 @@ static void
> build_guest_fsinfo_for_virtual_device(char const *syspath,
>      closedir(dir);
>  }
>
> +static bool is_disk_virtual(const char *devpath, Error **errp)
> +{
> +    g_autofree char *syspath = realpath(devpath, NULL);
> +
> +    if (!syspath) {
> +        error_setg_errno(errp, errno, "realpath(\"%s\")", devpath);
> +        return false;
> +    }
> +    return strstr(syspath, "/devices/virtual/block/") != NULL;
> +}
> +
>  /* Dispatch to functions for virtual/real device */
>  static void build_guest_fsinfo_for_device(char const *devpath,
>                                            GuestFilesystemInfo *fs,
>                                            Error **errp)
>  {
> +    ERRP_GUARD();
>      char *syspath = realpath(devpath, NULL);
> +    bool is_virtual = false;
>
>      if (!syspath) {
>          error_setg_errno(errp, errno, "realpath(\"%s\")", devpath);
> @@ -1167,8 +1180,11 @@ static void build_guest_fsinfo_for_device(char
> const *devpath,
>      }
>
>      g_debug("  parse sysfs path '%s'", syspath);
> -
> -    if (strstr(syspath, "/devices/virtual/block/")) {
> +    is_virtual = is_disk_virtual(syspath, errp);
> +    if (*errp != NULL) {
>

I think it's leaking syspath.

+        return;
> +    }
> +    if (is_virtual) {
>          build_guest_fsinfo_for_virtual_device(syspath, fs, errp);
>      } else {
>          build_guest_fsinfo_for_real_device(syspath, fs, errp);
> @@ -1177,6 +1193,270 @@ static void build_guest_fsinfo_for_device(char
> const *devpath,
>      free(syspath);
>  }
>
> +#ifdef CONFIG_LIBUDEV
> +
> +/*
> + * Wrapper around build_guest_fsinfo_for_device() for getting just
> + * the disk address.
> + */
> +static GuestDiskAddress *get_disk_address(const char *syspath, Error
> **errp)
> +{
> +    g_autoptr(GuestFilesystemInfo) fs = NULL;
> +
> +    fs = g_new0(GuestFilesystemInfo, 1);
> +    build_guest_fsinfo_for_device(syspath, fs, errp);
> +    if (fs->disk != NULL) {
> +        return g_steal_pointer(&fs->disk->value);
> +    }
> +    return NULL;
> +}
> +
> +static char *get_alias_for_syspath(const char *syspath)
> +{
> +    struct udev *udev = NULL;
> +    struct udev_device *udevice = NULL;
> +    char *ret = NULL;
> +
> +    udev = udev_new();
> +    if (udev == NULL) {
> +        g_debug("failed to query udev");
> +        goto out;
> +    }
> +    udevice = udev_device_new_from_syspath(udev, syspath);
> +    if (udevice == NULL) {
> +        g_debug("failed to query udev for path: %s", syspath);
> +        goto out;
> +    } else {
> +        const char *alias = udev_device_get_property_value(
> +            udevice, "DM_NAME");
> +        /*
> +         * NULL means there was an error and empty string means there is
> no
> +         * alias. In case of no alias we return NULL instead of empty
> string.
> +         */
> +        if (alias == NULL) {
> +            g_debug("failed to query udev for device alias for: %s",
> +                syspath);
> +        } else if (*alias != 0) {
> +            ret = g_strdup(alias);
> +        }
> +    }
> +
> +out:
> +    udev_unref(udev);
> +    udev_device_unref(udevice);
> +    return ret;
> +}
> +
> +static char *get_device_for_syspath(const char *syspath)
> +{
> +    struct udev *udev = NULL;
> +    struct udev_device *udevice = NULL;
> +    char *ret = NULL;
> +
> +    udev = udev_new();
> +    if (udev == NULL) {
> +        g_debug("failed to query udev");
> +        goto out;
> +    }
> +    udevice = udev_device_new_from_syspath(udev, syspath);
> +    if (udevice == NULL) {
> +        g_debug("failed to query udev for path: %s", syspath);
> +        goto out;
> +    } else {
> +        ret = g_strdup(udev_device_get_devnode(udevice));
> +    }
> +
> +out:
> +    udev_unref(udev);
> +    udev_device_unref(udevice);
> +    return ret;
> +}
> +
> +static void get_disk_deps(const char *disk_dir, GuestDiskInfo *disk)
> +{
> +    g_autofree char *deps_dir = NULL;
> +    const gchar *dep;
> +    GDir *dp_deps = NULL;
> +
> +    /* List dependent disks */
> +    deps_dir = g_strdup_printf("%s/slaves", disk_dir);
> +    g_debug("  listing entries in: %s", deps_dir);
> +    dp_deps = g_dir_open(deps_dir, 0, NULL);
> +    if (dp_deps == NULL) {
> +        g_debug("failed to list entries in %s", deps_dir);
> +        return;
> +    }
> +    while ((dep = g_dir_read_name(dp_deps)) != NULL) {
> +        g_autofree char *dep_dir = NULL;
> +        strList *dep_item = NULL;
> +        char *dev_name;
> +
> +        /* Add dependent disks */
> +        dep_dir = g_strdup_printf("%s/%s", deps_dir, dep);
> +        dev_name = get_device_for_syspath(dep_dir);
> +        if (dev_name != NULL) {
> +            g_debug("  adding dependent device: %s", dev_name);
> +            dep_item = g_new0(strList, 1);
> +            dep_item->value = dev_name;
> +            dep_item->next = disk->dependents;
> +            disk->dependents = dep_item;
> +        }
> +    }
> +    g_dir_close(dp_deps);
> +}
> +
> +/*
> + * Detect partitions subdirectory, name is "<disk_name><number>" or
> + * "<disk_name>p<number>"
> + *
> + * @disk_name -- last component of /sys path (e.g. sda)
> + * @disk_dir -- sys path of the disk (e.g. /sys/block/sda)
> + * @disk_dev -- device node of the disk (e.g. /dev/sda)
> + */
> +static GuestDiskInfoList *get_disk_partitions(
> +    GuestDiskInfoList *list,
> +    const char *disk_name, const char *disk_dir,
> +    const char *disk_dev)
> +{
> +    GuestDiskInfoList *item, *ret = list;
> +    struct dirent *de_disk;
> +    DIR *dp_disk = NULL;
> +    size_t len = strlen(disk_name);
> +
> +    dp_disk = opendir(disk_dir);
> +    while ((de_disk = readdir(dp_disk)) != NULL) {
> +        g_autofree char *partition_dir = NULL;
> +        char *dev_name;
> +        GuestDiskInfo *partition;
> +
> +        if (!(de_disk->d_type & DT_DIR)) {
> +            continue;
> +        }
> +
> +        if (!(strncmp(disk_name, de_disk->d_name, len) == 0 &&
> +            ((*(de_disk->d_name + len) == 'p' &&
> +            isdigit(*(de_disk->d_name + len + 1))) ||
> +                isdigit(*(de_disk->d_name + len))))) {
> +            continue;
> +        }
> +
> +        partition_dir = g_strdup_printf("%s/%s",
> +            disk_dir, de_disk->d_name);
> +        dev_name = get_device_for_syspath(partition_dir);
> +        if (dev_name == NULL) {
> +            g_debug("Failed to get device name for syspath: %s",
> +                disk_dir);
> +            continue;
> +        }
> +        partition = g_new0(GuestDiskInfo, 1);
> +        partition->name = dev_name;
> +        partition->partition = true;
> +        /* Add parent disk as dependent for easier tracking of hierarchy
> */
> +        partition->dependents = g_new0(strList, 1);
> +        partition->dependents->value = g_strdup(disk_dev);
> +
> +        item = g_new0(GuestDiskInfoList, 1);
> +        item->value = partition;
> +        item->next = ret;
> +        ret = item;
> +
> +    }
> +    closedir(dp_disk);
> +
> +    return ret;
> +}
> +
> +GuestDiskInfoList *qmp_guest_get_disks(Error **errp)
> +{
> +    GuestDiskInfoList *item, *ret = NULL;
> +    GuestDiskInfo *disk;
> +    DIR *dp = NULL;
> +    struct dirent *de = NULL;
> +
> +    g_debug("listing /sys/block directory");
> +    dp = opendir("/sys/block");
> +    if (dp == NULL) {
> +        error_setg_errno(errp, errno, "Can't open directory
> \"/sys/block\"");
> +        return NULL;
> +    }
> +    while ((de = readdir(dp)) != NULL) {
> +        g_autofree char *disk_dir = NULL, *line = NULL,
> +            *size_path = NULL, *deps_dir = NULL;
> +        char *dev_name;
> +        Error *local_err = NULL;
> +        if (de->d_type != DT_LNK) {
> +            g_debug("  skipping entry: %s", de->d_name);
> +            continue;
> +        }
> +
> +        /* Check size and skip zero-sized disks */
> +        g_debug("  checking disk size");
> +        size_path = g_strdup_printf("/sys/block/%s/size", de->d_name);
> +        if (!g_file_get_contents(size_path, &line, NULL, NULL)) {
> +            g_debug("  failed to read disk size");
> +            continue;
> +        }
> +        if (g_strcmp0(line, "0\n") == 0) {
> +            g_debug("  skipping zero-sized disk");
> +            continue;
> +        }
> +
> +        g_debug("  adding %s", de->d_name);
> +        disk_dir = g_strdup_printf("/sys/block/%s", de->d_name);
> +        dev_name = get_device_for_syspath(disk_dir);
> +        if (dev_name == NULL) {
> +            g_debug("Failed to get device name for syspath: %s",
> +                disk_dir);
> +            continue;
> +        }
> +        disk = g_new0(GuestDiskInfo, 1);
> +        disk->name = dev_name;
> +        disk->partition = false;
> +        disk->alias = get_alias_for_syspath(disk_dir);
> +        disk->has_alias = (disk->alias != NULL);
> +        item = g_new0(GuestDiskInfoList, 1);
> +        item->value = disk;
> +        item->next = ret;
> +        ret = item;
> +
> +        /* Get address for non-virtual devices */
> +        bool is_virtual = is_disk_virtual(disk_dir, &local_err);
> +        if (local_err != NULL) {
> +            g_debug("  failed to check disk path, ignoring error: %s",
> +                error_get_pretty(local_err));
> +            error_free(local_err);
> +            local_err = NULL;
> +            /* Don't try to get the address */
> +            is_virtual = true;
> +        }
> +        if (!is_virtual) {
> +            disk->address = get_disk_address(disk_dir, &local_err);
> +            if (local_err != NULL) {
> +                g_debug("  failed to get device info, ignoring error: %s",
> +                    error_get_pretty(local_err));
> +                error_free(local_err);
> +                local_err = NULL;
> +            } else if (disk->address != NULL) {
> +                disk->has_address = true;
> +            }
> +        }
> +
> +        get_disk_deps(disk_dir, disk);
> +        ret = get_disk_partitions(ret, de->d_name, disk_dir, dev_name);
> +    }
> +    return ret;
> +}
> +
> +#else
> +
> +GuestDiskInfoList *qmp_guest_get_disks(Error **errp)
> +{
> +    error_setg(errp, QERR_UNSUPPORTED);
> +    return NULL;
> +}
> +
> +#endif
> +
>  /* Return a list of the disk device(s)' info which @mount lies on */
>  static GuestFilesystemInfo *build_guest_fsinfo(struct FsMount *mount,
>                                                 Error **errp)
> @@ -2809,7 +3089,8 @@ GList *ga_command_blacklist_init(GList *blacklist)
>          const char *list[] = {
>              "guest-get-fsinfo", "guest-fsfreeze-status",
>              "guest-fsfreeze-freeze", "guest-fsfreeze-freeze-list",
> -            "guest-fsfreeze-thaw", "guest-get-fsinfo", NULL};
> +            "guest-fsfreeze-thaw", "guest-get-fsinfo",
> +            "guest-get-disks", NULL};
>          char **p = (char **)list;
>
>          while (*p) {
> @@ -3051,9 +3332,3 @@ GuestDeviceInfoList *qmp_guest_get_devices(Error
> **errp)
>
>      return NULL;
>  }
> -
> -GuestDiskInfoList *qmp_guest_get_disks(Error **errp)
> -{
> -    error_setg(errp, QERR_UNSUPPORTED);
> -    return NULL;
> -}
> --
> 2.28.0
>
>
lgtm otherwise

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 16463 bytes --]

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

* Re: [PATCH v3 3/3] qga: add implementation of guest-get-disks for Windows
  2020-10-07  7:46 ` [PATCH v3 3/3] qga: add implementation of guest-get-disks for Windows Tomáš Golembiovský
@ 2020-10-07  8:20   ` Marc-André Lureau
  2020-10-07 15:15     ` Tomáš Golembiovský
  0 siblings, 1 reply; 7+ messages in thread
From: Marc-André Lureau @ 2020-10-07  8:20 UTC (permalink / raw)
  To: Tomáš Golembiovský
  Cc: Daniel P. Berrangé, Yonggang Luo,
	Philippe Mathieu-Daudé, QEMU, Michael Roth

[-- Attachment #1: Type: text/plain, Size: 5170 bytes --]

Hi

On Wed, Oct 7, 2020 at 11:46 AM Tomáš Golembiovský <tgolembi@redhat.com>
wrote:

> The command lists all the physical disk drives. Unlike for Linux
> partitions and virtual volumes are not listed.
>
> Example output:
>
> {
>   "return": [
>     {
>       "name": "\\\\.\\PhysicalDrive0",
>       "partition": false,
>       "address": {
>         "serial": "QM00001",
>         "bus-type": "sata",
>         ...
>       },
>       "dependents": []
>     }
>   ]
> }
>
> Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
> ---
>  qga/commands-win32.c | 98 +++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 92 insertions(+), 6 deletions(-)
>
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index 0dd16315d7..7fe5676785 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -979,6 +979,92 @@ out:
>      return list;
>  }
>
> +GuestDiskInfoList *qmp_guest_get_disks(Error **errp)
> +{
>

ERRP_GUARD?

+    GuestDiskInfoList *new = NULL, *ret = NULL;
> +    HDEVINFO dev_info;
> +    SP_DEVICE_INTERFACE_DATA dev_iface_data;
> +    int i;
> +
> +    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 device
> tree");
> +        return NULL;
> +    }
> +
> +    g_debug("enumerating devices");
> +    dev_iface_data.cbSize = sizeof(SP_DEVICE_INTERFACE_DATA);
> +    for (i = 0;
> +        SetupDiEnumDeviceInterfaces(dev_info, NULL,
> &GUID_DEVINTERFACE_DISK,
> +            i, &dev_iface_data);
> +        i++) {
> +        GuestDiskAddress *address = NULL;
> +        GuestDiskInfo *disk = NULL;
> +        Error *local_err = NULL;
> +        g_autofree PSP_DEVICE_INTERFACE_DETAIL_DATA
> +            pdev_iface_detail_data = NULL;
> +        STORAGE_DEVICE_NUMBER sdn;
> +        HANDLE dev_file;
> +        DWORD size = 0;
> +
> +        g_debug("  getting device path");
> +        while (!SetupDiGetDeviceInterfaceDetail(dev_info, &dev_iface_data,
> +                pdev_iface_detail_data,
> +                size, &size,
> +                NULL)) {
> +            if (GetLastError() == ERROR_INSUFFICIENT_BUFFER) {
> +                pdev_iface_detail_data = g_realloc(pdev_iface_detail_data,
> +                    size);
> +                pdev_iface_detail_data->cbSize =
> +                    sizeof(*pdev_iface_detail_data);
> +            } else {
> +                g_debug("failed to get device interface details");
> +                continue;
>

Is "continue" appropriate here? Looks like this may loop forever easily.

+            }
> +        }
> +
> +        g_debug("  device: %s", pdev_iface_detail_data->DevicePath);
> +        dev_file = CreateFile(pdev_iface_detail_data->DevicePath, 0,
> +            FILE_SHARE_READ, NULL, OPEN_EXISTING, 0, NULL);
> +        if (!DeviceIoControl(dev_file, IOCTL_STORAGE_GET_DEVICE_NUMBER,
> +                NULL, 0, &sdn, sizeof(sdn), &size, NULL)) {
> +            CloseHandle(dev_file);
> +            debug_error("failed to get storage device number");
> +            continue;
> +        }
> +        CloseHandle(dev_file);
> +
> +        disk = g_new0(GuestDiskInfo, 1);
> +        disk->name = g_strdup_printf("\\\\.\\PhysicalDrive%lu",
> +            sdn.DeviceNumber);
> +
> +        g_debug("  number: %lu", sdn.DeviceNumber);
> +        address = g_malloc0(sizeof(GuestDiskAddress));
> +        address->has_dev = true;
> +        address->dev = g_strdup(disk->name);
> +        get_single_disk_info(sdn.DeviceNumber, address, &local_err);
> +        if (local_err) {
> +            g_debug("failed to get disk info: %s",
> +                error_get_pretty(local_err));
> +            error_free(local_err);
> +            qapi_free_GuestDiskAddress(address);
> +            address = NULL;
> +        } else {
> +            disk->address = address;
> +            disk->has_address = true;
> +        }
> +
> +        new = g_malloc0(sizeof(GuestDiskInfoList));
> +        new->value = disk;
> +        new->next = ret;
> +        ret = new;
> +    }
> +
> +    SetupDiDestroyDeviceInfoList(dev_info);
> +    return ret;
> +}
> +
>  #else
>
>  static GuestDiskAddressList *build_guest_disk_info(char *guid, Error
> **errp)
> @@ -986,6 +1072,12 @@ static GuestDiskAddressList
> *build_guest_disk_info(char *guid, Error **errp)
>      return NULL;
>  }
>
> +GuestDiskInfoList *qmp_guest_get_disks(Error **errp)
> +{
> +    error_setg(errp, QERR_UNSUPPORTED);
> +    return NULL;
> +}
> +
>  #endif /* CONFIG_QGA_NTDDSCSI */
>
>  static GuestFilesystemInfo *build_guest_fsinfo(char *guid, Error **errp)
> @@ -2457,9 +2549,3 @@ GuestDeviceInfoList *qmp_guest_get_devices(Error
> **errp)
>      }
>      return head;
>  }
> -
> -GuestDiskInfoList *qmp_guest_get_disks(Error **errp)
> -{
> -    error_setg(errp, QERR_UNSUPPORTED);
> -    return NULL;
> -}
> --
> 2.28.0
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 6908 bytes --]

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

* Re: [PATCH v3 3/3] qga: add implementation of guest-get-disks for Windows
  2020-10-07  8:20   ` Marc-André Lureau
@ 2020-10-07 15:15     ` Tomáš Golembiovský
  0 siblings, 0 replies; 7+ messages in thread
From: Tomáš Golembiovský @ 2020-10-07 15:15 UTC (permalink / raw)
  To: Marc-André Lureau
  Cc: Daniel P. Berrangé, Yonggang Luo,
	Philippe Mathieu-Daudé, QEMU, Michael Roth

On Wed, Oct 07, 2020 at 12:20:14PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Wed, Oct 7, 2020 at 11:46 AM Tomáš Golembiovský <tgolembi@redhat.com>
> wrote:
> 
> > The command lists all the physical disk drives. Unlike for Linux
> > partitions and virtual volumes are not listed.
> >
> > Example output:
> >
> > {
> >   "return": [
> >     {
> >       "name": "\\\\.\\PhysicalDrive0",
> >       "partition": false,
> >       "address": {
> >         "serial": "QM00001",
> >         "bus-type": "sata",
> >         ...
> >       },
> >       "dependents": []
> >     }
> >   ]
> > }
> >
> > Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
> > ---
> >  qga/commands-win32.c | 98 +++++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 92 insertions(+), 6 deletions(-)
> >
> > diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> > index 0dd16315d7..7fe5676785 100644
> > --- a/qga/commands-win32.c
> > +++ b/qga/commands-win32.c
> > @@ -979,6 +979,92 @@ out:
> >      return list;
> >  }
> >
> > +GuestDiskInfoList *qmp_guest_get_disks(Error **errp)
> > +{
> >
> 
> ERRP_GUARD?

Is it necessary? We're not dereferencing errp anywhere.

> 
> +    GuestDiskInfoList *new = NULL, *ret = NULL;
> > +    HDEVINFO dev_info;
> > +    SP_DEVICE_INTERFACE_DATA dev_iface_data;
> > +    int i;
> > +
> > +    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 device
> > tree");
> > +        return NULL;
> > +    }
> > +
> > +    g_debug("enumerating devices");
> > +    dev_iface_data.cbSize = sizeof(SP_DEVICE_INTERFACE_DATA);
> > +    for (i = 0;
> > +        SetupDiEnumDeviceInterfaces(dev_info, NULL,
> > &GUID_DEVINTERFACE_DISK,
> > +            i, &dev_iface_data);
> > +        i++) {
> > +        GuestDiskAddress *address = NULL;
> > +        GuestDiskInfo *disk = NULL;
> > +        Error *local_err = NULL;
> > +        g_autofree PSP_DEVICE_INTERFACE_DETAIL_DATA
> > +            pdev_iface_detail_data = NULL;
> > +        STORAGE_DEVICE_NUMBER sdn;
> > +        HANDLE dev_file;
> > +        DWORD size = 0;
> > +
> > +        g_debug("  getting device path");
> > +        while (!SetupDiGetDeviceInterfaceDetail(dev_info, &dev_iface_data,
> > +                pdev_iface_detail_data,
> > +                size, &size,
> > +                NULL)) {
> > +            if (GetLastError() == ERROR_INSUFFICIENT_BUFFER) {
> > +                pdev_iface_detail_data = g_realloc(pdev_iface_detail_data,
> > +                    size);
> > +                pdev_iface_detail_data->cbSize =
> > +                    sizeof(*pdev_iface_detail_data);
> > +            } else {
> > +                g_debug("failed to get device interface details");
> > +                continue;
> >
> 
> Is "continue" appropriate here? Looks like this may loop forever easily.

Duh! It would.

    Tomas

> 
> +            }
> > +        }
> > +
> > +        g_debug("  device: %s", pdev_iface_detail_data->DevicePath);
> > +        dev_file = CreateFile(pdev_iface_detail_data->DevicePath, 0,
> > +            FILE_SHARE_READ, NULL, OPEN_EXISTING, 0, NULL);
> > +        if (!DeviceIoControl(dev_file, IOCTL_STORAGE_GET_DEVICE_NUMBER,
> > +                NULL, 0, &sdn, sizeof(sdn), &size, NULL)) {
> > +            CloseHandle(dev_file);
> > +            debug_error("failed to get storage device number");
> > +            continue;
> > +        }
> > +        CloseHandle(dev_file);
> > +
> > +        disk = g_new0(GuestDiskInfo, 1);
> > +        disk->name = g_strdup_printf("\\\\.\\PhysicalDrive%lu",
> > +            sdn.DeviceNumber);
> > +
> > +        g_debug("  number: %lu", sdn.DeviceNumber);
> > +        address = g_malloc0(sizeof(GuestDiskAddress));
> > +        address->has_dev = true;
> > +        address->dev = g_strdup(disk->name);
> > +        get_single_disk_info(sdn.DeviceNumber, address, &local_err);
> > +        if (local_err) {
> > +            g_debug("failed to get disk info: %s",
> > +                error_get_pretty(local_err));
> > +            error_free(local_err);
> > +            qapi_free_GuestDiskAddress(address);
> > +            address = NULL;
> > +        } else {
> > +            disk->address = address;
> > +            disk->has_address = true;
> > +        }
> > +
> > +        new = g_malloc0(sizeof(GuestDiskInfoList));
> > +        new->value = disk;
> > +        new->next = ret;
> > +        ret = new;
> > +    }
> > +
> > +    SetupDiDestroyDeviceInfoList(dev_info);
> > +    return ret;
> > +}
> > +
> >  #else
> >
> >  static GuestDiskAddressList *build_guest_disk_info(char *guid, Error
> > **errp)
> > @@ -986,6 +1072,12 @@ static GuestDiskAddressList
> > *build_guest_disk_info(char *guid, Error **errp)
> >      return NULL;
> >  }
> >
> > +GuestDiskInfoList *qmp_guest_get_disks(Error **errp)
> > +{
> > +    error_setg(errp, QERR_UNSUPPORTED);
> > +    return NULL;
> > +}
> > +
> >  #endif /* CONFIG_QGA_NTDDSCSI */
> >
> >  static GuestFilesystemInfo *build_guest_fsinfo(char *guid, Error **errp)
> > @@ -2457,9 +2549,3 @@ GuestDeviceInfoList *qmp_guest_get_devices(Error
> > **errp)
> >      }
> >      return head;
> >  }
> > -
> > -GuestDiskInfoList *qmp_guest_get_disks(Error **errp)
> > -{
> > -    error_setg(errp, QERR_UNSUPPORTED);
> > -    return NULL;
> > -}
> > --
> > 2.28.0
> >
> >
> 
> -- 
> Marc-André Lureau

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



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

end of thread, other threads:[~2020-10-07 15:18 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-10-07  7:46 [PATCH v3 0/3] qga: add command guest-get-disks Tomáš Golembiovský
2020-10-07  7:46 ` [PATCH v3 1/3] " Tomáš Golembiovský
2020-10-07  7:46 ` [PATCH v3 2/3] qga: add implementation of guest-get-disks for Linux Tomáš Golembiovský
2020-10-07  8:13   ` Marc-André Lureau
2020-10-07  7:46 ` [PATCH v3 3/3] qga: add implementation of guest-get-disks for Windows Tomáš Golembiovský
2020-10-07  8:20   ` Marc-André Lureau
2020-10-07 15:15     ` Tomáš Golembiovský

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).