| Message ID | 20201103024343.894221-6-michael.roth@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | [PULL,v3,01/12] qga: Rename guest-get-devices return member 'address' to 'id' | expand |
[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: > # >
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 --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: #