Re: [PATCH v2] libxl: adjust handling of libxl_device_disk objects

[Jim Fehlig <jfehlig@xxxxxxxx>]

On 5/20/21 4:17 AM, Olaf Hering wrote:
> libxl objects are supposed to be initialized and disposed.
> Correct the usage of libxl_device_disk objects which are allocated on
> the stack. Initialize each one prior usage, and dispose them once done.
>
> Adjust libxlMakeDisk to use an already initialized object, it is owned
> by the caller.
>
> Adjust libxlMakeDiskList to initialize the list of objects, before they
> are filled by libxlMakeDisk. In case of error, the objects are disposed
> by libxl_domain_config_dispose.
>
> Signed-off-by: Olaf Hering <olaf@xxxxxxxxx>
> ---
>   src/libxl/libxl_conf.c   | 20 +++++---------------
>   src/libxl/libxl_driver.c |  6 ++++++
>   2 files changed, 11 insertions(+), 15 deletions(-)

Reviewed-by: Jim Fehlig <jfehlig@xxxxxxxx

> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> index 3149ee3b4a..f29a28a841 100644
> --- a/src/libxl/libxl_conf.c
> +++ b/src/libxl/libxl_conf.c
> @@ -1114,8 +1114,6 @@ libxlMakeDisk(virDomainDiskDef *l_disk, libxl_device_disk *x_disk)
>       int format = virDomainDiskGetFormat(l_disk);
>       int actual_type = virStorageSourceGetActualType(l_disk->src);
>   
> -    libxl_device_disk_init(x_disk);
> -
>       if (actual_type == VIR_STORAGE_TYPE_NETWORK) {
>           if (STRNEQ_NULLABLE(driver, "qemu")) {
>               virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> @@ -1265,26 +1263,18 @@ libxlMakeDiskList(virDomainDef *def, libxl_domain_config *d_config)
>   {
>       virDomainDiskDef **l_disks = def->disks;
>       int ndisks = def->ndisks;
> -    libxl_device_disk *x_disks;
>       size_t i;
>   
> -    x_disks = g_new0(libxl_device_disk, ndisks);
> +    d_config->disks = g_new0(libxl_device_disk, ndisks);
> +    d_config->num_disks = ndisks;
>   
>       for (i = 0; i < ndisks; i++) {
> -        if (libxlMakeDisk(l_disks[i], &x_disks[i]) < 0)
> -            goto error;
> +        libxl_device_disk_init(&d_config->disks[i]);
> +        if (libxlMakeDisk(l_disks[i], &d_config->disks[i]) < 0)
> +            return -1;
>       }
>   
> -    d_config->disks = x_disks;
> -    d_config->num_disks = ndisks;
> -
>       return 0;
> -
> - error:
> -    for (i = 0; i < ndisks; i++)
> -        libxl_device_disk_dispose(&x_disks[i]);
> -    VIR_FREE(x_disks);
> -    return -1;
>   }
>   
>   /*
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index d54cd41785..2b844bb3b5 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -2978,6 +2978,7 @@ libxlDomainChangeEjectableMedia(virDomainObj *vm, virDomainDiskDef *disk)
>       size_t i;
>       int ret = -1;
>   
> +    libxl_device_disk_init(&x_disk);
>       for (i = 0; i < vm->def->ndisks; i++) {
>           if (vm->def->disks[i]->bus == disk->bus &&
>               STREQ(vm->def->disks[i]->dst, disk->dst)) {
> @@ -3018,6 +3019,7 @@ libxlDomainChangeEjectableMedia(virDomainObj *vm, virDomainDiskDef *disk)
>       ret = 0;
>   
>    cleanup:
> +    libxl_device_disk_dispose(&x_disk);
>       virObjectUnref(cfg);
>       return ret;
>   }
> @@ -3030,6 +3032,7 @@ libxlDomainAttachDeviceDiskLive(virDomainObj *vm, virDomainDeviceDef *dev)
>       libxl_device_disk x_disk;
>       int ret = -1;
>   
> +    libxl_device_disk_init(&x_disk);
>       switch (l_disk->device)  {
>           case VIR_DOMAIN_DISK_DEVICE_CDROM:
>               ret = libxlDomainChangeEjectableMedia(vm, l_disk);
> @@ -3091,6 +3094,7 @@ libxlDomainAttachDeviceDiskLive(virDomainObj *vm, virDomainDeviceDef *dev)
>       }
>   
>    cleanup:
> +    libxl_device_disk_dispose(&x_disk);
>       virObjectUnref(cfg);
>       return ret;
>   }
> @@ -3329,6 +3333,7 @@ libxlDomainDetachDeviceDiskLive(virDomainObj *vm, virDomainDeviceDef *dev)
>       int idx;
>       int ret = -1;
>   
> +    libxl_device_disk_init(&x_disk);
>       switch (dev->data.disk->device)  {
>           case VIR_DOMAIN_DISK_DEVICE_DISK:
>               if (dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_XEN) {
> @@ -3380,6 +3385,7 @@ libxlDomainDetachDeviceDiskLive(virDomainObj *vm, virDomainDeviceDef *dev)
>       }
>   
>    cleanup:
> +    libxl_device_disk_dispose(&x_disk);
>       virObjectUnref(cfg);
>       return ret;
>   }