Re: [PATCH for-5.2] qga: update schema for guest-get-disks 'dependents' field

[Eric Blake]

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>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org