Re: [PATCH v2 10/23] domain-conf: add network def private data

[Michal Privoznik <mprivozn@xxxxxxxxxx>]

On 8/8/19 4:55 PM, marcandre.lureau@xxxxxxxxxx wrote:
> From: Marc-André Lureau <marcandre.lureau@xxxxxxxxxx>
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@xxxxxxxxxx>
> ---
>   src/conf/domain_conf.c | 21 ++++++++++++++++++++-
>   src/conf/domain_conf.h |  6 ++++++
>   2 files changed, 26 insertions(+), 1 deletion(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 0456369d55..fb0904177f 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -2454,6 +2454,7 @@ virDomainNetDefFree(virDomainNetDefPtr def)
>       if (!def)
>           return;
>       virDomainNetDefClear(def);
> +    virObjectUnref(def->privateData);
>       VIR_FREE(def);
>   }
>   
> @@ -11441,7 +11442,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
>       VIR_AUTOFREE(char *) trustGuestRxFilters = NULL;
>       VIR_AUTOFREE(char *) vhost_path = NULL;
>   
> -    if (VIR_ALLOC(def) < 0)
> +    if (!(def = virDomainNetDefNew(xmlopt)))
>           return NULL;
>   
>       ctxt->node = node;
> @@ -14337,6 +14338,24 @@ virDomainGraphicsDefNew(virDomainXMLOptionPtr xmlopt)
>   }
>   
>   
> +virDomainNetDefPtr
> +virDomainNetDefNew(virDomainXMLOptionPtr xmlopt)
> +{
> +    virDomainNetDefPtr def = NULL;
> +
> +    if (VIR_ALLOC(def) < 0)
> +        return NULL;
> +
> +    if (xmlopt && xmlopt->privateData.networkNew &&
> +        !(def->privateData = xmlopt->privateData.networkNew())) {
> +        VIR_FREE(def);
> +        def = NULL;

This call to 'def = NULL' is not needed. VIR_FREE() does that for us. However, I actually prefer using virDomainNetDefFree() as that is more fool proof if somebody ever allocs something else in @def after VIR_ALLOC() and before these lines.

> +    }
> +
> +    return def;
> +}
> +
> +
>   /* Parse the XML definition for a graphics device */
>   static virDomainGraphicsDefPtr
>   virDomainGraphicsDefParseXML(virDomainXMLOptionPtr xmlopt,
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 57ca2a8ad1..9bd196b53c 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -1018,6 +1018,7 @@ struct _virDomainNetDef {
>       unsigned int mtu;
>       virNetDevCoalescePtr coalesce;
>       virDomainVirtioOptionsPtr virtio;
> +    virObjectPtr privateData;
>   };
>   
>   typedef enum {
> @@ -2711,6 +2712,7 @@ struct _virDomainXMLPrivateDataCallbacks {
>       virDomainXMLPrivateDataNewFunc    chrSourceNew;
>       virDomainXMLPrivateDataNewFunc    vsockNew;
>       virDomainXMLPrivateDataNewFunc    graphicsNew;
> +    virDomainXMLPrivateDataNewFunc    networkNew;
>       virDomainXMLPrivateDataFormatFunc format;
>       virDomainXMLPrivateDataParseFunc  parse;
>       /* following function shall return a pointer which will be used as the
> @@ -2894,6 +2896,10 @@ virDomainChrDefPtr virDomainChrDefNew(virDomainXMLOptionPtr xmlopt);
>   
>   virDomainGraphicsDefPtr
>   virDomainGraphicsDefNew(virDomainXMLOptionPtr xmlopt);
> +
> +virDomainNetDefPtr
> +virDomainNetDefNew(virDomainXMLOptionPtr xmlopt);

This function must be exposed in libvirt_private.syms too.

I've identified other places where we VIR_ALLOC() a virDomainNetDef structure:

bhyveParsePCINet(), xenParseVif(), lxcCreateNetDef() and vboxDumpNetwork()

In some of them we already have xmlopt available, in others we might pass NULL safely:

diff --git c/src/bhyve/bhyve_parse_command.c w/src/bhyve/bhyve_parse_command.c
index 490381688c..7d460e9824 100644
--- c/src/bhyve/bhyve_parse_command.c
+++ w/src/bhyve/bhyve_parse_command.c
@@ -501,7 +501,7 @@ bhyveParsePCINet(virDomainDefPtr def,
     const char *separator = NULL;
     const char *mac = NULL;
 
-    if (VIR_ALLOC(net) < 0)
+    if (!(net = virDomainNetDefNew(xmlopt)))
         goto cleanup;
 
     /* As we only support interface type='bridge' and cannot
diff --git c/src/libvirt_private.syms w/src/libvirt_private.syms
index a34d92f5ef..f1fe7259f9 100644
--- c/src/libvirt_private.syms
+++ w/src/libvirt_private.syms
@@ -478,6 +478,7 @@ virDomainNetDefActualToNetworkPort;
 virDomainNetDefClear;
 virDomainNetDefFormat;
 virDomainNetDefFree;
+virDomainNetDefNew;
 virDomainNetDefToNetworkPort;
 virDomainNetFind;
 virDomainNetFindByName;
diff --git c/src/libxl/xen_common.c w/src/libxl/xen_common.c
index 7eb52c8c84..d327f03d73 100644
--- c/src/libxl/xen_common.c
+++ w/src/libxl/xen_common.c
@@ -1234,7 +1234,7 @@ xenParseVif(char *entry, const char *vif_typename)
         key = nextkey;
     }
 
-    if (VIR_ALLOC(net) < 0)
+    if (!(net = virDomainNetDefNew(NULL)))
         goto cleanup;
 
     if (mac[0]) {
diff --git c/src/lxc/lxc_native.c w/src/lxc/lxc_native.c
index b4c6e790d8..018eec6977 100644
--- c/src/lxc/lxc_native.c
+++ w/src/lxc/lxc_native.c
@@ -359,7 +359,7 @@ lxcCreateNetDef(const char *type,
     virDomainNetDefPtr net = NULL;
     virMacAddr macAddr;
 
-    if (VIR_ALLOC(net) < 0)
+    if (!(net = virDomainNetDefNew(NULL)))
         goto error;
 
     if (STREQ_NULLABLE(flag, "up"))
diff --git c/src/vbox/vbox_common.c w/src/vbox/vbox_common.c
index 49e657cdb7..ddabcb80ca 100644
--- c/src/vbox/vbox_common.c
+++ w/src/vbox/vbox_common.c
@@ -3692,7 +3692,7 @@ vboxDumpNetwork(vboxDriverPtr data, INetworkAdapter *adapter)
     char *utf8 = NULL;
     virDomainNetDefPtr net = NULL;
 
-    if (VIR_ALLOC(net) < 0)
+    if (!(net = virDomainNetDefNew(data->xmlopt)))
         return NULL;
 
     gVBoxAPI.UINetworkAdapter.GetAttachmentType(adapter, &attachmentType);


With this squashed in:

Reviewed-by: Michal Privoznik <mprivozn@xxxxxxxxxx>

Michal

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list