diff mbox series

[PULL,v3,05/12] qga: add command guest-get-disks

Message ID 20201103024343.894221-6-michael.roth@amd.com (mailing list archive)
State New, archived
Headers show
Series [PULL,v3,01/12] qga: Rename guest-get-devices return member 'address' to 'id' | expand

Commit Message

Michael Roth Nov. 3, 2020, 2:43 a.m. UTC
From: Tomáš Golembiovský <tgolembi@redhat.com>

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: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Michael Roth <michael.roth@amd.com>
---
 qga/commands-posix.c |  6 ++++++
 qga/commands-win32.c |  6 ++++++
 qga/qapi-schema.json | 31 +++++++++++++++++++++++++++++++
 3 files changed, 43 insertions(+)

Comments

Eric Blake Nov. 4, 2020, 9:56 p.m. UTC | #1
[Adding Markus in CC]

On 11/2/20 8:43 PM, Michael Roth wrote:
> From: Tomáš Golembiovský <tgolembi@redhat.com>
> 
> 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: Philippe Mathieu-Daudé <philmd@redhat.com>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Michael Roth <michael.roth@amd.com>
> ---
>  qga/commands-posix.c |  6 ++++++
>  qga/commands-win32.c |  6 ++++++
>  qga/qapi-schema.json | 31 +++++++++++++++++++++++++++++++
>  3 files changed, 43 insertions(+)

I know my review is late, since the PR is already accepted, but some
items that may be worth followup patches in the 5.2 cycle:


> +++ b/qga/qapi-schema.json
> @@ -865,6 +865,37 @@
>             '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
> +# @dependents: list of dependent devices; e.g. for LVs of the LVM this will
> +#              hold the list of PVs, for LUKS encrypted volume this will
> +#              contain the disk where the volume is placed.     (Linux)

Odd spacing before the comment.

Should @dependents be guarded by 'if', or are we avoiding the use of
'if' in qga for now and only using it in qmp?

Is 'dependents' the right name?  A common English use of 'dependent' is
when talking about a family: a parent's child is their dependent (that
is, a dependent tends to be the smaller entity).  But here, you are
using the term in the opposite direction: this storage device (such as a
LUKS encrypted drive) is declaraing a LARGER entity (the containing
block device) as its dependent.  Would 'dependencies' or 'depends-on' be
more accurate?

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

'dependents' is not an optional member, but documented as '(Linux)'
above; does that mean you always get "dependents":[] on the wire for
Windows, rather than omitting the field?

> +
> +##
> +# @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:
>  #
>
Michael Roth Nov. 8, 2020, 4:47 p.m. UTC | #2
Quoting Eric Blake (2020-11-04 15:56:17)
> [Adding Markus in CC]
> 
> On 11/2/20 8:43 PM, Michael Roth wrote:
> > From: Tomá\u0161 Golembiovský <tgolembi@redhat.com>
> > 
> > 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á\u0161 Golembiovský <tgolembi@redhat.com>
> > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > Signed-off-by: Michael Roth <michael.roth@amd.com>
> > ---
> >  qga/commands-posix.c |  6 ++++++
> >  qga/commands-win32.c |  6 ++++++
> >  qga/qapi-schema.json | 31 +++++++++++++++++++++++++++++++
> >  3 files changed, 43 insertions(+)
> 
> I know my review is late, since the PR is already accepted, but some
> items that may be worth followup patches in the 5.2 cycle:
> 
> 
> > +++ b/qga/qapi-schema.json
> > @@ -865,6 +865,37 @@
> >             '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
> > +# @dependents: list of dependent devices; e.g. for LVs of the LVM this will
> > +#              hold the list of PVs, for LUKS encrypted volume this will
> > +#              contain the disk where the volume is placed.     (Linux)
> 
> Odd spacing before the comment.
> 
> Should @dependents be guarded by 'if', or are we avoiding the use of
> 'if' in qga for now and only using it in qmp?

Marc-André's recent guest-ssh-{get,add,remove}-authorized-keys patches are
the first users for qga I think. The CONFIG_{LINUX,POSIX,W32} is
becoming pretty unwieldly so I've been planning on going back and using
schema conditionals and refactoring things a bit anyway so I'm ok with
adding it later as long as the documentation is accurate.

> 
> Is 'dependents' the right name?  A common English use of 'dependent' is
> when talking about a family: a parent's child is their dependent (that
> is, a dependent tends to be the smaller entity).  But here, you are
> using the term in the opposite direction: this storage device (such as a
> LUKS encrypted drive) is declaraing a LARGER entity (the containing
> block device) as its dependent.  Would 'dependencies' or 'depends-on' be
> more accurate?

Agreed, 'dependencies' is probably a better name. The 'slaves' term
used in /sys can be a bit confusing in this regard as well, but
captures the relationship a bit better than 'dependents' as least.

> 
> > +# @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', 'dependents': ['str'],
> > +           '*address': 'GuestDiskAddress', '*alias': 'str'} }
> 
> 'dependents' is not an optional member, but documented as '(Linux)'
> above; does that mean you always get "dependents":[] on the wire for
> Windows, rather than omitting the field?

Yes, which seems a bit misleading to management. This should be made
optional.

I'll send a patch to address these.

Thanks for the review!

-Mike

> 
> > +
> > +##
> > +# @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:
> >  #
> > 
> 
> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3226
> Virtualization:  qemu.org | libvirt.org
> 
>
diff mbox series

Patch

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 3bffee99d4..422144bcff 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -3051,3 +3051,9 @@  GuestDeviceInfoList *qmp_guest_get_devices(Error **errp)
 
     return NULL;
 }
+
+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 0c33d48aaa..f7bdd5a8b5 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -2458,3 +2458,9 @@  GuestDeviceInfoList *qmp_guest_get_devices(Error **errp)
     }
     return head;
 }
+
+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 fe10631e4c..e123a000be 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -865,6 +865,37 @@ 
            '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
+# @dependents: list of dependent devices; e.g. for LVs of the LVM this will
+#              hold the list of PVs, for LUKS encrypted volume this will
+#              contain the disk where the volume is placed.     (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', 'dependents': ['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:
 #