<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">2017-06-21 17:30 GMT+01:00 Laine Stump <span dir="ltr"><<a href="mailto:laine@laine.org" target="_blank">laine@laine.org</a>></span>:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="gmail-">On 06/21/2017 03:27 AM, Peter Krempa wrote:<br>
> On Tue, Jun 20, 2017 at 19:00:43 +0100, <a href="mailto:aruiz@gnome.org">aruiz@gnome.org</a> wrote:<br>
>> From: Alberto Ruiz <<a href="mailto:aruiz@gnome.org">aruiz@gnome.org</a>><br>
><br>
> Missing commit message.<br>
<br>
</span>And when composing the commit message, it's useful to include links to<br>
the associated BZ.<br>
<br>
<a href="https://bugzilla.redhat.com/show_bug.cgi?id=913446" rel="noreferrer" target="_blank">https://bugzilla.redhat.com/<wbr>show_bug.cgi?id=913446</a><br>
<br>
Also, I recall there being quite a lot of discussion in email (and<br>
possibly IRC) about the fact that people *think* they want a<br>
configurable lease time because they think that will eliminate cases of<br>
a DHCP lease being lost while a domain is paused. It was pointed out<br>
that lengthening the lease will *not* eliminate that problem (it just<br>
makes it happen less often).<br>
<br>
As an alternate (and better) solution to the problem of lost leases, we<br>
then added the "dhcp-authoritative" option to dnsmasq (commit<br>
4ac20b3ae4), which allows clients to re-acquire the same IP as they had<br>
for an expired lease (as long as it hasn't been acquired by someone else<br>
in the meantime, which is apparently unlikely unless all the other<br>
addresses in the pool are already assigned).<br>
<br>
I'm not saying this to discourage the idea of making leasetime<br>
configurable (I think we'd already agreed that it was reasonable to do<br>
so, but there were two competing patches posted, and neither of them was<br>
really push-ready), but just to make sure that nobody is disappointed if<br>
the results don't lead to the behavior they're hoping for.<br>
<div><div class="gmail-h5"><br>
<br>
><br>
>><br>
>> ---<br>
>> docs/schemas/basictypes.rng | 16 +++++<br>
>> docs/schemas/network.rng | 8 +++<br>
>> src/conf/network_conf.c | 78 ++++++++++++++++++++++-<br>
>> src/conf/network_conf.h | 3 +-<br>
>> src/network/bridge_driver.c | 49 +++++++++++++-<br>
>> tests/networkxml2confdata/<wbr>leasetime-days.conf | 17 +++++<br>
>> tests/networkxml2confdata/<wbr>leasetime-days.xml | 18 ++++++<br>
>> tests/networkxml2confdata/<wbr>leasetime-hours.conf | 17 +++++<br>
>> tests/networkxml2confdata/<wbr>leasetime-hours.xml | 18 ++++++<br>
>> tests/networkxml2confdata/<wbr>leasetime-infinite.conf | 17 +++++<br>
>> tests/networkxml2confdata/<wbr>leasetime-infinite.xml | 18 ++++++<br>
>> tests/networkxml2confdata/<wbr>leasetime-minutes.conf | 17 +++++<br>
>> tests/networkxml2confdata/<wbr>leasetime-minutes.xml | 18 ++++++<br>
>> tests/networkxml2confdata/<wbr>leasetime-seconds.conf | 17 +++++<br>
>> tests/networkxml2confdata/<wbr>leasetime-seconds.xml | 18 ++++++<br>
>> tests/networkxml2confdata/<wbr>leasetime.conf | 17 +++++<br>
>> tests/networkxml2confdata/<wbr>leasetime.xml | 18 ++++++<br>
>> tests/networkxml2conftest.c | 7 ++<br>
>> 18 files changed, 368 insertions(+), 3 deletions(-)<br>
>> create mode 100644 tests/networkxml2confdata/<wbr>leasetime-days.conf<br>
>> create mode 100644 tests/networkxml2confdata/<wbr>leasetime-days.xml<br>
>> create mode 100644 tests/networkxml2confdata/<wbr>leasetime-hours.conf<br>
>> create mode 100644 tests/networkxml2confdata/<wbr>leasetime-hours.xml<br>
>> create mode 100644 tests/networkxml2confdata/<wbr>leasetime-infinite.conf<br>
>> create mode 100644 tests/networkxml2confdata/<wbr>leasetime-infinite.xml<br>
>> create mode 100644 tests/networkxml2confdata/<wbr>leasetime-minutes.conf<br>
>> create mode 100644 tests/networkxml2confdata/<wbr>leasetime-minutes.xml<br>
>> create mode 100644 tests/networkxml2confdata/<wbr>leasetime-seconds.conf<br>
>> create mode 100644 tests/networkxml2confdata/<wbr>leasetime-seconds.xml<br>
>> create mode 100644 tests/networkxml2confdata/<wbr>leasetime.conf<br>
>> create mode 100644 tests/networkxml2confdata/<wbr>leasetime.xml<br>
>><br>
>> diff --git a/docs/schemas/basictypes.rng b/docs/schemas/basictypes.rng<br>
>> index 1b4f980e7..8a76c235a 100644<br>
>> --- a/docs/schemas/basictypes.rng<br>
>> +++ b/docs/schemas/basictypes.rng<br>
>> @@ -518,4 +518,20 @@<br>
>> </element><br>
>> </define><br>
>><br>
>> + <define name="leaseTimeUnit"><br>
>> + <choice><br>
>> + <value>seconds</value><br>
>> + <value>minutes</value><br>
>> + <value>hours</value><br>
>> + <value>days</value><br>
>> + </choice><br>
>> + </define><br>
<br>
</div></div>Maybe call this "timeUnit" in case some other attribute in the future<br>
needs it?<br>
<div><div class="gmail-h5"><br>
>> +<br>
>> + <define name="leaseTime"><br>
>> + <data type="long"><br>
>> + <param name="minInclusive">-1</param><br>
>> + <param name="maxInclusive"><wbr>4294967295</param><br>
>> + </data><br>
>> + </define><br>
>> +<br>
>> </grammar><br>
>> diff --git a/docs/schemas/network.rng b/docs/schemas/network.rng<br>
>> index 1a18e64b2..4b8056ab6 100644<br>
>> --- a/docs/schemas/network.rng<br>
>> +++ b/docs/schemas/network.rng<br>
>> @@ -340,6 +340,14 @@<br>
>> <!-- Define the range(s) of IP addresses that the DHCP<br>
>> server should hand out --><br>
>> <element name="dhcp"><br>
>> + <optional><br>
>> + <element name="leasetime"><br>
>> + <optional><br>
>> + <attribute name="unit"><ref name="leaseTimeUnit"/></<wbr>attribute><br>
><br>
> This does not follow the XML style used everywhere else.<br>
><br>
>> + </optional><br>
>> + <ref name="leaseTime"/><br>
>> + </element><br>
>> + </optional><br>
>> <zeroOrMore><br>
>> <element name="range"><br>
>> <attribute name="start"><ref name="ipAddr"/></attribute><br>
>> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c<br>
>> index aa397768c..6f051493f 100644<br>
>> --- a/src/conf/network_conf.c<br>
>> +++ b/src/conf/network_conf.c<br>
>> @@ -30,6 +30,8 @@<br>
>> #include <fcntl.h><br>
>> #include <string.h><br>
>> #include <dirent.h><br>
>> +#include <stdlib.h><br>
>> +#include <inttypes.h><br>
>><br>
>> #include "virerror.h"<br>
>> #include "datatypes.h"<br>
>> @@ -1031,12 +1033,82 @@ virNetworkDHCPHostDefParseXML(<wbr>const char *networkName,<br>
>> return ret;<br>
>> }<br>
>><br>
>> +static int64_t<br>
>> +virNetworkDHCPDefGetLeaseTime (xmlNodePtr node,<br>
<br>
</div></div>While it's true that this function "gets" the lease time from the<br>
xmlDoc, this is part of the process of parsing the XML, so it should be<br>
named virNetworkDHCPLeaseTimeParseXM<wbr>L(). (it's never been clear to me<br>
when/where the "Def" part of the name should be; I've always seen it as<br>
just three extra letters that add nothing; someone else may have a<br>
different opinion).<br>
<div><div class="gmail-h5"><br>
>> + xmlXPathContextPtr ctxt,<br>
>> + bool *error)<br>
><br>
> We usually return error from the function and if necessary return the<br>
> value through pointer in arguments (backwards as you did).<br>
><br>
>> +{<br>
>> + int64_t multiplier, result = -1;<br>
>> + char *leaseString, *leaseUnit;<br>
>> + xmlNodePtr save;<br>
>> +<br>
>> + *error = 0;<br>
>> +<br>
>> + save = ctxt->node;<br>
>> + ctxt->node = node;<br>
>> +<br>
>> + leaseString = virXPathString ("string(./leasetime/text())", ctxt);<br>
>> + leaseUnit = virXPathString ("string(./leasetime/@unit)", ctxt);<br>
>> +<br>
>> + /* If value is not present we set the value to -2 */<br>
>> + if (leaseString == NULL) {<br>
>> + result = -2;<br>
>> + goto cleanup;<br>
>> + }<br>
>> +<br>
>> + if (leaseUnit == NULL || strcmp (leaseUnit, "seconds") == 0)<br>
>> + multiplier = 1;<br>
>> + else if (strcmp (leaseUnit, "minutes") == 0)<br>
>> + multiplier = 60;<br>
>> + else if (strcmp (leaseUnit, "hours") == 0)<br>
>> + multiplier = 60 * 60;<br>
>> + else if (strcmp (leaseUnit, "days") == 0)<br>
>> + multiplier = 60 * 60 * 24;<br>
>> + else {<br>
>> + virReportError(VIR_ERR_XML_<wbr>ERROR,<br>
>> + _("invalid value for unit parameter in <leasetime> element found in <dhcp> network, "<br>
>> + "only 'seconds', 'minutes', 'hours' or 'days' are valid: %s"),<br>
>> + leaseUnit);<br>
>> + *error = 1;<br>
>> + goto cleanup;<br>
>> + }<br>
><br>
> Does not follow libvirt coding style.<br>
<br>
</div></div>In particular - if one clause of an if-else is in braces, then *all* of<br>
the clauses of that if-else much be in braces.<br>
<span class="gmail-"><br>
><br>
>> +<br>
>> + errno = 0;<br>
>> + result = (int64_t) strtoll((const char*)leaseString, NULL, 10);<br>
>> +<br>
>> + /* Report any errors parsing the string */<br>
>> + if (errno != 0) {<br>
>> + virReportError(VIR_ERR_XML_<wbr>ERROR,<br>
>> + _("<leasetime> value could not be converted to a signed integer: %s"),<br>
>> + leaseString);<br>
>> + *error = 1;<br>
>> + goto cleanup;<br>
>> + }<br>
>> +<br>
>> + result = result * multiplier;<br>
>> +<br>
>> + if (result > UINT32_MAX) {<br>
>> + virReportError(VIR_ERR_XML_<wbr>ERROR,<br>
>> + _("<leasetime> value cannot be greater than the equivalent of %" PRIo32 " seconds : %" PRId64),<br>
<br>
</span>We don't use gnulib's "PRIxxx" macros anywhere else in libvirt. Better<br>
to "go with the flow" and just use "%d" instead for consistency (or make<br>
a case for changing them elsewhere :-)<br></blockquote><div><br></div><div>I knew I added this for something:<br><br>This is the error I get when I just use "%d":<br><br><div>conf/network_conf.c: In function 'virNetworkDHCPLeaseTimeParseXML':</div><div>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=]</div><div> _("<leasetime> value cannot be greater than the equivalent of %d seconds : %d"),</div></div><div><br></div><div>If you can think of any alternative that complies with libvirt's code consistency let me know.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div><div class="gmail-h5"><br>
<br>
>> + UINT32_MAX, result);<br>
><br>
> Lines are too long<br>
><br>
>> + *error = 1;<br>
>> + }<br>
>> +<br>
>> +cleanup:<br>
>> + VIR_FREE(leaseString);<br>
>> + VIR_FREE(leaseUnit);<br>
>> + ctxt->node = save;<br>
>> + return result;<br>
>> +}<br>
>> +<br>
>> static int<br>
>> virNetworkDHCPDefParseXML(<wbr>const char *networkName,<br>
>> xmlNodePtr node,<br>
>> + xmlXPathContextPtr ctxt,<br>
>> virNetworkIPDefPtr def)<br>
>> {<br>
>> int ret = -1;<br>
>> + bool error;<br>
>> xmlNodePtr cur;<br>
>> virSocketAddrRange range;<br>
>> virNetworkDHCPHostDef host;<br>
>> @@ -1044,6 +1116,10 @@ virNetworkDHCPDefParseXML(<wbr>const char *networkName,<br>
>> memset(&range, 0, sizeof(range));<br>
>> memset(&host, 0, sizeof(host));<br>
>><br>
>> + def->leasetime = virNetworkDHCPDefGetLeaseTime (node, ctxt, &error);<br>
><br>
> This won't pass syntax-check<br>
><br>
>> + if (error)<br>
>> + goto cleanup;<br>
>> +<br>
>> cur = node->children;<br>
>> while (cur != NULL) {<br>
>> if (cur->type == XML_ELEMENT_NODE &&<br>
>> @@ -1607,7 +1683,7 @@ virNetworkIPDefParseXML(const char *networkName,<br>
>> while (cur != NULL) {<br>
>> if (cur->type == XML_ELEMENT_NODE &&<br>
>> xmlStrEqual(cur->name, BAD_CAST "dhcp")) {<br>
>> - if (virNetworkDHCPDefParseXML(<wbr>networkName, cur, def) < 0)<br>
>> + if (virNetworkDHCPDefParseXML(<wbr>networkName, cur, ctxt, def) < 0)<br>
>> goto cleanup;<br>
>> } else if (cur->type == XML_ELEMENT_NODE &&<br>
>> xmlStrEqual(cur->name, BAD_CAST "tftp")) {<br>
>> diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h<br>
>> index 3b227db6f..f7557f581 100644<br>
>> --- a/src/conf/network_conf.h<br>
>> +++ b/src/conf/network_conf.h<br>
>> @@ -170,7 +170,8 @@ struct _virNetworkIPDef {<br>
>> char *tftproot;<br>
>> char *bootfile;<br>
>> virSocketAddr bootserver;<br>
>> - };<br>
>> + int64_t leasetime;<br>
>> +};<br>
>><br>
>> typedef struct _virNetworkForwardIfDef virNetworkForwardIfDef;<br>
>> typedef virNetworkForwardIfDef *virNetworkForwardIfDefPtr;<br>
>> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c<br>
>> index 7b99acafa..e51e469c8 100644<br>
>> --- a/src/network/bridge_driver.c<br>
>> +++ b/src/network/bridge_driver.c<br>
>> @@ -41,6 +41,8 @@<br>
>> #include <sys/ioctl.h><br>
>> #include <net/if.h><br>
>> #include <dirent.h><br>
>> +#include <inttypes.h><br>
>> +#include <stdint.h><br>
>> #if HAVE_SYS_SYSCTL_H<br>
>> # include <sys/sysctl.h><br>
>> #endif<br>
>> @@ -903,6 +905,40 @@ networkBuildDnsmasqHostsList(<wbr>dnsmasqContext *dctx,<br>
>> return 0;<br>
>> }<br>
>><br>
>> +/* translates the leasetime value into a dnsmasq configuration string for dhcp-range/host */<br>
>> +static char *<br>
>> +<wbr>networkDnsmasqConfLeaseValueTo<wbr>String (int64_t leasetime)<br>
>> +{<br>
>> + char *result = NULL;<br>
>> + virBuffer leasebuf = VIR_BUFFER_INITIALIZER;<br>
>> +<br>
>> + /* Leasetime parameter set on the XML */<br>
>> + /* Less than -1 means there was no value set */<br>
>> + if (leasetime < -1) {<br>
>> + virBufferAsprintf(&leasebuf, "%s", "");<br>
>> + }<br>
>> + /* -1 means no expiration */<br>
>> + else if (leasetime == -1)<br>
<br>
</div></div>I *think* this is what you currently have:<br>
<br>
-1 : infinite<br>
0/unspecified : unspecified (use default)<br>
> 0 : actual lease time.<br>
<br>
How about this instead?<br>
<br>
In the xml:<br>
<br>
0 : infinite<br>
unspecified : unspecified<br>
> 0 : actual lease time<br>
<br>
In the internal object: have a separate "bool leasetime_specified" to<br>
differentiate between "0" and unspecified (there are a few other<br>
examples of this - search for "_specified" in src/conf/*.c)<br>
<span class="gmail-"><br>
<br>
>> + virBufferAsprintf(&leasebuf, ",infinite");<br>
>> + /* Minimum expiry value is 120 */<br>
>> + /* TODO: Discuss if we should up as we do here or just forward whatever value to dnsmasq */<br>
>> + else if (leasetime < 120)<br>
>> + virBufferAsprintf(&leasebuf, ",120");<br>
>> + /* DHCP value for lease time is a unsigned four octect integer */<br>
>> + else if (leasetime <= UINT32_MAX)<br>
>> + virBufferAsprintf(&leasebuf, ",%" PRId64, leasetime);<br>
>> + /* TODO: Discuss the use of "deprecated" for ipv6*/<br>
<br>
</span>I'm not sure how useful this would be for libvirt, since it's apparently<br>
used when renumbering a network. I think for now it can be ignored.<br>
<span class="gmail-"><br>
>> + /* TODO: Discuss what is the default value that we want as dnsmasq's is 1 hour */<br>
<br>
</span>Whatever the value has been for all these years, it needs to remain the<br>
same. Behavior should be unchanged if the XML is unchanged.<br>
<span class="gmail-"><br>
>> + /* TODO: Discuss what to do if value exceeds maximum, use default value for now */<br>
<br>
</span>If the specified value is too large, we need to log an error and fail.<br>
If we were to just ignore invalid values now, it makes it more difficult<br>
to change it to an error later (you would then have to validate it only<br>
when a definition is newly defined or edited, but not when all the<br>
configs are read at libvirtd startup).<br>
<div class="gmail-HOEnZb"><div class="gmail-h5"><br>
<br>
>> + else {<br>
>> + virBufferAsprintf(&leasebuf, "%s", "");<br>
>> + }<br>
>> +<br>
>> + result = virBufferContentAndReset(&<wbr>leasebuf);<br>
>> + virBufferFreeAndReset (&leasebuf);<br>
>> +<br>
>> + return result;<br>
>> +}<br>
>><br>
>> int<br>
>> networkDnsmasqConfContents(<wbr>virNetworkObjPtr network,<br>
>> @@ -1213,6 +1249,7 @@ networkDnsmasqConfContents(<wbr>virNetworkObjPtr network,<br>
>> }<br>
>> for (r = 0; r < ipdef->nranges; r++) {<br>
>> int thisRange;<br>
>> + char *leasestr;<br>
>><br>
>> if (!(saddr = virSocketAddrFormat(&ipdef-><wbr>ranges[r].start)) ||<br>
>> !(eaddr = virSocketAddrFormat(&ipdef-><wbr>ranges[r].end)))<br>
>> @@ -1220,12 +1257,22 @@ networkDnsmasqConfContents(<wbr>virNetworkObjPtr network,<br>
>><br>
>> virBufferAsprintf(&configbuf, "dhcp-range=%s,%s",<br>
>> saddr, eaddr);<br>
>> - if (VIR_SOCKET_ADDR_IS_FAMILY(&<wbr>ipdef->address, AF_INET6))<br>
>> +<br>
>> + /* Add ipv6 prefix length parameter if needed */<br>
>> + if (ipdef == ipv6def)<br>
>> virBufferAsprintf(&configbuf, ",%d", prefix);<br>
>> +<br>
>> + leasestr = networkDnsmasqConfLeaseValueTo<wbr>String (ipdef->leasetime);<br>
>> + if (!leasestr)<br>
>> + goto cleanup;<br>
>> + virBufferAsprintf(&configbuf, "%s", leasestr);<br>
>> +<br>
>> + /* Add the newline */<br>
>> virBufferAddLit(&configbuf, "\n");<br>
>><br>
>> VIR_FREE(saddr);<br>
>> VIR_FREE(eaddr);<br>
>> + VIR_FREE(leasestr);<br>
>> thisRange = virSocketAddrGetRange(&ipdef-><wbr>ranges[r].start,<br>
>> &ipdef->ranges[r].end,<br>
>> &ipdef->address,<br>
><br>
> Also this patch does not apply to current master.<br>
><br>
> Peter<br>
><br>
><br>
><br>
</div></div><span class="gmail-HOEnZb"><font color="#888888">> --<br>
> libvir-list mailing list<br>
> <a href="mailto:libvir-list@redhat.com">libvir-list@redhat.com</a><br>
> <a href="https://www.redhat.com/mailman/listinfo/libvir-list" rel="noreferrer" target="_blank">https://www.redhat.com/<wbr>mailman/listinfo/libvir-list</a><br>
><br>
<br>
</font></span></blockquote></div><br><br clear="all"><div><br></div>-- <br><div class="gmail_signature">Cheers,<br>Alberto Ruiz</div>
</div></div>