libxl objects are supposed to be initialized and disposed.
Adjust libxlMakeNic to use an already initialized object, it is owned by
the caller.

Adjust libxlMakeNicList to initialize the list of objects, before they
are filled by libxlMakeNic. 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 | 36 +++++++++++++++++-------------------
 1 file changed, 17 insertions(+), 19 deletions(-)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index 2d2aab7e66..e4afa578b0 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -1345,8 +1345,6 @@ libxlMakeNic(virDomainDef *def,
         return -1;
     }
 
-    libxl_device_nic_init(x_nic);
-
     virMacAddrGetRaw(&l_nic->mac, x_nic->mac);
 
     /*
@@ -1532,39 +1530,39 @@ libxlMakeNicList(virDomainDef *def,  libxl_domain_config *d_config)
 {
     virDomainNetDef **l_nics = def->nets;
     size_t nnics = def->nnets;
-    libxl_device_nic *x_nics;
     size_t i, nvnics = 0;
-
-    x_nics = g_new0(libxl_device_nic, nnics);
+    int ret = -1;
 
     for (i = 0; i < nnics; i++) {
         if (virDomainNetGetActualType(l_nics[i]) == VIR_DOMAIN_NET_TYPE_HOSTDEV)
             continue;
+        nvnics++;
+    }
+    if (!nvnics)
+        return 0;
+
+    d_config->nics = g_new0(libxl_device_nic, nvnics);
+    d_config->num_nics = nvnics;
+
+    for (i = 0; i < nvnics; i++)
+        libxl_device_nic_init(&d_config->nics[i]);
 
-        if (libxlMakeNic(def, l_nics[i], &x_nics[nvnics], false))
+    for (i = 0; i < nvnics; i++) {
+        if (libxlMakeNic(def, l_nics[i], &d_config->nics[i], false))
             goto error;
         /*
          * The devid (at least right now) will not get initialized by
          * libxl in the setup case but is required for starting the
          * device-model.
          */
-        if (x_nics[nvnics].devid < 0)
-            x_nics[nvnics].devid = nvnics;
-
-        nvnics++;
+        if (d_config->nics[i].devid < 0)
+            d_config->nics[i].devid = i;
     }
 
-    VIR_SHRINK_N(x_nics, nnics, nnics - nvnics);
-    d_config->nics = x_nics;
-    d_config->num_nics = nvnics;
-
-    return 0;
+    ret = 0;
 
  error:
-    for (i = 0; i < nnics; i++)
-        libxl_device_nic_dispose(&x_nics[i]);
-    VIR_FREE(x_nics);
-    return -1;
+    return ret;
 }
 
 int

On 5/19/21 3:37 AM, Olaf Hering wrote:
> libxl objects are supposed to be initialized and disposed.
> Adjust libxlMakeNic to use an already initialized object, it is owned by
> the caller.
> 
> Adjust libxlMakeNicList to initialize the list of objects, before they
> are filled by libxlMakeNic. 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 | 36 +++++++++++++++++-------------------
>   1 file changed, 17 insertions(+), 19 deletions(-)
> 
> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> index 2d2aab7e66..e4afa578b0 100644
> --- a/src/libxl/libxl_conf.c
> +++ b/src/libxl/libxl_conf.c
> @@ -1345,8 +1345,6 @@ libxlMakeNic(virDomainDef *def,
>           return -1;
>       }
>   
> -    libxl_device_nic_init(x_nic);
> -
>       virMacAddrGetRaw(&l_nic->mac, x_nic->mac);
>   
>       /*
> @@ -1532,39 +1530,39 @@ libxlMakeNicList(virDomainDef *def,  libxl_domain_config *d_config)
>   {
>       virDomainNetDef **l_nics = def->nets;
>       size_t nnics = def->nnets;
> -    libxl_device_nic *x_nics;
>       size_t i, nvnics = 0;
> -
> -    x_nics = g_new0(libxl_device_nic, nnics);
> +    int ret = -1;
>   
>       for (i = 0; i < nnics; i++) {
>           if (virDomainNetGetActualType(l_nics[i]) == VIR_DOMAIN_NET_TYPE_HOSTDEV)
>               continue;
> +        nvnics++;
> +    }
> +    if (!nvnics)
> +        return 0;
> +
> +    d_config->nics = g_new0(libxl_device_nic, nvnics);
> +    d_config->num_nics = nvnics;
> +
> +    for (i = 0; i < nvnics; i++)
> +        libxl_device_nic_init(&d_config->nics[i]);
>   
> -        if (libxlMakeNic(def, l_nics[i], &x_nics[nvnics], false))
> +    for (i = 0; i < nvnics; i++) {
> +        if (libxlMakeNic(def, l_nics[i], &d_config->nics[i], false))
>               goto error;
>           /*
>            * The devid (at least right now) will not get initialized by
>            * libxl in the setup case but is required for starting the
>            * device-model.
>            */
> -        if (x_nics[nvnics].devid < 0)
> -            x_nics[nvnics].devid = nvnics;
> -
> -        nvnics++;
> +        if (d_config->nics[i].devid < 0)
> +            d_config->nics[i].devid = i;
>       }

Same comment as the disk patch: initializing and making the NIC can occur in a 
single loop.

>   
> -    VIR_SHRINK_N(x_nics, nnics, nnics - nvnics);
> -    d_config->nics = x_nics;
> -    d_config->num_nics = nvnics;
> -
> -    return 0;
> +    ret = 0;
>   
>    error:
> -    for (i = 0; i < nnics; i++)
> -        libxl_device_nic_dispose(&x_nics[i]);
> -    VIR_FREE(x_nics);
> -    return -1;
> +    return ret;

Error label can be dropped here too.

Regards,
Jim