Re: [PATCH v4] libxl: adjust handling of libxl_device_nic objects

[Jim Fehlig]

On 5/25/21 3:55 PM, Olaf Hering wrote:

Am Tue, 25 May 2021 23:40:59 +0200
schrieb Olaf Hering :


In case of error the objects are disposed by libxl_domain_config_dispose.


I just realized this is not always true, unless this change is applied on top.
If libxlMakeNic fails, libxl_domain_config_dispose can not release anything.
With this additional change each successfully initialized object can be fully 
disposed.

Olaf

--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -1523,13 +1523,14 @@ libxlMakeNicList(virDomainDef *def,  
libxl_domain_config *d_config)
  libxl_device_nic *x_nics;
  size_t i, nvnics = 0;
  
-x_nics = g_new0(libxl_device_nic, nnics);

+d_config->nics = 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;
  
  libxl_device_nic_init(_nics[nvnics]);

+d_config->num_nics = nvnics + 1;


These changes make the function a little more difficult to read IMO. How about 
if we keep the goto, but relabel it to 'out' (another common pattern in 
libvirt)? E.g.


https://listman.redhat.com/archives/libvir-list/2021-May/msg00842.html

Regards,
Jim


  if (libxlMakeNic(def, l_nics[i], _nics[nvnics], false))
  return -1;
  /*
@@ -1544,8 +1545,6 @@ libxlMakeNicList(virDomainDef *def,  libxl_domain_config 
*d_config)
  }
  
  VIR_SHRINK_N(x_nics, nnics, nnics - nvnics);

-d_config->nics = x_nics;
-d_config->num_nics = nvnics;
  
  return 0;

  }

Re: [PATCH v4] libxl: adjust handling of libxl_device_nic objects

[Olaf Hering]

Am Tue, 25 May 2021 23:40:59 +0200
schrieb Olaf Hering :

> In case of error the objects are disposed by libxl_domain_config_dispose.

I just realized this is not always true, unless this change is applied on top.
If libxlMakeNic fails, libxl_domain_config_dispose can not release anything.
With this additional change each successfully initialized object can be fully 
disposed.

Olaf

--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -1523,13 +1523,14 @@ libxlMakeNicList(virDomainDef *def,  
libxl_domain_config *d_config)
 libxl_device_nic *x_nics;
 size_t i, nvnics = 0;
 
-x_nics = g_new0(libxl_device_nic, nnics);
+d_config->nics = 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;
 
 libxl_device_nic_init(_nics[nvnics]);
+d_config->num_nics = nvnics + 1;
 if (libxlMakeNic(def, l_nics[i], _nics[nvnics], false))
 return -1;
 /*
@@ -1544,8 +1545,6 @@ libxlMakeNicList(virDomainDef *def,  libxl_domain_config 
*d_config)
 }
 
 VIR_SHRINK_N(x_nics, nnics, nnics - nvnics);
-d_config->nics = x_nics;
-d_config->num_nics = nvnics;
 
 return 0;
 }


pgp2mzaQmza8D.pgp
Description: Digitale Signatur von OpenPGP

[PATCH v4] libxl: adjust handling of libxl_device_nic objects

[Olaf Hering]

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 
---
 src/libxl/libxl_conf.c | 11 ++-
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index 2ecbcf6911..c672bafbe9 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -1335,8 +1335,6 @@ libxlMakeNic(virDomainDef *def,
 return -1;
 }
 
-libxl_device_nic_init(x_nic);
-
 virMacAddrGetRaw(_nic->mac, x_nic->mac);
 
 /*
@@ -1531,8 +1529,9 @@ libxlMakeNicList(virDomainDef *def,  libxl_domain_config 
*d_config)
 if (virDomainNetGetActualType(l_nics[i]) == 
VIR_DOMAIN_NET_TYPE_HOSTDEV)
 continue;
 
+libxl_device_nic_init(_nics[nvnics]);
 if (libxlMakeNic(def, l_nics[i], _nics[nvnics], false))
-goto error;
+return -1;
 /*
  * The devid (at least right now) will not get initialized by
  * libxl in the setup case but is required for starting the
@@ -1549,12 +1548,6 @@ libxlMakeNicList(virDomainDef *def,  libxl_domain_config 
*d_config)
 d_config->num_nics = nvnics;
 
 return 0;
-
- error:
-for (i = 0; i < nnics; i++)
-libxl_device_nic_dispose(_nics[i]);
-VIR_FREE(x_nics);
-return -1;
 }
 
 int