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.

Signed-off-by: Olaf Hering <olaf@aepfle.de>
---
 src/libxl/libxl_conf.c | 34 +++++++++++++---------------------
 1 file changed, 13 insertions(+), 21 deletions(-)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index f29a28a841..8dc7e26cea 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -1339,8 +1339,6 @@ libxlMakeNic(virDomainDef *def,
         return -1;
     }
 
-    libxl_device_nic_init(x_nic);
-
     virMacAddrGetRaw(&l_nic->mac, x_nic->mac);
 
     /*
@@ -1526,39 +1524,33 @@ 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);
-
     for (i = 0; i < nnics; i++) {
         if (virDomainNetGetActualType(l_nics[i]) == VIR_DOMAIN_NET_TYPE_HOSTDEV)
             continue;
+        nvnics++;
+    }
+    if (!nvnics)
+        return 0;
 
-        if (libxlMakeNic(def, l_nics[i], &x_nics[nvnics], false))
-            goto error;
+    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], &d_config->nics[i], false))
+            return -1;
         /*
          * 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;
-
- error:
-    for (i = 0; i < nnics; i++)
-        libxl_device_nic_dispose(&x_nics[i]);
-    VIR_FREE(x_nics);
-    return -1;
 }
 
 int

On 5/20/21 4:31 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.
> 
> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> ---
>   src/libxl/libxl_conf.c | 34 +++++++++++++---------------------
>   1 file changed, 13 insertions(+), 21 deletions(-)
> 
> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> index f29a28a841..8dc7e26cea 100644
> --- a/src/libxl/libxl_conf.c
> +++ b/src/libxl/libxl_conf.c
> @@ -1339,8 +1339,6 @@ libxlMakeNic(virDomainDef *def,
>           return -1;
>       }
>   
> -    libxl_device_nic_init(x_nic);
> -
>       virMacAddrGetRaw(&l_nic->mac, x_nic->mac);
>   
>       /*
> @@ -1526,39 +1524,33 @@ 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);
> -
>       for (i = 0; i < nnics; i++) {
>           if (virDomainNetGetActualType(l_nics[i]) == VIR_DOMAIN_NET_TYPE_HOSTDEV)
>               continue;
> +        nvnics++;
> +    }
> +    if (!nvnics)
> +        return 0;
>   
> -        if (libxlMakeNic(def, l_nics[i], &x_nics[nvnics], false))
> -            goto error;
> +    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], &d_config->nics[i], false))

There's actually a subtle bug with this logic change. E.g. the list of libvirt 
NICs could be larger than libxl NICs, in cases where the former contains hostdev 
NICs. It is probably best to stick with the exiting logic and just fixup the 
handling of libxl NIC initialization and disposal.

Regards,
Jim

> +            return -1;
>           /*
>            * 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;
> -
> - error:
> -    for (i = 0; i < nnics; i++)
> -        libxl_device_nic_dispose(&x_nics[i]);
> -    VIR_FREE(x_nics);
> -    return -1;
>   }
>   
>   int
> 

Am Fri, 21 May 2021 10:49:15 -0600
schrieb Jim Fehlig <jfehlig@suse.com>:

> There's actually a subtle bug with this logic change.

Well, the devid index number used to depend on "nnics", now it would depend on "nvnics". That might be an incompatible change indeed.

Olaf