Hello Peter, 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.
2017-06-21 8:27 GMT+01:00 Peter Krempa <pkre...@redhat.com>: > On Tue, Jun 20, 2017 at 19:00:43 +0100, ar...@gnome.org wrote: > > From: Alberto Ruiz <ar...@gnome.org> > > Missing commit message. > > > > > --- > > 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> > > + > > + <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. > > > + </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, > > + 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. > > > + > > + 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), > > + 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) > > + 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*/ > > + /* TODO: Discuss what is the default value that we want as > dnsmasq's is 1 hour */ > > + /* TODO: Discuss what to do if value exceeds maximum, use default > value for now */ > > + 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 > -- Cheers, Alberto Ruiz
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list