| Message ID | 20201113183312.432630-1-michael.roth@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [for-5.2] qga: update schema for guest-get-disks 'dependents' field | expand |
On Fri, Nov 13, 2020 at 10:49 PM Michael Roth <michael.roth@amd.com> wrote: > The recently-added 'guest-get-disk' command returns a list of > GuestDiskInfo entries, which in turn have a 'dependents' field which > lists devices these entries are dependent upon. Thus, 'dependencies' > is a better name for this field. Address this by renaming the field > accordingly. > > Additionally, 'dependents' is specified as non-optional, even though > it's not implemented for w32. This is misleading, since it gives users > the impression that a particular disk might not have dependencies, > when in reality that information is simply not known to the guest > agent. Address this by making 'dependents' an optional field, and only > marking it as in-use when the facilities to obtain this information are > available to the guest agent. > > Cc: Eric Blake <eblake@redhat.com> > Cc: Tomáš Golembiovský <tgolembi@redhat.com> > Cc: Marc-André Lureau <marcandre.lureau@redhat.com> > Signed-off-by: Michael Roth <michael.roth@amd.com> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > qga/commands-posix.c | 10 ++++++---- > qga/qapi-schema.json | 8 ++++---- > 2 files changed, 10 insertions(+), 8 deletions(-) > > diff --git a/qga/commands-posix.c b/qga/commands-posix.c > index 12c1ba5ef7..c089e38120 100644 > --- a/qga/commands-posix.c > +++ b/qga/commands-posix.c > @@ -1285,6 +1285,7 @@ static void get_disk_deps(const char *disk_dir, > GuestDiskInfo *disk) > g_debug("failed to list entries in %s", deps_dir); > return; > } > + disk->has_dependencies = true; > while ((dep = g_dir_read_name(dp_deps)) != NULL) { > g_autofree char *dep_dir = NULL; > strList *dep_item = NULL; > @@ -1297,8 +1298,8 @@ static void get_disk_deps(const char *disk_dir, > GuestDiskInfo *disk) > g_debug(" adding dependent device: %s", dev_name); > dep_item = g_new0(strList, 1); > dep_item->value = dev_name; > - dep_item->next = disk->dependents; > - disk->dependents = dep_item; > + dep_item->next = disk->dependencies; > + disk->dependencies = dep_item; > } > } > g_dir_close(dp_deps); > @@ -1351,8 +1352,9 @@ static GuestDiskInfoList *get_disk_partitions( > partition->name = dev_name; > partition->partition = true; > /* Add parent disk as dependent for easier tracking of hierarchy > */ > - partition->dependents = g_new0(strList, 1); > - partition->dependents->value = g_strdup(disk_dev); > + partition->dependencies = g_new0(strList, 1); > + partition->dependencies->value = g_strdup(disk_dev); > + partition->has_dependencies = true; > > item = g_new0(GuestDiskInfoList, 1); > item->value = partition; > diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json > index 6ca85f995f..3b3d1d0bd9 100644 > --- a/qga/qapi-schema.json > +++ b/qga/qapi-schema.json > @@ -870,9 +870,9 @@ > # > # @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) > +# @dependencies: list of device dependencies; 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 > @@ -880,7 +880,7 @@ > # Since 5.2 > ## > { 'struct': 'GuestDiskInfo', > - 'data': {'name': 'str', 'partition': 'bool', 'dependents': ['str'], > + 'data': {'name': 'str', 'partition': 'bool', '*dependencies': ['str'], > '*address': 'GuestDiskAddress', '*alias': 'str'} } > > ## > -- > 2.25.1 > > >
On 11/13/20 12:33 PM, Michael Roth wrote: > The recently-added 'guest-get-disk' command returns a list of > GuestDiskInfo entries, which in turn have a 'dependents' field which > lists devices these entries are dependent upon. Thus, 'dependencies' > is a better name for this field. Address this by renaming the field > accordingly. > > Additionally, 'dependents' is specified as non-optional, even though > it's not implemented for w32. This is misleading, since it gives users > the impression that a particular disk might not have dependencies, > when in reality that information is simply not known to the guest > agent. Address this by making 'dependents' an optional field, and only > marking it as in-use when the facilities to obtain this information are > available to the guest agent. > > Cc: Eric Blake <eblake@redhat.com> > Cc: Tomáš Golembiovský <tgolembi@redhat.com> > Cc: Marc-André Lureau <marcandre.lureau@redhat.com> > Signed-off-by: Michael Roth <michael.roth@amd.com> > --- > qga/commands-posix.c | 10 ++++++---- > qga/qapi-schema.json | 8 ++++---- > 2 files changed, 10 insertions(+), 8 deletions(-) > > diff --git a/qga/commands-posix.c b/qga/commands-posix.c > index 12c1ba5ef7..c089e38120 100644 > --- a/qga/commands-posix.c > +++ b/qga/commands-posix.c > @@ -1285,6 +1285,7 @@ static void get_disk_deps(const char *disk_dir, GuestDiskInfo *disk) > g_debug("failed to list entries in %s", deps_dir); > return; > } > + disk->has_dependencies = true; > while ((dep = g_dir_read_name(dp_deps)) != NULL) { > g_autofree char *dep_dir = NULL; > strList *dep_item = NULL; > @@ -1297,8 +1298,8 @@ static void get_disk_deps(const char *disk_dir, GuestDiskInfo *disk) > g_debug(" adding dependent device: %s", dev_name); > dep_item = g_new0(strList, 1); > dep_item->value = dev_name; > - dep_item->next = disk->dependents; > - disk->dependents = dep_item; > + dep_item->next = disk->dependencies; > + disk->dependencies = dep_item; You could use QAPI_LIST_PREPEND() here (which was just recently added); but if not, then my work to use that macro in more places in 6.0 will revisit this code. > } > } > g_dir_close(dp_deps); > @@ -1351,8 +1352,9 @@ static GuestDiskInfoList *get_disk_partitions( > partition->name = dev_name; > partition->partition = true; > /* Add parent disk as dependent for easier tracking of hierarchy */ > - partition->dependents = g_new0(strList, 1); > - partition->dependents->value = g_strdup(disk_dev); > + partition->dependencies = g_new0(strList, 1); > + partition->dependencies->value = g_strdup(disk_dev); Same here. Reviewed-by: Eric Blake <eblake@redhat.com>
diff --git a/qga/commands-posix.c b/qga/commands-posix.c index 12c1ba5ef7..c089e38120 100644 --- a/qga/commands-posix.c +++ b/qga/commands-posix.c @@ -1285,6 +1285,7 @@ static void get_disk_deps(const char *disk_dir, GuestDiskInfo *disk) g_debug("failed to list entries in %s", deps_dir); return; } + disk->has_dependencies = true; while ((dep = g_dir_read_name(dp_deps)) != NULL) { g_autofree char *dep_dir = NULL; strList *dep_item = NULL; @@ -1297,8 +1298,8 @@ static void get_disk_deps(const char *disk_dir, GuestDiskInfo *disk) g_debug(" adding dependent device: %s", dev_name); dep_item = g_new0(strList, 1); dep_item->value = dev_name; - dep_item->next = disk->dependents; - disk->dependents = dep_item; + dep_item->next = disk->dependencies; + disk->dependencies = dep_item; } } g_dir_close(dp_deps); @@ -1351,8 +1352,9 @@ static GuestDiskInfoList *get_disk_partitions( partition->name = dev_name; partition->partition = true; /* Add parent disk as dependent for easier tracking of hierarchy */ - partition->dependents = g_new0(strList, 1); - partition->dependents->value = g_strdup(disk_dev); + partition->dependencies = g_new0(strList, 1); + partition->dependencies->value = g_strdup(disk_dev); + partition->has_dependencies = true; item = g_new0(GuestDiskInfoList, 1); item->value = partition; diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json index 6ca85f995f..3b3d1d0bd9 100644 --- a/qga/qapi-schema.json +++ b/qga/qapi-schema.json @@ -870,9 +870,9 @@ # # @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) +# @dependencies: list of device dependencies; 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 @@ -880,7 +880,7 @@ # Since 5.2 ## { 'struct': 'GuestDiskInfo', - 'data': {'name': 'str', 'partition': 'bool', 'dependents': ['str'], + 'data': {'name': 'str', 'partition': 'bool', '*dependencies': ['str'], '*address': 'GuestDiskAddress', '*alias': 'str'} } ##
The recently-added 'guest-get-disk' command returns a list of GuestDiskInfo entries, which in turn have a 'dependents' field which lists devices these entries are dependent upon. Thus, 'dependencies' is a better name for this field. Address this by renaming the field accordingly. Additionally, 'dependents' is specified as non-optional, even though it's not implemented for w32. This is misleading, since it gives users the impression that a particular disk might not have dependencies, when in reality that information is simply not known to the guest agent. Address this by making 'dependents' an optional field, and only marking it as in-use when the facilities to obtain this information are available to the guest agent. Cc: Eric Blake <eblake@redhat.com> Cc: Tomáš Golembiovský <tgolembi@redhat.com> Cc: Marc-André Lureau <marcandre.lureau@redhat.com> Signed-off-by: Michael Roth <michael.roth@amd.com> --- qga/commands-posix.c | 10 ++++++---- qga/qapi-schema.json | 8 ++++---- 2 files changed, 10 insertions(+), 8 deletions(-)