Re: [PATCH 3/4] network: rework networkGetNetworkAddress to make it can get IPv6 address

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

 




On 02/13/2015 02:17 AM, Luyao Huang wrote:
> Export the required helpers and rework networkGetNetworkAddress to
> make it can get IPv6 address.
> 
> Signed-off-by: Luyao Huang <lhuang@xxxxxxxxxx>
> ---
>  src/conf/network_conf.c     |  2 +-
>  src/conf/network_conf.h     |  1 +
>  src/libvirt_private.syms    |  1 +
>  src/network/bridge_driver.c | 50 ++++++++++++++++++++++++++++++++-------------
>  src/network/bridge_driver.h |  6 ++++--
>  src/qemu/qemu_command.c     |  4 ++--
>  6 files changed, 45 insertions(+), 19 deletions(-)
> 
> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
> index f947d89..9eb640b 100644
> --- a/src/conf/network_conf.c
> +++ b/src/conf/network_conf.c
> @@ -148,7 +148,7 @@ virNetworkDHCPHostDefClear(virNetworkDHCPHostDefPtr def)
>      VIR_FREE(def->name);
>  }
>  
> -static void
> +void

I believe this is unnecessary

>  virNetworkIpDefClear(virNetworkIpDefPtr def)
>  {
>      VIR_FREE(def->family);
> diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h
> index 484522e..0c4b559 100644
> --- a/src/conf/network_conf.h
> +++ b/src/conf/network_conf.h
> @@ -310,6 +310,7 @@ virNetworkObjPtr virNetworkFindByName(virNetworkObjListPtr nets,
>  void virNetworkDefFree(virNetworkDefPtr def);
>  void virNetworkObjFree(virNetworkObjPtr net);
>  void virNetworkObjListFree(virNetworkObjListPtr vms);
> +void virNetworkIpDefClear(virNetworkIpDefPtr def);

Same unnecessary

>  
>  
>  typedef bool (*virNetworkObjListFilter)(virConnectPtr conn,
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index f60911c..ff942d8 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -559,6 +559,7 @@ virNetworkDeleteConfig;
>  virNetworkFindByName;
>  virNetworkFindByUUID;
>  virNetworkForwardTypeToString;
> +virNetworkIpDefClear;

Unnecessary

>  virNetworkIpDefNetmask;
>  virNetworkIpDefPrefix;
>  virNetworkLoadAllConfigs;
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index 2798010..d1afd16 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -4487,6 +4487,7 @@ networkReleaseActualDevice(virDomainDefPtr dom,
>   * networkGetNetworkAddress:
>   * @netname: the name of a network
>   * @netaddr: string representation of IP address for that network.
> + * @family: IP family

   @want_ipv6: boolean to force return of IPv6 address for that network

>   *
>   * Attempt to return an IP (v4) address associated with the named
>   * network. If a libvirt virtual network, that will be provided in the
> @@ -4503,12 +4504,13 @@ networkReleaseActualDevice(virDomainDefPtr dom,
>   * completely unsupported.
>   */
>  int
> -networkGetNetworkAddress(const char *netname, char **netaddr)
> +networkGetNetworkAddress(const char *netname, char **netaddr, int family)

s/int family/bool want_ipv6/

>  {
>      int ret = -1;
>      virNetworkObjPtr network;
>      virNetworkDefPtr netdef;
> -    virNetworkIpDefPtr ipdef;
> +    virNetworkIpDefPtr ipdef = NULL;
> +    virNetworkIpDefPtr ipdef6 = NULL;

The ipdef6 is unnecessary

>      virSocketAddr addr;
>      virSocketAddrPtr addrptr = NULL;
>      char *dev_name = NULL;
> @@ -4529,15 +4531,9 @@ networkGetNetworkAddress(const char *netname, char **netaddr)
>      case VIR_NETWORK_FORWARD_NONE:
>      case VIR_NETWORK_FORWARD_NAT:
>      case VIR_NETWORK_FORWARD_ROUTE:
> -        /* if there's an ipv4def, get it's address */
> +        /* if there's an ipv4def or ipv6def, get it's address */
>          ipdef = virNetworkDefGetIpByIndex(netdef, AF_INET, 0);
> -        if (!ipdef) {
> -            virReportError(VIR_ERR_INTERNAL_ERROR,
> -                           _("network '%s' doesn't have an IPv4 address"),
> -                           netdef->name);
> -            break;
> -        }
> -        addrptr = &ipdef->address;
> +        ipdef6 = virNetworkDefGetIpByIndex(netdef, AF_INET6, 0);

Not sure I follow why we're losing the error reporting here...

I'd change this to:


    if (want_ipv6)
        ipdef = virNetworkDefGetIpByIndex(netdef, AF_INET6, 0);
    else
        ipdef = virNetworkDefGetIpByIndex(netdef, AF_INET, 0);
    if (!ipdef) {
        virReportError(VIR_ERR_INTERNAL_ERROR,
                      _("network '%s' doesn't have an '%s' address"),
                      netdef->name, want_ipv6 ? "IPv6" : "IPv4");
        break;
    }
    addrptr = &ipdef->address;

NB: virNetworkDefGetIpByIndex() doesn't return a COPY that we must FREE,
it returns a pointer to something owned elsewhere


>          break;
>  
>      case VIR_NETWORK_FORWARD_BRIDGE:
> @@ -4561,10 +4557,32 @@ networkGetNetworkAddress(const char *netname, char **netaddr)
>          break;
>      }
>  
> -    if (dev_name) {
> -        if (virNetDevGetIPv4Address(dev_name, &addr) < 0)
> -            goto error;
> -        addrptr = &addr;
> +    switch ((virDomainGraphicsListenFamily) family) {
> +    case VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_DEFAULT:
> +    case VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_IPV4:
> +        if (ipdef)
> +            addrptr = &ipdef->address;
> +        if (dev_name) {
> +            if (virNetDevGetIPv4Address(dev_name, &addr) < 0)
> +                goto error;
> +            addrptr = &addr;
> +        }
> +        /*if the family is default and we do not get a ipv4 address
> +         *in this place, we will try to get a ipv6 address
> +         */
> +        if (addrptr || family == VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_IPV4)
> +            break;
> +    case VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_IPV6:
> +        if (ipdef6)
> +            addrptr = &ipdef6->address;
> +        if (dev_name) {
> +            if (virNetDevGetIPv6Address(dev_name, &addr) < 0)
> +                goto error;
> +            addrptr = &addr;
> +        }
> +        break;
> +    case VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_LAST:
> +        break;

This code would then just be:

    if (dev_name) {
        if (virNetDevGetIPAddress(dev_name, &addr, want_ipv6) < 0)
            goto error;
        addrptr = &addr;
    }

While I can appreciate the logic to "default to" returning the IPv6
address if no IPv4 address was found, I think those details can/should
be left up to the caller to decide whether returning an IPv6 address is
acceptable. Falling through to a try to find/return an IPv6 address
while perhaps being "kind" could result in some caller getting something
it didn't expect and even worse, didn't know how to parse!


>      }
>  
>      if (!(addrptr &&
> @@ -4579,6 +4597,10 @@ networkGetNetworkAddress(const char *netname, char **netaddr)
>      return ret;
>  
>   error:
> +    if (ipdef6)
> +        virNetworkIpDefClear(ipdef6);
> +    if (ipdef)
> +        virNetworkIpDefClear(ipdef);

Since virNetworkDefGetIpByIndex is not creating a copy, but rather is
returning the "&def->ips[i];" value, I don't believe we want to do any
sort of clear here as it then becomes unusable for no reason.

>      goto cleanup;
>  }
>  
> diff --git a/src/network/bridge_driver.h b/src/network/bridge_driver.h
> index 2f801ee..c684c15 100644
> --- a/src/network/bridge_driver.h
> +++ b/src/network/bridge_driver.h
> @@ -44,7 +44,9 @@ int networkReleaseActualDevice(virDomainDefPtr dom,
>                                 virDomainNetDefPtr iface)
>      ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
>  
> -int networkGetNetworkAddress(const char *netname, char **netaddr)
> +int networkGetNetworkAddress(const char *netname,
> +                             char **netaddr,
> +                             int family)
>      ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
>  
>  int networkDnsmasqConfContents(virNetworkObjPtr network,
> @@ -57,7 +59,7 @@ int networkDnsmasqConfContents(virNetworkObjPtr network,
>  #  define networkAllocateActualDevice(dom, iface) 0
>  #  define networkNotifyActualDevice(dom, iface) (dom=dom, iface=iface, 0)
>  #  define networkReleaseActualDevice(dom, iface) (dom=dom, iface=iface, 0)
> -#  define networkGetNetworkAddress(netname, netaddr) (-2)
> +#  define networkGetNetworkAddress(netname, netaddr, family) (-2)
>  #  define networkDnsmasqConfContents(network, pidfile, configstr, \
>                      dctx, caps) 0
>  # endif
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 9c25788..6bd8f69 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -7277,7 +7277,7 @@ qemuBuildGraphicsVNCCommandLine(virQEMUDriverConfigPtr cfg,
>              listenNetwork = virDomainGraphicsListenGetNetwork(graphics, 0);
>              if (!listenNetwork)
>                  break;
> -            ret = networkGetNetworkAddress(listenNetwork, &netAddr);
> +            ret = networkGetNetworkAddress(listenNetwork, &netAddr, graphics->listens->family);
>              if (ret <= -2) {
>                  virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>                                 "%s", _("network-based listen not possible, "
> @@ -7441,7 +7441,7 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg,
>          listenNetwork = virDomainGraphicsListenGetNetwork(graphics, 0);
>          if (!listenNetwork)
>              break;
> -        ret = networkGetNetworkAddress(listenNetwork, &netAddr);
> +        ret = networkGetNetworkAddress(listenNetwork, &netAddr, graphics->listens->family);
>          if (ret <= -2) {
>              virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>                             "%s", _("network-based listen not possible, "
> 

These two turn into a boolean 3rd parameter:

(graphics->listens->ipv6 == VIR_TRISTATE_BOOL_YES)

and should be on a separate line to avoid the longer than 80 characters
in a line...

John

--
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]