11:18 a.m.
This series is based on latest series from Alberto. It includes a new
entry called <leasetime/> under <dhcp/> scope to add a default lease
time for range and host options for dnsmasq. There is no point to
configure both separately. If they are defined (range and/or host), they
should have the same lease time value.
This series includes some test cases to cover lease time XML syntax
also.
Julio Faracco (2):
conf: Add <leasetime/> option for <dhcp/> settings
tests: Add tests for <leasetime/> to cover dnsmasq settings
docs/schemas/basictypes.rng | 9 +++
docs/schemas/network.rng | 11 ++++
src/conf/network_conf.c | 62 ++++++++++++++++++-
src/conf/network_conf.h | 14 +++++
src/libvirt_private.syms | 2 +
src/network/bridge_driver.c | 37 ++++++++++-
src/util/virdnsmasq.c | 40 ++++++------
src/util/virdnsmasq.h | 1 +
.../networkxml2confdata/leasetime-hours.conf | 16 +++++
tests/networkxml2confdata/leasetime-hours.xml | 12 ++++
.../leasetime-infinite.conf | 16 +++++
.../leasetime-infinite.xml | 12 ++++
.../leasetime-minutes.conf | 16 +++++
.../networkxml2confdata/leasetime-minutes.xml | 12 ++++
.../leasetime-seconds.conf | 16 +++++
.../networkxml2confdata/leasetime-seconds.xml | 12 ++++
tests/networkxml2conftest.c | 4 ++
tests/networkxml2xmlin/leasetime-hours.xml | 12 ++++
tests/networkxml2xmlin/leasetime-infinite.xml | 12 ++++
tests/networkxml2xmlin/leasetime-minutes.xml | 12 ++++
tests/networkxml2xmlin/leasetime-seconds.xml | 12 ++++
tests/networkxml2xmlout/leasetime-hours.xml | 14 +++++
.../networkxml2xmlout/leasetime-infinite.xml | 14 +++++
tests/networkxml2xmlout/leasetime-minutes.xml | 14 +++++
tests/networkxml2xmlout/leasetime-seconds.xml | 14 +++++
tests/networkxml2xmltest.c | 4 ++
26 files changed, 376 insertions(+), 24 deletions(-)
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/networkxml2xmlin/leasetime-hours.xml
create mode 100644 tests/networkxml2xmlin/leasetime-infinite.xml
create mode 100644 tests/networkxml2xmlin/leasetime-minutes.xml
create mode 100644 tests/networkxml2xmlin/leasetime-seconds.xml
create mode 100644 tests/networkxml2xmlout/leasetime-hours.xml
create mode 100644 tests/networkxml2xmlout/leasetime-infinite.xml
create mode 100644 tests/networkxml2xmlout/leasetime-minutes.xml
create mode 100644 tests/networkxml2xmlout/leasetime-seconds.xml
--
2.24.1
11:18 a.m.
New subject: [PATCH 1/2] conf: Add <leasetime/> option for <dhcp/> settings
If an user is trying to configure a dhcp neetwork settings, it is not
possible to change the leasetime of a range or a host entry. This is
available using dnsmasq extra options, but they are associated with
dhcp-range or dhcp-hosts fields. This patch implements a default
leasetime for both. If this XML entry is defined, it applies leasetime
for each range or host defined under DHCP scope.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=913446
Signed-off-by: Julio Faracco <jcfaracco(a)gmail.com>
---
docs/schemas/basictypes.rng | 9 ++++++
docs/schemas/network.rng | 11 +++++++
src/conf/network_conf.c | 62 ++++++++++++++++++++++++++++++++++++-
src/conf/network_conf.h | 14 +++++++++
src/libvirt_private.syms | 2 ++
src/network/bridge_driver.c | 37 ++++++++++++++++++++--
src/util/virdnsmasq.c | 40 ++++++++++++------------
src/util/virdnsmasq.h | 1 +
8 files changed, 152 insertions(+), 24 deletions(-)
diff --git a/docs/schemas/basictypes.rng b/docs/schemas/basictypes.rng
index 81465273c8..12f085c101 100644
--- a/docs/schemas/basictypes.rng
+++ b/docs/schemas/basictypes.rng
@@ -607,4 +607,13 @@
</element>
</define>
+ <define name="leaseUnit">
+ <choice>
+ <value>seconds</value>
+ <value>minutes</value>
+ <value>hours</value>
+ <value>infinite</value>
+ </choice>
+ </define>
+
</grammar>
diff --git a/docs/schemas/network.rng b/docs/schemas/network.rng
index 60453225d6..9a93529d52 100644
--- a/docs/schemas/network.rng
+++ b/docs/schemas/network.rng
@@ -398,6 +398,17 @@
</optional>
</element>
</optional>
+ <optional>
+ <element name="leasetime">
+ <optional>
+ <attribute name="unit"><ref
name="leaseUnit"/></attribute>
+ </optional>
+ <choice>
+ <data type="unsignedLong"/>
+ <empty/>
+ </choice>
+ </element>
+ </optional>
</interleave>
</element>
</optional>
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index 819b645df7..e6e82ee9fc 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -70,6 +70,14 @@ VIR_ENUM_IMPL(virNetworkTaint,
"hook-script",
);
+VIR_ENUM_IMPL(virNetworkDHCPLeaseTimeUnit,
+ VIR_NETWORK_DHCP_LEASETIME_UNIT_LAST,
+ "seconds",
+ "minutes",
+ "hours",
+ "infinite",
+);
+
static virClassPtr virNetworkXMLOptionClass;
static void
@@ -552,9 +560,50 @@ virNetworkDHCPHostDefParseXML(const char *networkName,
}
+static int
+virNetworkDHCPLeaseTimeDefParseXML(virNetworkIPDefPtr def,
+ xmlNodePtr node,
+ xmlXPathContextPtr ctxt)
+{
+ g_autofree char *leasetime = NULL, *leaseunit = NULL;
+
+ if (!(leaseunit = virXMLPropString(node, "unit")))
+ def->leaseunit = VIR_NETWORK_DHCP_LEASETIME_UNIT_SECONDS;
+ else
+ def->leaseunit = virNetworkDHCPLeaseTimeUnitTypeFromString(leaseunit);
+
+ if (def->leaseunit == VIR_NETWORK_DHCP_LEASETIME_UNIT_INFINITE)
+ return 0;
+
+ if (!(leasetime = virXPathString("string(./dhcp/leasetime)", ctxt)))
+ return -1;
+
+ if (virStrToLong_ul(leasetime, NULL, 10, &def->leasetime) < 0)
+ return -1;
+
+ /* This boundary check is related to dnsmasq man page settings:
+ * "The minimum lease time is two minutes." */
+ if ((def->leaseunit == VIR_NETWORK_DHCP_LEASETIME_UNIT_SECONDS &&
+ def->leasetime < 120) ||
+ (def->leaseunit == VIR_NETWORK_DHCP_LEASETIME_UNIT_MINUTES &&
+ def->leasetime < 2)) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("The minimum lease time should be greater "
+ "than 2 minutes"));
+ return -1;
+ }
+
+ if (def->leasetime > 0)
+ return 0;
+
+ return -1;
+}
+
+
static int
virNetworkDHCPDefParseXML(const char *networkName,
xmlNodePtr node,
+ xmlXPathContextPtr ctxt,
virNetworkIPDefPtr def)
{
int ret = -1;
@@ -583,7 +632,11 @@ virNetworkDHCPDefParseXML(const char *networkName,
goto cleanup;
if (VIR_APPEND_ELEMENT(def->hosts, def->nhosts, host) < 0)
goto cleanup;
+ } else if (cur->type == XML_ELEMENT_NODE &&
+ virXMLNodeNameEqual(cur, "leasetime")) {
+ if (virNetworkDHCPLeaseTimeDefParseXML(def, cur, ctxt) < 0)
+ goto cleanup;
} else if (VIR_SOCKET_ADDR_IS_FAMILY(&def->address, AF_INET) &&
cur->type == XML_ELEMENT_NODE &&
virXMLNodeNameEqual(cur, "bootp")) {
@@ -1143,7 +1196,7 @@ virNetworkIPDefParseXML(const char *networkName,
}
if ((dhcp = virXPathNode("./dhcp[1]", ctxt)) &&
- virNetworkDHCPDefParseXML(networkName, dhcp, def) < 0)
+ virNetworkDHCPDefParseXML(networkName, dhcp, ctxt, def) < 0)
goto cleanup;
if (virXPathNode("./tftp[1]", ctxt)) {
@@ -2343,6 +2396,13 @@ virNetworkIPDefFormat(virBufferPtr buf,
virBufferAddLit(buf, "/>\n");
}
+ if (def->leaseunit == VIR_NETWORK_DHCP_LEASETIME_UNIT_INFINITE) {
+ virBufferAddLit(buf, "<leasetime
unit='infinite'/>\n");
+ } else if (def->leasetime) {
+ virBufferAsprintf(buf, "<leasetime
unit='%s'>%lu</leasetime>\n",
+
virNetworkDHCPLeaseTimeUnitTypeToString(def->leaseunit),
+ def->leasetime);
+ }
virBufferAdjustIndent(buf, -2);
virBufferAddLit(buf, "</dhcp>\n");
diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h
index db7243eef5..66a5e1ad4f 100644
--- a/src/conf/network_conf.h
+++ b/src/conf/network_conf.h
@@ -80,6 +80,17 @@ typedef enum {
VIR_NETWORK_FORWARD_HOSTDEV_DEVICE_LAST,
} virNetworkForwardHostdevDeviceType;
+typedef enum {
+ VIR_NETWORK_DHCP_LEASETIME_UNIT_SECONDS = 0,
+ VIR_NETWORK_DHCP_LEASETIME_UNIT_MINUTES,
+ VIR_NETWORK_DHCP_LEASETIME_UNIT_HOURS,
+ VIR_NETWORK_DHCP_LEASETIME_UNIT_INFINITE,
+
+ VIR_NETWORK_DHCP_LEASETIME_UNIT_LAST,
+} virNetworkDHCPLeaseTimeUnitType;
+
+VIR_ENUM_DECL(virNetworkDHCPLeaseTimeUnit);
+
/* The backend driver used for devices from the pool. Currently used
* only for PCI devices (vfio vs. kvm), but could be used for other
* device types in the future.
@@ -176,6 +187,9 @@ struct _virNetworkIPDef {
size_t nhosts; /* Zero or more dhcp hosts */
virNetworkDHCPHostDefPtr hosts;
+ unsigned long leasetime;
+ virNetworkDHCPLeaseTimeUnitType leaseunit;
+
char *tftproot;
char *bootfile;
virSocketAddr bootserver;
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index ec367653d5..79bb5cc160 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -770,6 +770,8 @@ virNetworkDefParseNode;
virNetworkDefParseString;
virNetworkDefParseXML;
virNetworkDefUpdateSection;
+virNetworkDHCPLeaseTimeUnitTypeFromString;
+virNetworkDHCPLeaseTimeUnitTypeToString;
virNetworkForwardTypeToString;
virNetworkIPDefNetmask;
virNetworkIPDefPrefix;
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index f06099297a..d587e266de 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -966,6 +966,27 @@ static int networkConnectIsAlive(virConnectPtr conn G_GNUC_UNUSED)
}
+static char *
+networkBuildDnsmasqLeaseTime(virNetworkIPDefPtr ipdef)
+{
+ char *leasetime = NULL;
+ const char *unit;
+ g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
+
+ if (ipdef->leaseunit == VIR_NETWORK_DHCP_LEASETIME_UNIT_INFINITE) {
+ virBufferAddLit(&buf, "infinite");
+ } else if (ipdef->leasetime) {
+ unit = virNetworkDHCPLeaseTimeUnitTypeToString(ipdef->leaseunit);
+ /* We get only first compatible char from string: 's', 'm' or
'h' */
+ virBufferAsprintf(&buf, "%lu%c", ipdef->leasetime, unit[0]);
+ }
+
+ leasetime = virBufferContentAndReset(&buf);
+
+ return leasetime;
+}
+
+
/* the following does not build a file, it builds a list
* which is later saved into a file
*/
@@ -975,6 +996,9 @@ networkBuildDnsmasqDhcpHostsList(dnsmasqContext *dctx,
{
size_t i;
bool ipv6 = false;
+ g_autofree char *leasetime = NULL;
+
+ leasetime = networkBuildDnsmasqLeaseTime(ipdef);
if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET6))
ipv6 = true;
@@ -982,7 +1006,8 @@ networkBuildDnsmasqDhcpHostsList(dnsmasqContext *dctx,
virNetworkDHCPHostDefPtr host = &(ipdef->hosts[i]);
if (VIR_SOCKET_ADDR_VALID(&host->ip))
if (dnsmasqAddDhcpHost(dctx, host->mac, &host->ip,
- host->name, host->id, ipv6) < 0)
+ host->name, host->id, leasetime,
+ ipv6) < 0)
return -1;
}
@@ -1381,13 +1406,14 @@ networkDnsmasqConfContents(virNetworkObjPtr obj,
}
for (r = 0; r < ipdef->nranges; r++) {
int thisRange;
+ g_autofree char *leasetime = NULL;
if (!(saddr = virSocketAddrFormat(&ipdef->ranges[r].start)) ||
!(eaddr = virSocketAddrFormat(&ipdef->ranges[r].end)))
goto cleanup;
if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET6)) {
- virBufferAsprintf(&configbuf, "dhcp-range=%s,%s,%d\n",
+ virBufferAsprintf(&configbuf, "dhcp-range=%s,%s,%d",
saddr, eaddr, prefix);
} else {
/* IPv4 - dnsmasq requires a netmask rather than prefix */
@@ -1404,10 +1430,15 @@ networkDnsmasqConfContents(virNetworkObjPtr obj,
if (!(netmaskStr = virSocketAddrFormat(&netmask)))
goto cleanup;
- virBufferAsprintf(&configbuf, "dhcp-range=%s,%s,%s\n",
+ virBufferAsprintf(&configbuf, "dhcp-range=%s,%s,%s",
saddr, eaddr, netmaskStr);
}
+ if ((leasetime = networkBuildDnsmasqLeaseTime(ipdef)))
+ virBufferAsprintf(&configbuf, ",%s", leasetime);
+
+ virBufferAddLit(&configbuf, "\n");
+
VIR_FREE(saddr);
VIR_FREE(eaddr);
thisRange = virSocketAddrGetRange(&ipdef->ranges[r].start,
diff --git a/src/util/virdnsmasq.c b/src/util/virdnsmasq.c
index adc6f96bb6..fc977a47f5 100644
--- a/src/util/virdnsmasq.c
+++ b/src/util/virdnsmasq.c
@@ -296,11 +296,14 @@ hostsfileAdd(dnsmasqHostsfile *hostsfile,
virSocketAddr *ip,
const char *name,
const char *id,
+ const char *leasetime,
bool ipv6)
{
- char *ipstr = NULL;
+ g_autofree char *ipstr = NULL;
+ g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
+
if (VIR_REALLOC_N(hostsfile->hosts, hostsfile->nhosts + 1) < 0)
- goto error;
+ return -1;
if (!(ipstr = virSocketAddrFormat(ip)))
return -1;
@@ -308,34 +311,30 @@ hostsfileAdd(dnsmasqHostsfile *hostsfile,
/* the first test determines if it is a dhcpv6 host */
if (ipv6) {
if (name && id) {
- hostsfile->hosts[hostsfile->nhosts].host =
g_strdup_printf("id:%s,%s,[%s]",
- id, name, ipstr);
+ virBufferAsprintf(&buf, "id:%s,%s", id, name);
} else if (name && !id) {
- hostsfile->hosts[hostsfile->nhosts].host =
g_strdup_printf("%s,[%s]",
- name, ipstr);
+ virBufferAsprintf(&buf, "%s", name);
} else if (!name && id) {
- hostsfile->hosts[hostsfile->nhosts].host =
g_strdup_printf("id:%s,[%s]",
- id, ipstr);
+ virBufferAsprintf(&buf, "id:%s", id);
}
+ virBufferAsprintf(&buf, ",[%s]", ipstr);
} else if (name && mac) {
- hostsfile->hosts[hostsfile->nhosts].host =
g_strdup_printf("%s,%s,%s",
- mac, ipstr, name);
+ virBufferAsprintf(&buf, "%s,%s,%s", mac, ipstr, name);
} else if (name && !mac) {
- hostsfile->hosts[hostsfile->nhosts].host =
g_strdup_printf("%s,%s", name,
- ipstr);
+ virBufferAsprintf(&buf, "%s,%s", name, ipstr);
} else {
- hostsfile->hosts[hostsfile->nhosts].host =
g_strdup_printf("%s,%s", mac,
- ipstr);
+ virBufferAsprintf(&buf, "%s,%s", mac, ipstr);
}
- VIR_FREE(ipstr);
+
+ if (leasetime)
+ virBufferAsprintf(&buf, ",%s", leasetime);
+
+ if (!(hostsfile->hosts[hostsfile->nhosts].host =
virBufferContentAndReset(&buf)))
+ return -1;
hostsfile->nhosts++;
return 0;
-
- error:
- VIR_FREE(ipstr);
- return -1;
}
static dnsmasqHostsfile *
@@ -501,9 +500,10 @@ dnsmasqAddDhcpHost(dnsmasqContext *ctx,
virSocketAddr *ip,
const char *name,
const char *id,
+ const char *leasetime,
bool ipv6)
{
- return hostsfileAdd(ctx->hostsfile, mac, ip, name, id, ipv6);
+ return hostsfileAdd(ctx->hostsfile, mac, ip, name, id, leasetime, ipv6);
}
/*
diff --git a/src/util/virdnsmasq.h b/src/util/virdnsmasq.h
index ff0e56d635..6fbc101e64 100644
--- a/src/util/virdnsmasq.h
+++ b/src/util/virdnsmasq.h
@@ -87,6 +87,7 @@ int dnsmasqAddDhcpHost(dnsmasqContext *ctx,
virSocketAddr *ip,
const char *name,
const char *id,
+ const char *leasetime,
bool ipv6);
int dnsmasqAddHost(dnsmasqContext *ctx,
virSocketAddr *ip,
--
2.24.1
10:51 a.m.
New subject: [PATCH 1/2] conf: Add <leasetime/> option for <dhcp/> settings
On Wed, Apr 15, 2020 at 01:18:49PM -0300, Julio Faracco wrote:
If an user is trying to configure a dhcp neetwork settings, it is
not
possible to change the leasetime of a range or a host entry. This is
available using dnsmasq extra options, but they are associated with
dhcp-range or dhcp-hosts fields. This patch implements a default
leasetime for both. If this XML entry is defined, it applies leasetime
for each range or host defined under DHCP scope.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=913446
Signed-off-by: Julio Faracco <jcfaracco(a)gmail.com>
---
docs/schemas/basictypes.rng | 9 ++++++
docs/schemas/network.rng | 11 +++++++
src/conf/network_conf.c | 62 ++++++++++++++++++++++++++++++++++++-
src/conf/network_conf.h | 14 +++++++++
src/libvirt_private.syms | 2 ++
src/network/bridge_driver.c | 37 ++++++++++++++++++++--
src/util/virdnsmasq.c | 40 ++++++++++++------------
src/util/virdnsmasq.h | 1 +
8 files changed, 152 insertions(+), 24 deletions(-)
diff --git a/docs/schemas/basictypes.rng b/docs/schemas/basictypes.rng
index 81465273c8..12f085c101 100644
--- a/docs/schemas/basictypes.rng
+++ b/docs/schemas/basictypes.rng
@@ -607,4 +607,13 @@
</element>
</define>
+ <define name="leaseUnit">
+ <choice>
+ <value>seconds</value>
+ <value>minutes</value>
+ <value>hours</value>
+ <value>infinite</value>
"infinite" is not really a unit - it is a value, so I don't think
we should have this. At least one version of Alberto's patches
used the value "0" to indicate no expiry, which I think is
reasonable, as 0 is otherwise invalid.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
11:18 a.m.
New subject: [PATCH 2/2] tests: Add tests for <leasetime/> to cover dnsmasq settings
New tests are required to cover some new XML syntax entry for
<leasetime/> option. This includes schema testing and other features
like unit attribute and leasetime value.
Signed-off-by: Julio Faracco <jcfaracco(a)gmail.com>
---
tests/networkxml2confdata/leasetime-hours.conf | 16 ++++++++++++++++
tests/networkxml2confdata/leasetime-hours.xml | 12 ++++++++++++
.../networkxml2confdata/leasetime-infinite.conf | 16 ++++++++++++++++
tests/networkxml2confdata/leasetime-infinite.xml | 12 ++++++++++++
tests/networkxml2confdata/leasetime-minutes.conf | 16 ++++++++++++++++
tests/networkxml2confdata/leasetime-minutes.xml | 12 ++++++++++++
tests/networkxml2confdata/leasetime-seconds.conf | 16 ++++++++++++++++
tests/networkxml2confdata/leasetime-seconds.xml | 12 ++++++++++++
tests/networkxml2conftest.c | 4 ++++
tests/networkxml2xmlin/leasetime-hours.xml | 12 ++++++++++++
tests/networkxml2xmlin/leasetime-infinite.xml | 12 ++++++++++++
tests/networkxml2xmlin/leasetime-minutes.xml | 12 ++++++++++++
tests/networkxml2xmlin/leasetime-seconds.xml | 12 ++++++++++++
tests/networkxml2xmlout/leasetime-hours.xml | 14 ++++++++++++++
tests/networkxml2xmlout/leasetime-infinite.xml | 14 ++++++++++++++
tests/networkxml2xmlout/leasetime-minutes.xml | 14 ++++++++++++++
tests/networkxml2xmlout/leasetime-seconds.xml | 14 ++++++++++++++
tests/networkxml2xmltest.c | 4 ++++
18 files changed, 224 insertions(+)
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/networkxml2xmlin/leasetime-hours.xml
create mode 100644 tests/networkxml2xmlin/leasetime-infinite.xml
create mode 100644 tests/networkxml2xmlin/leasetime-minutes.xml
create mode 100644 tests/networkxml2xmlin/leasetime-seconds.xml
create mode 100644 tests/networkxml2xmlout/leasetime-hours.xml
create mode 100644 tests/networkxml2xmlout/leasetime-infinite.xml
create mode 100644 tests/networkxml2xmlout/leasetime-minutes.xml
create mode 100644 tests/networkxml2xmlout/leasetime-seconds.xml
diff --git a/tests/networkxml2confdata/leasetime-hours.conf
b/tests/networkxml2confdata/leasetime-hours.conf
new file mode 100644
index 0000000000..1599d4633e
--- /dev/null
+++ b/tests/networkxml2confdata/leasetime-hours.conf
@@ -0,0 +1,16 @@
+##WARNING: THIS IS AN AUTO-GENERATED FILE. CHANGES TO IT ARE LIKELY TO BE
+##OVERWRITTEN AND LOST. Changes to this configuration should be made using:
+## virsh net-edit default
+## or other application using the libvirt API.
+##
+## dnsmasq conf file created by libvirt
+strict-order
+except-interface=lo
+bind-dynamic
+interface=virbr0
+dhcp-range=192.168.122.2,192.168.122.254,255.255.255.0,1h
+dhcp-no-override
+dhcp-authoritative
+dhcp-lease-max=253
+dhcp-hostsfile=/var/lib/libvirt/dnsmasq/default.hostsfile
+addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts
diff --git a/tests/networkxml2confdata/leasetime-hours.xml
b/tests/networkxml2confdata/leasetime-hours.xml
new file mode 100644
index 0000000000..8df196e84e
--- /dev/null
+++ b/tests/networkxml2confdata/leasetime-hours.xml
@@ -0,0 +1,12 @@
+<network xmlns:dnsmasq="http://libvirt.org/schemas/network/dnsmasq/1.0">
+ <name>default</name>
+ <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid>
+ <bridge name="virbr0"/>
+ <forward mode="nat" dev="eth1"/>
+ <ip address="192.168.122.1" netmask="255.255.255.0">
+ <dhcp>
+ <range start="192.168.122.2" end="192.168.122.254"/>
+ <leasetime unit="hours">1</leasetime>
+ </dhcp>
+ </ip>
+</network>
diff --git a/tests/networkxml2confdata/leasetime-infinite.conf
b/tests/networkxml2confdata/leasetime-infinite.conf
new file mode 100644
index 0000000000..883ced6ade
--- /dev/null
+++ b/tests/networkxml2confdata/leasetime-infinite.conf
@@ -0,0 +1,16 @@
+##WARNING: THIS IS AN AUTO-GENERATED FILE. CHANGES TO IT ARE LIKELY TO BE
+##OVERWRITTEN AND LOST. Changes to this configuration should be made using:
+## virsh net-edit default
+## or other application using the libvirt API.
+##
+## dnsmasq conf file created by libvirt
+strict-order
+except-interface=lo
+bind-dynamic
+interface=virbr0
+dhcp-range=192.168.122.2,192.168.122.254,255.255.255.0,infinite
+dhcp-no-override
+dhcp-authoritative
+dhcp-lease-max=253
+dhcp-hostsfile=/var/lib/libvirt/dnsmasq/default.hostsfile
+addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts
diff --git a/tests/networkxml2confdata/leasetime-infinite.xml
b/tests/networkxml2confdata/leasetime-infinite.xml
new file mode 100644
index 0000000000..9ae3c4f7f7
--- /dev/null
+++ b/tests/networkxml2confdata/leasetime-infinite.xml
@@ -0,0 +1,12 @@
+<network xmlns:dnsmasq="http://libvirt.org/schemas/network/dnsmasq/1.0">
+ <name>default</name>
+ <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid>
+ <bridge name="virbr0"/>
+ <forward mode="nat" dev="eth1"/>
+ <ip address="192.168.122.1" netmask="255.255.255.0">
+ <dhcp>
+ <range start="192.168.122.2" end="192.168.122.254"/>
+ <leasetime unit="infinite"/>
+ </dhcp>
+ </ip>
+</network>
diff --git a/tests/networkxml2confdata/leasetime-minutes.conf
b/tests/networkxml2confdata/leasetime-minutes.conf
new file mode 100644
index 0000000000..c093501a35
--- /dev/null
+++ b/tests/networkxml2confdata/leasetime-minutes.conf
@@ -0,0 +1,16 @@
+##WARNING: THIS IS AN AUTO-GENERATED FILE. CHANGES TO IT ARE LIKELY TO BE
+##OVERWRITTEN AND LOST. Changes to this configuration should be made using:
+## virsh net-edit default
+## or other application using the libvirt API.
+##
+## dnsmasq conf file created by libvirt
+strict-order
+except-interface=lo
+bind-dynamic
+interface=virbr0
+dhcp-range=192.168.122.2,192.168.122.254,255.255.255.0,10m
+dhcp-no-override
+dhcp-authoritative
+dhcp-lease-max=253
+dhcp-hostsfile=/var/lib/libvirt/dnsmasq/default.hostsfile
+addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts
diff --git a/tests/networkxml2confdata/leasetime-minutes.xml
b/tests/networkxml2confdata/leasetime-minutes.xml
new file mode 100644
index 0000000000..e531db2848
--- /dev/null
+++ b/tests/networkxml2confdata/leasetime-minutes.xml
@@ -0,0 +1,12 @@
+<network xmlns:dnsmasq="http://libvirt.org/schemas/network/dnsmasq/1.0">
+ <name>default</name>
+ <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid>
+ <bridge name="virbr0"/>
+ <forward mode="nat" dev="eth1"/>
+ <ip address="192.168.122.1" netmask="255.255.255.0">
+ <dhcp>
+ <range start="192.168.122.2" end="192.168.122.254"/>
+ <leasetime unit="minutes">10</leasetime>
+ </dhcp>
+ </ip>
+</network>
diff --git a/tests/networkxml2confdata/leasetime-seconds.conf
b/tests/networkxml2confdata/leasetime-seconds.conf
new file mode 100644
index 0000000000..4acc2839c5
--- /dev/null
+++ b/tests/networkxml2confdata/leasetime-seconds.conf
@@ -0,0 +1,16 @@
+##WARNING: THIS IS AN AUTO-GENERATED FILE. CHANGES TO IT ARE LIKELY TO BE
+##OVERWRITTEN AND LOST. Changes to this configuration should be made using:
+## virsh net-edit default
+## or other application using the libvirt API.
+##
+## dnsmasq conf file created by libvirt
+strict-order
+except-interface=lo
+bind-dynamic
+interface=virbr0
+dhcp-range=192.168.122.2,192.168.122.254,255.255.255.0,1000s
+dhcp-no-override
+dhcp-authoritative
+dhcp-lease-max=253
+dhcp-hostsfile=/var/lib/libvirt/dnsmasq/default.hostsfile
+addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts
diff --git a/tests/networkxml2confdata/leasetime-seconds.xml
b/tests/networkxml2confdata/leasetime-seconds.xml
new file mode 100644
index 0000000000..4b4a57110e
--- /dev/null
+++ b/tests/networkxml2confdata/leasetime-seconds.xml
@@ -0,0 +1,12 @@
+<network xmlns:dnsmasq="http://libvirt.org/schemas/network/dnsmasq/1.0">
+ <name>default</name>
+ <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid>
+ <bridge name="virbr0"/>
+ <forward mode="nat" dev="eth1"/>
+ <ip address="192.168.122.1" netmask="255.255.255.0">
+ <dhcp>
+ <range start="192.168.122.2" end="192.168.122.254"/>
+ <leasetime>1000</leasetime>
+ </dhcp>
+ </ip>
+</network>
diff --git a/tests/networkxml2conftest.c b/tests/networkxml2conftest.c
index a8355272b9..3cb6658f51 100644
--- a/tests/networkxml2conftest.c
+++ b/tests/networkxml2conftest.c
@@ -142,6 +142,10 @@ mymain(void)
DO_TEST("dhcp6host-routed-network", dhcpv6);
DO_TEST("ptr-domains-auto", dhcpv6);
DO_TEST("dnsmasq-options", dhcpv6);
+ DO_TEST("leasetime-seconds", full);
+ DO_TEST("leasetime-minutes", full);
+ DO_TEST("leasetime-hours", full);
+ DO_TEST("leasetime-infinite", full);
virObjectUnref(dhcpv6);
virObjectUnref(full);
diff --git a/tests/networkxml2xmlin/leasetime-hours.xml
b/tests/networkxml2xmlin/leasetime-hours.xml
new file mode 100644
index 0000000000..d8075111a4
--- /dev/null
+++ b/tests/networkxml2xmlin/leasetime-hours.xml
@@ -0,0 +1,12 @@
+<network>
+ <name>default</name>
+ <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid>
+ <bridge name="virbr0"/>
+ <forward mode="nat" dev="eth1"/>
+ <ip address="192.168.122.1" netmask="255.255.255.0">
+ <dhcp>
+ <range start="192.168.122.2" end="192.168.122.254"/>
+ <leasetime unit="hours">1</leasetime>
+ </dhcp>
+ </ip>
+</network>
diff --git a/tests/networkxml2xmlin/leasetime-infinite.xml
b/tests/networkxml2xmlin/leasetime-infinite.xml
new file mode 100644
index 0000000000..d0357620b0
--- /dev/null
+++ b/tests/networkxml2xmlin/leasetime-infinite.xml
@@ -0,0 +1,12 @@
+<network>
+ <name>default</name>
+ <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid>
+ <bridge name="virbr0"/>
+ <forward mode="nat" dev="eth1"/>
+ <ip address="192.168.122.1" netmask="255.255.255.0">
+ <dhcp>
+ <range start="192.168.122.2" end="192.168.122.254"/>
+ <leasetime unit="infinite"/>
+ </dhcp>
+ </ip>
+</network>
diff --git a/tests/networkxml2xmlin/leasetime-minutes.xml
b/tests/networkxml2xmlin/leasetime-minutes.xml
new file mode 100644
index 0000000000..21169766a4
--- /dev/null
+++ b/tests/networkxml2xmlin/leasetime-minutes.xml
@@ -0,0 +1,12 @@
+<network>
+ <name>default</name>
+ <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid>
+ <bridge name="virbr0"/>
+ <forward mode="nat" dev="eth1"/>
+ <ip address="192.168.122.1" netmask="255.255.255.0">
+ <dhcp>
+ <range start="192.168.122.2" end="192.168.122.254"/>
+ <leasetime unit="minutes">10</leasetime>
+ </dhcp>
+ </ip>
+</network>
diff --git a/tests/networkxml2xmlin/leasetime-seconds.xml
b/tests/networkxml2xmlin/leasetime-seconds.xml
new file mode 100644
index 0000000000..4cf64b851e
--- /dev/null
+++ b/tests/networkxml2xmlin/leasetime-seconds.xml
@@ -0,0 +1,12 @@
+<network>
+ <name>default</name>
+ <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid>
+ <bridge name="virbr0"/>
+ <forward mode="nat" dev="eth1"/>
+ <ip address="192.168.122.1" netmask="255.255.255.0">
+ <dhcp>
+ <range start="192.168.122.2" end="192.168.122.254"/>
+ <leasetime unit="seconds">200</leasetime>
+ </dhcp>
+ </ip>
+</network>
diff --git a/tests/networkxml2xmlout/leasetime-hours.xml
b/tests/networkxml2xmlout/leasetime-hours.xml
new file mode 100644
index 0000000000..ed85f77569
--- /dev/null
+++ b/tests/networkxml2xmlout/leasetime-hours.xml
@@ -0,0 +1,14 @@
+<network>
+ <name>default</name>
+ <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid>
+ <forward dev='eth1' mode='nat'>
+ <interface dev='eth1'/>
+ </forward>
+ <bridge name='virbr0' stp='on' delay='0'/>
+ <ip address='192.168.122.1' netmask='255.255.255.0'>
+ <dhcp>
+ <range start='192.168.122.2' end='192.168.122.254'/>
+ <leasetime unit='hours'>1</leasetime>
+ </dhcp>
+ </ip>
+</network>
diff --git a/tests/networkxml2xmlout/leasetime-infinite.xml
b/tests/networkxml2xmlout/leasetime-infinite.xml
new file mode 100644
index 0000000000..4373357588
--- /dev/null
+++ b/tests/networkxml2xmlout/leasetime-infinite.xml
@@ -0,0 +1,14 @@
+<network>
+ <name>default</name>
+ <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid>
+ <forward dev='eth1' mode='nat'>
+ <interface dev='eth1'/>
+ </forward>
+ <bridge name='virbr0' stp='on' delay='0'/>
+ <ip address='192.168.122.1' netmask='255.255.255.0'>
+ <dhcp>
+ <range start='192.168.122.2' end='192.168.122.254'/>
+ <leasetime unit='infinite'/>
+ </dhcp>
+ </ip>
+</network>
diff --git a/tests/networkxml2xmlout/leasetime-minutes.xml
b/tests/networkxml2xmlout/leasetime-minutes.xml
new file mode 100644
index 0000000000..9e3d8591fa
--- /dev/null
+++ b/tests/networkxml2xmlout/leasetime-minutes.xml
@@ -0,0 +1,14 @@
+<network>
+ <name>default</name>
+ <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid>
+ <forward dev='eth1' mode='nat'>
+ <interface dev='eth1'/>
+ </forward>
+ <bridge name='virbr0' stp='on' delay='0'/>
+ <ip address='192.168.122.1' netmask='255.255.255.0'>
+ <dhcp>
+ <range start='192.168.122.2' end='192.168.122.254'/>
+ <leasetime unit='minutes'>10</leasetime>
+ </dhcp>
+ </ip>
+</network>
diff --git a/tests/networkxml2xmlout/leasetime-seconds.xml
b/tests/networkxml2xmlout/leasetime-seconds.xml
new file mode 100644
index 0000000000..a4e428f84b
--- /dev/null
+++ b/tests/networkxml2xmlout/leasetime-seconds.xml
@@ -0,0 +1,14 @@
+<network>
+ <name>default</name>
+ <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid>
+ <forward dev='eth1' mode='nat'>
+ <interface dev='eth1'/>
+ </forward>
+ <bridge name='virbr0' stp='on' delay='0'/>
+ <ip address='192.168.122.1' netmask='255.255.255.0'>
+ <dhcp>
+ <range start='192.168.122.2' end='192.168.122.254'/>
+ <leasetime unit='seconds'>200</leasetime>
+ </dhcp>
+ </ip>
+</network>
diff --git a/tests/networkxml2xmltest.c b/tests/networkxml2xmltest.c
index ec679e72ee..700744785a 100644
--- a/tests/networkxml2xmltest.c
+++ b/tests/networkxml2xmltest.c
@@ -160,6 +160,10 @@ mymain(void)
DO_TEST("metadata");
DO_TEST("set-mtu");
DO_TEST("dnsmasq-options");
+ DO_TEST("leasetime-seconds");
+ DO_TEST("leasetime-minutes");
+ DO_TEST("leasetime-hours");
+ DO_TEST("leasetime-infinite");
DO_TEST("isolated-ports");
return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
--
2.24.1
11:25 a.m.
I resubmitted this series because our team needs to hack dnsmasq
settings to change lease time. This feature would be so important to
us to avoid workarounds.
It is based on Alberto's patch from 2017. But personally I don't like
this approach.
IMHO, it would be nice to have specific attributes to configure lease time.
For example:
<range ... leasetime="10m"/>
<host ... leasetime="20m"/>
They can be different from each other.
I still think that the idea should be better developed.
I don't like that my example also (it is just an example).
That's why I submitted... To listen opinions from others.
--
Julio Cesar Faracco
Em qua., 15 de abr. de 2020 às 13:19, Julio Faracco
<jcfaracco(a)gmail.com> escreveu:
This series is based on latest series from Alberto. It includes a new
entry called <leasetime/> under <dhcp/> scope to add a default lease
time for range and host options for dnsmasq. There is no point to
configure both separately. If they are defined (range and/or host), they
should have the same lease time value.
This series includes some test cases to cover lease time XML syntax
also.
Julio Faracco (2):
conf: Add <leasetime/> option for <dhcp/> settings
tests: Add tests for <leasetime/> to cover dnsmasq settings
docs/schemas/basictypes.rng | 9 +++
docs/schemas/network.rng | 11 ++++
src/conf/network_conf.c | 62 ++++++++++++++++++-
src/conf/network_conf.h | 14 +++++
src/libvirt_private.syms | 2 +
src/network/bridge_driver.c | 37 ++++++++++-
src/util/virdnsmasq.c | 40 ++++++------
src/util/virdnsmasq.h | 1 +
.../networkxml2confdata/leasetime-hours.conf | 16 +++++
tests/networkxml2confdata/leasetime-hours.xml | 12 ++++
.../leasetime-infinite.conf | 16 +++++
.../leasetime-infinite.xml | 12 ++++
.../leasetime-minutes.conf | 16 +++++
.../networkxml2confdata/leasetime-minutes.xml | 12 ++++
.../leasetime-seconds.conf | 16 +++++
.../networkxml2confdata/leasetime-seconds.xml | 12 ++++
tests/networkxml2conftest.c | 4 ++
tests/networkxml2xmlin/leasetime-hours.xml | 12 ++++
tests/networkxml2xmlin/leasetime-infinite.xml | 12 ++++
tests/networkxml2xmlin/leasetime-minutes.xml | 12 ++++
tests/networkxml2xmlin/leasetime-seconds.xml | 12 ++++
tests/networkxml2xmlout/leasetime-hours.xml | 14 +++++
.../networkxml2xmlout/leasetime-infinite.xml | 14 +++++
tests/networkxml2xmlout/leasetime-minutes.xml | 14 +++++
tests/networkxml2xmlout/leasetime-seconds.xml | 14 +++++
tests/networkxml2xmltest.c | 4 ++
26 files changed, 376 insertions(+), 24 deletions(-)
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/networkxml2xmlin/leasetime-hours.xml
create mode 100644 tests/networkxml2xmlin/leasetime-infinite.xml
create mode 100644 tests/networkxml2xmlin/leasetime-minutes.xml
create mode 100644 tests/networkxml2xmlin/leasetime-seconds.xml
create mode 100644 tests/networkxml2xmlout/leasetime-hours.xml
create mode 100644 tests/networkxml2xmlout/leasetime-infinite.xml
create mode 100644 tests/networkxml2xmlout/leasetime-minutes.xml
create mode 100644 tests/networkxml2xmlout/leasetime-seconds.xml
--
2.24.1
11 a.m.
On Wed, Apr 15, 2020 at 01:25:37PM -0300, Julio Faracco wrote:
I resubmitted this series because our team needs to hack dnsmasq
settings to change lease time. This feature would be so important to
us to avoid workarounds.
It is based on Alberto's patch from 2017. But personally I don't like
this approach.
IMHO, it would be nice to have specific attributes to configure lease time.
For example:
<range ... leasetime="10m"/>
<host ... leasetime="20m"/>
It is generally considered bad practice to have an XML attribute
value which then has to be parsed again to extract information.
For this reason, libvirt will use two attrbutes, one of the
value and one for the units (with some sensible default
units if not specified), even though this is admittedly
more verbose.
I agree it would be useful to have lease time per-host, as well
as globally though, and IIRC one of the original versions of
this patch did support that.
We could do one of
<ip address="192.168.122.1" netmask="255.255.255.0">
<dhcp>
<range start="192.168.122.2" end="192.168.122.254"
lease="12" units="hours"/>
<host id="0:1:0:1:18:aa:62:fe:0:16:3e:44:55:66"
ip="2001:db8:ca2:2:3::2" lease="30" units="mins"/>
</dhcp>
</ip>
or
<ip address="192.168.122.1" netmask="255.255.255.0">
<dhcp>
<range start="192.168.122.2" end="192.168.122.254"
leasetime="12" leaseunits="hours"/>
<host id="0:1:0:1:18:aa:62:fe:0:16:3e:44:55:66"
ip="2001:db8:ca2:2:3::2" leasetime="30"
leaseunits="mins"/>
</dhcp>
</ip>
or
<ip address="192.168.122.1" netmask="255.255.255.0">
<dhcp>
<range start="192.168.122.2" end="192.168.122.254">
<lease expiry="12" units="hours"/>
</range>
<host id="0:1:0:1:18:aa:62:fe:0:16:3e:44:55:66"
ip="2001:db8:ca2:2:3::2">
<lease expiry="30" units="mins"/>
</host>
</dhcp>
</ip>
In all of these we can default to "minutes" if no units are given.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
11:16 p.m.
On 4/17/20 12:00 PM, Daniel P. Berrangé wrote:
On Wed, Apr 15, 2020 at 01:25:37PM -0300, Julio Faracco wrote:
> I resubmitted this series because our team needs to hack dnsmasq
> settings to change lease time. This feature would be so important to
> us to avoid workarounds.
>
> It is based on Alberto's patch from 2017. But personally I don't like
> this approach.
> IMHO, it would be nice to have specific attributes to configure lease time.
> For example:
> <range ... leasetime="10m"/>
> <host ... leasetime="20m"/>
It is generally considered bad practice to have an XML attribute
value which then has to be parsed again to extract information.
For this reason, libvirt will use two attrbutes, one of the
value and one for the units (with some sensible default
units if not specified), even though this is admittedly
more verbose.
I agree it would be useful to have lease time per-host, as well
as globally though, and IIRC one of the original versions of
this patch did support that.
The name of the original contributor that you (Julio) referenced in your
cover letter sounded familiar, and I tried to find the original BZ for
this when I saw your patches on the list, but I failed (bugzilla kept
timing out on a *very* basic search term - this seems to happen to 80%
of my queries these days...) But then it passed by in mail when Dan was
cleaning up all the upstream libvirt BZes and moving them to gitlab :-),
so just so everyone has all the background info:
https://bugzilla.redhat.com/show_bug.cgi?id=913446
I also found the other similar patch from Nehal J Wani from around the
same time:
https://www.redhat.com/archives/libvir-list/2016-September/msg01063.html
Without talking about the specifics of either patch, my recollection of
the discussion from that time was that both contributors were trying to
use a leasetime setting to solve a problem that it wouldn't have solved
- their issue was that if a guest was turned off during the time when
its lease expired, then when the guest was subsequently restarted it
would end up with a different IP address. Of course setting a longer
lease expiry would only delay the problem, not eliminate it. Further
discussion revealed that if libvirt would just set the
"dhcp-authoritative" option in the dnsmasq config, then dnsmasq would
attempt to reissue the same IP to a guest even if its lease had already
expired - Martin Wilck made this change to libvirt in commit 4ac20b3ae4,
which seemed to satisfy the people who had thought they needed to modify
the dhcp lease time, and so neither of the lease time patches was pushed
upstream.
The reason I bring up this old history is just as a cautionary tale that
sometimes what you think you need isn't really what you actually need :-)
(Recently Cole added the dnsmasq private namespace to the <network> XML,
but that is only useful to add an entire option line, and can't do what
you need, which is adding an extra option to every dhcp-host and
dhcp-range line; there unfortunately is no standalone dnsmasq option to
specify a global lease time)
We could do one of
<ip address="192.168.122.1" netmask="255.255.255.0">
<dhcp>
<range start="192.168.122.2" end="192.168.122.254"
lease="12" units="hours"/>
<host id="0:1:0:1:18:aa:62:fe:0:16:3e:44:55:66"
ip="2001:db8:ca2:2:3::2" lease="30" units="mins"/>
</dhcp>
</ip>
or
<ip address="192.168.122.1" netmask="255.255.255.0">
<dhcp>
<range start="192.168.122.2" end="192.168.122.254"
leasetime="12" leaseunits="hours"/>
<host id="0:1:0:1:18:aa:62:fe:0:16:3e:44:55:66"
ip="2001:db8:ca2:2:3::2" leasetime="30"
leaseunits="mins"/>
</dhcp>
</ip>
or
<ip address="192.168.122.1" netmask="255.255.255.0">
<dhcp>
<range start="192.168.122.2" end="192.168.122.254">
<lease expiry="12" units="hours"/>
</range>
<host id="0:1:0:1:18:aa:62:fe:0:16:3e:44:55:66"
ip="2001:db8:ca2:2:3::2">
<lease expiry="30" units="mins"/>
</host>
</dhcp>
</ip>
Nehal's patch had used this syntax:
<leasetime units='hours'>12</leasetime>
based on (I guess) the syntax of libvirt's <memory> element:
<memory unit='KiB'>524288</memory>
I don't have a preference for any of them, just thought I would point
out existing usage in libvirt that has a value/units combination.
In all of these we can default to "minutes" if no units are given.
Regards,
Daniel
4:16 a.m.
On Sun, Apr 19, 2020 at 12:16:39AM -0400, Laine Stump wrote:
On 4/17/20 12:00 PM, Daniel P. Berrangé wrote:
> On Wed, Apr 15, 2020 at 01:25:37PM -0300, Julio Faracco wrote:
> > I resubmitted this series because our team needs to hack dnsmasq
> > settings to change lease time. This feature would be so important to
> > us to avoid workarounds.
> >
> > It is based on Alberto's patch from 2017. But personally I don't like
> > this approach.
> > IMHO, it would be nice to have specific attributes to configure lease time.
> > For example:
> > <range ... leasetime="10m"/>
> > <host ... leasetime="20m"/>
> It is generally considered bad practice to have an XML attribute
> value which then has to be parsed again to extract information.
>
> For this reason, libvirt will use two attrbutes, one of the
> value and one for the units (with some sensible default
> units if not specified), even though this is admittedly
> more verbose.
>
> I agree it would be useful to have lease time per-host, as well
> as globally though, and IIRC one of the original versions of
> this patch did support that.
The name of the original contributor that you (Julio) referenced in your
cover letter sounded familiar, and I tried to find the original BZ for this
when I saw your patches on the list, but I failed (bugzilla kept timing out
on a *very* basic search term - this seems to happen to 80% of my queries
these days...) But then it passed by in mail when Dan was cleaning up all
the upstream libvirt BZes and moving them to gitlab :-), so just so everyone
has all the background info:
https://bugzilla.redhat.com/show_bug.cgi?id=913446
I also found the other similar patch from Nehal J Wani from around the same
time:
https://www.redhat.com/archives/libvir-list/2016-September/msg01063.html
Without talking about the specifics of either patch, my recollection of the
discussion from that time was that both contributors were trying to use a
leasetime setting to solve a problem that it wouldn't have solved - their
issue was that if a guest was turned off during the time when its lease
expired, then when the guest was subsequently restarted it would end up with
a different IP address. Of course setting a longer lease expiry would only
delay the problem, not eliminate it. Further discussion revealed that if
libvirt would just set the "dhcp-authoritative" option in the dnsmasq
config, then dnsmasq would attempt to reissue the same IP to a guest even if
its lease had already expired - Martin Wilck made this change to libvirt in
commit 4ac20b3ae4, which seemed to satisfy the people who had thought they
needed to modify the dhcp lease time, and so neither of the lease time
patches was pushed upstream.
The reason I bring up this old history is just as a cautionary tale that
sometimes what you think you need isn't really what you actually need :-)
(Recently Cole added the dnsmasq private namespace to the <network> XML, but
that is only useful to add an entire option line, and can't do what you
need, which is adding an extra option to every dhcp-host and dhcp-range
line; there unfortunately is no standalone dnsmasq option to specify a
global lease time)
>
> We could do one of
>
> <ip address="192.168.122.1" netmask="255.255.255.0">
> <dhcp>
> <range start="192.168.122.2" end="192.168.122.254"
lease="12" units="hours"/>
> <host id="0:1:0:1:18:aa:62:fe:0:16:3e:44:55:66"
ip="2001:db8:ca2:2:3::2" lease="30" units="mins"/>
> </dhcp>
> </ip>
>
> or
>
> <ip address="192.168.122.1" netmask="255.255.255.0">
> <dhcp>
> <range start="192.168.122.2" end="192.168.122.254"
leasetime="12" leaseunits="hours"/>
> <host id="0:1:0:1:18:aa:62:fe:0:16:3e:44:55:66"
ip="2001:db8:ca2:2:3::2" leasetime="30"
leaseunits="mins"/>
> </dhcp>
> </ip>
>
> or
>
> <ip address="192.168.122.1" netmask="255.255.255.0">
> <dhcp>
> <range start="192.168.122.2"
end="192.168.122.254">
> <lease expiry="12" units="hours"/>
> </range>
> <host id="0:1:0:1:18:aa:62:fe:0:16:3e:44:55:66"
ip="2001:db8:ca2:2:3::2">
> <lease expiry="30" units="mins"/>
> </host>
> </dhcp>
> </ip>
Nehal's patch had used this syntax:
<leasetime units='hours'>12</leasetime>
based on (I guess) the syntax of libvirt's <memory> element:
<memory unit='KiB'>524288</memory>
I don't have a preference for any of them, just thought I would point out
existing usage in libvirt that has a value/units combination.
Annoyingly it seems we used "units" and "unit", though
"unit" wins by
a large margin over "units"
I guess I have a slight preference for the last option, with the use of
the child-element, as it is the cleanest XML design, even if it is slightly
more verbose.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
1663
days inactive
1668
days old
7 comments
3 participants
participants (3)
-
Daniel P. Berrangé -
Julio Faracco -
Laine Stump