2017-06-22 21:47 GMT+01:00 Laine Stump <laine@xxxxxxxxx>: > On 06/22/2017 12:21 PM, Alberto Ruiz wrote: >> Hello Laine, >> >> 2017-06-21 17:30 GMT+01:00 Laine Stump <laine@xxxxxxxxx >> <mailto:laine@xxxxxxxxx>>: >> >> On 06/21/2017 03:27 AM, Peter Krempa wrote: >> > On Tue, Jun 20, 2017 at 19:00:43 +0100, aruiz@xxxxxxxxx <mailto:aruiz@xxxxxxxxx> wrote: >> >> From: Alberto Ruiz <aruiz@xxxxxxxxx <mailto:aruiz@xxxxxxxxx>> >> > >> > 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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
