Re: [PATCH 15/28] conf: single object containing list of IP addresses, list of routes

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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__ */
> 

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



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]