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

Alberto Ruiz aruiz at gnome.org
Thu Jun 22 17:12:12 UTC 2017


2017-06-21 17:30 GMT+01:00 Laine Stump <laine at laine.org>:

> On 06/21/2017 03:27 AM, Peter Krempa wrote:
> > On Tue, Jun 20, 2017 at 19:00:43 +0100, aruiz at gnome.org wrote:
> >> From: Alberto Ruiz <aruiz at gnome.org>
> >
> > Missing commit message.
>
> And when composing the commit message, it's useful to include links to
> the associated BZ.
>
>   https://bugzilla.redhat.com/show_bug.cgi?id=913446
>
> Also, I recall there being quite a lot of discussion in email (and
> possibly IRC) about the fact that people *think* they want a
> configurable lease time because they think that will eliminate cases of
> a DHCP lease being lost while a domain is paused. It was pointed out
> that lengthening the lease will *not* eliminate that problem (it just
> makes it happen less often).
>
> As an alternate (and better) solution to the problem of lost leases, we
> then added the "dhcp-authoritative" option to dnsmasq (commit
> 4ac20b3ae4), which allows clients to re-acquire the same IP as they had
> for an expired lease (as long as it hasn't been acquired by someone else
> in the meantime, which is apparently unlikely unless all the other
> addresses in the pool are already assigned).
>
> I'm not saying this to discourage the idea of making leasetime
> configurable (I think we'd already agreed that it was reasonable to do
> so, but there were two competing patches posted, and neither of them was
> really push-ready), but just to make sure that nobody is disappointed if
> the results don't lead to the behavior they're hoping for.
>
>
> >
> >>
> >> ---
> >>  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>
>
> Maybe call this "timeUnit" in case some other attribute in the future
> needs it?
>
> >> +
> >> +  <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,
>
> While it's true that this function "gets" the lease time from the
> xmlDoc, this is part of the process of parsing the XML, so it should be
> named virNetworkDHCPLeaseTimeParseXML(). (it's never been clear to me
> when/where the "Def" part of the name should be; I've always seen it as
> just three extra letters that add nothing; someone else may have a
> different opinion).
>
> >> +                               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.
>
> In particular - if one clause of an if-else is in braces, then *all* of
> the clauses of that if-else much be in braces.
>
> >
> >> +
> >> +    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),
>
> We don't use gnulib's "PRIxxx" macros anywhere else in libvirt. Better
> to "go with the flow" and just use "%d" instead for consistency (or make
> a case for changing them elsewhere :-)
>

I knew I added this for something:

This is the error I get when I just use "%d":

conf/network_conf.c: In function 'virNetworkDHCPLeaseTimeParseXML':
conf/network_conf.c:575:26: error: format '%d' expects argument of type
'int', but argument 8 has type 'int64_t {aka long int}' [-Werror=format=]
                        _("<leasetime> value cannot be greater than the
equivalent of %d seconds : %d"),

If you can think of any alternative that complies with libvirt's code
consistency let me know.


>
>
> >> +                       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)
>
> I *think* this is what you currently have:
>
>   -1 : infinite
>   0/unspecified : unspecified (use default)
>   > 0 : actual lease time.
>
> How about this instead?
>
> In the xml:
>
>   0 : infinite
>   unspecified : unspecified
>   > 0 : actual lease time
>
> In the internal object: have a separate "bool leasetime_specified" to
> differentiate between "0" and unspecified (there are a few other
> examples of this - search for "_specified" in src/conf/*.c)
>
>
> >> +        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*/
>
> I'm not sure how useful this would be for libvirt, since it's apparently
> used when renumbering a network. I think for now it can be ignored.
>
> >> +    /* TODO: Discuss what is the default value that we want as
> dnsmasq's is 1 hour */
>
> Whatever the value has been for all these years, it needs to remain the
> same. Behavior should be unchanged if the XML is unchanged.
>
> >> +    /* TODO: Discuss what to do if value exceeds maximum, use default
> value for now */
>
> If the specified value is too large, we need to log an error and fail.
> If we were to just ignore invalid values now, it makes it more difficult
> to change it to an error later (you would then have to validate it only
> when a definition is newly defined or edited, but not when all the
> configs are read at libvirtd startup).
>
>
> >> +    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
> >
> >
> >
> > --
> > libvir-list mailing list
> > libvir-list at redhat.com
> > https://www.redhat.com/mailman/listinfo/libvir-list
> >
>
>


-- 
Cheers,
Alberto Ruiz
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170622/3f1a07f0/attachment-0001.htm>


More information about the libvir-list mailing list