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

Re: [libvirt] [PATCH 1/3] leasetime support for <dhcp> globally



Hello Peter,

Thanks for the comments, I will update the patch with your feedback, and sorry I thought I rebased it before I sent it, will fix that too.

2017-06-21 8:27 GMT+01:00 Peter Krempa <pkrempa redhat com>:
On Tue, Jun 20, 2017 at 19:00:43 +0100, aruiz gnome org wrote:
> From: Alberto Ruiz <aruiz gnome org>

Missing commit message.

>
> ---
>  docs/schemas/basictypes.rng                       | 16 +++++
>  docs/schemas/network.rng                          |  8 +++
>  src/conf/network_conf.c                           | 78 ++++++++++++++++++++++-
>  src/conf/network_conf.h                           |  3 +-
>  src/network/bridge_driver.c                       | 49 +++++++++++++-
>  tests/networkxml2confdata/leasetime-days.conf     | 17 +++++
>  tests/networkxml2confdata/leasetime-days.xml      | 18 ++++++
>  tests/networkxml2confdata/leasetime-hours.conf    | 17 +++++
>  tests/networkxml2confdata/leasetime-hours.xml     | 18 ++++++
>  tests/networkxml2confdata/leasetime-infinite.conf | 17 +++++
>  tests/networkxml2confdata/leasetime-infinite.xml  | 18 ++++++
>  tests/networkxml2confdata/leasetime-minutes.conf  | 17 +++++
>  tests/networkxml2confdata/leasetime-minutes.xml   | 18 ++++++
>  tests/networkxml2confdata/leasetime-seconds.conf  | 17 +++++
>  tests/networkxml2confdata/leasetime-seconds.xml   | 18 ++++++
>  tests/networkxml2confdata/leasetime.conf          | 17 +++++
>  tests/networkxml2confdata/leasetime.xml           | 18 ++++++
>  tests/networkxml2conftest.c                       |  7 ++
>  18 files changed, 368 insertions(+), 3 deletions(-)
>  create mode 100644 tests/networkxml2confdata/leasetime-days.conf
>  create mode 100644 tests/networkxml2confdata/leasetime-days.xml
>  create mode 100644 tests/networkxml2confdata/leasetime-hours.conf
>  create mode 100644 tests/networkxml2confdata/leasetime-hours.xml
>  create mode 100644 tests/networkxml2confdata/leasetime-infinite.conf
>  create mode 100644 tests/networkxml2confdata/leasetime-infinite.xml
>  create mode 100644 tests/networkxml2confdata/leasetime-minutes.conf
>  create mode 100644 tests/networkxml2confdata/leasetime-minutes.xml
>  create mode 100644 tests/networkxml2confdata/leasetime-seconds.conf
>  create mode 100644 tests/networkxml2confdata/leasetime-seconds.xml
>  create mode 100644 tests/networkxml2confdata/leasetime.conf
>  create mode 100644 tests/networkxml2confdata/leasetime.xml
>
> diff --git a/docs/schemas/basictypes.rng b/docs/schemas/basictypes.rng
> index 1b4f980e7..8a76c235a 100644
> --- a/docs/schemas/basictypes.rng
> +++ b/docs/schemas/basictypes.rng
> @@ -518,4 +518,20 @@
>      </element>
>    </define>
>
> +  <define name="leaseTimeUnit">
> +    <choice>
> +      <value>seconds</value>
> +      <value>minutes</value>
> +      <value>hours</value>
> +      <value>days</value>
> +    </choice>
> +  </define>
> +
> +  <define name="leaseTime">
> +    <data type="long">
> +      <param name="minInclusive">-1</param>
> +      <param name="maxInclusive">4294967295</param>
> +    </data>
> +  </define>
> +
>  </grammar>
> diff --git a/docs/schemas/network.rng b/docs/schemas/network.rng
> index 1a18e64b2..4b8056ab6 100644
> --- a/docs/schemas/network.rng
> +++ b/docs/schemas/network.rng
> @@ -340,6 +340,14 @@
>                <!-- Define the range(s) of IP addresses that the DHCP
>                     server should hand out -->
>                <element name="dhcp">
> +                <optional>
> +                  <element name="leasetime">
> +                    <optional>
> +                      <attribute name="unit"><ref name="leaseTimeUnit"/></attribute>

This does not follow the XML style used everywhere else.

> +                    </optional>
> +                    <ref name="leaseTime"/>
> +                  </element>
> +                </optional>
>                  <zeroOrMore>
>                    <element name="range">
>                      <attribute name="start"><ref name="ipAddr"/></attribute>
> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
> index aa397768c..6f051493f 100644
> --- a/src/conf/network_conf.c
> +++ b/src/conf/network_conf.c
> @@ -30,6 +30,8 @@
>  #include <fcntl.h>
>  #include <string.h>
>  #include <dirent.h>
> +#include <stdlib.h>
> +#include <inttypes.h>
>
>  #include "virerror.h"
>  #include "datatypes.h"
> @@ -1031,12 +1033,82 @@ virNetworkDHCPHostDefParseXML(const char *networkName,
>      return ret;
>  }
>
> +static int64_t
> +virNetworkDHCPDefGetLeaseTime (xmlNodePtr node,
> +                               xmlXPathContextPtr ctxt,
> +                               bool *error)

We usually return error from the function and if necessary return the
value through pointer in arguments (backwards as you did).

> +{
> +    int64_t multiplier, result = -1;
> +    char *leaseString, *leaseUnit;
> +    xmlNodePtr save;
> +
> +    *error = 0;
> +
> +    save = ctxt->node;
> +    ctxt->node = node;
> +
> +    leaseString = virXPathString ("string(./leasetime/text())", ctxt);
> +    leaseUnit   = virXPathString ("string(./leasetime/@unit)", ctxt);
> +
> +    /* If value is not present we set the value to -2 */
> +    if (leaseString == NULL) {
> +        result = -2;
> +        goto cleanup;
> +    }
> +
> +    if (leaseUnit == NULL || strcmp (leaseUnit, "seconds") == 0)
> +        multiplier = 1;
> +    else if (strcmp (leaseUnit, "minutes") == 0)
> +        multiplier = 60;
> +    else if (strcmp (leaseUnit, "hours") == 0)
> +        multiplier = 60 * 60;
> +    else if (strcmp (leaseUnit, "days") == 0)
> +        multiplier = 60 * 60 * 24;
> +    else {
> +        virReportError(VIR_ERR_XML_ERROR,
> +                       _("invalid value for unit parameter in <leasetime> element found in <dhcp> network, "
> +                         "only 'seconds', 'minutes', 'hours' or 'days' are valid: %s"),
> +                       leaseUnit);
> +        *error = 1;
> +        goto cleanup;
> +    }

Does not follow libvirt coding style.

> +
> +    errno = 0;
> +    result = (int64_t) strtoll((const char*)leaseString, NULL, 10);
> +
> +    /* Report any errors parsing the string */
> +    if (errno != 0) {
> +        virReportError(VIR_ERR_XML_ERROR,
> +                       _("<leasetime> value could not be converted to a signed integer: %s"),
> +                      leaseString);
> +        *error = 1;
> +        goto cleanup;
> +    }
> +
> +    result = result * multiplier;
> +
> +    if (result > UINT32_MAX) {
> +        virReportError(VIR_ERR_XML_ERROR,
> +                       _("<leasetime> value cannot be greater than the equivalent of %" PRIo32 " seconds : %" PRId64),
> +                       UINT32_MAX, result);

Lines are too long

> +        *error = 1;
> +    }
> +
> +cleanup:
> +    VIR_FREE(leaseString);
> +    VIR_FREE(leaseUnit);
> +    ctxt->node = save;
> +    return result;
> +}
> +
>  static int
>  virNetworkDHCPDefParseXML(const char *networkName,
>                            xmlNodePtr node,
> +                          xmlXPathContextPtr ctxt,
>                            virNetworkIPDefPtr def)
>  {
>      int ret = -1;
> +    bool error;
>      xmlNodePtr cur;
>      virSocketAddrRange range;
>      virNetworkDHCPHostDef host;
> @@ -1044,6 +1116,10 @@ virNetworkDHCPDefParseXML(const char *networkName,
>      memset(&range, 0, sizeof(range));
>      memset(&host, 0, sizeof(host));
>
> +    def->leasetime = virNetworkDHCPDefGetLeaseTime (node, ctxt, &error);

This won't pass syntax-check

> +    if (error)
> +        goto cleanup;
> +
>      cur = node->children;
>      while (cur != NULL) {
>          if (cur->type == XML_ELEMENT_NODE &&
> @@ -1607,7 +1683,7 @@ virNetworkIPDefParseXML(const char *networkName,
>      while (cur != NULL) {
>          if (cur->type == XML_ELEMENT_NODE &&
>              xmlStrEqual(cur->name, BAD_CAST "dhcp")) {
> -            if (virNetworkDHCPDefParseXML(networkName, cur, def) < 0)
> +            if (virNetworkDHCPDefParseXML(networkName, cur, ctxt, def) < 0)
>                  goto cleanup;
>          } else if (cur->type == XML_ELEMENT_NODE &&
>                     xmlStrEqual(cur->name, BAD_CAST "tftp")) {
> diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h
> index 3b227db6f..f7557f581 100644
> --- a/src/conf/network_conf.h
> +++ b/src/conf/network_conf.h
> @@ -170,7 +170,8 @@ struct _virNetworkIPDef {
>      char *tftproot;
>      char *bootfile;
>      virSocketAddr bootserver;
> -   };
> +    int64_t leasetime;
> +};
>
>  typedef struct _virNetworkForwardIfDef virNetworkForwardIfDef;
>  typedef virNetworkForwardIfDef *virNetworkForwardIfDefPtr;
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index 7b99acafa..e51e469c8 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -41,6 +41,8 @@
>  #include <sys/ioctl.h>
>  #include <net/if.h>
>  #include <dirent.h>
> +#include <inttypes.h>
> +#include <stdint.h>
>  #if HAVE_SYS_SYSCTL_H
>  # include <sys/sysctl.h>
>  #endif
> @@ -903,6 +905,40 @@ networkBuildDnsmasqHostsList(dnsmasqContext *dctx,
>      return 0;
>  }
>
> +/* translates the leasetime value into a dnsmasq configuration string for dhcp-range/host */
> +static char *
> +networkDnsmasqConfLeaseValueToString (int64_t leasetime)
> +{
> +    char *result = NULL;
> +    virBuffer leasebuf = VIR_BUFFER_INITIALIZER;
> +
> +    /* Leasetime parameter set on the XML */
> +    /* Less than -1 means there was no value set */
> +    if (leasetime < -1) {
> +        virBufferAsprintf(&leasebuf, "%s", "");
> +    }
> +    /* -1 means no expiration */
> +    else if (leasetime == -1)
> +        virBufferAsprintf(&leasebuf, ",infinite");
> +    /* Minimum expiry value is 120 */
> +    /* TODO: Discuss if we should up as we do here or just forward whatever value to dnsmasq */
> +    else if (leasetime < 120)
> +        virBufferAsprintf(&leasebuf, ",120");
> +    /* DHCP value for lease time is a unsigned four octect integer */
> +    else if (leasetime <= UINT32_MAX)
> +        virBufferAsprintf(&leasebuf, ",%" PRId64, leasetime);
> +    /* TODO: Discuss the use of "deprecated" for ipv6*/
> +    /* TODO: Discuss what is the default value that we want as dnsmasq's is 1 hour */
> +    /* TODO: Discuss what to do if value exceeds maximum, use default value for now */
> +    else {
> +        virBufferAsprintf(&leasebuf, "%s", "");
> +    }
> +
> +    result = virBufferContentAndReset(&leasebuf);
> +    virBufferFreeAndReset (&leasebuf);
> +
> +    return result;
> +}
>
>  int
>  networkDnsmasqConfContents(virNetworkObjPtr network,
> @@ -1213,6 +1249,7 @@ networkDnsmasqConfContents(virNetworkObjPtr network,
>          }
>          for (r = 0; r < ipdef->nranges; r++) {
>              int thisRange;
> +            char *leasestr;
>
>              if (!(saddr = virSocketAddrFormat(&ipdef->ranges[r].start)) ||
>                  !(eaddr = virSocketAddrFormat(&ipdef->ranges[r].end)))
> @@ -1220,12 +1257,22 @@ networkDnsmasqConfContents(virNetworkObjPtr network,
>
>              virBufferAsprintf(&configbuf, "dhcp-range=%s,%s",
>                                saddr, eaddr);
> -            if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET6))
> +
> +            /* Add ipv6 prefix length parameter if needed */
> +            if (ipdef == ipv6def)
>                  virBufferAsprintf(&configbuf, ",%d", prefix);
> +
> +            leasestr = networkDnsmasqConfLeaseValueToString (ipdef->leasetime);
> +            if (!leasestr)
> +                goto cleanup;
> +            virBufferAsprintf(&configbuf, "%s", leasestr);
> +
> +            /* Add the newline */
>              virBufferAddLit(&configbuf, "\n");
>
>              VIR_FREE(saddr);
>              VIR_FREE(eaddr);
> +            VIR_FREE(leasestr);
>              thisRange = virSocketAddrGetRange(&ipdef->ranges[r].start,
>                                                &ipdef->ranges[r].end,
>                                                &ipdef->address,

Also this patch does not apply to current master.

Peter



--
Cheers,
Alberto Ruiz

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