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

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



2017-06-22 21:47 GMT+01:00 Laine Stump <laine laine org>:
> On 06/22/2017 12:21 PM, Alberto Ruiz wrote:
>> Hello Laine,
>>
>> 2017-06-21 17:30 GMT+01:00 Laine Stump <laine laine org
>> <mailto:laine laine org>>:
>>
>>     On 06/21/2017 03:27 AM, Peter Krempa wrote:
>>     > On Tue, Jun 20, 2017 at 19:00:43 +0100, aruiz gnome org <mailto:aruiz gnome org> wrote:
>>     >> From: Alberto Ruiz <aruiz gnome org <mailto:aruiz 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
>>     <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.
>>
>>
>> My main motivation _was_ the loss of IP addresses after reboot on GNOME
>> Boxes.
>>
>> However, given that I've written the patches already and some people
>> might find this useful, I'm okay with going ahead and get them ready for
>> approval.
>>
>>
>>     >
>>     >>
>>     >> ---
>>     >>  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?
>>
>>
>> sounds good to me. noted
>>
>>
>>
>>     >> +
>>     >> +  <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.
>>
>>
>> I'm not sure I understand what specifically violates the XML style,
>> range/ipAddr below seems to be written following the same convention?
>
> Yeah, I looked and there are quite a few examples of this in some of the
> files, many going back a very long time (e.g. some that still show Jim
> Meyering on the git blame, and even one or two DV's). So while it's not
> very common, there is prior art. (Unless Peter is talking about
> something other than the placement of multiple elements on a single
> line). I don't have any opinion one way or the other on this, but if
> we're going to mandate "no multiple elements on a single line" as part
> of our official coding style guidelines, then we should remove all
> existing examples of it first.
>
>
>>
>>
>>     >> +                    </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).
>>
>>
>> Okay. Will rename.
>>
>>
>>     >> +                               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.
>>
>>
>> Noted. Will fix.
>>
>>
>>     >
>>     >> +
>>     >> +    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 :-)
>>
>>
>>     >> +                       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)
>>
>>
>> Works for me.
>>
>>
>>     >> +        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.
>>
>>
>> okay
>>
>>
>>
>>     >> +    /* 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.
>>
>>
>> okay, makes sense
>>
>>
>>     >> +    /* 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).
>>
>>
>> noted
>>
>>
>>
>>
>>     >> +    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
>>
>>
>> Yep, my bad. I've already rebased to master in my branch but I want to
>> address the feedback given before I submit the updated patches.
>>
>> By the way, how am I supposed to submit the updated patchset? I'm not
>> used to review code by email.
>
> I usually use "git send-email -v2 --cover-letter --annotate -3"
>
>  -v2 : adds "v2" after "PATCH" in the subject
>  --cover-letter : creates a "PATCH v2 0/3" email where
>                   you can describe the purpose of the
>                   series as a whole (won't be added to git)
>  --annotate : load all of the patches (and the cover letter) into
>               your favorite text editor to allow adding in
>               comments about what was changed in V2 of each patch
>               (and compose and add a proper subject to the cover
>               letter). The notes about what was changed between
>               versions should be placed after the "---" line
>               so they won't be included in the commit log that's
>               eventually pushed.
>
>
>  -3 : the number of commits to send from the tip of your branch
>
> You don't need to make them a reply to the V1 email - just leave them as
> their own thread.
>
> If you look in the list archives for patches with "PATCH v2 0/x" you'll
> see some good examples.

The thing is that to make rebasing to master I ended up sqashing the
three patches into one as managing changes was becoming a bit of a
nightmare and I think I ended up mixing them up a bit in the rebase
process. So I'm afraid I'm going to have to start a new thread when I
submit the updates.

-- 
Cheers,
Alberto Ruiz


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