Re: [PULL v3 06/12] qga: add implementation of guest-get-disks for Linux

[Peter Maydell]

On Tue, 3 Nov 2020 at 02:45, Michael Roth  wrote:
>
> From: Tomáš Golembiovský 
>
> 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.

Hi; Coverity points out a resource leak in this function
(CID 1436130):


> +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");

Here we opendir()...

> +if (dp == NULL) {
> +error_setg_errno(errp, errno, "Can't open directory \"/sys/block\"");
> +return NULL;
> +}
> +while ((de = readdir(dp)) != NULL) {
[stuff]
> +}
> +return ret;

...but we forget to closedir() it again.

> +}

thanks
-- PMM

[PULL v3 06/12] qga: add implementation of guest-get-disks for Linux

[Michael Roth]

From: Tomáš Golembiovský 

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ý 
Reviewed-by: Marc-André Lureau 
*add missing stub for !defined(CONFIG_FSFREEZE)
*remove unused deps_dir variable
Signed-off-by: Michael Roth 
---
 qga/commands-posix.c | 303 +--
 1 file changed, 292 insertions(+), 11 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 422144bcff..3711080d07 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;
 
-free(syspath);
+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)