On Fri, Sep 23, 2016 at 12:19:49 +0530, Nehal J Wani wrote:
> The default dhcp lease time set by dnsmasq is only one hour, which can be
> pretty small for developers relying on ip address(es) to be consistent
> across reboots.
This should be handled through the leases file which is supposed to
remember the IP addresses given to a host despite a short lease time. Is
that broken for you?
>
> This patch adds support for setting a lease time in the network definition.
> For now, all IP ranges under one <dhcp> will share a common leasetime.
>
> An example:
> <dhcp>
> <leasetime units='hours'>12</leasetime>
> </dhcp>
This sounds like a reasonable feature, but I don't really want to sell
it as a workaround for the problem you've described above. Setting the
lease time is useful, but guests should get the same IP addresses
thanks to the lease file.
>
> The value for attribute 'units' can be seconds/hours/days/weeks/s/h/d/w.
> If the leasetime specified is -1, it is considered to be infinite.
>
> docs/schema/{basictypes,network}.rng
> * Add datatype long
> * Introduce timeUnits, scaledTime
> * Add schema for tag: leasetime
>
> src/util/virutil.{c,h}
> * Introduce virScaleTime()
>
> src/conf/network_conf.h
> * Extend virNetworkIPDef to accomodate leastime
> * Define VIR_NETWORK_DHCP_LEASE_TIME_MAX
>
> src/conf/network_conf.c
> * Introduce virNetworkDHCPLeaseTimeParseXML
> * Modify virNetworkDHCPDefParseXML to have XPath context pointer
>
> tests/*
> * Add test cases for xml parsing and conversion
>
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=913446
> ---
> docs/schemas/basictypes.rng | 20 +++++++
> docs/schemas/network.rng | 10 +++-
> src/conf/network_conf.c | 67 ++++++++++++++++++++-
> src/conf/network_conf.h | 11 ++++
> src/network/bridge_driver.c | 8 +++
> src/util/virutil.c | 70 ++++++++++++++++++++++
> src/util/virutil.h | 3 +
> .../isolated-network-with-lease-time.conf | 17 ++++++
> .../isolated-network-with-lease-time.xml | 24 ++++++++
> tests/networkxml2conftest.c | 1 +
> .../isolated-network-with-lease-time.xml | 24 ++++++++
> .../isolated-network-with-lease-time.xml | 24 ++++++++
> tests/networkxml2xmltest.c | 1 +
> 13 files changed, 276 insertions(+), 4 deletions(-)
> create mode 100644 tests/networkxml2confdata/isolated-network-with-lease-time.conf
> create mode 100644 tests/networkxml2confdata/isolated-network-with-lease-time.xml
> create mode 100644 tests/networkxml2xmlin/isolated-network-with-lease-time.xml
> create mode 100644 tests/networkxml2xmlout/isolated-network-with-lease-time.xml
>
> diff --git a/docs/schemas/basictypes.rng b/docs/schemas/basictypes.rng
> index 1b4f980..023edfb 100644
> --- a/docs/schemas/basictypes.rng
> +++ b/docs/schemas/basictypes.rng
> @@ -13,6 +13,12 @@
> <param name='pattern'>[0-9]+</param>
> </data>
> </define>
> + <!-- Our long doesn"t allow a leading "+" in its lexical form -->
> + <define name='long'>
> + <data type='long'>
This looks weird. Also why would you need a separate type for this?
> + <param name='pattern'>-?[0-9]+</param>
> + </data>
> + </define>
>
> <define name='hexuint'>
> <data type='string'>
> @@ -283,6 +289,20 @@
> <ref name='unsignedLong'/>
> </define>
>
> + <define name='timeUnit'>
> + <data type='string'>
> + <param name='pattern'>(seconds|minutes|hours|days|weeks|s|m|h|d|w)</param>
> + </data>
> + </define>
> + <define name='scaledTime'>
> + <optional>
> + <attribute name='timeUnit'>
> + <ref name='timeUnit'/>
> + </attribute>
> + </optional>
> + <ref name='long'/>
> + </define>
> +
> <define name="pciDomain">
> <ref name="uint16"/>
> </define>
> diff --git a/docs/schemas/network.rng b/docs/schemas/network.rng
> index 1a18e64..66b65bc 100644
> --- a/docs/schemas/network.rng
> +++ b/docs/schemas/network.rng
> @@ -337,9 +337,15 @@
> </element>
> </optional>
> <optional>
> - <!-- Define the range(s) of IP addresses that the DHCP
> - server should hand out -->
> <element name="dhcp">
> + <optional>
> + <element name="leasetime">
> + <ref name="scaledTime"/>
> + <attribute name="unit"><ref name="timeUnit"/></attribute>
> + </element>
> + </optional>
> + <!-- Define the range(s) of IP addresses that the DHCP
> + server should hand out -->
> <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 aa39776..d2372f2 100644
> --- a/src/conf/network_conf.c
> +++ b/src/conf/network_conf.c
> @@ -1032,21 +1032,76 @@ virNetworkDHCPHostDefParseXML(const char *networkName,
> }
>
> static int
> +virNetworkDHCPLeaseTimeParseXML(const char *networkName,
> + virNetworkIPDefPtr def,
> + xmlXPathContextPtr ctxt,
> + xmlNodePtr node)
> +{
> + int ret = -1;
> + int scale = 1;
Scale doesn't need a separate value since it won't change in this case.
> + xmlNodePtr save;
> + char *unit = NULL;
> + int leasetimeRV = 0;
This variable is written and read just once, so it's not needed.
> + long long leasetime;
This variable needs to be initialized, since if the XPath evaluation
does not find the node it will be undefined.
This won't happen here but the compiler might complain.
> +
> + save = ctxt->node;
> + ctxt->node = node;
> +
> + leasetimeRV = virXPathLongLong("string(./text())", ctxt, &leasetime);
> + if (leasetimeRV == -2) {
> + virReportError(VIR_ERR_XML_ERROR,
> + _("Invalid long long value specified for leasetime "
> + "in definition of network '%s'"),
> + networkName);
> + goto cleanup;
> + } else {
> + if (leasetime < 0) {
> + leasetime = -1; /* infinite */
> + } else {
> + unit = virXPathString("string(./@unit)", ctxt);
> + if (virScaleTime(&leasetime, unit, scale,
> + VIR_NETWORK_DHCP_LEASE_TIME_MAX) < 0) {
> + // let virScaleTime() report the appropriate error
'//' comments should not be used in libvirt. Also the comment does not
make sense since it's obvious what is happening.
> + goto cleanup;
> + }
> + }
> + }
> +
> + def->leasetime = leasetime;
> + ret = 0;
> +
> + cleanup:
> + VIR_FREE(unit);
> + ctxt->node = save;
> + return ret;
> +}
> +
> +static int
> virNetworkDHCPDefParseXML(const char *networkName,
> xmlNodePtr node,
> + xmlXPathContextPtr ctxt,
> virNetworkIPDefPtr def)
> {
> int ret = -1;
> - xmlNodePtr cur;
> + xmlNodePtr cur, save;
Please don't declare multiple variables per line.
> virSocketAddrRange range;
> virNetworkDHCPHostDef host;
>
> memset(&range, 0, sizeof(range));
> memset(&host, 0, sizeof(host));
>
> + save = ctxt->node;
> + ctxt->node = node;
> +
> cur = node->children;
> while (cur != NULL) {
> if (cur->type == XML_ELEMENT_NODE &&
> + xmlStrEqual(cur->name, BAD_CAST "leasetime")) {
> +
> + if (virNetworkDHCPLeaseTimeParseXML(networkName, def, ctxt, cur) < 0)
> + goto cleanup;
> +
> + } else if (cur->type == XML_ELEMENT_NODE &&
> xmlStrEqual(cur->name, BAD_CAST "range")) {
>
> if (virSocketAddrRangeParseXML(networkName, def, cur, &range) < 0)
[...]
> @@ -2675,6 +2731,13 @@ virNetworkIPDefFormat(virBufferPtr buf,
> virBufferAddLit(buf, "<dhcp>\n");
> virBufferAdjustIndent(buf, 2);
>
> + if (def->leasetime) {
> + virBufferAddLit(buf, "<leasetime");
> + virBufferAsprintf(buf, " unit='seconds'>%lld",
Why aren't we remembering the unit the user used originally? Other
places can't do that since the unit was added later, but with this newly
added code you can remember and format back the unit just fine.
> + def->leasetime);
> + virBufferAddLit(buf, "</leasetime>\n");
> + }
> +
> for (i = 0; i < def->nranges; i++) {
> char *saddr = virSocketAddrFormat(&def->ranges[i].start);
> if (!saddr)
> diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h
> index 3b227db..965fd3b 100644
> --- a/src/conf/network_conf.h
> +++ b/src/conf/network_conf.h
[...]
> @@ -167,6 +176,8 @@ struct _virNetworkIPDef {
> size_t nhosts; /* Zero or more dhcp hosts */
> virNetworkDHCPHostDefPtr hosts;
>
> + long long leasetime; /* DHCP lease time for all ranges */
You should explain the special values of 0 and -1 here, also state the
unit.
> +
> char *tftproot;
> char *bootfile;
> virSocketAddr bootserver;
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index 7b99aca..fa628d3 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -1222,6 +1222,14 @@ networkDnsmasqConfContents(virNetworkObjPtr network,
> saddr, eaddr);
> if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET6))
> virBufferAsprintf(&configbuf, ",%d", prefix);
> +
> + if (ipdef->leasetime) {
> + if (ipdef->leasetime == -1)
> + virBufferAddLit(&configbuf, ",infinite");
> + else
> + virBufferAsprintf(&configbuf, ",%lld", ipdef->leasetime);
> + }
> +
> virBufferAddLit(&configbuf, "\n");
>
> VIR_FREE(saddr);
> diff --git a/src/util/virutil.c b/src/util/virutil.c
> index b57a195..81a60e6 100644
> --- a/src/util/virutil.c
> +++ b/src/util/virutil.c
> @@ -275,6 +275,76 @@ virHexToBin(unsigned char c)
> }
> }
>
> +/**
> + * virScaleTime:
This change should be done as a separate patch.
> + * @value: pointer to the integer which is supposed to hold value
> + * @unit: pointer to the string holding the unit
> + * @scale: integer holding the value of scale
^^ at least this value is not documented enough.
> + * @limit: upper limit on scaled value
> + *
> + * Scale an integer @value in-place by an optional case-insensitive @unit,
The statement about case insensitivity doesn't look to be true according
to the code blow.
> + * defaulting to @scale if @unit is NULL or empty. Recognized units include
> + * (w)eeks, (d)ays, (h)ours, (m)inutes and (s)econds. Ensure that the result
> + * does not exceed @limit. Return 0 on success, -1 with error message raised
> + * on failure.
This does not document the scale of the returned value at all.
> + */
> +int
> +virScaleTime(long long *value, const char *unit,
> + long long scale, long long limit)
Scale can't really be negative. Also the result being negative does not
make much sense to me.
> +{
> + size_t i;
> + static char const* const allowed_units[] = {"s", "m", "h", "d", "w",
> + "seconds", "minutes", "hours", "days", "weeks"};
> +
> + size_t n_allowed_units = ARRAY_CARDINALITY(allowed_units);
> +
> + if (!unit || !*unit) {
> + if (!scale) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Invalid scale %lld"), scale);
> + return -1;
> + }
> + unit = "";
> + } else {
Scale is not reset here, which is wrong since it would depend on user
input.
> + for (i = 0; i < n_allowed_units; i++) {
> + if (STREQ(unit, allowed_units[i])) {
> + switch (*unit) {
> + case 'w':
> + scale *= 7;
> + /* fall through */
> + case 'd':
> + scale *= 24;
> + /* fall through */
> + case 'h':
> + scale *= 60;
> + /* fall through */
> + case 'm':
> + scale *= 60;
> + /* fall through */
> + case 's':
> + break;
> + }
> + break;
> + }
> + }
> + if (i == n_allowed_units) {
> + virReportError(VIR_ERR_INVALID_ARG,
> + _("Unknown time unit '%s'"), unit);
> + return -1;
> + }
> + }
> +
> + if (*value && *value > (limit / scale)) {
> + virReportError(VIR_ERR_OVERFLOW, _("Value too large: %lld %s"),
> + *value, unit);
> + return -1;
> + }
> +
> + *value *= scale;
> + return 0;
> +}
> +
> +
> /* Scale an integer VALUE in-place by an optional case-insensitive
> * SUFFIX, defaulting to SCALE if suffix is NULL or empty (scale is
> * typically 1 or 1024). Recognized suffixes include 'b' or 'bytes',
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list