[libvirt] [PATCH 1/3] leasetime support for <dhcp> globally
Alberto Ruiz
aruiz at gnome.org
Wed Jun 21 11:59:48 UTC 2017
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 <pkrempa at redhat.com>:
> On Tue, Jun 20, 2017 at 19:00:43 +0100, aruiz at gnome.org wrote:
> > From: Alberto Ruiz <aruiz at 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170621/577a1914/attachment-0001.htm>
More information about the libvir-list
mailing list