[libvirt] [PATCH 1/3] leasetime support for <dhcp> globally
Alberto Ruiz
aruiz at gnome.org
Thu Jun 22 16:21:34 UTC 2017
Hello Laine,
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.
>
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?
> >> + </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.
Cheers!
> >
> >
> >
> > --
> > 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/9698b3a0/attachment-0001.htm>
More information about the libvir-list
mailing list