[libvirt] [PATCH 1/3] leasetime support for <dhcp> globally
Alberto Ruiz
aruiz at gnome.org
Fri Jun 23 00:07:23 UTC 2017
2017-06-22 21:47 GMT+01:00 Laine Stump <laine at 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 at laine.org
>> <mailto: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 <mailto:aruiz at gnome.org> wrote:
>> >> From: Alberto Ruiz <aruiz at gnome.org <mailto: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
>> <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
More information about the libvir-list
mailing list