[libvirt] [PATCH 2/3] lease time support per host in dnsmasq

[Laine Stump]

On 06/20/2017 02:00 PM, aruiz at gnome.org wrote:
> From: Alberto Ruiz <aruiz at gnome.org>
> 
> ---
>  src/network/bridge_driver.c | 56 ++++++++++++++++++++++++++-------------------
>  src/util/virdnsmasq.c       | 52 +++++++++++++++++++----------------------
>  src/util/virdnsmasq.h       |  1 +
>  3 files changed, 57 insertions(+), 52 deletions(-)

As far as I can see, this doesn't set a different lease time for each
static host entry (which is what the title implies), but just sets the
single specified leasetime for *all* static host entries.

Aside from that, I'm not sure what is the value of setting a leastime on
a static host entry. An explanation of that in the commit log would be
helpful in determining whether or not there's even a point to doing this.

Also, I forgot to say it wrt to the previous patch, but you need to add
something to docs/formatnetwork.html.in to document the new config knob.

> 
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index e51e469c8..1cffd4dcf 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -861,30 +861,6 @@ networkKillDaemon(pid_t pid, const char *daemonName, const char *networkName)
>      return ret;
>  }
>  
> -/* the following does not build a file, it builds a list
> - * which is later saved into a file
> - */
> -
> -static int
> -networkBuildDnsmasqDhcpHostsList(dnsmasqContext *dctx,
> -                                 virNetworkIPDefPtr ipdef)
> -{
> -    size_t i;
> -    bool ipv6 = false;
> -
> -    if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET6))
> -        ipv6 = true;
> -    for (i = 0; i < ipdef->nhosts; i++) {
> -        virNetworkDHCPHostDefPtr host = &(ipdef->hosts[i]);
> -        if (VIR_SOCKET_ADDR_VALID(&host->ip))
> -            if (dnsmasqAddDhcpHost(dctx, host->mac, &host->ip,
> -                                   host->name, host->id, ipv6) < 0)
> -                return -1;
> -    }
> -
> -    return 0;
> -}
> -
>  static int
>  networkBuildDnsmasqHostsList(dnsmasqContext *dctx,
>                               virNetworkDNSDefPtr dnsdef)
> @@ -940,6 +916,38 @@ networkDnsmasqConfLeaseValueToString (int64_t leasetime)
>      return result;
>  }
>  
> +/* the following does not build a file, it builds a list
> + * which is later saved into a file
> + */
> +
> +static int
> +networkBuildDnsmasqDhcpHostsList(dnsmasqContext *dctx,
> +                                 virNetworkIPDefPtr ipdef)
> +{
> +    int ret = -1;
> +    size_t i;
> +    bool ipv6 = false;
> +    char *leasetime = networkDnsmasqConfLeaseValueToString(ipdef->leasetime);
> +
> +    if (!leasetime)
> +        goto cleanup;
> +
> +    if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET6))
> +        ipv6 = true;
> +    for (i = 0; i < ipdef->nhosts; i++) {
> +        virNetworkDHCPHostDefPtr host = &(ipdef->hosts[i]);
> +        if (VIR_SOCKET_ADDR_VALID(&host->ip))
> +            if (dnsmasqAddDhcpHost(dctx, host->mac, &host->ip,
> +                                   host->name, host->id, leasetime, ipv6) < 0)
> +                goto cleanup;
> +    }
> +
> +    ret = 0;
> +cleanup:
> +    VIR_FREE(leasetime);
> +    return ret;
> +}
> +
>  int
>  networkDnsmasqConfContents(virNetworkObjPtr network,
>                             const char *pidfile,
> diff --git a/src/util/virdnsmasq.c b/src/util/virdnsmasq.c
> index 1b78c1fad..92f834fe7 100644
> --- a/src/util/virdnsmasq.c
> +++ b/src/util/virdnsmasq.c
> @@ -308,52 +308,47 @@ hostsfileAdd(dnsmasqHostsfile *hostsfile,
>               virSocketAddr *ip,
>               const char *name,
>               const char *id,
> +             const char *leasetime,
>               bool ipv6)
>  {
> +    int ret = -1;
>      char *ipstr = NULL;
> +    virBuffer hostbuf = VIR_BUFFER_INITIALIZER;
> +
>      if (VIR_REALLOC_N(hostsfile->hosts, hostsfile->nhosts + 1) < 0)
>          goto error;
>  
>      if (!(ipstr = virSocketAddrFormat(ip)))
> -        return -1;
> +        goto error;
>  
>      /* the first test determines if it is a dhcpv6 host */
>      if (ipv6) {
> -        if (name && id) {
> -            if (virAsprintf(&hostsfile->hosts[hostsfile->nhosts].host,
> -                            "id:%s,%s,[%s]", id, name, ipstr) < 0)
> -                goto error;
> -        } else if (name && !id) {
> -            if (virAsprintf(&hostsfile->hosts[hostsfile->nhosts].host,
> -                            "%s,[%s]", name, ipstr) < 0)
> -                goto error;
> -        } else if (!name && id) {
> -            if (virAsprintf(&hostsfile->hosts[hostsfile->nhosts].host,
> -                            "id:%s,[%s]", id, ipstr) < 0)
> -                goto error;
> -        }
> +        if (name && id)
> +            virBufferAsprintf(&hostbuf, "id:%s,%s,[%s]", id, name, ipstr);
> +        else if (name && !id)
> +            virBufferAsprintf(&hostbuf, "%s,[%s]", name, ipstr);
> +        else if (!name && id)
> +            virBufferAsprintf(&hostbuf, "id:%s,[%s]", id, ipstr);
>      } else if (name && mac) {
> -        if (virAsprintf(&hostsfile->hosts[hostsfile->nhosts].host, "%s,%s,%s",
> -                        mac, ipstr, name) < 0)
> -            goto error;
> +        virBufferAsprintf(&hostbuf, "%s,%s,%s", mac, ipstr, name);
>      } else if (name && !mac) {
> -        if (virAsprintf(&hostsfile->hosts[hostsfile->nhosts].host, "%s,%s",
> -                        name, ipstr) < 0)
> -            goto error;
> +        virBufferAsprintf(&hostbuf, "%s,%s", name, ipstr);
>      } else {
> -        if (virAsprintf(&hostsfile->hosts[hostsfile->nhosts].host, "%s,%s",
> -                        mac, ipstr) < 0)
> -            goto error;
> +        virBufferAsprintf(&hostbuf, "%s,%s", mac, ipstr);
>      }
> -    VIR_FREE(ipstr);
>  
> -    hostsfile->nhosts++;
> +    /* The leasetime string already includes comma if there's any value at all */
> +    virBufferAsprintf(&hostbuf, "%s", leasetime);
>  
> -    return 0;
> +    if (!(hostsfile->hosts[hostsfile->nhosts].host = virBufferContentAndReset (&hostbuf)))
> +      goto error;
>  
> +    hostsfile->nhosts++;
> +    ret = 0;
>   error:
> +    virBufferFreeAndReset(&hostbuf);
>      VIR_FREE(ipstr);
> -    return -1;
> +    return ret;
>  }
>  
>  static dnsmasqHostsfile *
> @@ -524,9 +519,10 @@ dnsmasqAddDhcpHost(dnsmasqContext *ctx,
>                     virSocketAddr *ip,
>                     const char *name,
>                     const char *id,
> +                   const char *leasetime,
>                     bool ipv6)
>  {
> -    return hostsfileAdd(ctx->hostsfile, mac, ip, name, id, ipv6);
> +    return hostsfileAdd(ctx->hostsfile, mac, ip, name, id, leasetime, ipv6);
>  }
>  
>  /*
> diff --git a/src/util/virdnsmasq.h b/src/util/virdnsmasq.h
> index f47bea3ab..c3ea271d4 100644
> --- a/src/util/virdnsmasq.h
> +++ b/src/util/virdnsmasq.h
> @@ -88,6 +88,7 @@ int              dnsmasqAddDhcpHost(dnsmasqContext *ctx,
>                                      virSocketAddr *ip,
>                                      const char *name,
>                                      const char *id,
> +                                    const char *leastime,
>                                      bool ipv6);
>  int              dnsmasqAddHost(dnsmasqContext *ctx,
>                                  virSocketAddr *ip,
>