On Fri, Sep 23, 2016 at 1:20 PM, Peter Krempa <pkrempa@xxxxxxxxxx> wrote: > 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? We can't ask developers to parse the .status file and the DHCP leases API would never show expired leases. Or am I misinterpreting what you mean by 'remember'? Even dnsmasq depends on the output for the init event sent to the leaseshelper program, which will not output lease which have expired. > >> >> 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. Actually, the major reason behind this feature was mentioned at https://www.redhat.com/archives/libvir-list/2016-April/msg01071.html Let's say that the lease time is 2 hours and I shutdown my VM. Now I reboot it after 2hours 5mins, the lease will be gone, since it will be expired. The .status file is read and the leases which have expired are removed. So, there is no guarantee that my VM will get the same IP address again. Hence the developer would want to increase the lease time to something big, like 1 week, and go to sleep happily when the VM is turned off at night :) > >> >> 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? > I didn't find any available data type for signed integers. Which one would you rather use? (Signed, because we want to support -1 too) >> + <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. I have a query regarding this. If we rely on virScaleTime to report the appropriate error, it wouldn't be including the network name. Is that bad? Should I be modifying virScaleTime to return different return values for different errors and shape the error message appropriately here? (That would sort be inconsistent with virScaleInteger which doesn't have different error codes for different scenarios) Whoops on the comment. Should have read hacking.html more carefully! > >> + 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. > Actually, I followed what was originally done for memory units. Even if user specifies MiB, everything is stored in KiB and on dumpxml, the user is shown the unit in KiB. In case we want to remember the unit specified by the user, we'll have to add another member in the _virNetworkIPDef struct. Since we haven't done this anywhere, I was apprehensive about having inconsistent behaviour for 'unit'. >> + 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. With case insensitivity, 'Seconds' and 'SEconDs' would also be valid inputs, and it would be difficult to accommodate it in the .rng file, plus I was told that if attributes are words, then we shouldn't be going for case-insensitivity anyway. I'll fix this in the next patch set. > >> + * 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. Should this be changed to unsigned long long or unsigned long? Since leasetime has to be a signed integer, what would be the ideal way to pass it to the modified function? Should I typecast it? > >> +{ >> + 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', -- Nehal J Wani -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list