[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PULL v3 05/12] qga: add command guest-get-disks
From: |
Michael Roth |
Subject: |
Re: [PULL v3 05/12] qga: add command guest-get-disks |
Date: |
Sun, 08 Nov 2020 10:47:23 -0600 |
User-agent: |
alot/0.9 |
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
>
>
- [PULL v3 09/12] qga: add ssh-{add,remove}-authorized-keys, (continued)
- [PULL v3 09/12] qga: add ssh-{add,remove}-authorized-keys, Michael Roth, 2020/11/02
- [PULL v3 10/12] qga: add *reset argument to ssh-add-authorized-keys, Michael Roth, 2020/11/02
- [PULL v3 11/12] meson: minor simplification, Michael Roth, 2020/11/02
- [PULL v3 12/12] qga: add ssh-get-authorized-keys, Michael Roth, 2020/11/02
- [PULL v3 01/12] qga: Rename guest-get-devices return member 'address' to 'id', Michael Roth, 2020/11/02
- [PULL v3 02/12] qga: Use common time encoding for guest-get-devices 'driver-date', Michael Roth, 2020/11/02
- [PULL v3 03/12] qga-win: Fix guest-get-devices error API violations, Michael Roth, 2020/11/02
- [PULL v3 04/12] qga: Flatten simple union GuestDeviceId, Michael Roth, 2020/11/02
- [PULL v3 05/12] qga: add command guest-get-disks, Michael Roth, 2020/11/02
- [PULL v3 06/12] qga: add implementation of guest-get-disks for Linux, Michael Roth, 2020/11/02
- [PULL v3 07/12] qga: add implementation of guest-get-disks for Windows, Michael Roth, 2020/11/02
- [PULL v3 08/12] glib-compat: add g_unix_get_passwd_entry_qemu(), Michael Roth, 2020/11/02
- Re: [PULL v3 00/12] qemu-ga patch queue for soft-freeze, Peter Maydell, 2020/11/03