[PATCH 0/1] qga: add command guest-get-disk

[PATCH 0/1] qga: add command guest-get-disk

[Tomáš Golembiovský]

Adds command to list root 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.

Tomáš Golembiovský (1):
  qga: add command guest-get-disks

 qga/commands-posix.c | 91 +++++++++++++++++++++++++++++++++++++++++++-
 qga/commands-win32.c | 83 ++++++++++++++++++++++++++++++++++++++++
 qga/qapi-schema.json | 13 +++++++
 3 files changed, 186 insertions(+), 1 deletion(-)

-- 
2.25.0

[PATCH 1/1] qga: add command guest-get-disks

[Tomáš Golembiovský]

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 allows listing information about attached root disks of the VM. This is
usefull for management applications for mapping virtualized devices or
pass-through devices to device names in the guest OS.

Output is similar to the list of partitions of guest-get-fsinfo, except that
the disks are mapped instead of partitions.

Example output:

{
  "return": [
    {
      "serial": "SAMSUNG_123456789",
      "bus-type": "sata",
      "bus": 0,
      "unit": 0,
      "pci-controller": {
        "bus": 0,
        "slot": 31,
        "domain": 0,
        "function": 2
      },
      "dev": "/dev/sda",
      "target": 0
    },
    ...
  ]
}

Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
---
 qga/commands-posix.c | 91 +++++++++++++++++++++++++++++++++++++++++++-
 qga/commands-win32.c | 83 ++++++++++++++++++++++++++++++++++++++++
 qga/qapi-schema.json | 13 +++++++
 3 files changed, 186 insertions(+), 1 deletion(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 744c2b5a5d..4cebec96a6 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -62,6 +62,8 @@ extern char **environ;
 #endif
 #endif
 
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(GuestFilesystemInfo, qapi_free_GuestFilesystemInfo)
+
 static void ga_wait_child(pid_t pid, int *status, Error **errp)
 {
     pid_t rpid;
@@ -1177,6 +1179,92 @@ static void build_guest_fsinfo_for_device(char const *devpath,
     free(syspath);
 }
 
+GuestDiskAddressList *qmp_guest_get_disks(Error **errp)
+{
+    GuestDiskAddressList *item, *ret = NULL;
+    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, *slaves_dir = NULL;
+        g_autoptr(GuestFilesystemInfo) fs = NULL;
+        uint64_t slaves = 0;
+        struct dirent *de_slaves;
+        DIR *dp_slaves = NULL;
+        FILE *fp = NULL;
+        size_t n;
+        Error *local_err = NULL;
+        if (de->d_type != DT_LNK) {
+            g_debug("  skipping entry: %s", de->d_name);
+            continue;
+        }
+
+        slaves_dir = g_strdup_printf("/sys/block/%s/slaves", de->d_name);
+        if (slaves_dir == NULL) {
+            g_debug("  failed to open directory %s", slaves_dir);
+            continue;
+        }
+        g_debug("  counting entries in: %s", slaves_dir);
+        dp_slaves = opendir(slaves_dir);
+        while ((de_slaves = readdir(dp_slaves)) != NULL) {
+            if ((strcmp(".", de_slaves->d_name) == 0) ||
+                (strcmp("..", de_slaves->d_name) == 0)) {
+                continue;
+            }
+            slaves++;
+        }
+        closedir(dp_slaves);
+        g_debug("    counted %lu items", slaves);
+        if (slaves != 0) {
+            continue;
+        }
+
+        g_debug("  checking disk size");
+        size_path = g_strdup_printf("/sys/block/%s/size", de->d_name);
+        fp = fopen(size_path, "r");
+        if (!fp) {
+            g_debug("  failed to read disk size");
+            continue;
+        }
+        if (getline(&line, &n, fp) == -1) {
+            g_debug("  failed to read disk size");
+            fclose(fp);
+            continue;
+        }
+        fclose(fp);
+        if (strcmp(line, "0\n") == 0) {
+            g_debug("  skipping zero-sized disk");
+            continue;
+        }
+
+        fs = g_malloc0(sizeof(*fs));
+        g_debug("  adding %s", de->d_name);
+        disk_dir = g_strdup_printf("/sys/block/%s", de->d_name);
+        build_guest_fsinfo_for_device(disk_dir, fs, &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);
+            continue;
+        } else if (fs->disk == NULL) {
+            g_debug("  skipping unknown disk");
+            continue;
+        }
+        item = g_steal_pointer(&fs->disk);
+        g_assert(item->next == NULL); /* expecting just a single disk */
+        item->next = ret;
+        ret = item;
+    }
+    return ret;
+}
+
 /* 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 +2897,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) {
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index aaa71f147b..0bafa2dc4c 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -945,6 +945,83 @@ out:
     return list;
 }
 
+GuestDiskAddressList *qmp_guest_get_disks(Error **errp)
+{
+    GuestDiskAddressList *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 *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_malloc(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);
+
+        g_debug("  number: %lu", sdn.DeviceNumber);
+        disk = g_malloc0(sizeof(GuestDiskAddress));
+        disk->has_dev = true;
+        disk->dev = g_strdup_printf("\\\\.\\PhysicalDrive%lu",
+            sdn.DeviceNumber);
+        get_single_disk_info(sdn.DeviceNumber, disk, &local_err);
+        if (local_err) {
+            g_debug("failed to get disk info: %s",
+                error_get_pretty(local_err));
+            error_free(local_err);
+            g_free(disk);
+            continue;
+        }
+        new = g_malloc0(sizeof(GuestDiskAddressList));
+        new->value = disk;
+        new->next = ret;
+        ret = new;
+    }
+
+    SetupDiDestroyDeviceInfoList(dev_info);
+    return ret;
+}
+
 #else
 
 static GuestDiskAddressList *build_guest_disk_info(char *guid, Error **errp)
@@ -952,6 +1029,12 @@ static GuestDiskAddressList *build_guest_disk_info(char *guid, Error **errp)
     return NULL;
 }
 
+GuestDiskAddressList *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)
diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index 408a662ea5..5d8fa1c283 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -862,6 +862,19 @@
            'bus': 'int', 'target': 'int', 'unit': 'int',
            '*serial': 'str', '*dev': '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': ['GuestDiskAddress'] }
+
 ##
 # @GuestFilesystemInfo:
 #
-- 
2.25.0

Re: [PATCH 1/1] qga: add command guest-get-disks

[Philippe Mathieu-Daudé]

On 8/6/20 11:03 AM, Tomáš Golembiovský wrote:
> 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 allows listing information about attached root disks of the VM. This is
> usefull for management applications for mapping virtualized devices or
> pass-through devices to device names in the guest OS.
> 
> Output is similar to the list of partitions of guest-get-fsinfo, except that
> the disks are mapped instead of partitions.
> 
> Example output:
> 
> {
>   "return": [
>     {
>       "serial": "SAMSUNG_123456789",
>       "bus-type": "sata",
>       "bus": 0,
>       "unit": 0,
>       "pci-controller": {
>         "bus": 0,
>         "slot": 31,
>         "domain": 0,
>         "function": 2
>       },
>       "dev": "/dev/sda",
>       "target": 0
>     },
>     ...
>   ]
> }
> 
> Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
> ---
>  qga/commands-posix.c | 91 +++++++++++++++++++++++++++++++++++++++++++-
>  qga/commands-win32.c | 83 ++++++++++++++++++++++++++++++++++++++++
>  qga/qapi-schema.json | 13 +++++++
>  3 files changed, 186 insertions(+), 1 deletion(-)

Not sure this is better, but splitting this in 3 could help different
people reviewing (developers not familiar with the Windows API might
feel not comfortable adding a R-b tag for the POSIX part, and
reciprocally).

1/ qapi + stubs
2/ POSIX implementation
3/ Win32 implementation

Re: [PATCH 1/1] qga: add command guest-get-disks

[Michael Roth]

Quoting Tomáš Golembiovský (2020-08-06 04:03:06)
> 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 allows listing information about attached root disks of the VM. This is
> usefull for management applications for mapping virtualized devices or
> pass-through devices to device names in the guest OS.
> 
> Output is similar to the list of partitions of guest-get-fsinfo, except that
> the disks are mapped instead of partitions.
> 
> Example output:
> 
> {
>   "return": [
>     {
>       "serial": "SAMSUNG_123456789",
>       "bus-type": "sata",
>       "bus": 0,
>       "unit": 0,
>       "pci-controller": {
>         "bus": 0,
>         "slot": 31,
>         "domain": 0,
>         "function": 2
>       },
>       "dev": "/dev/sda",
>       "target": 0
>     },
>     ...
>   ]
> }
> 
> Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
> ---
>  qga/commands-posix.c | 91 +++++++++++++++++++++++++++++++++++++++++++-
>  qga/commands-win32.c | 83 ++++++++++++++++++++++++++++++++++++++++
>  qga/qapi-schema.json | 13 +++++++
>  3 files changed, 186 insertions(+), 1 deletion(-)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 744c2b5a5d..4cebec96a6 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -62,6 +62,8 @@ extern char **environ;
>  #endif
>  #endif
> 
> +G_DEFINE_AUTOPTR_CLEANUP_FUNC(GuestFilesystemInfo, qapi_free_GuestFilesystemInfo)
> +
>  static void ga_wait_child(pid_t pid, int *status, Error **errp)
>  {
>      pid_t rpid;
> @@ -1177,6 +1179,92 @@ static void build_guest_fsinfo_for_device(char const *devpath,
>      free(syspath);
>  }
> 
> +GuestDiskAddressList *qmp_guest_get_disks(Error **errp)
> +{
> +    GuestDiskAddressList *item, *ret = NULL;
> +    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, *slaves_dir = NULL;
> +        g_autoptr(GuestFilesystemInfo) fs = NULL;
> +        uint64_t slaves = 0;
> +        struct dirent *de_slaves;
> +        DIR *dp_slaves = NULL;
> +        FILE *fp = NULL;
> +        size_t n;
> +        Error *local_err = NULL;
> +        if (de->d_type != DT_LNK) {
> +            g_debug("  skipping entry: %s", de->d_name);
> +            continue;
> +        }
> +
> +        slaves_dir = g_strdup_printf("/sys/block/%s/slaves", de->d_name);
> +        if (slaves_dir == NULL) {
> +            g_debug("  failed to open directory %s", slaves_dir);
> +            continue;
> +        }
> +        g_debug("  counting entries in: %s", slaves_dir);
> +        dp_slaves = opendir(slaves_dir);
> +        while ((de_slaves = readdir(dp_slaves)) != NULL) {
> +            if ((strcmp(".", de_slaves->d_name) == 0) ||
> +                (strcmp("..", de_slaves->d_name) == 0)) {
> +                continue;
> +            }
> +            slaves++;
> +        }
> +        closedir(dp_slaves);
> +        g_debug("    counted %lu items", slaves);
> +        if (slaves != 0) {
> +            continue;
> +        }

For guest-get-fsinfo we skip returning any data about virtual devices
(dm/md/etc.) and just return the physical disks underlying whatever
hierarchy the filesystem is built on, which sort of makes sense as far
as making sure we know what devices are currently in use.

But for guest-get-disks we seem to care about usage beyond just mounted
filesystems, which to me would suggest things like whether a disk is
part of an LVM/RAID/multipath volume. But by skipping virtual devices
with "/sys/block/*/slaves" entries we lose that information...

So I guess I'm not sure I understand the use-cases you have in mind here.
Can you provide some examples? And do you anticipate we would need to add
an interface to report this hierarchy at some point?

> +
> +        g_debug("  checking disk size");
> +        size_path = g_strdup_printf("/sys/block/%s/size", de->d_name);
> +        fp = fopen(size_path, "r");
> +        if (!fp) {
> +            g_debug("  failed to read disk size");
> +            continue;
> +        }
> +        if (getline(&line, &n, fp) == -1) {

We need to set n=0, otherwise an uninitialized value might be interpreted as
the size of line's pre-allocated buffer

> +            g_debug("  failed to read disk size");
> +            fclose(fp);
> +            continue;
> +        }
> +        fclose(fp);
> +        if (strcmp(line, "0\n") == 0) {
> +            g_debug("  skipping zero-sized disk");
> +            continue;
> +        }
> +
> +        fs = g_malloc0(sizeof(*fs));
> +        g_debug("  adding %s", de->d_name);
> +        disk_dir = g_strdup_printf("/sys/block/%s", de->d_name);
> +        build_guest_fsinfo_for_device(disk_dir, fs, &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);
> +            continue;
> +        } else if (fs->disk == NULL) {
> +            g_debug("  skipping unknown disk");
> +            continue;
> +        }
> +        item = g_steal_pointer(&fs->disk);

Does this ensure that fs itself doesn't still get auto-free'd? In any
case, it seems awkward to allocate a GuestFilesystemInfo just so we can
re-use build_guest_fsinfo_for_device(). Can code be refactored into a
separate build_guest_disk_info_for_device() or something that just takes
a GuestDiskAddressList* directly?

> +        g_assert(item->next == NULL); /* expecting just a single disk */
> +        item->next = ret;
> +        ret = item;
> +    }
> +    return ret;
> +}
> +
>  /* 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 +2897,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) {
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index aaa71f147b..0bafa2dc4c 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -945,6 +945,83 @@ out:
>      return list;
>  }
> 
> +GuestDiskAddressList *qmp_guest_get_disks(Error **errp)
> +{
> +    GuestDiskAddressList *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 *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_malloc(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);
> +
> +        g_debug("  number: %lu", sdn.DeviceNumber);
> +        disk = g_malloc0(sizeof(GuestDiskAddress));
> +        disk->has_dev = true;
> +        disk->dev = g_strdup_printf("\\\\.\\PhysicalDrive%lu",
> +            sdn.DeviceNumber);
> +        get_single_disk_info(sdn.DeviceNumber, disk, &local_err);
> +        if (local_err) {
> +            g_debug("failed to get disk info: %s",
> +                error_get_pretty(local_err));
> +            error_free(local_err);
> +            g_free(disk);
> +            continue;
> +        }
> +        new = g_malloc0(sizeof(GuestDiskAddressList));
> +        new->value = disk;
> +        new->next = ret;
> +        ret = new;
> +    }
> +
> +    SetupDiDestroyDeviceInfoList(dev_info);
> +    return ret;
> +}
> +
>  #else
> 
>  static GuestDiskAddressList *build_guest_disk_info(char *guid, Error **errp)
> @@ -952,6 +1029,12 @@ static GuestDiskAddressList *build_guest_disk_info(char *guid, Error **errp)
>      return NULL;
>  }
> 
> +GuestDiskAddressList *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)
> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> index 408a662ea5..5d8fa1c283 100644
> --- a/qga/qapi-schema.json
> +++ b/qga/qapi-schema.json
> @@ -862,6 +862,19 @@
>             'bus': 'int', 'target': 'int', 'unit': 'int',
>             '*serial': 'str', '*dev': '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': ['GuestDiskAddress'] }
> +
>  ##
>  # @GuestFilesystemInfo:
>  #
> -- 
> 2.25.0
> 

Re: [PATCH 1/1] qga: add command guest-get-disks

[Michael Roth]

Quoting Philippe Mathieu-Daudé (2020-08-06 05:44:44)
> On 8/6/20 11:03 AM, Tomáš Golembiovský wrote:
> > 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 allows listing information about attached root disks of the VM. This is
> > usefull for management applications for mapping virtualized devices or
> > pass-through devices to device names in the guest OS.
> > 
> > Output is similar to the list of partitions of guest-get-fsinfo, except that
> > the disks are mapped instead of partitions.
> > 
> > Example output:
> > 
> > {
> >   "return": [
> >     {
> >       "serial": "SAMSUNG_123456789",
> >       "bus-type": "sata",
> >       "bus": 0,
> >       "unit": 0,
> >       "pci-controller": {
> >         "bus": 0,
> >         "slot": 31,
> >         "domain": 0,
> >         "function": 2
> >       },
> >       "dev": "/dev/sda",
> >       "target": 0
> >     },
> >     ...
> >   ]
> > }
> > 
> > Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
> > ---
> >  qga/commands-posix.c | 91 +++++++++++++++++++++++++++++++++++++++++++-
> >  qga/commands-win32.c | 83 ++++++++++++++++++++++++++++++++++++++++
> >  qga/qapi-schema.json | 13 +++++++
> >  3 files changed, 186 insertions(+), 1 deletion(-)
> 
> Not sure this is better, but splitting this in 3 could help different
> people reviewing (developers not familiar with the Windows API might
> feel not comfortable adding a R-b tag for the POSIX part, and
> reciprocally).
> 
> 1/ qapi + stubs
> 2/ POSIX implementation
> 3/ Win32 implementation
> 

+1

Re: [PATCH 1/1] qga: add command guest-get-disks

[Tomáš Golembiovský]

On Wed, 12 Aug 2020 16:29:38 -0500
Michael Roth <mdroth@linux.vnet.ibm.com> wrote:

> Quoting Tomáš Golembiovský (2020-08-06 04:03:06)
> > 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 allows listing information about attached root disks of the VM. This is
> > usefull for management applications for mapping virtualized devices or
> > pass-through devices to device names in the guest OS.
> > 
> > Output is similar to the list of partitions of guest-get-fsinfo, except that
> > the disks are mapped instead of partitions.
> > 
> > Example output:
> > 
> > {
> >   "return": [
> >     {
> >       "serial": "SAMSUNG_123456789",
> >       "bus-type": "sata",
> >       "bus": 0,
> >       "unit": 0,
> >       "pci-controller": {
> >         "bus": 0,
> >         "slot": 31,
> >         "domain": 0,
> >         "function": 2
> >       },
> >       "dev": "/dev/sda",
> >       "target": 0
> >     },
> >     ...
> >   ]
> > }
> > 
> > Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
> > ---
> >  qga/commands-posix.c | 91 +++++++++++++++++++++++++++++++++++++++++++-
> >  qga/commands-win32.c | 83 ++++++++++++++++++++++++++++++++++++++++
> >  qga/qapi-schema.json | 13 +++++++
> >  3 files changed, 186 insertions(+), 1 deletion(-)
> > 
> > diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> > index 744c2b5a5d..4cebec96a6 100644
> > --- a/qga/commands-posix.c
> > +++ b/qga/commands-posix.c
> > @@ -62,6 +62,8 @@ extern char **environ;
> >  #endif
> >  #endif
> > 
> > +G_DEFINE_AUTOPTR_CLEANUP_FUNC(GuestFilesystemInfo, qapi_free_GuestFilesystemInfo)
> > +
> >  static void ga_wait_child(pid_t pid, int *status, Error **errp)
> >  {
> >      pid_t rpid;
> > @@ -1177,6 +1179,92 @@ static void build_guest_fsinfo_for_device(char const *devpath,
> >      free(syspath);
> >  }
> > 
> > +GuestDiskAddressList *qmp_guest_get_disks(Error **errp)
> > +{
> > +    GuestDiskAddressList *item, *ret = NULL;
> > +    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, *slaves_dir = NULL;
> > +        g_autoptr(GuestFilesystemInfo) fs = NULL;
> > +        uint64_t slaves = 0;
> > +        struct dirent *de_slaves;
> > +        DIR *dp_slaves = NULL;
> > +        FILE *fp = NULL;
> > +        size_t n;
> > +        Error *local_err = NULL;
> > +        if (de->d_type != DT_LNK) {
> > +            g_debug("  skipping entry: %s", de->d_name);
> > +            continue;
> > +        }
> > +
> > +        slaves_dir = g_strdup_printf("/sys/block/%s/slaves", de->d_name);
> > +        if (slaves_dir == NULL) {
> > +            g_debug("  failed to open directory %s", slaves_dir);
> > +            continue;
> > +        }
> > +        g_debug("  counting entries in: %s", slaves_dir);
> > +        dp_slaves = opendir(slaves_dir);
> > +        while ((de_slaves = readdir(dp_slaves)) != NULL) {
> > +            if ((strcmp(".", de_slaves->d_name) == 0) ||
> > +                (strcmp("..", de_slaves->d_name) == 0)) {
> > +                continue;
> > +            }
> > +            slaves++;
> > +        }
> > +        closedir(dp_slaves);
> > +        g_debug("    counted %lu items", slaves);
> > +        if (slaves != 0) {
> > +            continue;
> > +        }
> 
> For guest-get-fsinfo we skip returning any data about virtual devices
> (dm/md/etc.) and just return the physical disks underlying whatever
> hierarchy the filesystem is built on, which sort of makes sense as far
> as making sure we know what devices are currently in use.
> 
> But for guest-get-disks we seem to care about usage beyond just mounted
> filesystems, which to me would suggest things like whether a disk is
> part of an LVM/RAID/multipath volume. But by skipping virtual devices
> with "/sys/block/*/slaves" entries we lose that information...
> 
> So I guess I'm not sure I understand the use-cases you have in mind here.
> Can you provide some examples? And do you anticipate we would need to add
> an interface to report this hierarchy at some point?

The intent is to provide information about devices that were specified
when starting the VM and not about complete disk hierarchy. The
reasoning behind it is for management applications to be able to show
what names in guest OS correspond to the disks in the VM definition. But
if you suggest that reporting the hierarchy similar to what lsblk
provides would be useful I can do that for Linux. The issue is with
Windows where I don't know how to do that. 

> 
> > +
> > +        g_debug("  checking disk size");
> > +        size_path = g_strdup_printf("/sys/block/%s/size", de->d_name);
> > +        fp = fopen(size_path, "r");
> > +        if (!fp) {
> > +            g_debug("  failed to read disk size");
> > +            continue;
> > +        }
> > +        if (getline(&line, &n, fp) == -1) {
> 
> We need to set n=0, otherwise an uninitialized value might be interpreted as
> the size of line's pre-allocated buffer
> 
> > +            g_debug("  failed to read disk size");
> > +            fclose(fp);
> > +            continue;
> > +        }
> > +        fclose(fp);
> > +        if (strcmp(line, "0\n") == 0) {
> > +            g_debug("  skipping zero-sized disk");
> > +            continue;
> > +        }
> > +
> > +        fs = g_malloc0(sizeof(*fs));
> > +        g_debug("  adding %s", de->d_name);
> > +        disk_dir = g_strdup_printf("/sys/block/%s", de->d_name);
> > +        build_guest_fsinfo_for_device(disk_dir, fs, &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);
> > +            continue;
> > +        } else if (fs->disk == NULL) {
> > +            g_debug("  skipping unknown disk");
> > +            continue;
> > +        }
> > +        item = g_steal_pointer(&fs->disk);
> 
> Does this ensure that fs itself doesn't still get auto-free'd?

Yes, it does.


> In any
> case, it seems awkward to allocate a GuestFilesystemInfo just so we can
> re-use build_guest_fsinfo_for_device(). Can code be refactored into a
> separate build_guest_disk_info_for_device() or something that just takes
> a GuestDiskAddressList* directly?
> 

It is little bit cumbersome, I agree. Unfortunately refactoring it is
little bit tricky and would make the build_guestfs_info_*() functions
less readable. But moving the code into separate function to hide this
is certainly an option.

    Tomas

> > +        g_assert(item->next == NULL); /* expecting just a single disk */
> > +        item->next = ret;
> > +        ret = item;
> > +    }
> > +    return ret;
> > +}
> > +
> >  /* 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 +2897,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) {
> > diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> > index aaa71f147b..0bafa2dc4c 100644
> > --- a/qga/commands-win32.c
> > +++ b/qga/commands-win32.c
> > @@ -945,6 +945,83 @@ out:
> >      return list;
> >  }
> > 
> > +GuestDiskAddressList *qmp_guest_get_disks(Error **errp)
> > +{
> > +    GuestDiskAddressList *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 *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_malloc(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);
> > +
> > +        g_debug("  number: %lu", sdn.DeviceNumber);
> > +        disk = g_malloc0(sizeof(GuestDiskAddress));
> > +        disk->has_dev = true;
> > +        disk->dev = g_strdup_printf("\\\\.\\PhysicalDrive%lu",
> > +            sdn.DeviceNumber);
> > +        get_single_disk_info(sdn.DeviceNumber, disk, &local_err);
> > +        if (local_err) {
> > +            g_debug("failed to get disk info: %s",
> > +                error_get_pretty(local_err));
> > +            error_free(local_err);
> > +            g_free(disk);
> > +            continue;
> > +        }
> > +        new = g_malloc0(sizeof(GuestDiskAddressList));
> > +        new->value = disk;
> > +        new->next = ret;
> > +        ret = new;
> > +    }
> > +
> > +    SetupDiDestroyDeviceInfoList(dev_info);
> > +    return ret;
> > +}
> > +
> >  #else
> > 
> >  static GuestDiskAddressList *build_guest_disk_info(char *guid, Error **errp)
> > @@ -952,6 +1029,12 @@ static GuestDiskAddressList *build_guest_disk_info(char *guid, Error **errp)
> >      return NULL;
> >  }
> > 
> > +GuestDiskAddressList *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)
> > diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> > index 408a662ea5..5d8fa1c283 100644
> > --- a/qga/qapi-schema.json
> > +++ b/qga/qapi-schema.json
> > @@ -862,6 +862,19 @@
> >             'bus': 'int', 'target': 'int', 'unit': 'int',
> >             '*serial': 'str', '*dev': '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': ['GuestDiskAddress'] }
> > +
> >  ##
> >  # @GuestFilesystemInfo:
> >  #
> > -- 
> > 2.25.0
> > 
> 


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