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
>