Re: [PATCH v4 2/3] qga: add implementation of guest-get-disks for Linux

[Tomáš Golembiovský]

On Tue, Oct 13, 2020 at 11:26:45AM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Mon, Oct 12, 2020 at 12:36 PM 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 | 296 +++++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 285 insertions(+), 11 deletions(-)
> >
> > diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> > index 422144bcff..14683dfbd5 100644
> > --- a/qga/commands-posix.c
> > +++ b/qga/commands-posix.c
> > @@ -1150,13 +1150,27 @@ 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)
> >  {
> > -    char *syspath = realpath(devpath, NULL);
> > +    ERRP_GUARD();
> > +    g_autofree char *syspath = NULL;
> > +    bool is_virtual = false;
> >
> > +    syspath = realpath(devpath, NULL);
> >      if (!syspath) {
> >          error_setg_errno(errp, errno, "realpath(\"%s\")", devpath);
> >          return;
> > @@ -1167,16 +1181,281 @@ 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);
> >      }
> > +}
> > +
> > +#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;
> >
> > -    free(syspath);
> > +        /* 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);
> >
> 
> Any reason to use both readdir and g_dir APIs in the same patch? Maybe use
> g_dir alone?

g_dir_* provides only names of the entries in the other places where
opendir() is used we check also type of the entry (d_type).

Tomas

> 
> +    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 +3088,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 +3331,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
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> -- 
> Marc-André Lureau

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