[Adding Markus in CC]
On 11/2/20 8:43 PM, Michael Roth wrote:
> From: Tomáš Golembiovský <tgole...@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ý <tgole...@redhat.com>
> Reviewed-by: Philippe Mathieu-Daudé <phi...@redhat.com>
> Reviewed-by: Marc-André Lureau <marcandre.lur...@redhat.com>
> Signed-off-by: Michael Roth <michael.r...@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:
> #
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
