<div dir="ltr">Hello Peter,<br><br>Thanks for the comments, I will update the patch with your feedback, and sorry I thought I rebased it before I sent it, will fix that too.</div><div class="gmail_extra"><br><div class="gmail_quote">2017-06-21 8:27 GMT+01:00 Peter Krempa <span dir="ltr"><<a href="mailto:pkrempa@redhat.com" target="_blank">pkrempa@redhat.com</a>></span>:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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>
<div><div class="h5"><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>
> + <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>
</div></div>This does not follow the XML style used everywhere else.<br>
<div><div class="h5"><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>
> + xmlXPathContextPtr ctxt,<br>
> + bool *error)<br>
<br>
</div></div>We usually return error from the function and if necessary return the<br>
value through pointer in arguments (backwards as you did).<br>
<div><div class="h5"><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>
</div></div>Does not follow libvirt coding style.<br>
<span class=""><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>
> + UINT32_MAX, result);<br>
<br>
</span>Lines are too long<br>
<span class=""><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>
</span>This won't pass syntax-check<br>
<div><div class="h5"><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>
> + 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>
> + /* TODO: Discuss what is the default value that we want as dnsmasq's is 1 hour */<br>
> + /* TODO: Discuss what to do if value exceeds maximum, use default value for now */<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>
</div></div>Also this patch does not apply to current master.<br>
<span class="HOEnZb"><font color="#888888"><br>
Peter<br>
</font></span></blockquote></div><br><br clear="all"><div><br></div>-- <br><div class="gmail_signature" data-smartmail="gmail_signature">Cheers,<br>Alberto Ruiz</div>
</div>