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

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

[Tomáš Golembiovský]

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.

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 | 241 ++++++++++++++++++++++++++++++++++++++++++-
 qga/commands-win32.c |  91 ++++++++++++++++
 qga/qapi-schema.json |  29 ++++++
 3 files changed, 360 insertions(+), 1 deletion(-)

-- 
2.25.0

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

[Tomáš Golembiovský]

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>
---
 qga/commands-posix.c |  6 ++++++
 qga/commands-win32.c |  6 ++++++
 qga/qapi-schema.json | 29 +++++++++++++++++++++++++++++
 3 files changed, 41 insertions(+)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 744c2b5a5d..f99731af51 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -3042,3 +3042,9 @@ GuestOSInfo *qmp_guest_get_osinfo(Error **errp)
 
     return info;
 }
+
+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 aaa71f147b..e9976a0c46 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -2229,3 +2229,9 @@ GuestOSInfo *qmp_guest_get_osinfo(Error **errp)
 
     return info;
 }
+
+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 408a662ea5..70b54e0d07 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -862,6 +862,35 @@
            '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
+# @slaves: list of slave devices (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', 'slaves': ['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.25.0

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

[Philippe Mathieu-Daudé]

On 9/7/20 11:14 AM, Tomáš Golembiovský wrote:
> 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>
> ---
>  qga/commands-posix.c |  6 ++++++
>  qga/commands-win32.c |  6 ++++++
>  qga/qapi-schema.json | 29 +++++++++++++++++++++++++++++
>  3 files changed, 41 insertions(+)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

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

[Marc-André Lureau]

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

On Mon, Sep 7, 2020 at 1:16 PM Tomáš Golembiovský <tgolembi@redhat.com>
wrote:

> 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: Marc-André Lureau <marcandre.lureau@redhat.com>

---
>  qga/commands-posix.c |  6 ++++++
>  qga/commands-win32.c |  6 ++++++
>  qga/qapi-schema.json | 29 +++++++++++++++++++++++++++++
>  3 files changed, 41 insertions(+)
>
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 744c2b5a5d..f99731af51 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -3042,3 +3042,9 @@ GuestOSInfo *qmp_guest_get_osinfo(Error **errp)
>
>      return info;
>  }
> +
> +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 aaa71f147b..e9976a0c46 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -2229,3 +2229,9 @@ GuestOSInfo *qmp_guest_get_osinfo(Error **errp)
>
>      return info;
>  }
> +
> +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 408a662ea5..70b54e0d07 100644
> --- a/qga/qapi-schema.json
> +++ b/qga/qapi-schema.json
> @@ -862,6 +862,35 @@
>             '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
> +# @slaves: list of slave devices (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', 'slaves': ['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.25.0
>
>
>

-- 
Marc-André Lureau

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

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

[Daniel P. Berrangé]

On Mon, Sep 07, 2020 at 11:14:40AM +0200, Tomáš Golembiovský wrote:
> 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>
> ---
>  qga/commands-posix.c |  6 ++++++
>  qga/commands-win32.c |  6 ++++++
>  qga/qapi-schema.json | 29 +++++++++++++++++++++++++++++
>  3 files changed, 41 insertions(+)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 744c2b5a5d..f99731af51 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -3042,3 +3042,9 @@ GuestOSInfo *qmp_guest_get_osinfo(Error **errp)
>  
>      return info;
>  }
> +
> +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 aaa71f147b..e9976a0c46 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -2229,3 +2229,9 @@ GuestOSInfo *qmp_guest_get_osinfo(Error **errp)
>  
>      return info;
>  }
> +
> +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 408a662ea5..70b54e0d07 100644
> --- a/qga/qapi-schema.json
> +++ b/qga/qapi-schema.json
> @@ -862,6 +862,35 @@
>             '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
> +# @slaves: list of slave devices (Linux)

What are "slave devices" ?

For that matter, please don't use the term "slaves" at all in any
new code.

> +# @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', 'slaves': ['str'],
> +           '*address': 'GuestDiskAddress', '*alias': 'str'} }

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[Tomáš Golembiovský]

On Tue, Oct 06, 2020 at 09:36:32AM +0100, Daniel P. Berrangé wrote:
> On Mon, Sep 07, 2020 at 11:14:40AM +0200, Tomáš Golembiovský wrote:
> > 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>
> > ---
> >  qga/commands-posix.c |  6 ++++++
> >  qga/commands-win32.c |  6 ++++++
> >  qga/qapi-schema.json | 29 +++++++++++++++++++++++++++++
> >  3 files changed, 41 insertions(+)
> > 
> > diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> > index 744c2b5a5d..f99731af51 100644
> > --- a/qga/commands-posix.c
> > +++ b/qga/commands-posix.c
> > @@ -3042,3 +3042,9 @@ GuestOSInfo *qmp_guest_get_osinfo(Error **errp)
> >  
> >      return info;
> >  }
> > +
> > +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 aaa71f147b..e9976a0c46 100644
> > --- a/qga/commands-win32.c
> > +++ b/qga/commands-win32.c
> > @@ -2229,3 +2229,9 @@ GuestOSInfo *qmp_guest_get_osinfo(Error **errp)
> >  
> >      return info;
> >  }
> > +
> > +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 408a662ea5..70b54e0d07 100644
> > --- a/qga/qapi-schema.json
> > +++ b/qga/qapi-schema.json
> > @@ -862,6 +862,35 @@
> >             '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
> > +# @slaves: list of slave devices (Linux)
> 
> What are "slave devices" ?

That is how Linux calls dependent devices. E.g. PVs in your LVM setup
are considered "slaves" to your LVs. Or if you have LUKS volume called
"foo" on your "bar" drive then "bar" would be listed as a "slave" for
"foo".

The dependency is in the opposite direction then I have always pictured
it in my had, but I guess that there are reasons for that.

> For that matter, please don't use the term "slaves" at all in any
> new code.

Fair enough... so how shall we call those devices? Dependents,
dependencies, parents... ? Ideas welcome.

    Tomas

> 
> > +# @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', 'slaves': ['str'],
> > +           '*address': 'GuestDiskAddress', '*alias': 'str'} }
> 
> Regards,
> Daniel
> -- 
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
> 
> 

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

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

[Daniel P. Berrangé]

On Tue, Oct 06, 2020 at 03:53:08PM +0200, Tomáš Golembiovský wrote:
> On Tue, Oct 06, 2020 at 09:36:32AM +0100, Daniel P. Berrangé wrote:
> > On Mon, Sep 07, 2020 at 11:14:40AM +0200, Tomáš Golembiovský wrote:
> > > 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>
> > > ---
> > >  qga/commands-posix.c |  6 ++++++
> > >  qga/commands-win32.c |  6 ++++++
> > >  qga/qapi-schema.json | 29 +++++++++++++++++++++++++++++
> > >  3 files changed, 41 insertions(+)
> > > 
> > > diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> > > index 744c2b5a5d..f99731af51 100644
> > > --- a/qga/commands-posix.c
> > > +++ b/qga/commands-posix.c
> > > @@ -3042,3 +3042,9 @@ GuestOSInfo *qmp_guest_get_osinfo(Error **errp)
> > >  
> > >      return info;
> > >  }
> > > +
> > > +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 aaa71f147b..e9976a0c46 100644
> > > --- a/qga/commands-win32.c
> > > +++ b/qga/commands-win32.c
> > > @@ -2229,3 +2229,9 @@ GuestOSInfo *qmp_guest_get_osinfo(Error **errp)
> > >  
> > >      return info;
> > >  }
> > > +
> > > +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 408a662ea5..70b54e0d07 100644
> > > --- a/qga/qapi-schema.json
> > > +++ b/qga/qapi-schema.json
> > > @@ -862,6 +862,35 @@
> > >             '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
> > > +# @slaves: list of slave devices (Linux)
> > 
> > What are "slave devices" ?
> 
> That is how Linux calls dependent devices. E.g. PVs in your LVM setup
> are considered "slaves" to your LVs. Or if you have LUKS volume called
> "foo" on your "bar" drive then "bar" would be listed as a "slave" for
> "foo".
> 
> The dependency is in the opposite direction then I have always pictured
> it in my had, but I guess that there are reasons for that.
> 
> > For that matter, please don't use the term "slaves" at all in any
> > new code.
> 
> Fair enough... so how shall we call those devices? Dependents,
> dependencies, parents... ? Ideas welcome.

Sounds like "dependents" sounds like a reasonable term for what
this is expressing.

Giving the LVM VG/PV and LUKS examples in the QAPI docs would be
useful.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[Tomáš Golembiovský]

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

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

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

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index f99731af51..3babc25c09 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -62,6 +62,9 @@ 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;
@@ -1150,6 +1153,21 @@ 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;
+    }
+    if (strstr(syspath, "/devices/virtual/block/")) {
+        return true;
+    } else {
+        return false;
+    }
+}
+
 /* Dispatch to functions for virtual/real device */
 static void build_guest_fsinfo_for_device(char const *devpath,
                                           GuestFilesystemInfo *fs,
@@ -1168,6 +1186,7 @@ static void build_guest_fsinfo_for_device(char const *devpath,
 
     g_debug("  parse sysfs path '%s'", syspath);
 
+    /* TODO: use is_disk_virtual() */
     if (strstr(syspath, "/devices/virtual/block/")) {
         build_guest_fsinfo_for_virtual_device(syspath, fs, errp);
     } else {
@@ -1177,6 +1196,225 @@ 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();
+    udevice = udev_device_new_from_syspath(udev, syspath);
+    if (udev == NULL || udevice == NULL) {
+        g_debug("failed to query udev");
+    } else {
+        const char *alias = udev_device_get_property_value(
+            udevice, "DM_NAME");
+        if (alias != NULL && *alias != 0) {
+            ret = g_strdup(alias);
+        }
+    }
+
+    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();
+    udevice = udev_device_new_from_syspath(udev, syspath);
+    if (udev == NULL || udevice == NULL) {
+        g_debug("failed to query udev");
+    } else {
+        ret = g_strdup(udev_device_get_devnode(udevice));
+    }
+    udev_unref(udev);
+    udev_device_unref(udevice);
+    return ret;
+}
+
+GuestDiskInfoList *qmp_guest_get_disks(Error **errp)
+{
+    GuestDiskInfoList *item, *ret = NULL;
+    GuestDiskInfo *disk, *partition;
+    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_dir = NULL, *slaves_dir = NULL;
+        struct dirent *de_disk, *de_slaves;
+        DIR *dp_disk = NULL, *dp_slaves = NULL;
+        FILE *fp = NULL;
+        size_t n = 0;
+        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_dir = g_strdup_printf("/sys/block/%s/size", de->d_name);
+        fp = fopen(size_dir, "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;
+        }
+
+        g_debug("  adding %s", de->d_name);
+        disk_dir = g_strdup_printf("/sys/block/%s", de->d_name);
+        disk = g_new0(GuestDiskInfo, 1);
+        disk->name = get_device_for_syspath(disk_dir);
+        disk->partition = false;
+        disk->alias = get_alias_for_syspath(disk_dir);
+        if (disk->alias != NULL) {
+            disk->has_alias = true;
+        }
+        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;
+            }
+        }
+
+        /* List slave disks */
+        slaves_dir = g_strdup_printf("%s/slaves", disk_dir);
+        g_debug("  listing entries in: %s", slaves_dir);
+        dp_slaves = opendir(slaves_dir);
+        while ((de_slaves = readdir(dp_slaves)) != NULL) {
+            g_autofree char *slave_dir = NULL;
+            char *slave_dev;
+            strList *slave_item = NULL;
+
+            if ((strcmp(".", de_slaves->d_name) == 0) ||
+                (strcmp("..", de_slaves->d_name) == 0)) {
+                continue;
+            }
+
+            /* Add slave disks */
+            slave_dir = g_strdup_printf("%s/%s",
+                slaves_dir, de_slaves->d_name);
+            slave_dev = get_device_for_syspath(slave_dir);
+            if (slave_dev != NULL) {
+                g_debug("  adding slave device: %s", slave_dev);
+                slave_item = g_new0(strList, 1);
+                slave_item->value = slave_dev;
+                slave_item->next = disk->slaves;
+                disk->slaves = slave_item;
+            }
+        }
+        closedir(dp_slaves);
+
+        /*
+         * Detect partitions subdirectory name is "<parent><number>" or
+         * "<parent>p<number>"
+         */
+        dp_disk = opendir(disk_dir);
+        while ((de_disk = readdir(dp_disk)) != NULL) {
+            size_t len;
+            g_autofree char *partition_dir = NULL;
+
+            if (!(de_disk->d_type & DT_DIR)) {
+                continue;
+            }
+
+            len = strlen(de->d_name);
+            if (!(strncmp(de->d_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);
+            partition = g_new0(GuestDiskInfo, 1);
+            partition->name = get_device_for_syspath(partition_dir);
+            partition->partition = true;
+            /* Add parent disk as slave for easier tracking of hierarchy */
+            partition->slaves = g_new0(strList, 1);
+            partition->slaves->value = g_strdup(disk->name);
+
+            item = g_new0(GuestDiskInfoList, 1);
+            item->value = partition;
+            item->next = ret;
+            ret = item;
+
+        }
+        closedir(dp_disk);
+    }
+    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 +3047,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) {
@@ -3042,9 +3281,3 @@ GuestOSInfo *qmp_guest_get_osinfo(Error **errp)
 
     return info;
 }
-
-GuestDiskInfoList *qmp_guest_get_disks(Error **errp)
-{
-    error_setg(errp, QERR_UNSUPPORTED);
-    return NULL;
-}
-- 
2.25.0

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

[Marc-André Lureau]

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

Hi

On Mon, Sep 7, 2020 at 1:17 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 slave disks is also listed and
> /dev path is used as a handle so it can be matched with "name" filed of
>

field

other returned disk entries. For disk partitions the "slave" list is
> populated with the the parent device for easier tracking of hierarchy.
>
> Example output:
> {
>   "return": [
>     ...
>     {
>       "name": "/dev/dm-0",
>       "partition": false,
>       "slaves": [
>         "/dev/sda2"
>       ],
>       "alias": "luks-7062202e-5b9b-433e-81e8-6628c40da9f7"
>     },
>     {
>       "name": "/dev/sda2",
>       "partition": true,
>       "slaves": [
>         "/dev/sda"
>       ]
>     },
>     {
>       "name": "/dev/sda",
>       "partition": false,
>       "address": {
>         "serial": "SAMSUNG_MZ7LN512HCHP-000L1_S1ZKNXAG822493",
>         "bus-type": "sata",
>         ...
>         "dev": "/dev/sda",
>         "target": 0
>       },
>       "slaves": []
>     },
>     ...
>   ]
> }
>
> Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
> ---
>  qga/commands-posix.c | 247 +++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 240 insertions(+), 7 deletions(-)
>
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index f99731af51..3babc25c09 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -62,6 +62,9 @@ extern char **environ;
>  #endif
>  #endif
>
> +G_DEFINE_AUTOPTR_CLEANUP_FUNC(GuestFilesystemInfo,
> +    qapi_free_GuestFilesystemInfo)
> +
>

This will now conflict with qapi-gen generated headers.

 static void ga_wait_child(pid_t pid, int *status, Error **errp)
>  {
>      pid_t rpid;
> @@ -1150,6 +1153,21 @@ 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;
> +    }
> +    if (strstr(syspath, "/devices/virtual/block/")) {
> +        return true;
> +    } else {
> +        return false;
> +    }
>

 simply to "return strstr(syspath, "/devices/virtual/block/") != NULL;" ?
(Or strstr(syspath, "/devices/virtual/block/") ? true : false )

+}
> +
>  /* Dispatch to functions for virtual/real device */
>  static void build_guest_fsinfo_for_device(char const *devpath,
>                                            GuestFilesystemInfo *fs,
> @@ -1168,6 +1186,7 @@ static void build_guest_fsinfo_for_device(char const
> *devpath,
>
>      g_debug("  parse sysfs path '%s'", syspath);
>
> +    /* TODO: use is_disk_virtual() */
>

just do it, no?

     if (strstr(syspath, "/devices/virtual/block/")) {
>          build_guest_fsinfo_for_virtual_device(syspath, fs, errp);
>      } else {
> @@ -1177,6 +1196,225 @@ 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);
>

Heap allocation / auto wasn't really necessary here, but ok.


> +    build_guest_fsinfo_for_device(syspath, fs, errp);
> +    if (fs->disk != NULL) {
> +        return g_steal_pointer(&fs->disk->value);
> +    }
> +    return NULL;
>

Could also be a onliner, but perhaps less readable.

+}
> +
> +static char *get_alias_for_syspath(const char *syspath)
> +{
> +    struct udev *udev = NULL;
> +    struct udev_device *udevice = NULL;
> +    char *ret = NULL;
> +
> +    udev = udev_new();
>

I would have g_return_val_if_fail(udev != NULL, NULL); here as,

+    udevice = udev_device_new_from_syspath(udev, syspath);
>

udev_device_new_from_syspath() might crash otherwise.


> +    if (udev == NULL || udevice == NULL) {
> +        g_debug("failed to query udev");
> +    } else {
> +        const char *alias = udev_device_get_property_value(
> +            udevice, "DM_NAME");
> +        if (alias != NULL && *alias != 0) {
> +            ret = g_strdup(alias);
>

g_strdup() works fine with NULL. Is there "" empty aliases? Why not report
them too?

+        }
> +    }
> +
> +    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();
> +    udevice = udev_device_new_from_syspath(udev, syspath);
> +    if (udev == NULL || udevice == NULL) {
>

Same as above

+        g_debug("failed to query udev");
> +    } else {
> +        ret = g_strdup(udev_device_get_devnode(udevice));
> +    }
> +    udev_unref(udev);
> +    udev_device_unref(udevice);
> +    return ret;
> +}
> +
>
+GuestDiskInfoList *qmp_guest_get_disks(Error **errp)
> +{
> +    GuestDiskInfoList *item, *ret = NULL;
> +    GuestDiskInfo *disk, *partition;
> +    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_dir = NULL, *slaves_dir = NULL;
> +        struct dirent *de_disk, *de_slaves;
> +        DIR *dp_disk = NULL, *dp_slaves = NULL;
> +        FILE *fp = NULL;
> +        size_t n = 0;
> +        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_dir = g_strdup_printf("/sys/block/%s/size", de->d_name);
> +        fp = fopen(size_dir, "r");
> +        if (!fp) {
> +            g_debug("  failed to read disk size");
> +            continue;
> +        }
> +        if (getline(&line, &n, fp) == -1) {
> +            g_debug("  failed to read disk size");
>

line: getline(3) "This buffer should be freed by the user program even if
getline() failed."

+            fclose(fp);
> +            continue;
> +        }
> +        fclose(fp);
> +        if (strcmp(line, "0\n") == 0) {
>

It would be slightly better to  defend against NULL crash here, with
g_strcmp0()

+            g_debug("  skipping zero-sized disk");
> +            continue;
> +        }
> +
>

line is never freed?

+        g_debug("  adding %s", de->d_name);
> +        disk_dir = g_strdup_printf("/sys/block/%s", de->d_name);
> +        disk = g_new0(GuestDiskInfo, 1);
> +        disk->name = get_device_for_syspath(disk_dir);
> +        disk->partition = false;
> +        disk->alias = get_alias_for_syspath(disk_dir);
> +        if (disk->alias != NULL) {
> +            disk->has_alias = true;
> +        }
>

Could be a single line too

+        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;
> +            }
> +        }
> +
> +        /* List slave disks */
> +        slaves_dir = g_strdup_printf("%s/slaves", disk_dir);
> +        g_debug("  listing entries in: %s", slaves_dir);
> +        dp_slaves = opendir(slaves_dir);
> +        while ((de_slaves = readdir(dp_slaves)) != NULL) {
> +            g_autofree char *slave_dir = NULL;
> +            char *slave_dev;
> +            strList *slave_item = NULL;
> +
> +            if ((strcmp(".", de_slaves->d_name) == 0) ||
> +                (strcmp("..", de_slaves->d_name) == 0)) {
>
+                continue;
> +            }
> +
> +            /* Add slave disks */
> +            slave_dir = g_strdup_printf("%s/%s",
> +                slaves_dir, de_slaves->d_name);
> +            slave_dev = get_device_for_syspath(slave_dir);
> +            if (slave_dev != NULL) {
> +                g_debug("  adding slave device: %s", slave_dev);
> +                slave_item = g_new0(strList, 1);
> +                slave_item->value = slave_dev;
> +                slave_item->next = disk->slaves;
> +                disk->slaves = slave_item;
> +            }
> +        }
> +        closedir(dp_slaves);
> +
> +        /*
> +         * Detect partitions subdirectory name is "<parent><number>" or
> +         * "<parent>p<number>"
> +         */
> +        dp_disk = opendir(disk_dir);
> +        while ((de_disk = readdir(dp_disk)) != NULL) {
> +            size_t len;
> +            g_autofree char *partition_dir = NULL;
> +
> +            if (!(de_disk->d_type & DT_DIR)) {
> +                continue;
> +            }
> +
> +            len = strlen(de->d_name);
> +            if (!(strncmp(de->d_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);
> +            partition = g_new0(GuestDiskInfo, 1);
> +            partition->name = get_device_for_syspath(partition_dir);
> +            partition->partition = true;
> +            /* Add parent disk as slave for easier tracking of hierarchy
> */
> +            partition->slaves = g_new0(strList, 1);
> +            partition->slaves->value = g_strdup(disk->name);
> +
> +            item = g_new0(GuestDiskInfoList, 1);
> +            item->value = partition;
> +            item->next = ret;
> +            ret = item;
> +
> +        }
> +        closedir(dp_disk);
> +    }
> +    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 +3047,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) {
> @@ -3042,9 +3281,3 @@ GuestOSInfo *qmp_guest_get_osinfo(Error **errp)
>
>      return info;
>  }
> -
> -GuestDiskInfoList *qmp_guest_get_disks(Error **errp)
> -{
> -    error_setg(errp, QERR_UNSUPPORTED);
> -    return NULL;
> -}
> --
> 2.25.0
>
>
>

-- 
Marc-André Lureau

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

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

[Tomáš Golembiovský]

On Tue, Sep 29, 2020 at 07:22:00PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Mon, Sep 7, 2020 at 1:17 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 slave disks is also listed and
> > /dev path is used as a handle so it can be matched with "name" filed of
> >
> 
> field
> 
> other returned disk entries. For disk partitions the "slave" list is
> > populated with the the parent device for easier tracking of hierarchy.
> >
> > Example output:
> > {
> >   "return": [
> >     ...
> >     {
> >       "name": "/dev/dm-0",
> >       "partition": false,
> >       "slaves": [
> >         "/dev/sda2"
> >       ],
> >       "alias": "luks-7062202e-5b9b-433e-81e8-6628c40da9f7"
> >     },
> >     {
> >       "name": "/dev/sda2",
> >       "partition": true,
> >       "slaves": [
> >         "/dev/sda"
> >       ]
> >     },
> >     {
> >       "name": "/dev/sda",
> >       "partition": false,
> >       "address": {
> >         "serial": "SAMSUNG_MZ7LN512HCHP-000L1_S1ZKNXAG822493",
> >         "bus-type": "sata",
> >         ...
> >         "dev": "/dev/sda",
> >         "target": 0
> >       },
> >       "slaves": []
> >     },
> >     ...
> >   ]
> > }
> >
> > Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
> > ---
> >  qga/commands-posix.c | 247 +++++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 240 insertions(+), 7 deletions(-)
> >
> > diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> > index f99731af51..3babc25c09 100644
> > --- a/qga/commands-posix.c
> > +++ b/qga/commands-posix.c
> > @@ -62,6 +62,9 @@ extern char **environ;
> >  #endif
> >  #endif
> >
> > +G_DEFINE_AUTOPTR_CLEANUP_FUNC(GuestFilesystemInfo,
> > +    qapi_free_GuestFilesystemInfo)
> > +
> >
> 
> This will now conflict with qapi-gen generated headers.
> 
>  static void ga_wait_child(pid_t pid, int *status, Error **errp)
> >  {
> >      pid_t rpid;
> > @@ -1150,6 +1153,21 @@ 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;
> > +    }
> > +    if (strstr(syspath, "/devices/virtual/block/")) {
> > +        return true;
> > +    } else {
> > +        return false;
> > +    }
> >
> 
>  simply to "return strstr(syspath, "/devices/virtual/block/") != NULL;" ?
> (Or strstr(syspath, "/devices/virtual/block/") ? true : false )
> 
> +}
> > +
> >  /* Dispatch to functions for virtual/real device */
> >  static void build_guest_fsinfo_for_device(char const *devpath,
> >                                            GuestFilesystemInfo *fs,
> > @@ -1168,6 +1186,7 @@ static void build_guest_fsinfo_for_device(char const
> > *devpath,
> >
> >      g_debug("  parse sysfs path '%s'", syspath);
> >
> > +    /* TODO: use is_disk_virtual() */
> >
> 
> just do it, no?

It's great that I put a note there otherwise I might have forgotten to
do it. ;)

> 
>      if (strstr(syspath, "/devices/virtual/block/")) {
> >          build_guest_fsinfo_for_virtual_device(syspath, fs, errp);
> >      } else {
> > @@ -1177,6 +1196,225 @@ 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);
> >
> 
> Heap allocation / auto wasn't really necessary here, but ok.

I used it so that qapi_free_GuestFilesystemInfo() is called on function
exit in all cases. I am not sure if I could do that if `fs` were on the
stack.


> 
> 
> > +    build_guest_fsinfo_for_device(syspath, fs, errp);
> > +    if (fs->disk != NULL) {
> > +        return g_steal_pointer(&fs->disk->value);
> > +    }
> > +    return NULL;
> >
> 
> Could also be a onliner, but perhaps less readable.

Yeah, I prefer it this way.

> 
> +}
> > +
> > +static char *get_alias_for_syspath(const char *syspath)
> > +{
> > +    struct udev *udev = NULL;
> > +    struct udev_device *udevice = NULL;
> > +    char *ret = NULL;
> > +
> > +    udev = udev_new();
> >
> 
> I would have g_return_val_if_fail(udev != NULL, NULL); here as,
> 
> +    udevice = udev_device_new_from_syspath(udev, syspath);
> >
> 
> udev_device_new_from_syspath() might crash otherwise.

That is probably true. This may require fixes at other places too.

> 
> 
> > +    if (udev == NULL || udevice == NULL) {
> > +        g_debug("failed to query udev");
> > +    } else {
> > +        const char *alias = udev_device_get_property_value(
> > +            udevice, "DM_NAME");
> > +        if (alias != NULL && *alias != 0) {
> > +            ret = g_strdup(alias);
> >
> 
> g_strdup() works fine with NULL. Is there "" empty aliases? Why not report
> them too?

NULL means an error and empty alias means there is no alias. I will put
that in a comment there. In QAPI we have alias as optional which seems
preferable rather than always returning empty string.

> 
> +        }
> > +    }
> > +
> > +    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();
> > +    udevice = udev_device_new_from_syspath(udev, syspath);
> > +    if (udev == NULL || udevice == NULL) {
> >
> 
> Same as above
> 
> +        g_debug("failed to query udev");
> > +    } else {
> > +        ret = g_strdup(udev_device_get_devnode(udevice));
> > +    }
> > +    udev_unref(udev);
> > +    udev_device_unref(udevice);
> > +    return ret;
> > +}
> > +
> >
> +GuestDiskInfoList *qmp_guest_get_disks(Error **errp)
> > +{
> > +    GuestDiskInfoList *item, *ret = NULL;
> > +    GuestDiskInfo *disk, *partition;
> > +    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_dir = NULL, *slaves_dir = NULL;
> > +        struct dirent *de_disk, *de_slaves;
> > +        DIR *dp_disk = NULL, *dp_slaves = NULL;
> > +        FILE *fp = NULL;
> > +        size_t n = 0;
> > +        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_dir = g_strdup_printf("/sys/block/%s/size", de->d_name);
> > +        fp = fopen(size_dir, "r");
> > +        if (!fp) {
> > +            g_debug("  failed to read disk size");
> > +            continue;
> > +        }
> > +        if (getline(&line, &n, fp) == -1) {
> > +            g_debug("  failed to read disk size");
> >
> 
> line: getline(3) "This buffer should be freed by the user program even if
> getline() failed."

That is handled by the g_autofree, or am I missing something? `line`
will get out of scope after every interation (even with continue). Or do
you prefer to have it explicit and free as soon as we don't need it?

> 
> +            fclose(fp);
> > +            continue;
> > +        }
> > +        fclose(fp);
> > +        if (strcmp(line, "0\n") == 0) {
> >
> 
> It would be slightly better to  defend against NULL crash here, with
> g_strcmp0()
> 
> +            g_debug("  skipping zero-sized disk");
> > +            continue;
> > +        }
> > +
> >
> 
> line is never freed?
> 
> +        g_debug("  adding %s", de->d_name);
> > +        disk_dir = g_strdup_printf("/sys/block/%s", de->d_name);
> > +        disk = g_new0(GuestDiskInfo, 1);
> > +        disk->name = get_device_for_syspath(disk_dir);
> > +        disk->partition = false;
> > +        disk->alias = get_alias_for_syspath(disk_dir);
> > +        if (disk->alias != NULL) {
> > +            disk->has_alias = true;
> > +        }
> >
> 
> Could be a single line too
> 
> +        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;
> > +            }
> > +        }
> > +
> > +        /* List slave disks */
> > +        slaves_dir = g_strdup_printf("%s/slaves", disk_dir);
> > +        g_debug("  listing entries in: %s", slaves_dir);
> > +        dp_slaves = opendir(slaves_dir);
> > +        while ((de_slaves = readdir(dp_slaves)) != NULL) {
> > +            g_autofree char *slave_dir = NULL;
> > +            char *slave_dev;
> > +            strList *slave_item = NULL;
> > +
> > +            if ((strcmp(".", de_slaves->d_name) == 0) ||
> > +                (strcmp("..", de_slaves->d_name) == 0)) {
> >
> +                continue;
> > +            }
> > +
> > +            /* Add slave disks */
> > +            slave_dir = g_strdup_printf("%s/%s",
> > +                slaves_dir, de_slaves->d_name);
> > +            slave_dev = get_device_for_syspath(slave_dir);
> > +            if (slave_dev != NULL) {
> > +                g_debug("  adding slave device: %s", slave_dev);
> > +                slave_item = g_new0(strList, 1);
> > +                slave_item->value = slave_dev;
> > +                slave_item->next = disk->slaves;
> > +                disk->slaves = slave_item;
> > +            }
> > +        }
> > +        closedir(dp_slaves);
> > +
> > +        /*
> > +         * Detect partitions subdirectory name is "<parent><number>" or
> > +         * "<parent>p<number>"
> > +         */
> > +        dp_disk = opendir(disk_dir);
> > +        while ((de_disk = readdir(dp_disk)) != NULL) {
> > +            size_t len;
> > +            g_autofree char *partition_dir = NULL;
> > +
> > +            if (!(de_disk->d_type & DT_DIR)) {
> > +                continue;
> > +            }
> > +
> > +            len = strlen(de->d_name);
> > +            if (!(strncmp(de->d_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);
> > +            partition = g_new0(GuestDiskInfo, 1);
> > +            partition->name = get_device_for_syspath(partition_dir);
> > +            partition->partition = true;
> > +            /* Add parent disk as slave for easier tracking of hierarchy
> > */
> > +            partition->slaves = g_new0(strList, 1);
> > +            partition->slaves->value = g_strdup(disk->name);
> > +
> > +            item = g_new0(GuestDiskInfoList, 1);
> > +            item->value = partition;
> > +            item->next = ret;
> > +            ret = item;
> > +
> > +        }
> > +        closedir(dp_disk);
> > +    }
> > +    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 +3047,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) {
> > @@ -3042,9 +3281,3 @@ GuestOSInfo *qmp_guest_get_osinfo(Error **errp)
> >
> >      return info;
> >  }
> > -
> > -GuestDiskInfoList *qmp_guest_get_disks(Error **errp)
> > -{
> > -    error_setg(errp, QERR_UNSUPPORTED);
> > -    return NULL;
> > -}
> > --
> > 2.25.0
> >
> >
> >
> 
> -- 
> Marc-André Lureau

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

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

[Marc-André Lureau]

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

Hi

On Tue, Oct 6, 2020 at 12:31 PM Tomáš Golembiovský <tgolembi@redhat.com>
wrote:

> On Tue, Sep 29, 2020 at 07:22:00PM +0400, Marc-André Lureau wrote:
>
> > > +        if (getline(&line, &n, fp) == -1) {
> > > +            g_debug("  failed to read disk size");
> > >
> >
> > line: getline(3) "This buffer should be freed by the user program even if
> > getline() failed."
>
> That is handled by the g_autofree, or am I missing something? `line`
> will get out of scope after every interation (even with continue). Or do
>
>
Ah right, I got confused.

thanks

-- 
Marc-André Lureau

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

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

[Daniel P. Berrangé]

On Mon, Sep 07, 2020 at 11:14:41AM +0200, Tomáš Golembiovský wrote:
> The command lists all disks (real and virtual) as well as disk
> partitions. For each disk the list of slave disks is also listed and
> /dev path is used as a handle so it can be matched with "name" filed of
> other returned disk entries. For disk partitions the "slave" list is
> populated with the the parent device for easier tracking of hierarchy.
> 
> Example output:
> {
>   "return": [
>     ...
>     {
>       "name": "/dev/dm-0",
>       "partition": false,
>       "slaves": [
>         "/dev/sda2"
>       ],
>       "alias": "luks-7062202e-5b9b-433e-81e8-6628c40da9f7"
>     },
>     {
>       "name": "/dev/sda2",
>       "partition": true,
>       "slaves": [
>         "/dev/sda"
>       ]
>     },
>     {
>       "name": "/dev/sda",
>       "partition": false,
>       "address": {
>         "serial": "SAMSUNG_MZ7LN512HCHP-000L1_S1ZKNXAG822493",
>         "bus-type": "sata",
>         ...
>         "dev": "/dev/sda",
>         "target": 0
>       },
>       "slaves": []
>     },
>     ...
>   ]
> }
> 
> Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
> ---
>  qga/commands-posix.c | 247 +++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 240 insertions(+), 7 deletions(-)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index f99731af51..3babc25c09 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -62,6 +62,9 @@ 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;
> @@ -1150,6 +1153,21 @@ 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;
> +    }
> +    if (strstr(syspath, "/devices/virtual/block/")) {
> +        return true;
> +    } else {
> +        return false;
> +    }
> +}
> +
>  /* Dispatch to functions for virtual/real device */
>  static void build_guest_fsinfo_for_device(char const *devpath,
>                                            GuestFilesystemInfo *fs,
> @@ -1168,6 +1186,7 @@ static void build_guest_fsinfo_for_device(char const *devpath,
>  
>      g_debug("  parse sysfs path '%s'", syspath);
>  
> +    /* TODO: use is_disk_virtual() */
>      if (strstr(syspath, "/devices/virtual/block/")) {
>          build_guest_fsinfo_for_virtual_device(syspath, fs, errp);
>      } else {
> @@ -1177,6 +1196,225 @@ 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();
> +    udevice = udev_device_new_from_syspath(udev, syspath);
> +    if (udev == NULL || udevice == NULL) {
> +        g_debug("failed to query udev");
> +    } else {
> +        const char *alias = udev_device_get_property_value(
> +            udevice, "DM_NAME");
> +        if (alias != NULL && *alias != 0) {
> +            ret = g_strdup(alias);
> +        }
> +    }
> +
> +    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();
> +    udevice = udev_device_new_from_syspath(udev, syspath);
> +    if (udev == NULL || udevice == NULL) {
> +        g_debug("failed to query udev");
> +    } else {
> +        ret = g_strdup(udev_device_get_devnode(udevice));
> +    }
> +    udev_unref(udev);
> +    udev_device_unref(udevice);
> +    return ret;
> +}
> +
> +GuestDiskInfoList *qmp_guest_get_disks(Error **errp)
> +{
> +    GuestDiskInfoList *item, *ret = NULL;
> +    GuestDiskInfo *disk, *partition;
> +    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_dir = NULL, *slaves_dir = NULL;
> +        struct dirent *de_disk, *de_slaves;
> +        DIR *dp_disk = NULL, *dp_slaves = NULL;
> +        FILE *fp = NULL;
> +        size_t n = 0;
> +        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_dir = g_strdup_printf("/sys/block/%s/size", de->d_name);
> +        fp = fopen(size_dir, "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);

These 10 lines can be reduced to just

    g_file_get_contents(size_dir, &line, NULL, NULL)
    

> +        if (strcmp(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);
> +        disk = g_new0(GuestDiskInfo, 1);
> +        disk->name = get_device_for_syspath(disk_dir);
> +        disk->partition = false;
> +        disk->alias = get_alias_for_syspath(disk_dir);
> +        if (disk->alias != NULL) {
> +            disk->has_alias = true;
> +        }
> +        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;
> +            }
> +        }
> +
> +        /* List slave disks */
> +        slaves_dir = g_strdup_printf("%s/slaves", disk_dir);
> +        g_debug("  listing entries in: %s", slaves_dir);
> +        dp_slaves = opendir(slaves_dir);
> +        while ((de_slaves = readdir(dp_slaves)) != NULL) {
> +            g_autofree char *slave_dir = NULL;
> +            char *slave_dev;
> +            strList *slave_item = NULL;
> +
> +            if ((strcmp(".", de_slaves->d_name) == 0) ||
> +                (strcmp("..", de_slaves->d_name) == 0)) {
> +                continue;
> +            }
> +
> +            /* Add slave disks */
> +            slave_dir = g_strdup_printf("%s/%s",
> +                slaves_dir, de_slaves->d_name);
> +            slave_dev = get_device_for_syspath(slave_dir);
> +            if (slave_dev != NULL) {
> +                g_debug("  adding slave device: %s", slave_dev);
> +                slave_item = g_new0(strList, 1);
> +                slave_item->value = slave_dev;
> +                slave_item->next = disk->slaves;
> +                disk->slaves = slave_item;
> +            }
> +        }
> +        closedir(dp_slaves);

Since you are only using d_name, you can use g_dir_open and
g_dir_read_name and g_dir_close. This always skips . and ..
for you.

> +
> +        /*
> +         * Detect partitions subdirectory name is "<parent><number>" or
> +         * "<parent>p<number>"
> +         */
> +        dp_disk = opendir(disk_dir);
> +        while ((de_disk = readdir(dp_disk)) != NULL) {
> +            size_t len;
> +            g_autofree char *partition_dir = NULL;
> +
> +            if (!(de_disk->d_type & DT_DIR)) {
> +                continue;
> +            }
> +
> +            len = strlen(de->d_name);
> +            if (!(strncmp(de->d_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);
> +            partition = g_new0(GuestDiskInfo, 1);
> +            partition->name = get_device_for_syspath(partition_dir);
> +            partition->partition = true;
> +            /* Add parent disk as slave for easier tracking of hierarchy */
> +            partition->slaves = g_new0(strList, 1);
> +            partition->slaves->value = g_strdup(disk->name);
> +
> +            item = g_new0(GuestDiskInfoList, 1);
> +            item->value = partition;
> +            item->next = ret;
> +            ret = item;
> +
> +        }
> +        closedir(dp_disk);
> +    }
> +    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 +3047,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) {
> @@ -3042,9 +3281,3 @@ GuestOSInfo *qmp_guest_get_osinfo(Error **errp)
>  
>      return info;
>  }
> -
> -GuestDiskInfoList *qmp_guest_get_disks(Error **errp)
> -{
> -    error_setg(errp, QERR_UNSUPPORTED);
> -    return NULL;
> -}
> -- 
> 2.25.0
> 
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

[PATCH v2 3/3] qga: add implementation of guest-get-disks for Windows

[Tomáš Golembiovský]

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",
        ...
      },
      "slaves": []
    }
  ]
}

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

diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index e9976a0c46..9ac847a187 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -945,6 +945,91 @@ 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_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);
+
+        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)
@@ -952,6 +1037,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)
@@ -2229,9 +2320,3 @@ GuestOSInfo *qmp_guest_get_osinfo(Error **errp)
 
     return info;
 }
-
-GuestDiskInfoList *qmp_guest_get_disks(Error **errp)
-{
-    error_setg(errp, QERR_UNSUPPORTED);
-    return NULL;
-}
-- 
2.25.0

Re: [PATCH v2 3/3] qga: add implementation of guest-get-disks for Windows

[Philippe Mathieu-Daudé]

Cc'ing Yonggang Luo for review.

On 9/7/20 11:14 AM, Tomáš Golembiovský 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",
>         ...
>       },
>       "slaves": []
>     }
>   ]
> }
> 
> Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
> ---
>  qga/commands-win32.c | 97 +++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 91 insertions(+), 6 deletions(-)
> 
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index e9976a0c46..9ac847a187 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -945,6 +945,91 @@ 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_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);
> +
> +        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)
> @@ -952,6 +1037,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)
> @@ -2229,9 +2320,3 @@ GuestOSInfo *qmp_guest_get_osinfo(Error **errp)
>  
>      return info;
>  }
> -
> -GuestDiskInfoList *qmp_guest_get_disks(Error **errp)
> -{
> -    error_setg(errp, QERR_UNSUPPORTED);
> -    return NULL;
> -}
> 

Re: [PATCH v2 3/3] qga: add implementation of guest-get-disks for Windows

[Marc-André Lureau]

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

Hi

On Mon, Sep 7, 2020 at 1:15 PM 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",
>         ...
>       },
>       "slaves": []
>     }
>   ]
> }
>
> Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
> ---
>  qga/commands-win32.c | 97 +++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 91 insertions(+), 6 deletions(-)
>
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index e9976a0c46..9ac847a187 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -945,6 +945,91 @@ 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_malloc(size);
>

Since this is called in a loop, you should use g_realloc() to avoid
potential leaks.

+                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)
> @@ -952,6 +1037,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)
> @@ -2229,9 +2320,3 @@ GuestOSInfo *qmp_guest_get_osinfo(Error **errp)
>
>      return info;
>  }
> -
> -GuestDiskInfoList *qmp_guest_get_disks(Error **errp)
> -{
> -    error_setg(errp, QERR_UNSUPPORTED);
> -    return NULL;
> -}
> --
> 2.25.0
>
>
>
The rest looks ok to me.

-- 
Marc-André Lureau

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

Re: [PATCH v2 0/3] qga: add command guest-get-disk

[Tomáš Golembiovský]

gentle reminder

On Mon, Sep 07, 2020 at 11:14:39AM +0200, Tomáš Golembiovský wrote:
> 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.
> 
> 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 | 241 ++++++++++++++++++++++++++++++++++++++++++-
>  qga/commands-win32.c |  91 ++++++++++++++++
>  qga/qapi-schema.json |  29 ++++++
>  3 files changed, 360 insertions(+), 1 deletion(-)
> 
> -- 
> 2.25.0
> 

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