[libvirt] [PATCH 15/28] conf: single object containing list of IP addresses, list of routes
John Ferlan
jferlan at redhat.com
Fri Jun 24 11:44:38 UTC 2016
On 06/22/2016 01:37 PM, Laine Stump wrote:
> There are currently two places in the domain where this combination is
> used, and there is about to be another. This patch puts them together
> for brevity and uniformity.
>
> As with the newly-renamed virNetDevIPAddr and virNetDevIPRoute
> objects, the new virNetDevIPInfo object will need to be accessed by a
> utility function that calls low level Netlink functions (so we don't
> want it to be in the conf directory) and will be called from multiple
> hypervisor drivers (so it can't be in any hypervisor directory); the
> most appropriate place is thus once again the util directory.
>
> The parse and format functions are in conf/domain_conf.c because only
> the domain XML (i.e. *not* the network XML) has this exact combination
> of IP addresses plus routes. They will end up being static, but since
> they aren't being called yet, they are declared right before their
> definition to avoid a "defined but not used" compile error. That will
> be changed to static once they are in use. Likewise
> virDomainNetIPInfoFormat() will end up being the only caller to
> virDomainNetRoutesFormat() and virDomainNetIPsFormat(), so it will
> just subsume those functions, but we can't do that until they are no
> longer called.
>
> (It would have been nice to include the interface name within the
> virNetDevIPInfo object (with a slight name change), but that can't
> be done cleanly, because in each case the interface name is provided
> in a different place in the XML relative to the routes and IP
> addresses, so putting it in this object would actually make the code
> more confused rather than simpler).
> ---
> docs/schemas/domaincommon.rng | 27 +++++++++++++++++
> src/conf/domain_conf.c | 68 +++++++++++++++++++++++++++++++++++++++++++
> src/libvirt_private.syms | 1 +
> src/util/virnetdevip.c | 16 ++++++++++
> src/util/virnetdevip.h | 11 +++++++
> 5 files changed, 123 insertions(+)
>
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index b81b558..ab89dab 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -2629,6 +2629,33 @@
> </optional>
> </interleave>
> </define>
> +
> + <!--
> + All ip-related info for either the host or guest side of an interface
> + -->
> + <define name="interface-ip-info">
> + <zeroOrMore>
> + <element name="ip">
> + <attribute name="address">
> + <ref name="ipAddr"/>
> + </attribute>
> + <optional>
> + <attribute name="family">
> + <ref name="addr-family"/>
> + </attribute>
> + </optional>
> + <optional>
> + <attribute name="prefix">
> + <ref name="ipPrefix"/>
> + </attribute>
> + </optional>
> + <empty/>
> + </element>
> + </zeroOrMore>
> + <zeroOrMore>
> + <ref name="route"/>
> + </zeroOrMore>
> + </define>
> <!--
> An emulator description is just a path to the binary used for the task
> -->
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index f380271..548c750 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -6173,6 +6173,58 @@ virDomainNetIPParseXML(xmlNodePtr node)
> return ret;
> }
>
> +
> +/* fill in a virNetDevIPInfoPtr from the <route> and <ip>
> + * elements found in the given XML context.
> + *
> + * return 0 on success (including none found) and -1 on failure.
> + */
> +int
> +virDomainNetIPInfoParseXML(const char *source,
> + xmlXPathContextPtr ctxt,
> + virNetDevIPInfoPtr def);
Why? If it needs to be "higher" to avoid the forward reference for
future callers, then so be it.
> +int
static int
> +virDomainNetIPInfoParseXML(const char *source,
> + xmlXPathContextPtr ctxt,
> + virNetDevIPInfoPtr def)
> +
> +{
> + xmlNodePtr *nodes = NULL;
> + virNetDevIPAddrPtr ip = NULL;
> + virNetDevIPRoutePtr route = NULL;
> + int nnodes;
> + int ret = -1;
> + size_t i;
> +
> + if ((nnodes = virXPathNodeSet("./ip", ctxt, &nodes)) < 0)
> + goto cleanup;
> +
> + for (i = 0; i < nnodes; i++) {
> + if (!(ip = virDomainNetIPParseXML(nodes[i])) ||
> + VIR_APPEND_ELEMENT(def->ips, def->nips, ip) < 0)
> + goto cleanup;
> + }
> + VIR_FREE(nodes);
> +
> + if ((nnodes = virXPathNodeSet("./route", ctxt, &nodes)) < 0)
> + goto cleanup;
> +
> + for (i = 0; i < nnodes; i++) {
> + if (!(route = virNetDevIPRouteParseXML(source, nodes[i], ctxt)) ||
> + VIR_APPEND_ELEMENT(def->routes, def->nroutes, route) < 0)
> + goto cleanup;
> + }
> +
> + ret = 0;
> + cleanup:
> + if (ret < 0)
> + virNetDevIPInfoClear(def);
> + VIR_FREE(ip);
Seeing just VIR_FREE(ip) made me go look at how it was allocated - guess
I was (now) concerned that something would be allocated into ip that
wasn't free'd properly (eg. no virNetDevIPFree() API ...
Anyway, the ip->address is written with the result of a getaddrinfo in
virSocketAddrParseInternal, which when free'd should be done by
freeaddrinfo, right?
I think this is existing, but fixable... at some point in time.
> + virNetDevIPRouteFree(route);
> + VIR_FREE(nodes);
> + return ret;
> +}
> +
> static int
> virDomainHostdevDefParseXMLCaps(xmlNodePtr node ATTRIBUTE_UNUSED,
> xmlXPathContextPtr ctxt,
> @@ -20311,6 +20363,22 @@ virDomainNetRoutesFormat(virBufferPtr buf,
> return 0;
> }
>
> +
> +int
> +virDomainNetIPInfoFormat(virBufferPtr buf,
> + virNetDevIPInfoPtr def);
Same complaint.
> +int
static int
ACK with the forward ref and static int used. I think you need a "new"
patch at some point in time to handle the getaddrinfo/freeaddrinfo...
John
> +virDomainNetIPInfoFormat(virBufferPtr buf,
> + virNetDevIPInfoPtr def)
> +{
> + if (virDomainNetIPsFormat(buf, def->ips, def->nips) < 0)
> + return -1;
> + if (virDomainNetRoutesFormat(buf, def->routes, def->nroutes) < 0)
> + return -1;
> + return 0;
> +}
> +
> +
> static int
> virDomainHostdevDefFormatSubsys(virBufferPtr buf,
> virDomainHostdevDefPtr def,
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 151cf9f..f6c3d45 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1925,6 +1925,7 @@ virNetDevBridgeSetVlanFiltering;
> virNetDevIPAddrAdd;
> virNetDevIPAddrDel;
> virNetDevIPAddrGet;
> +virNetDevIPInfoClear;
> virNetDevIPRouteAdd;
> virNetDevIPRouteFree;
> virNetDevIPRouteGetAddress;
> diff --git a/src/util/virnetdevip.c b/src/util/virnetdevip.c
> index 619f926..376d4ad 100644
> --- a/src/util/virnetdevip.c
> +++ b/src/util/virnetdevip.c
> @@ -845,3 +845,19 @@ virNetDevIPRouteGetGateway(virNetDevIPRoutePtr def)
> return &def->gateway;
> return NULL;
> }
> +
> +/* manipulating the virNetDevIPInfo object */
> +
> +void
> +virNetDevIPInfoClear(virNetDevIPInfoPtr ip)
> +{
> + size_t i;
> +
> + for (i = 0; i < ip->nips; i++)
> + VIR_FREE(ip->ips[i]);
> + VIR_FREE(ip->ips);
> +
> + for (i = 0; i < ip->nroutes; i++)
> + virNetDevIPRouteFree(ip->routes[i]);
> + VIR_FREE(ip->routes);
> +}
> diff --git a/src/util/virnetdevip.h b/src/util/virnetdevip.h
> index 7a07b73..be41636 100644
> --- a/src/util/virnetdevip.h
> +++ b/src/util/virnetdevip.h
> @@ -47,6 +47,14 @@ typedef struct {
> virSocketAddr gateway; /* gateway IP address for ip-route */
> } virNetDevIPRoute, *virNetDevIPRoutePtr;
>
> +/* A full set of all IP config info for a network device */
> +typedef struct {
> + size_t nips;
> + virNetDevIPAddrPtr *ips;
> + size_t nroutes;
> + virNetDevIPRoutePtr *routes;
> +} virNetDevIPInfo, *virNetDevIPInfoPtr;
> +
> /* manipulating/querying the netdev */
> int virNetDevIPAddrAdd(const char *ifname,
> virSocketAddr *addr,
> @@ -76,4 +84,7 @@ int virNetDevIPRouteGetPrefix(virNetDevIPRoutePtr def);
> unsigned int virNetDevIPRouteGetMetric(virNetDevIPRoutePtr def);
> virSocketAddrPtr virNetDevIPRouteGetGateway(virNetDevIPRoutePtr def);
>
> +/* virNetDevIPInfo object */
> +void virNetDevIPInfoClear(virNetDevIPInfoPtr ip);
> +
> #endif /* __VIR_NETDEVIP_H__ */
>
More information about the libvir-list
mailing list