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.

The usage of g_new0 is suspicious in the context of libxl because the
memory allocated via glib is released with plain free() inside libxl.

Signed-off-by: Olaf Hering <olaf@aepfle.de>
---
 src/libxl/libxl_conf.c   | 22 +++++++++-------------
 src/libxl/libxl_driver.c |  6 ++++++
 2 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index 3149ee3b4a..2d2aab7e66 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,24 @@ 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;
+    int ret = -1;
+
+    d_config->disks = g_new0(libxl_device_disk, ndisks);
+    d_config->num_disks = ndisks;
 
-    x_disks = g_new0(libxl_device_disk, ndisks);
+    for (i = 0; i < ndisks; i++)
+        libxl_device_disk_init(&d_config->disks[i]);
 
     for (i = 0; i < ndisks; i++) {
-        if (libxlMakeDisk(l_disks[i], &x_disks[i]) < 0)
+        if (libxlMakeDisk(l_disks[i], &d_config->disks[i]) < 0)
             goto error;
     }
 
-    d_config->disks = x_disks;
-    d_config->num_disks = ndisks;
-
-    return 0;
+    ret = 0;
 
  error:
-    for (i = 0; i < ndisks; i++)
-        libxl_device_disk_dispose(&x_disks[i]);
-    VIR_FREE(x_disks);
-    return -1;
+    return ret;
 }
 
 /*
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;
 }

On 5/19/21 2:42 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.
> 
> The usage of g_new0 is suspicious in the context of libxl because the
> memory allocated via glib is released with plain free() inside libxl.

After looking at the glib memory allocation doc again, I'm suspicious about your 
suspicion :-)

https://developer.gnome.org/glib/stable/glib-Memory-Allocation.html

I recall being confused about the text in the Description before. All is clear 
and I'm confident of my understanding - until the last tiny paragraph:

"Since GLib 2.46 g_malloc() is hardcoded to always use the system malloc 
implementation."

Err, what? Ok, cool, I guess it is fine to use plain free with g_new et.al.

IIRC this has been discussed on the list in the past, likely buried in some 
unrelated mail and difficult to find. Perhaps others can chime in and give us 
enough confidence to no longer be suspicious.

> 
> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> ---
>   src/libxl/libxl_conf.c   | 22 +++++++++-------------
>   src/libxl/libxl_driver.c |  6 ++++++
>   2 files changed, 15 insertions(+), 13 deletions(-)
> 
> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> index 3149ee3b4a..2d2aab7e66 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,24 @@ 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;
> +    int ret = -1;
> +
> +    d_config->disks = g_new0(libxl_device_disk, ndisks);
> +    d_config->num_disks = ndisks;
>   
> -    x_disks = g_new0(libxl_device_disk, ndisks);
> +    for (i = 0; i < ndisks; i++)
> +        libxl_device_disk_init(&d_config->disks[i]);
>   
>       for (i = 0; i < ndisks; i++) {
> -        if (libxlMakeDisk(l_disks[i], &x_disks[i]) < 0)
> +        if (libxlMakeDisk(l_disks[i], &d_config->disks[i]) < 0)
>               goto error;
>       }

Initializing and making the disk can be done in one loop.

>   
> -    d_config->disks = x_disks;
> -    d_config->num_disks = ndisks;
> -
> -    return 0;
> +    ret = 0;
>   
>    error:
> -    for (i = 0; i < ndisks; i++)
> -        libxl_device_disk_dispose(&x_disks[i]);
> -    VIR_FREE(x_disks);
> -    return -1;
> +    return ret;

You can drop the error label and return success or failure inline.

Regards,
Jim

>   }
>   
>   /*
> 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;
>   }
>