[libvirt] [PATCH 1/3] leasetime support for <dhcp> globally
From: Alberto Ruiz <aruiz@gnome.org> --- 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> + </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) +{ + 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; + } + + 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); + *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); + 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, diff --git a/tests/networkxml2confdata/leasetime-days.conf b/tests/networkxml2confdata/leasetime-days.conf new file mode 100644 index 000000000..9501e2f8a --- /dev/null +++ b/tests/networkxml2confdata/leasetime-days.conf @@ -0,0 +1,17 @@ +##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,86400 +dhcp-no-override +dhcp-range=2001:db8:ac10:fd01::1:10,2001:db8:ac10:fd01::1:ff,64,86400 +dhcp-lease-max=493 +dhcp-hostsfile=/var/lib/libvirt/dnsmasq/default.hostsfile +addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts +enable-ra diff --git a/tests/networkxml2confdata/leasetime-days.xml b/tests/networkxml2confdata/leasetime-days.xml new file mode 100644 index 000000000..b990b4d68 --- /dev/null +++ b/tests/networkxml2confdata/leasetime-days.xml @@ -0,0 +1,18 @@ +<network> + <name>default</name> + <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid> + <forward dev='eth1' mode='nat'/> + <bridge name='virbr0' stp='on' delay='0'/> + <ip address='192.168.122.1' netmask='255.255.255.0'> + <dhcp> + <leasetime unit="days">1</leasetime> + <range start='192.168.122.2' end='192.168.122.254'/> + </dhcp> + </ip> + <ip family='ipv6' address='2001:db8:ac10:fd01::1' prefix='64'> + <dhcp> + <leasetime unit="days">1</leasetime> + <range start='2001:db8:ac10:fd01::1:10' end='2001:db8:ac10:fd01::1:ff'/> + </dhcp> + </ip> +</network> diff --git a/tests/networkxml2confdata/leasetime-hours.conf b/tests/networkxml2confdata/leasetime-hours.conf new file mode 100644 index 000000000..021a769bc --- /dev/null +++ b/tests/networkxml2confdata/leasetime-hours.conf @@ -0,0 +1,17 @@ +##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,3600 +dhcp-no-override +dhcp-range=2001:db8:ac10:fd01::1:10,2001:db8:ac10:fd01::1:ff,64,3600 +dhcp-lease-max=493 +dhcp-hostsfile=/var/lib/libvirt/dnsmasq/default.hostsfile +addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts +enable-ra diff --git a/tests/networkxml2confdata/leasetime-hours.xml b/tests/networkxml2confdata/leasetime-hours.xml new file mode 100644 index 000000000..3b9609601 --- /dev/null +++ b/tests/networkxml2confdata/leasetime-hours.xml @@ -0,0 +1,18 @@ +<network> + <name>default</name> + <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid> + <forward dev='eth1' mode='nat'/> + <bridge name='virbr0' stp='on' delay='0'/> + <ip address='192.168.122.1' netmask='255.255.255.0'> + <dhcp> + <leasetime unit="hours">1</leasetime> + <range start='192.168.122.2' end='192.168.122.254'/> + </dhcp> + </ip> + <ip family='ipv6' address='2001:db8:ac10:fd01::1' prefix='64'> + <dhcp> + <leasetime unit="hours">1</leasetime> + <range start='2001:db8:ac10:fd01::1:10' end='2001:db8:ac10:fd01::1:ff'/> + </dhcp> + </ip> +</network> diff --git a/tests/networkxml2confdata/leasetime-infinite.conf b/tests/networkxml2confdata/leasetime-infinite.conf new file mode 100644 index 000000000..cc21135de --- /dev/null +++ b/tests/networkxml2confdata/leasetime-infinite.conf @@ -0,0 +1,17 @@ +##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,infinite +dhcp-no-override +dhcp-range=2001:db8:ac10:fd01::1:10,2001:db8:ac10:fd01::1:ff,64,infinite +dhcp-lease-max=493 +dhcp-hostsfile=/var/lib/libvirt/dnsmasq/default.hostsfile +addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts +enable-ra diff --git a/tests/networkxml2confdata/leasetime-infinite.xml b/tests/networkxml2confdata/leasetime-infinite.xml new file mode 100644 index 000000000..bc8740ee6 --- /dev/null +++ b/tests/networkxml2confdata/leasetime-infinite.xml @@ -0,0 +1,18 @@ +<network> + <name>default</name> + <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid> + <forward dev='eth1' mode='nat'/> + <bridge name='virbr0' stp='on' delay='0'/> + <ip address='192.168.122.1' netmask='255.255.255.0'> + <dhcp> + <leasetime>-1</leasetime> + <range start='192.168.122.2' end='192.168.122.254'/> + </dhcp> + </ip> + <ip family='ipv6' address='2001:db8:ac10:fd01::1' prefix='64'> + <dhcp> + <leasetime>-1</leasetime> + <range start='2001:db8:ac10:fd01::1:10' end='2001:db8:ac10:fd01::1:ff'/> + </dhcp> + </ip> +</network> diff --git a/tests/networkxml2confdata/leasetime-minutes.conf b/tests/networkxml2confdata/leasetime-minutes.conf new file mode 100644 index 000000000..db688951b --- /dev/null +++ b/tests/networkxml2confdata/leasetime-minutes.conf @@ -0,0 +1,17 @@ +##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,300 +dhcp-no-override +dhcp-range=2001:db8:ac10:fd01::1:10,2001:db8:ac10:fd01::1:ff,64,300 +dhcp-lease-max=493 +dhcp-hostsfile=/var/lib/libvirt/dnsmasq/default.hostsfile +addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts +enable-ra diff --git a/tests/networkxml2confdata/leasetime-minutes.xml b/tests/networkxml2confdata/leasetime-minutes.xml new file mode 100644 index 000000000..e7a27afe6 --- /dev/null +++ b/tests/networkxml2confdata/leasetime-minutes.xml @@ -0,0 +1,18 @@ +<network> + <name>default</name> + <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid> + <forward dev='eth1' mode='nat'/> + <bridge name='virbr0' stp='on' delay='0'/> + <ip address='192.168.122.1' netmask='255.255.255.0'> + <dhcp> + <leasetime unit="minutes">5</leasetime> + <range start='192.168.122.2' end='192.168.122.254'/> + </dhcp> + </ip> + <ip family='ipv6' address='2001:db8:ac10:fd01::1' prefix='64'> + <dhcp> + <leasetime unit="minutes">5</leasetime> + <range start='2001:db8:ac10:fd01::1:10' end='2001:db8:ac10:fd01::1:ff'/> + </dhcp> + </ip> +</network> diff --git a/tests/networkxml2confdata/leasetime-seconds.conf b/tests/networkxml2confdata/leasetime-seconds.conf new file mode 100644 index 000000000..635896b29 --- /dev/null +++ b/tests/networkxml2confdata/leasetime-seconds.conf @@ -0,0 +1,17 @@ +##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,125 +dhcp-no-override +dhcp-range=2001:db8:ac10:fd01::1:10,2001:db8:ac10:fd01::1:ff,64,125 +dhcp-lease-max=493 +dhcp-hostsfile=/var/lib/libvirt/dnsmasq/default.hostsfile +addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts +enable-ra diff --git a/tests/networkxml2confdata/leasetime-seconds.xml b/tests/networkxml2confdata/leasetime-seconds.xml new file mode 100644 index 000000000..56b07f8ae --- /dev/null +++ b/tests/networkxml2confdata/leasetime-seconds.xml @@ -0,0 +1,18 @@ +<network> + <name>default</name> + <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid> + <forward dev='eth1' mode='nat'/> + <bridge name='virbr0' stp='on' delay='0'/> + <ip address='192.168.122.1' netmask='255.255.255.0'> + <dhcp> + <leasetime unit="seconds">125</leasetime> + <range start='192.168.122.2' end='192.168.122.254'/> + </dhcp> + </ip> + <ip family='ipv6' address='2001:db8:ac10:fd01::1' prefix='64'> + <dhcp> + <leasetime unit="seconds">125</leasetime> + <range start='2001:db8:ac10:fd01::1:10' end='2001:db8:ac10:fd01::1:ff'/> + </dhcp> + </ip> +</network> diff --git a/tests/networkxml2confdata/leasetime.conf b/tests/networkxml2confdata/leasetime.conf new file mode 100644 index 000000000..72a2f6926 --- /dev/null +++ b/tests/networkxml2confdata/leasetime.conf @@ -0,0 +1,17 @@ +##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,122 +dhcp-no-override +dhcp-range=2001:db8:ac10:fd01::1:10,2001:db8:ac10:fd01::1:ff,64,121 +dhcp-lease-max=493 +dhcp-hostsfile=/var/lib/libvirt/dnsmasq/default.hostsfile +addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts +enable-ra diff --git a/tests/networkxml2confdata/leasetime.xml b/tests/networkxml2confdata/leasetime.xml new file mode 100644 index 000000000..fdbb15fc0 --- /dev/null +++ b/tests/networkxml2confdata/leasetime.xml @@ -0,0 +1,18 @@ +<network> + <name>default</name> + <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid> + <forward dev='eth1' mode='nat'/> + <bridge name='virbr0' stp='on' delay='0'/> + <ip address='192.168.122.1' netmask='255.255.255.0'> + <dhcp> + <leasetime>122</leasetime> + <range start='192.168.122.2' end='192.168.122.254'/> + </dhcp> + </ip> + <ip family='ipv6' address='2001:db8:ac10:fd01::1' prefix='64'> + <dhcp> + <leasetime>121</leasetime> + <range start='2001:db8:ac10:fd01::1:10' end='2001:db8:ac10:fd01::1:ff'/> + </dhcp> + </ip> +</network> diff --git a/tests/networkxml2conftest.c b/tests/networkxml2conftest.c index 65a0e3218..223a56f54 100644 --- a/tests/networkxml2conftest.c +++ b/tests/networkxml2conftest.c @@ -129,6 +129,13 @@ mymain(void) DO_TEST("dhcp6-network", dhcpv6); DO_TEST("dhcp6-nat-network", dhcpv6); DO_TEST("dhcp6host-routed-network", dhcpv6); + DO_TEST("leasetime", dhcpv6); + DO_TEST("leasetime-seconds", dhcpv6); + DO_TEST("leasetime-hours", dhcpv6); + DO_TEST("leasetime-minutes", dhcpv6); + DO_TEST("leasetime-hours", dhcpv6); + DO_TEST("leasetime-days", dhcpv6); + DO_TEST("leasetime-infinite", dhcpv6); virObjectUnref(dhcpv6); virObjectUnref(full); -- 2.13.0
From: Alberto Ruiz <aruiz@gnome.org> --- src/network/bridge_driver.c | 56 ++++++++++++++++++++++++++------------------- src/util/virdnsmasq.c | 52 +++++++++++++++++++---------------------- src/util/virdnsmasq.h | 1 + 3 files changed, 57 insertions(+), 52 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index e51e469c8..1cffd4dcf 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -861,30 +861,6 @@ networkKillDaemon(pid_t pid, const char *daemonName, const char *networkName) return ret; } -/* the following does not build a file, it builds a list - * which is later saved into a file - */ - -static int -networkBuildDnsmasqDhcpHostsList(dnsmasqContext *dctx, - virNetworkIPDefPtr ipdef) -{ - size_t i; - bool ipv6 = false; - - if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET6)) - ipv6 = true; - for (i = 0; i < ipdef->nhosts; i++) { - 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) - return -1; - } - - return 0; -} - static int networkBuildDnsmasqHostsList(dnsmasqContext *dctx, virNetworkDNSDefPtr dnsdef) @@ -940,6 +916,38 @@ networkDnsmasqConfLeaseValueToString (int64_t leasetime) return result; } +/* the following does not build a file, it builds a list + * which is later saved into a file + */ + +static int +networkBuildDnsmasqDhcpHostsList(dnsmasqContext *dctx, + virNetworkIPDefPtr ipdef) +{ + int ret = -1; + size_t i; + bool ipv6 = false; + char *leasetime = networkDnsmasqConfLeaseValueToString(ipdef->leasetime); + + if (!leasetime) + goto cleanup; + + if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET6)) + ipv6 = true; + for (i = 0; i < ipdef->nhosts; i++) { + virNetworkDHCPHostDefPtr host = &(ipdef->hosts[i]); + if (VIR_SOCKET_ADDR_VALID(&host->ip)) + if (dnsmasqAddDhcpHost(dctx, host->mac, &host->ip, + host->name, host->id, leasetime, ipv6) < 0) + goto cleanup; + } + + ret = 0; +cleanup: + VIR_FREE(leasetime); + return ret; +} + int networkDnsmasqConfContents(virNetworkObjPtr network, const char *pidfile, diff --git a/src/util/virdnsmasq.c b/src/util/virdnsmasq.c index 1b78c1fad..92f834fe7 100644 --- a/src/util/virdnsmasq.c +++ b/src/util/virdnsmasq.c @@ -308,52 +308,47 @@ hostsfileAdd(dnsmasqHostsfile *hostsfile, virSocketAddr *ip, const char *name, const char *id, + const char *leasetime, bool ipv6) { + int ret = -1; char *ipstr = NULL; + virBuffer hostbuf = VIR_BUFFER_INITIALIZER; + if (VIR_REALLOC_N(hostsfile->hosts, hostsfile->nhosts + 1) < 0) goto error; if (!(ipstr = virSocketAddrFormat(ip))) - return -1; + goto error; /* the first test determines if it is a dhcpv6 host */ if (ipv6) { - if (name && id) { - if (virAsprintf(&hostsfile->hosts[hostsfile->nhosts].host, - "id:%s,%s,[%s]", id, name, ipstr) < 0) - goto error; - } else if (name && !id) { - if (virAsprintf(&hostsfile->hosts[hostsfile->nhosts].host, - "%s,[%s]", name, ipstr) < 0) - goto error; - } else if (!name && id) { - if (virAsprintf(&hostsfile->hosts[hostsfile->nhosts].host, - "id:%s,[%s]", id, ipstr) < 0) - goto error; - } + if (name && id) + virBufferAsprintf(&hostbuf, "id:%s,%s,[%s]", id, name, ipstr); + else if (name && !id) + virBufferAsprintf(&hostbuf, "%s,[%s]", name, ipstr); + else if (!name && id) + virBufferAsprintf(&hostbuf, "id:%s,[%s]", id, ipstr); } else if (name && mac) { - if (virAsprintf(&hostsfile->hosts[hostsfile->nhosts].host, "%s,%s,%s", - mac, ipstr, name) < 0) - goto error; + virBufferAsprintf(&hostbuf, "%s,%s,%s", mac, ipstr, name); } else if (name && !mac) { - if (virAsprintf(&hostsfile->hosts[hostsfile->nhosts].host, "%s,%s", - name, ipstr) < 0) - goto error; + virBufferAsprintf(&hostbuf, "%s,%s", name, ipstr); } else { - if (virAsprintf(&hostsfile->hosts[hostsfile->nhosts].host, "%s,%s", - mac, ipstr) < 0) - goto error; + virBufferAsprintf(&hostbuf, "%s,%s", mac, ipstr); } - VIR_FREE(ipstr); - hostsfile->nhosts++; + /* The leasetime string already includes comma if there's any value at all */ + virBufferAsprintf(&hostbuf, "%s", leasetime); - return 0; + if (!(hostsfile->hosts[hostsfile->nhosts].host = virBufferContentAndReset (&hostbuf))) + goto error; + hostsfile->nhosts++; + ret = 0; error: + virBufferFreeAndReset(&hostbuf); VIR_FREE(ipstr); - return -1; + return ret; } static dnsmasqHostsfile * @@ -524,9 +519,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 f47bea3ab..c3ea271d4 100644 --- a/src/util/virdnsmasq.h +++ b/src/util/virdnsmasq.h @@ -88,6 +88,7 @@ int dnsmasqAddDhcpHost(dnsmasqContext *ctx, virSocketAddr *ip, const char *name, const char *id, + const char *leastime, bool ipv6); int dnsmasqAddHost(dnsmasqContext *ctx, virSocketAddr *ip, -- 2.13.0
On 06/20/2017 02:00 PM, aruiz@gnome.org wrote:
From: Alberto Ruiz <aruiz@gnome.org>
--- src/network/bridge_driver.c | 56 ++++++++++++++++++++++++++------------------- src/util/virdnsmasq.c | 52 +++++++++++++++++++---------------------- src/util/virdnsmasq.h | 1 + 3 files changed, 57 insertions(+), 52 deletions(-)
As far as I can see, this doesn't set a different lease time for each static host entry (which is what the title implies), but just sets the single specified leasetime for *all* static host entries. Aside from that, I'm not sure what is the value of setting a leastime on a static host entry. An explanation of that in the commit log would be helpful in determining whether or not there's even a point to doing this. Also, I forgot to say it wrt to the previous patch, but you need to add something to docs/formatnetwork.html.in to document the new config knob.
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index e51e469c8..1cffd4dcf 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -861,30 +861,6 @@ networkKillDaemon(pid_t pid, const char *daemonName, const char *networkName) return ret; }
-/* the following does not build a file, it builds a list - * which is later saved into a file - */ - -static int -networkBuildDnsmasqDhcpHostsList(dnsmasqContext *dctx, - virNetworkIPDefPtr ipdef) -{ - size_t i; - bool ipv6 = false; - - if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET6)) - ipv6 = true; - for (i = 0; i < ipdef->nhosts; i++) { - 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) - return -1; - } - - return 0; -} - static int networkBuildDnsmasqHostsList(dnsmasqContext *dctx, virNetworkDNSDefPtr dnsdef) @@ -940,6 +916,38 @@ networkDnsmasqConfLeaseValueToString (int64_t leasetime) return result; }
+/* the following does not build a file, it builds a list + * which is later saved into a file + */ + +static int +networkBuildDnsmasqDhcpHostsList(dnsmasqContext *dctx, + virNetworkIPDefPtr ipdef) +{ + int ret = -1; + size_t i; + bool ipv6 = false; + char *leasetime = networkDnsmasqConfLeaseValueToString(ipdef->leasetime); + + if (!leasetime) + goto cleanup; + + if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET6)) + ipv6 = true; + for (i = 0; i < ipdef->nhosts; i++) { + virNetworkDHCPHostDefPtr host = &(ipdef->hosts[i]); + if (VIR_SOCKET_ADDR_VALID(&host->ip)) + if (dnsmasqAddDhcpHost(dctx, host->mac, &host->ip, + host->name, host->id, leasetime, ipv6) < 0) + goto cleanup; + } + + ret = 0; +cleanup: + VIR_FREE(leasetime); + return ret; +} + int networkDnsmasqConfContents(virNetworkObjPtr network, const char *pidfile, diff --git a/src/util/virdnsmasq.c b/src/util/virdnsmasq.c index 1b78c1fad..92f834fe7 100644 --- a/src/util/virdnsmasq.c +++ b/src/util/virdnsmasq.c @@ -308,52 +308,47 @@ hostsfileAdd(dnsmasqHostsfile *hostsfile, virSocketAddr *ip, const char *name, const char *id, + const char *leasetime, bool ipv6) { + int ret = -1; char *ipstr = NULL; + virBuffer hostbuf = VIR_BUFFER_INITIALIZER; + if (VIR_REALLOC_N(hostsfile->hosts, hostsfile->nhosts + 1) < 0) goto error;
if (!(ipstr = virSocketAddrFormat(ip))) - return -1; + goto error;
/* the first test determines if it is a dhcpv6 host */ if (ipv6) { - if (name && id) { - if (virAsprintf(&hostsfile->hosts[hostsfile->nhosts].host, - "id:%s,%s,[%s]", id, name, ipstr) < 0) - goto error; - } else if (name && !id) { - if (virAsprintf(&hostsfile->hosts[hostsfile->nhosts].host, - "%s,[%s]", name, ipstr) < 0) - goto error; - } else if (!name && id) { - if (virAsprintf(&hostsfile->hosts[hostsfile->nhosts].host, - "id:%s,[%s]", id, ipstr) < 0) - goto error; - } + if (name && id) + virBufferAsprintf(&hostbuf, "id:%s,%s,[%s]", id, name, ipstr); + else if (name && !id) + virBufferAsprintf(&hostbuf, "%s,[%s]", name, ipstr); + else if (!name && id) + virBufferAsprintf(&hostbuf, "id:%s,[%s]", id, ipstr); } else if (name && mac) { - if (virAsprintf(&hostsfile->hosts[hostsfile->nhosts].host, "%s,%s,%s", - mac, ipstr, name) < 0) - goto error; + virBufferAsprintf(&hostbuf, "%s,%s,%s", mac, ipstr, name); } else if (name && !mac) { - if (virAsprintf(&hostsfile->hosts[hostsfile->nhosts].host, "%s,%s", - name, ipstr) < 0) - goto error; + virBufferAsprintf(&hostbuf, "%s,%s", name, ipstr); } else { - if (virAsprintf(&hostsfile->hosts[hostsfile->nhosts].host, "%s,%s", - mac, ipstr) < 0) - goto error; + virBufferAsprintf(&hostbuf, "%s,%s", mac, ipstr); } - VIR_FREE(ipstr);
- hostsfile->nhosts++; + /* The leasetime string already includes comma if there's any value at all */ + virBufferAsprintf(&hostbuf, "%s", leasetime);
- return 0; + if (!(hostsfile->hosts[hostsfile->nhosts].host = virBufferContentAndReset (&hostbuf))) + goto error;
+ hostsfile->nhosts++; + ret = 0; error: + virBufferFreeAndReset(&hostbuf); VIR_FREE(ipstr); - return -1; + return ret; }
static dnsmasqHostsfile * @@ -524,9 +519,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 f47bea3ab..c3ea271d4 100644 --- a/src/util/virdnsmasq.h +++ b/src/util/virdnsmasq.h @@ -88,6 +88,7 @@ int dnsmasqAddDhcpHost(dnsmasqContext *ctx, virSocketAddr *ip, const char *name, const char *id, + const char *leastime, bool ipv6); int dnsmasqAddHost(dnsmasqContext *ctx, virSocketAddr *ip,
From: Alberto Ruiz <aruiz@gnome.org> --- src/libvirt_private.syms | 1 + src/network/bridge_driver.c | 12 ++++- src/network/bridge_driver.h | 1 + src/util/virdnsmasq.c | 54 +++++++++++++++++----- src/util/virdnsmasq.h | 1 + .../dhcp6-nat-network.hostsfile | 7 +++ tests/networkxml2confdata/dhcp6-network.hostsfile | 5 ++ .../dhcp6host-routed-network.hostsfile | 7 +++ .../nat-network-dns-srv-record-minimal.hostsfile | 2 + .../nat-network-dns-srv-record.hostsfile | 2 + .../nat-network-dns-txt-record.hostsfile | 2 + .../nat-network-name-with-quotes.hostsfile | 2 + tests/networkxml2confdata/nat-network.hostsfile | 2 + tests/networkxml2conftest.c | 39 ++++++++++++---- 14 files changed, 116 insertions(+), 21 deletions(-) create mode 100644 tests/networkxml2confdata/dhcp6-nat-network.hostsfile create mode 100644 tests/networkxml2confdata/dhcp6-network.hostsfile create mode 100644 tests/networkxml2confdata/dhcp6host-routed-network.hostsfile create mode 100644 tests/networkxml2confdata/nat-network-dns-srv-record-minimal.hostsfile create mode 100644 tests/networkxml2confdata/nat-network-dns-srv-record.hostsfile create mode 100644 tests/networkxml2confdata/nat-network-dns-txt-record.hostsfile create mode 100644 tests/networkxml2confdata/nat-network-name-with-quotes.hostsfile create mode 100644 tests/networkxml2confdata/nat-network.hostsfile diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 923afd187..c93385958 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1497,6 +1497,7 @@ dnsmasqCapsRefresh; dnsmasqContextFree; dnsmasqContextNew; dnsmasqDelete; +dnsmasqDhcpHostsToString; dnsmasqReload; dnsmasqSave; diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 1cffd4dcf..1d2ada208 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -952,6 +952,7 @@ int networkDnsmasqConfContents(virNetworkObjPtr network, const char *pidfile, char **configstr, + char **hostsfilestr, dnsmasqContext *dctx, dnsmasqCapsPtr caps ATTRIBUTE_UNUSED) { @@ -1311,6 +1312,15 @@ networkDnsmasqConfContents(virNetworkObjPtr network, if (networkBuildDnsmasqDhcpHostsList(dctx, ipdef) < 0) goto cleanup; + /* Return the contents of the hostsfile if requested */ + if (hostsfilestr) { + *hostsfilestr = dnsmasqDhcpHostsToString (dctx->hostsfile->hosts, + dctx->hostsfile->nhosts); + + if (!hostsfilestr) + goto cleanup; + } + /* Note: the following is IPv4 only */ if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET)) { if (ipdef->nranges || ipdef->nhosts) @@ -1410,7 +1420,7 @@ networkBuildDhcpDaemonCommandLine(virNetworkDriverStatePtr driver, network->dnsmasqPid = -1; - if (networkDnsmasqConfContents(network, pidfile, &configstr, + if (networkDnsmasqConfContents(network, pidfile, &configstr, NULL, dctx, dnsmasq_caps) < 0) goto cleanup; if (!configstr) diff --git a/src/network/bridge_driver.h b/src/network/bridge_driver.h index ff7f921ed..c653c507f 100644 --- a/src/network/bridge_driver.h +++ b/src/network/bridge_driver.h @@ -53,6 +53,7 @@ int networkGetActualType(virDomainNetDefPtr iface) int networkDnsmasqConfContents(virNetworkObjPtr network, const char *pidfile, char **configstr, + char **hostsfilestr, dnsmasqContext *dctx, dnsmasqCapsPtr caps); diff --git a/src/util/virdnsmasq.c b/src/util/virdnsmasq.c index 92f834fe7..94c9a3bb1 100644 --- a/src/util/virdnsmasq.c +++ b/src/util/virdnsmasq.c @@ -386,10 +386,9 @@ hostsfileWrite(const char *path, dnsmasqDhcpHost *hosts, unsigned int nhosts) { - char *tmp; + char *tmp, *content = NULL; FILE *f; bool istmp = true; - size_t i; int rc = 0; /* even if there are 0 hosts, create a 0 length file, to allow @@ -407,17 +406,21 @@ hostsfileWrite(const char *path, } } - for (i = 0; i < nhosts; i++) { - if (fputs(hosts[i].host, f) == EOF || fputc('\n', f) == EOF) { - rc = -errno; - VIR_FORCE_FCLOSE(f); + if (!(content = dnsmasqDhcpHostsToString(hosts, nhosts))) { + rc = -ENOMEM; + goto cleanup; + } - if (istmp) - unlink(tmp); + if (fputs(content, f) == EOF) { + rc = -errno; + VIR_FORCE_FCLOSE(f); + + if (istmp) + unlink(tmp); + + goto cleanup; + } - goto cleanup; - } - } if (VIR_FCLOSE(f) == EOF) { rc = -errno; @@ -431,6 +434,7 @@ hostsfileWrite(const char *path, } cleanup: + VIR_FREE(content); VIR_FREE(tmp); return rc; @@ -888,3 +892,31 @@ dnsmasqCapsGet(dnsmasqCapsPtr caps, dnsmasqCapsFlags flag) return caps && virBitmapIsBitSet(caps->flags, flag); } + +/** dnsmasqDhcpHostsToString: + * + * Turns a vector of dnsmasqDhcpHost into the string that is ought to be + * stored in the hostsfile, this functionality is split to make hostsfiles + * testable. Returs NULL if nhosts is 0. + */ +char * +dnsmasqDhcpHostsToString (dnsmasqDhcpHost *hosts, + unsigned int nhosts) +{ + int i; + char *result = NULL; + virBuffer hostsfilebuf = VIR_BUFFER_INITIALIZER; + + if (nhosts == 0) + goto cleanup; + + for (i = 0; i < nhosts; i++) { + virBufferAsprintf(&hostsfilebuf, "%s\n", hosts[i].host); + } + + result = virBufferContentAndReset(&hostsfilebuf); + +cleanup: + virBufferFreeAndReset(&hostsfilebuf); + return result; +} diff --git a/src/util/virdnsmasq.h b/src/util/virdnsmasq.h index c3ea271d4..1795bc83b 100644 --- a/src/util/virdnsmasq.h +++ b/src/util/virdnsmasq.h @@ -106,6 +106,7 @@ int dnsmasqCapsRefresh(dnsmasqCapsPtr *caps, const char *binaryPath); bool dnsmasqCapsGet(dnsmasqCapsPtr caps, dnsmasqCapsFlags flag); const char *dnsmasqCapsGetBinaryPath(dnsmasqCapsPtr caps); unsigned long dnsmasqCapsGetVersion(dnsmasqCapsPtr caps); +char *dnsmasqDhcpHostsToString(dnsmasqDhcpHost *hosts, unsigned int nhosts); # define DNSMASQ_DHCPv6_MAJOR_REQD 2 # define DNSMASQ_DHCPv6_MINOR_REQD 64 diff --git a/tests/networkxml2confdata/dhcp6-nat-network.hostsfile b/tests/networkxml2confdata/dhcp6-nat-network.hostsfile new file mode 100644 index 000000000..de659b98c --- /dev/null +++ b/tests/networkxml2confdata/dhcp6-nat-network.hostsfile @@ -0,0 +1,7 @@ +00:16:3e:77:e2:ed,192.168.122.10,a.example.com +00:16:3e:3e:a9:1a,192.168.122.11,b.example.com +id:0:4:7e:7d:f0:7d:a8:bc:c5:d2:13:32:11:ed:16:ea:84:63,[2001:db8:ac10:fd01::1:20] +paul,[2001:db8:ac10:fd01::1:21] +id:0:3:0:1:0:16:3e:11:22:33,peter.xyz,[2001:db8:ac10:fd01::1:22] +id:0:3:0:1:0:16:3e:44:55:33,[2001:db8:ac10:fd01::1:23] +id:0:1:0:1:18:aa:62:fe:0:16:3e:44:55:66,badbob,[2001:db8:ac10:fd01::1:24] diff --git a/tests/networkxml2confdata/dhcp6-network.hostsfile b/tests/networkxml2confdata/dhcp6-network.hostsfile new file mode 100644 index 000000000..9dfb172ce --- /dev/null +++ b/tests/networkxml2confdata/dhcp6-network.hostsfile @@ -0,0 +1,5 @@ +id:0:4:7e:7d:f0:7d:a8:bc:c5:d2:13:32:11:ed:16:ea:84:63,[2001:db8:ac10:fd01::1:20] +paul,[2001:db8:ac10:fd01::1:21] +id:0:3:0:1:0:16:3e:11:22:33,peter.xyz,[2001:db8:ac10:fd01::1:22] +id:0:3:0:1:0:16:3e:44:55:33,[2001:db8:ac10:fd01::1:23] +id:0:1:0:1:18:aa:62:fe:0:16:3e:44:55:66,badbob,[2001:db8:ac10:fd01::1:24] diff --git a/tests/networkxml2confdata/dhcp6host-routed-network.hostsfile b/tests/networkxml2confdata/dhcp6host-routed-network.hostsfile new file mode 100644 index 000000000..de659b98c --- /dev/null +++ b/tests/networkxml2confdata/dhcp6host-routed-network.hostsfile @@ -0,0 +1,7 @@ +00:16:3e:77:e2:ed,192.168.122.10,a.example.com +00:16:3e:3e:a9:1a,192.168.122.11,b.example.com +id:0:4:7e:7d:f0:7d:a8:bc:c5:d2:13:32:11:ed:16:ea:84:63,[2001:db8:ac10:fd01::1:20] +paul,[2001:db8:ac10:fd01::1:21] +id:0:3:0:1:0:16:3e:11:22:33,peter.xyz,[2001:db8:ac10:fd01::1:22] +id:0:3:0:1:0:16:3e:44:55:33,[2001:db8:ac10:fd01::1:23] +id:0:1:0:1:18:aa:62:fe:0:16:3e:44:55:66,badbob,[2001:db8:ac10:fd01::1:24] diff --git a/tests/networkxml2confdata/nat-network-dns-srv-record-minimal.hostsfile b/tests/networkxml2confdata/nat-network-dns-srv-record-minimal.hostsfile new file mode 100644 index 000000000..deb3f00ac --- /dev/null +++ b/tests/networkxml2confdata/nat-network-dns-srv-record-minimal.hostsfile @@ -0,0 +1,2 @@ +00:16:3e:77:e2:ed,192.168.122.10,a.example.com +00:16:3e:3e:a9:1a,192.168.122.11,b.example.com diff --git a/tests/networkxml2confdata/nat-network-dns-srv-record.hostsfile b/tests/networkxml2confdata/nat-network-dns-srv-record.hostsfile new file mode 100644 index 000000000..deb3f00ac --- /dev/null +++ b/tests/networkxml2confdata/nat-network-dns-srv-record.hostsfile @@ -0,0 +1,2 @@ +00:16:3e:77:e2:ed,192.168.122.10,a.example.com +00:16:3e:3e:a9:1a,192.168.122.11,b.example.com diff --git a/tests/networkxml2confdata/nat-network-dns-txt-record.hostsfile b/tests/networkxml2confdata/nat-network-dns-txt-record.hostsfile new file mode 100644 index 000000000..deb3f00ac --- /dev/null +++ b/tests/networkxml2confdata/nat-network-dns-txt-record.hostsfile @@ -0,0 +1,2 @@ +00:16:3e:77:e2:ed,192.168.122.10,a.example.com +00:16:3e:3e:a9:1a,192.168.122.11,b.example.com diff --git a/tests/networkxml2confdata/nat-network-name-with-quotes.hostsfile b/tests/networkxml2confdata/nat-network-name-with-quotes.hostsfile new file mode 100644 index 000000000..deb3f00ac --- /dev/null +++ b/tests/networkxml2confdata/nat-network-name-with-quotes.hostsfile @@ -0,0 +1,2 @@ +00:16:3e:77:e2:ed,192.168.122.10,a.example.com +00:16:3e:3e:a9:1a,192.168.122.11,b.example.com diff --git a/tests/networkxml2confdata/nat-network.hostsfile b/tests/networkxml2confdata/nat-network.hostsfile new file mode 100644 index 000000000..deb3f00ac --- /dev/null +++ b/tests/networkxml2confdata/nat-network.hostsfile @@ -0,0 +1,2 @@ +00:16:3e:77:e2:ed,192.168.122.10,a.example.com +00:16:3e:3e:a9:1a,192.168.122.11,b.example.com diff --git a/tests/networkxml2conftest.c b/tests/networkxml2conftest.c index 223a56f54..6b8324aa2 100644 --- a/tests/networkxml2conftest.c +++ b/tests/networkxml2conftest.c @@ -19,9 +19,13 @@ #define VIR_FROM_THIS VIR_FROM_NONE static int -testCompareXMLToConfFiles(const char *inxml, const char *outconf, dnsmasqCapsPtr caps) +testCompareXMLToConfFiles(const char *inxml, + const char *outconf, + const char *outhostsfile, + dnsmasqCapsPtr caps) { - char *actual = NULL; + char *actualconf = NULL; + char *actualhosts = NULL; int ret = -1; virNetworkDefPtr dev = NULL; virNetworkObjPtr obj = NULL; @@ -41,17 +45,30 @@ testCompareXMLToConfFiles(const char *inxml, const char *outconf, dnsmasqCapsPtr if (dctx == NULL) goto fail; - if (networkDnsmasqConfContents(obj, pidfile, &actual, + if (networkDnsmasqConfContents(obj, pidfile, &actualconf, &actualhosts, dctx, caps) < 0) goto fail; - if (virTestCompareToFile(actual, outconf) < 0) + if (virTestCompareToFile(actualconf, outconf) < 0) goto fail; + if (virFileExists(outhostsfile)) { + if (!actualhosts) { + fprintf(stderr, "%s: hostsfile exists but the configuration did not specify any host", outhostsfile); + goto fail; + } else if (virTestCompareToFile(actualhosts, outhostsfile) < 0) { + goto fail; + } + } else if (actualhosts) { + fprintf(stderr, "%s: file does not exist but actual data was expected", outhostsfile); + goto fail; + } + ret = 0; fail: - VIR_FREE(actual); + VIR_FREE(actualconf); + VIR_FREE(actualhosts); VIR_FREE(pidfile); virCommandFree(cmd); virObjectUnref(obj); @@ -70,20 +87,24 @@ testCompareXMLToConfHelper(const void *data) int result = -1; const testInfo *info = data; char *inxml = NULL; - char *outxml = NULL; + char *outconf = NULL; + char *outhostsfile = NULL; if (virAsprintf(&inxml, "%s/networkxml2confdata/%s.xml", abs_srcdir, info->name) < 0 || - virAsprintf(&outxml, "%s/networkxml2confdata/%s.conf", + virAsprintf(&outconf, "%s/networkxml2confdata/%s.conf", + abs_srcdir, info->name) < 0 || + virAsprintf(&outhostsfile, "%s/networkxml2confdata/%s.hostsfile", abs_srcdir, info->name) < 0) { goto cleanup; } - result = testCompareXMLToConfFiles(inxml, outxml, info->caps); + result = testCompareXMLToConfFiles(inxml, outconf, outhostsfile, info->caps); cleanup: VIR_FREE(inxml); - VIR_FREE(outxml); + VIR_FREE(outconf); + VIR_FREE(outhostsfile); return result; } -- 2.13.0
On Tue, Jun 20, 2017 at 19:00:43 +0100, aruiz@gnome.org wrote:
From: Alberto Ruiz <aruiz@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
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@redhat.com>:
On Tue, Jun 20, 2017 at 19:00:43 +0100, aruiz@gnome.org wrote:
From: Alberto Ruiz <aruiz@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
On 06/21/2017 03:27 AM, Peter Krempa wrote:
On Tue, Jun 20, 2017 at 19:00:43 +0100, aruiz@gnome.org wrote:
From: Alberto Ruiz <aruiz@gnome.org>
Missing commit message.
And when composing the commit message, it's useful to include links to the associated BZ. https://bugzilla.redhat.com/show_bug.cgi?id=913446 Also, I recall there being quite a lot of discussion in email (and possibly IRC) about the fact that people *think* they want a configurable lease time because they think that will eliminate cases of a DHCP lease being lost while a domain is paused. It was pointed out that lengthening the lease will *not* eliminate that problem (it just makes it happen less often). As an alternate (and better) solution to the problem of lost leases, we then added the "dhcp-authoritative" option to dnsmasq (commit 4ac20b3ae4), which allows clients to re-acquire the same IP as they had for an expired lease (as long as it hasn't been acquired by someone else in the meantime, which is apparently unlikely unless all the other addresses in the pool are already assigned). I'm not saying this to discourage the idea of making leasetime configurable (I think we'd already agreed that it was reasonable to do so, but there were two competing patches posted, and neither of them was really push-ready), but just to make sure that nobody is disappointed if the results don't lead to the behavior they're hoping for.
--- 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>
Maybe call this "timeUnit" in case some other attribute in the future needs it?
+ + <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,
While it's true that this function "gets" the lease time from the xmlDoc, this is part of the process of parsing the XML, so it should be named virNetworkDHCPLeaseTimeParseXML(). (it's never been clear to me when/where the "Def" part of the name should be; I've always seen it as just three extra letters that add nothing; someone else may have a different opinion).
+ 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.
In particular - if one clause of an if-else is in braces, then *all* of the clauses of that if-else much be in braces.
+ + 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),
We don't use gnulib's "PRIxxx" macros anywhere else in libvirt. Better to "go with the flow" and just use "%d" instead for consistency (or make a case for changing them elsewhere :-)
+ 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)
I *think* this is what you currently have: -1 : infinite 0/unspecified : unspecified (use default)
0 : actual lease time.
How about this instead? In the xml: 0 : infinite unspecified : unspecified
0 : actual lease time
In the internal object: have a separate "bool leasetime_specified" to differentiate between "0" and unspecified (there are a few other examples of this - search for "_specified" in src/conf/*.c)
+ 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*/
I'm not sure how useful this would be for libvirt, since it's apparently used when renumbering a network. I think for now it can be ignored.
+ /* TODO: Discuss what is the default value that we want as dnsmasq's is 1 hour */
Whatever the value has been for all these years, it needs to remain the same. Behavior should be unchanged if the XML is unchanged.
+ /* TODO: Discuss what to do if value exceeds maximum, use default value for now */
If the specified value is too large, we need to log an error and fail. If we were to just ignore invalid values now, it makes it more difficult to change it to an error later (you would then have to validate it only when a definition is newly defined or edited, but not when all the configs are read at libvirtd startup).
+ 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
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Hello Laine, 2017-06-21 17:30 GMT+01:00 Laine Stump <laine@laine.org>:
On 06/21/2017 03:27 AM, Peter Krempa wrote:
On Tue, Jun 20, 2017 at 19:00:43 +0100, aruiz@gnome.org wrote:
From: Alberto Ruiz <aruiz@gnome.org>
Missing commit message.
And when composing the commit message, it's useful to include links to the associated BZ.
https://bugzilla.redhat.com/show_bug.cgi?id=913446
Also, I recall there being quite a lot of discussion in email (and possibly IRC) about the fact that people *think* they want a configurable lease time because they think that will eliminate cases of a DHCP lease being lost while a domain is paused. It was pointed out that lengthening the lease will *not* eliminate that problem (it just makes it happen less often).
As an alternate (and better) solution to the problem of lost leases, we then added the "dhcp-authoritative" option to dnsmasq (commit 4ac20b3ae4), which allows clients to re-acquire the same IP as they had for an expired lease (as long as it hasn't been acquired by someone else in the meantime, which is apparently unlikely unless all the other addresses in the pool are already assigned).
I'm not saying this to discourage the idea of making leasetime configurable (I think we'd already agreed that it was reasonable to do so, but there were two competing patches posted, and neither of them was really push-ready), but just to make sure that nobody is disappointed if the results don't lead to the behavior they're hoping for.
My main motivation _was_ the loss of IP addresses after reboot on GNOME Boxes. However, given that I've written the patches already and some people might find this useful, I'm okay with going ahead and get them ready for approval.
--- 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>
Maybe call this "timeUnit" in case some other attribute in the future needs it?
sounds good to me. noted
+ + <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.
I'm not sure I understand what specifically violates the XML style, range/ipAddr below seems to be written following the same convention?
+ </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,
While it's true that this function "gets" the lease time from the xmlDoc, this is part of the process of parsing the XML, so it should be named virNetworkDHCPLeaseTimeParseXML(). (it's never been clear to me when/where the "Def" part of the name should be; I've always seen it as just three extra letters that add nothing; someone else may have a different opinion).
Okay. Will rename.
+ 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.
In particular - if one clause of an if-else is in braces, then *all* of the clauses of that if-else much be in braces.
Noted. Will fix.
+ + 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),
We don't use gnulib's "PRIxxx" macros anywhere else in libvirt. Better to "go with the flow" and just use "%d" instead for consistency (or make a case for changing them elsewhere :-)
+ 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)
I *think* this is what you currently have:
-1 : infinite 0/unspecified : unspecified (use default)
0 : actual lease time.
How about this instead?
In the xml:
0 : infinite unspecified : unspecified
0 : actual lease time
In the internal object: have a separate "bool leasetime_specified" to differentiate between "0" and unspecified (there are a few other examples of this - search for "_specified" in src/conf/*.c)
Works for me.
+ 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*/
I'm not sure how useful this would be for libvirt, since it's apparently used when renumbering a network. I think for now it can be ignored.
okay
+ /* TODO: Discuss what is the default value that we want as dnsmasq's is 1 hour */
Whatever the value has been for all these years, it needs to remain the same. Behavior should be unchanged if the XML is unchanged.
okay, makes sense
+ /* TODO: Discuss what to do if value exceeds maximum, use default value for now */
If the specified value is too large, we need to log an error and fail. If we were to just ignore invalid values now, it makes it more difficult to change it to an error later (you would then have to validate it only when a definition is newly defined or edited, but not when all the configs are read at libvirtd startup).
noted
+ 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
Yep, my bad. I've already rebased to master in my branch but I want to address the feedback given before I submit the updated patches. By the way, how am I supposed to submit the updated patchset? I'm not used to review code by email. Cheers!
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- Cheers, Alberto Ruiz
On 06/22/2017 12:21 PM, Alberto Ruiz wrote:
Hello Laine,
2017-06-21 17:30 GMT+01:00 Laine Stump <laine@laine.org <mailto:laine@laine.org>>:
On 06/21/2017 03:27 AM, Peter Krempa wrote: > On Tue, Jun 20, 2017 at 19:00:43 +0100, aruiz@gnome.org <mailto:aruiz@gnome.org> wrote: >> From: Alberto Ruiz <aruiz@gnome.org <mailto:aruiz@gnome.org>> > > Missing commit message.
And when composing the commit message, it's useful to include links to the associated BZ.
https://bugzilla.redhat.com/show_bug.cgi?id=913446 <https://bugzilla.redhat.com/show_bug.cgi?id=913446>
Also, I recall there being quite a lot of discussion in email (and possibly IRC) about the fact that people *think* they want a configurable lease time because they think that will eliminate cases of a DHCP lease being lost while a domain is paused. It was pointed out that lengthening the lease will *not* eliminate that problem (it just makes it happen less often).
As an alternate (and better) solution to the problem of lost leases, we then added the "dhcp-authoritative" option to dnsmasq (commit 4ac20b3ae4), which allows clients to re-acquire the same IP as they had for an expired lease (as long as it hasn't been acquired by someone else in the meantime, which is apparently unlikely unless all the other addresses in the pool are already assigned).
I'm not saying this to discourage the idea of making leasetime configurable (I think we'd already agreed that it was reasonable to do so, but there were two competing patches posted, and neither of them was really push-ready), but just to make sure that nobody is disappointed if the results don't lead to the behavior they're hoping for.
My main motivation _was_ the loss of IP addresses after reboot on GNOME Boxes.
However, given that I've written the patches already and some people might find this useful, I'm okay with going ahead and get them ready for approval.
> >> >> --- >> 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>
Maybe call this "timeUnit" in case some other attribute in the future needs it?
sounds good to me. noted
>> + >> + <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.
I'm not sure I understand what specifically violates the XML style, range/ipAddr below seems to be written following the same convention?
Yeah, I looked and there are quite a few examples of this in some of the files, many going back a very long time (e.g. some that still show Jim Meyering on the git blame, and even one or two DV's). So while it's not very common, there is prior art. (Unless Peter is talking about something other than the placement of multiple elements on a single line). I don't have any opinion one way or the other on this, but if we're going to mandate "no multiple elements on a single line" as part of our official coding style guidelines, then we should remove all existing examples of it first.
>> + </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,
While it's true that this function "gets" the lease time from the xmlDoc, this is part of the process of parsing the XML, so it should be named virNetworkDHCPLeaseTimeParseXML(). (it's never been clear to me when/where the "Def" part of the name should be; I've always seen it as just three extra letters that add nothing; someone else may have a different opinion).
Okay. Will rename.
>> + 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.
In particular - if one clause of an if-else is in braces, then *all* of the clauses of that if-else much be in braces.
Noted. Will fix.
> >> + >> + 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),
We don't use gnulib's "PRIxxx" macros anywhere else in libvirt. Better to "go with the flow" and just use "%d" instead for consistency (or make a case for changing them elsewhere :-)
>> + 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)
I *think* this is what you currently have:
-1 : infinite 0/unspecified : unspecified (use default) > 0 : actual lease time.
How about this instead?
In the xml:
0 : infinite unspecified : unspecified > 0 : actual lease time
In the internal object: have a separate "bool leasetime_specified" to differentiate between "0" and unspecified (there are a few other examples of this - search for "_specified" in src/conf/*.c)
Works for me.
>> + 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*/
I'm not sure how useful this would be for libvirt, since it's apparently used when renumbering a network. I think for now it can be ignored.
okay
>> + /* TODO: Discuss what is the default value that we want as dnsmasq's is 1 hour */
Whatever the value has been for all these years, it needs to remain the same. Behavior should be unchanged if the XML is unchanged.
okay, makes sense
>> + /* TODO: Discuss what to do if value exceeds maximum, use default value for now */
If the specified value is too large, we need to log an error and fail. If we were to just ignore invalid values now, it makes it more difficult to change it to an error later (you would then have to validate it only when a definition is newly defined or edited, but not when all the configs are read at libvirtd startup).
noted
>> + 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
Yep, my bad. I've already rebased to master in my branch but I want to address the feedback given before I submit the updated patches.
By the way, how am I supposed to submit the updated patchset? I'm not used to review code by email.
I usually use "git send-email -v2 --cover-letter --annotate -3" -v2 : adds "v2" after "PATCH" in the subject --cover-letter : creates a "PATCH v2 0/3" email where you can describe the purpose of the series as a whole (won't be added to git) --annotate : load all of the patches (and the cover letter) into your favorite text editor to allow adding in comments about what was changed in V2 of each patch (and compose and add a proper subject to the cover letter). The notes about what was changed between versions should be placed after the "---" line so they won't be included in the commit log that's eventually pushed. -3 : the number of commits to send from the tip of your branch You don't need to make them a reply to the V1 email - just leave them as their own thread. If you look in the list archives for patches with "PATCH v2 0/x" you'll see some good examples.
2017-06-22 21:47 GMT+01:00 Laine Stump <laine@laine.org>:
On 06/22/2017 12:21 PM, Alberto Ruiz wrote:
Hello Laine,
2017-06-21 17:30 GMT+01:00 Laine Stump <laine@laine.org <mailto:laine@laine.org>>:
On 06/21/2017 03:27 AM, Peter Krempa wrote: > On Tue, Jun 20, 2017 at 19:00:43 +0100, aruiz@gnome.org <mailto:aruiz@gnome.org> wrote: >> From: Alberto Ruiz <aruiz@gnome.org <mailto:aruiz@gnome.org>> > > Missing commit message.
And when composing the commit message, it's useful to include links to the associated BZ.
https://bugzilla.redhat.com/show_bug.cgi?id=913446 <https://bugzilla.redhat.com/show_bug.cgi?id=913446>
Also, I recall there being quite a lot of discussion in email (and possibly IRC) about the fact that people *think* they want a configurable lease time because they think that will eliminate cases of a DHCP lease being lost while a domain is paused. It was pointed out that lengthening the lease will *not* eliminate that problem (it just makes it happen less often).
As an alternate (and better) solution to the problem of lost leases, we then added the "dhcp-authoritative" option to dnsmasq (commit 4ac20b3ae4), which allows clients to re-acquire the same IP as they had for an expired lease (as long as it hasn't been acquired by someone else in the meantime, which is apparently unlikely unless all the other addresses in the pool are already assigned).
I'm not saying this to discourage the idea of making leasetime configurable (I think we'd already agreed that it was reasonable to do so, but there were two competing patches posted, and neither of them was really push-ready), but just to make sure that nobody is disappointed if the results don't lead to the behavior they're hoping for.
My main motivation _was_ the loss of IP addresses after reboot on GNOME Boxes.
However, given that I've written the patches already and some people might find this useful, I'm okay with going ahead and get them ready for approval.
> >> >> --- >> 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>
Maybe call this "timeUnit" in case some other attribute in the future needs it?
sounds good to me. noted
>> + >> + <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.
I'm not sure I understand what specifically violates the XML style, range/ipAddr below seems to be written following the same convention?
Yeah, I looked and there are quite a few examples of this in some of the files, many going back a very long time (e.g. some that still show Jim Meyering on the git blame, and even one or two DV's). So while it's not very common, there is prior art. (Unless Peter is talking about something other than the placement of multiple elements on a single line). I don't have any opinion one way or the other on this, but if we're going to mandate "no multiple elements on a single line" as part of our official coding style guidelines, then we should remove all existing examples of it first.
>> + </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,
While it's true that this function "gets" the lease time from the xmlDoc, this is part of the process of parsing the XML, so it should be named virNetworkDHCPLeaseTimeParseXML(). (it's never been clear to me when/where the "Def" part of the name should be; I've always seen it as just three extra letters that add nothing; someone else may have a different opinion).
Okay. Will rename.
>> + 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.
In particular - if one clause of an if-else is in braces, then *all* of the clauses of that if-else much be in braces.
Noted. Will fix.
> >> + >> + 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),
We don't use gnulib's "PRIxxx" macros anywhere else in libvirt. Better to "go with the flow" and just use "%d" instead for consistency (or make a case for changing them elsewhere :-)
>> + 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)
I *think* this is what you currently have:
-1 : infinite 0/unspecified : unspecified (use default) > 0 : actual lease time.
How about this instead?
In the xml:
0 : infinite unspecified : unspecified > 0 : actual lease time
In the internal object: have a separate "bool leasetime_specified" to differentiate between "0" and unspecified (there are a few other examples of this - search for "_specified" in src/conf/*.c)
Works for me.
>> + 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*/
I'm not sure how useful this would be for libvirt, since it's apparently used when renumbering a network. I think for now it can be ignored.
okay
>> + /* TODO: Discuss what is the default value that we want as dnsmasq's is 1 hour */
Whatever the value has been for all these years, it needs to remain the same. Behavior should be unchanged if the XML is unchanged.
okay, makes sense
>> + /* TODO: Discuss what to do if value exceeds maximum, use default value for now */
If the specified value is too large, we need to log an error and fail. If we were to just ignore invalid values now, it makes it more difficult to change it to an error later (you would then have to validate it only when a definition is newly defined or edited, but not when all the configs are read at libvirtd startup).
noted
>> + 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
Yep, my bad. I've already rebased to master in my branch but I want to address the feedback given before I submit the updated patches.
By the way, how am I supposed to submit the updated patchset? I'm not used to review code by email.
I usually use "git send-email -v2 --cover-letter --annotate -3"
-v2 : adds "v2" after "PATCH" in the subject --cover-letter : creates a "PATCH v2 0/3" email where you can describe the purpose of the series as a whole (won't be added to git) --annotate : load all of the patches (and the cover letter) into your favorite text editor to allow adding in comments about what was changed in V2 of each patch (and compose and add a proper subject to the cover letter). The notes about what was changed between versions should be placed after the "---" line so they won't be included in the commit log that's eventually pushed.
-3 : the number of commits to send from the tip of your branch
You don't need to make them a reply to the V1 email - just leave them as their own thread.
If you look in the list archives for patches with "PATCH v2 0/x" you'll see some good examples.
The thing is that to make rebasing to master I ended up sqashing the three patches into one as managing changes was becoming a bit of a nightmare and I think I ended up mixing them up a bit in the rebase process. So I'm afraid I'm going to have to start a new thread when I submit the updates. -- Cheers, Alberto Ruiz
2017-06-21 17:30 GMT+01:00 Laine Stump <laine@laine.org>:
On 06/21/2017 03:27 AM, Peter Krempa wrote:
On Tue, Jun 20, 2017 at 19:00:43 +0100, aruiz@gnome.org wrote:
From: Alberto Ruiz <aruiz@gnome.org>
Missing commit message.
And when composing the commit message, it's useful to include links to the associated BZ.
https://bugzilla.redhat.com/show_bug.cgi?id=913446
Also, I recall there being quite a lot of discussion in email (and possibly IRC) about the fact that people *think* they want a configurable lease time because they think that will eliminate cases of a DHCP lease being lost while a domain is paused. It was pointed out that lengthening the lease will *not* eliminate that problem (it just makes it happen less often).
As an alternate (and better) solution to the problem of lost leases, we then added the "dhcp-authoritative" option to dnsmasq (commit 4ac20b3ae4), which allows clients to re-acquire the same IP as they had for an expired lease (as long as it hasn't been acquired by someone else in the meantime, which is apparently unlikely unless all the other addresses in the pool are already assigned).
I'm not saying this to discourage the idea of making leasetime configurable (I think we'd already agreed that it was reasonable to do so, but there were two competing patches posted, and neither of them was really push-ready), but just to make sure that nobody is disappointed if the results don't lead to the behavior they're hoping for.
--- 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>
Maybe call this "timeUnit" in case some other attribute in the future needs it?
+ + <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,
While it's true that this function "gets" the lease time from the xmlDoc, this is part of the process of parsing the XML, so it should be named virNetworkDHCPLeaseTimeParseXML(). (it's never been clear to me when/where the "Def" part of the name should be; I've always seen it as just three extra letters that add nothing; someone else may have a different opinion).
+ 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.
In particular - if one clause of an if-else is in braces, then *all* of the clauses of that if-else much be in braces.
+ + 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),
We don't use gnulib's "PRIxxx" macros anywhere else in libvirt. Better to "go with the flow" and just use "%d" instead for consistency (or make a case for changing them elsewhere :-)
I knew I added this for something: This is the error I get when I just use "%d": conf/network_conf.c: In function 'virNetworkDHCPLeaseTimeParseXML': conf/network_conf.c:575:26: error: format '%d' expects argument of type 'int', but argument 8 has type 'int64_t {aka long int}' [-Werror=format=] _("<leasetime> value cannot be greater than the equivalent of %d seconds : %d"), If you can think of any alternative that complies with libvirt's code consistency let me know.
+ 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)
I *think* this is what you currently have:
-1 : infinite 0/unspecified : unspecified (use default)
0 : actual lease time.
How about this instead?
In the xml:
0 : infinite unspecified : unspecified
0 : actual lease time
In the internal object: have a separate "bool leasetime_specified" to differentiate between "0" and unspecified (there are a few other examples of this - search for "_specified" in src/conf/*.c)
+ 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*/
I'm not sure how useful this would be for libvirt, since it's apparently used when renumbering a network. I think for now it can be ignored.
+ /* TODO: Discuss what is the default value that we want as dnsmasq's is 1 hour */
Whatever the value has been for all these years, it needs to remain the same. Behavior should be unchanged if the XML is unchanged.
+ /* TODO: Discuss what to do if value exceeds maximum, use default value for now */
If the specified value is too large, we need to log an error and fail. If we were to just ignore invalid values now, it makes it more difficult to change it to an error later (you would then have to validate it only when a definition is newly defined or edited, but not when all the configs are read at libvirtd startup).
+ 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
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- Cheers, Alberto Ruiz
On 06/22/2017 01:12 PM, Alberto Ruiz wrote:
2017-06-21 17:30 GMT+01:00 Laine Stump <laine@laine.org <mailto:laine@laine.org>>:
On 06/21/2017 03:27 AM, Peter Krempa wrote: > On Tue, Jun 20, 2017 at 19:00:43 +0100, aruiz@gnome.org <mailto:aruiz@gnome.org> wrote: >> From: Alberto Ruiz <aruiz@gnome.org <mailto:aruiz@gnome.org>> > >> + >> + if (result > UINT32_MAX) { >> + virReportError(VIR_ERR_XML_ERROR, >> + _("<leasetime> value cannot be greater than the equivalent of %" PRIo32 " seconds : %" PRId64),
We don't use gnulib's "PRIxxx" macros anywhere else in libvirt. Better to "go with the flow" and just use "%d" instead for consistency (or make a case for changing them elsewhere :-)
(Is it possible for you to change the "quotation" character in your email client so that when you inline quote the original in a reply, it has "> " at the beginning of the line rather than " "? Using blanks as the quotation character makes it more likely to confuse who wrote which parts (especially when everything isn't starting in column 1))
I knew I added this for something:
This is the error I get when I just use "%d":
conf/network_conf.c: In function 'virNetworkDHCPLeaseTimeParseXML': conf/network_conf.c:575:26: error: format '%d' expects argument of type 'int', but argument 8 has type 'int64_t {aka long int}' [-Werror=format=] _("<leasetime> value cannot be greater than the equivalent of %d seconds : %d"),
If you can think of any alternative that complies with libvirt's code consistency let me know.
Looking through the other struct definitions that are filled in from XML, we don't use any types that directly specify the number of bits. Instead we use int, long, and long long (sometimes with unsigned, sometimes without). On x86_64, long and long long are both 64 bits. I think I would just use "unsigned long" (assuming you agree to not use "-1" as a special value), then use %lu for the formatting string.
2017-06-22 22:05 GMT+01:00 Laine Stump <laine@laine.org>:
On 06/22/2017 01:12 PM, Alberto Ruiz wrote:
2017-06-21 17:30 GMT+01:00 Laine Stump <laine@laine.org <mailto:laine@laine.org>>:
On 06/21/2017 03:27 AM, Peter Krempa wrote: > On Tue, Jun 20, 2017 at 19:00:43 +0100, aruiz@gnome.org <mailto:aruiz@gnome.org> wrote: >> From: Alberto Ruiz <aruiz@gnome.org <mailto:aruiz@gnome.org>> > >> + >> + if (result > UINT32_MAX) { >> + virReportError(VIR_ERR_XML_ERROR, >> + _("<leasetime> value cannot be greater than the equivalent of %" PRIo32 " seconds : %" PRId64),
We don't use gnulib's "PRIxxx" macros anywhere else in libvirt. Better to "go with the flow" and just use "%d" instead for consistency (or make a case for changing them elsewhere :-)
(Is it possible for you to change the "quotation" character in your email client so that when you inline quote the original in a reply, it has "> " at the beginning of the line rather than " "? Using blanks as the quotation character makes it more likely to confuse who wrote which parts (especially when everything isn't starting in column 1))
I'll try to reply using plain text (using gmail here)
I knew I added this for something:
This is the error I get when I just use "%d":
conf/network_conf.c: In function 'virNetworkDHCPLeaseTimeParseXML': conf/network_conf.c:575:26: error: format '%d' expects argument of type 'int', but argument 8 has type 'int64_t {aka long int}' [-Werror=format=] _("<leasetime> value cannot be greater than the equivalent of %d seconds : %d"),
If you can think of any alternative that complies with libvirt's code consistency let me know.
Looking through the other struct definitions that are filled in from XML, we don't use any types that directly specify the number of bits. Instead we use int, long, and long long (sometimes with unsigned, sometimes without).
On x86_64, long and long long are both 64 bits. I think I would just use "unsigned long" (assuming you agree to not use "-1" as a special value), then use %lu for the formatting string.
I'm actually switching to use uint32_t so %lu should work now, will give it a try. -- Cheers, Alberto Ruiz
2017-06-21 17:30 GMT+01:00 Laine Stump <laine@laine.org>:
On 06/21/2017 03:27 AM, Peter Krempa wrote:
On Tue, Jun 20, 2017 at 19:00:43 +0100, aruiz@gnome.org wrote:
From: Alberto Ruiz <aruiz@gnome.org>
Missing commit message.
And when composing the commit message, it's useful to include links to the associated BZ.
https://bugzilla.redhat.com/show_bug.cgi?id=913446
Also, I recall there being quite a lot of discussion in email (and possibly IRC) about the fact that people *think* they want a configurable lease time because they think that will eliminate cases of a DHCP lease being lost while a domain is paused. It was pointed out that lengthening the lease will *not* eliminate that problem (it just makes it happen less often).
As an alternate (and better) solution to the problem of lost leases, we then added the "dhcp-authoritative" option to dnsmasq (commit 4ac20b3ae4), which allows clients to re-acquire the same IP as they had for an expired lease (as long as it hasn't been acquired by someone else in the meantime, which is apparently unlikely unless all the other addresses in the pool are already assigned).
I'm not saying this to discourage the idea of making leasetime configurable (I think we'd already agreed that it was reasonable to do so, but there were two competing patches posted, and neither of them was really push-ready), but just to make sure that nobody is disappointed if the results don't lead to the behavior they're hoping for.
--- 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>
Maybe call this "timeUnit" in case some other attribute in the future needs it?
+ + <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,
While it's true that this function "gets" the lease time from the xmlDoc, this is part of the process of parsing the XML, so it should be named virNetworkDHCPLeaseTimeParseXML(). (it's never been clear to me when/where the "Def" part of the name should be; I've always seen it as just three extra letters that add nothing; someone else may have a different opinion).
+ 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.
In particular - if one clause of an if-else is in braces, then *all* of the clauses of that if-else much be in braces.
+ + 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),
We don't use gnulib's "PRIxxx" macros anywhere else in libvirt. Better to "go with the flow" and just use "%d" instead for consistency (or make a case for changing them elsewhere :-)
+ 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)
I *think* this is what you currently have:
-1 : infinite 0/unspecified : unspecified (use default)
0 : actual lease time.
How about this instead?
In the xml:
0 : infinite unspecified : unspecified
0 : actual lease time
In the internal object: have a separate "bool leasetime_specified" to differentiate between "0" and unspecified (there are a few other examples of this - search for "_specified" in src/conf/*.c)
+ 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)
I'd like feedback on this. My guess is that in the dnsmasq case I should just log an error and quit.
+ 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*/
I'm not sure how useful this would be for libvirt, since it's apparently used when renumbering a network. I think for now it can be ignored.
+ /* TODO: Discuss what is the default value that we want as dnsmasq's is 1 hour */
Whatever the value has been for all these years, it needs to remain the same. Behavior should be unchanged if the XML is unchanged.
+ /* TODO: Discuss what to do if value exceeds maximum, use default value for now */
I think that, since I'm splitting value and set/unset in two different fields in the struct, we can use uint32 which is the type used by the dhcp lease in the RFC. This means that we can worry about this at parse time.
If the specified value is too large, we need to log an error and fail. If we were to just ignore invalid values now, it makes it more difficult to change it to an error later (you would then have to validate it only when a definition is newly defined or edited, but not when all the configs are read at libvirtd startup).
+ 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
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- Cheers, Alberto Ruiz
From: Alberto Ruiz <aruiz@gnome.org> Fixes #913446 This patch addresses a few problems found by the initial reviews: * leaseTimeUnit RNG type renamed to timeUnit * virNetworkDHCPDefGetLeaseTime() renamed to virNetworkDHCPLeaseTimeParseXML() * consistent use of braces in if-else-if * use %lu instead of PRId64 * use 0 as infinite lease * add a leasetime_defined field to struct _virNetworkIPDef to describe whether the value was set in the xml configuration or not * use uint32_t for the leasetime instead of int64_t * fail on all invalid leasetime values * squash all patches into one --- docs/schemas/basictypes.rng | 16 +++ docs/schemas/network.rng | 8 ++ src/conf/network_conf.c | 93 +++++++++++++++- src/conf/network_conf.h | 5 +- src/libvirt_private.syms | 1 + src/network/bridge_driver.c | 119 ++++++++++++++++----- src/network/bridge_driver.h | 1 + src/util/virdnsmasq.c | 106 +++++++++++------- src/util/virdnsmasq.h | 2 + .../dhcp6-nat-network.hostsfile | 7 ++ tests/networkxml2confdata/dhcp6-network.hostsfile | 5 + .../dhcp6host-routed-network.hostsfile | 7 ++ tests/networkxml2confdata/leasetime-days.conf | 18 ++++ tests/networkxml2confdata/leasetime-days.xml | 18 ++++ tests/networkxml2confdata/leasetime-hours.conf | 18 ++++ tests/networkxml2confdata/leasetime-hours.xml | 18 ++++ tests/networkxml2confdata/leasetime-infinite.conf | 18 ++++ tests/networkxml2confdata/leasetime-infinite.xml | 18 ++++ tests/networkxml2confdata/leasetime-minutes.conf | 18 ++++ tests/networkxml2confdata/leasetime-minutes.xml | 18 ++++ tests/networkxml2confdata/leasetime-seconds.conf | 18 ++++ tests/networkxml2confdata/leasetime-seconds.xml | 18 ++++ tests/networkxml2confdata/leasetime.conf | 18 ++++ tests/networkxml2confdata/leasetime.xml | 18 ++++ .../nat-network-dns-srv-record-minimal.hostsfile | 2 + .../nat-network-dns-srv-record.hostsfile | 2 + .../nat-network-dns-txt-record.hostsfile | 2 + .../nat-network-name-with-quotes.hostsfile | 2 + tests/networkxml2confdata/nat-network.hostsfile | 2 + .../networkxml2confdata/ptr-domains-auto.hostsfile | 2 + tests/networkxml2conftest.c | 45 ++++++-- 31 files changed, 571 insertions(+), 72 deletions(-) create mode 100644 tests/networkxml2confdata/dhcp6-nat-network.hostsfile create mode 100644 tests/networkxml2confdata/dhcp6-network.hostsfile create mode 100644 tests/networkxml2confdata/dhcp6host-routed-network.hostsfile 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 create mode 100644 tests/networkxml2confdata/nat-network-dns-srv-record-minimal.hostsfile create mode 100644 tests/networkxml2confdata/nat-network-dns-srv-record.hostsfile create mode 100644 tests/networkxml2confdata/nat-network-dns-txt-record.hostsfile create mode 100644 tests/networkxml2confdata/nat-network-name-with-quotes.hostsfile create mode 100644 tests/networkxml2confdata/nat-network.hostsfile create mode 100644 tests/networkxml2confdata/ptr-domains-auto.hostsfile diff --git a/docs/schemas/basictypes.rng b/docs/schemas/basictypes.rng index 1ea667cdf..9db19c7f0 100644 --- a/docs/schemas/basictypes.rng +++ b/docs/schemas/basictypes.rng @@ -564,4 +564,20 @@ </element> </define> + <define name="timeUnit"> + <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 1048dabf3..a0d878e4a 100644 --- a/docs/schemas/network.rng +++ b/docs/schemas/network.rng @@ -357,6 +357,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="timeUnit"/></attribute> + </optional> + <ref name="leaseTime"/> + </element> + </optional> <interleave> <zeroOrMore> <element name="range"> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 3ebf67ff5..8431ee806 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -29,6 +29,8 @@ #include <sys/stat.h> #include <fcntl.h> #include <string.h> +#include <stdlib.h> +#include <inttypes.h> #include "virerror.h" #include "datatypes.h" @@ -514,8 +516,92 @@ virNetworkDHCPHostDefParseXML(const char *networkName, static int +virNetworkDHCPLeaseTimeParseXML (xmlNodePtr node, + xmlXPathContextPtr ctxt, + uint32_t *lease, + bool *defined) +{ + int ret = 0; + uint32_t multiplier; + char *leaseString, *leaseUnit; + xmlNodePtr save; + + *defined = 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) { + goto cleanup; + } + if (leaseString[0] == '-') { + virReportError(VIR_ERR_XML_ERROR, + _("<leasetime> value (%s) cannot be negative"), + leaseString); + } + + *defined = 1; + + 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); + ret = -1; + goto cleanup; + } + + errno = 0; + *lease = (uint32_t) strtoul((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); + ret = -1; + goto cleanup; + } + + if (*lease > (UINT32_MAX / multiplier)) { + virReportError (VIR_ERR_XML_ERROR, + _("<leasetime> value %lu %s exceeds the maximum of %lu seconds"), + (unsigned long)*lease, leaseUnit, (unsigned long)UINT32_MAX); + ret = -1; + goto cleanup; + } + + *lease = *lease * multiplier; + +cleanup: + VIR_FREE(leaseString); + VIR_FREE(leaseUnit); + ctxt->node = save; + return ret; +} + + +static int virNetworkDHCPDefParseXML(const char *networkName, xmlNodePtr node, + xmlXPathContextPtr ctxt, virNetworkIPDefPtr def) { int ret = -1; @@ -526,6 +612,11 @@ virNetworkDHCPDefParseXML(const char *networkName, memset(&range, 0, sizeof(range)); memset(&host, 0, sizeof(host)); + if (virNetworkDHCPLeaseTimeParseXML (node, ctxt, + &def->leasetime, + &def->leasetime_defined)) + goto cleanup; + cur = node->children; while (cur != NULL) { if (cur->type == XML_ELEMENT_NODE && @@ -1104,7 +1195,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)) { diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index e9a5baf5b..466220e81 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -175,7 +175,10 @@ struct _virNetworkIPDef { char *tftproot; char *bootfile; virSocketAddr bootserver; - }; + + uint32_t leasetime; + bool leasetime_defined; +}; typedef struct _virNetworkForwardIfDef virNetworkForwardIfDef; typedef virNetworkForwardIfDef *virNetworkForwardIfDefPtr; diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c1e9471c5..5ef33dcfe 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1561,6 +1561,7 @@ dnsmasqCapsRefresh; dnsmasqContextFree; dnsmasqContextNew; dnsmasqDelete; +dnsmasqDhcpHostsToString; dnsmasqReload; dnsmasqSave; diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 3ba70180b..cdb0230ab 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 @@ -948,6 +950,62 @@ networkKillDaemon(pid_t pid, const char *daemonName, const char *networkName) return ret; } +static int +networkBuildDnsmasqHostsList(dnsmasqContext *dctx, + virNetworkDNSDefPtr dnsdef) +{ + size_t i, j; + + if (dnsdef) { + for (i = 0; i < dnsdef->nhosts; i++) { + virNetworkDNSHostDefPtr host = &(dnsdef->hosts[i]); + if (VIR_SOCKET_ADDR_VALID(&host->ip)) { + for (j = 0; j < host->nnames; j++) + if (dnsmasqAddHost(dctx, &host->ip, host->names[j]) < 0) + return -1; + } + } + } + + return 0; +} + +/* translates the leasetime value into a dnsmasq configuration string + * for dhcp-range/host */ +static char * +networkDnsmasqConfLeaseValueToString (int64_t leasetime, bool defined) +{ + char *result = NULL; + virBuffer leasebuf = VIR_BUFFER_INITIALIZER; + + /* Leasetime parameter set on the XML */ + + /* defined = FALSE means we fallback to dnsmasq defaults*/ + if (defined == 0) { + virBufferAsprintf(&leasebuf, "%s", ""); + } + /* 0 means no expiration */ + else if (leasetime == 0) { + virBufferAsprintf(&leasebuf, ",infinite"); + } + /* DHCP value for lease time is a unsigned four octect integer */ + else if (leasetime <= UINT32_MAX) { + virBufferAsprintf(&leasebuf, ",%lu", (unsigned long)leasetime); + } + else { + if (leasetime < 120) + virReportError (VIR_ERR_CONFIG_UNSUPPORTED, + _("lease time must be greater than 120 seconds")); + goto cleanup; + } + + result = virBufferContentAndReset(&leasebuf); + +cleanup: + virBufferFreeAndReset (&leasebuf); + return result; +} + /* the following does not build a file, it builds a list * which is later saved into a file */ @@ -956,8 +1014,14 @@ static int networkBuildDnsmasqDhcpHostsList(dnsmasqContext *dctx, virNetworkIPDefPtr ipdef) { + int ret = -1; size_t i; bool ipv6 = false; + char *leasetime = networkDnsmasqConfLeaseValueToString(ipdef->leasetime, + ipdef->leasetime_defined); + + if (!leasetime) + goto cleanup; if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET6)) ipv6 = true; @@ -965,31 +1029,14 @@ 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) - return -1; - } - - return 0; -} - -static int -networkBuildDnsmasqHostsList(dnsmasqContext *dctx, - virNetworkDNSDefPtr dnsdef) -{ - size_t i, j; - - if (dnsdef) { - for (i = 0; i < dnsdef->nhosts; i++) { - virNetworkDNSHostDefPtr host = &(dnsdef->hosts[i]); - if (VIR_SOCKET_ADDR_VALID(&host->ip)) { - for (j = 0; j < host->nnames; j++) - if (dnsmasqAddHost(dctx, &host->ip, host->names[j]) < 0) - return -1; - } - } + host->name, host->id, leasetime, ipv6) < 0) + goto cleanup; } - return 0; + ret = 0; +cleanup: + VIR_FREE(leasetime); + return ret; } @@ -1034,6 +1081,7 @@ int networkDnsmasqConfContents(virNetworkObjPtr network, const char *pidfile, char **configstr, + char **hostsfilestr, dnsmasqContext *dctx, dnsmasqCapsPtr caps ATTRIBUTE_UNUSED) { @@ -1362,6 +1410,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))) @@ -1369,12 +1418,23 @@ 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, + ipdef->leasetime_defined); + 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, @@ -1405,6 +1465,15 @@ networkDnsmasqConfContents(virNetworkObjPtr network, if (networkBuildDnsmasqDhcpHostsList(dctx, ipdef) < 0) goto cleanup; + /* Return the contents of the hostsfile if requested */ + if (hostsfilestr) { + *hostsfilestr = dnsmasqDhcpHostsToString (dctx->hostsfile->hosts, + dctx->hostsfile->nhosts); + + if (!hostsfilestr) + goto cleanup; + } + /* Note: the following is IPv4 only */ if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET)) { if (ipdef->nranges || ipdef->nhosts) { @@ -1506,7 +1575,7 @@ networkBuildDhcpDaemonCommandLine(virNetworkDriverStatePtr driver, network->dnsmasqPid = -1; - if (networkDnsmasqConfContents(network, pidfile, &configstr, + if (networkDnsmasqConfContents(network, pidfile, &configstr, NULL, dctx, dnsmasq_caps) < 0) goto cleanup; if (!configstr) diff --git a/src/network/bridge_driver.h b/src/network/bridge_driver.h index 7832b6031..56329eb59 100644 --- a/src/network/bridge_driver.h +++ b/src/network/bridge_driver.h @@ -53,6 +53,7 @@ int networkGetActualType(virDomainNetDefPtr iface) int networkDnsmasqConfContents(virNetworkObjPtr network, const char *pidfile, char **configstr, + char **hostsfilestr, dnsmasqContext *dctx, dnsmasqCapsPtr caps); diff --git a/src/util/virdnsmasq.c b/src/util/virdnsmasq.c index 1b78c1fad..94c9a3bb1 100644 --- a/src/util/virdnsmasq.c +++ b/src/util/virdnsmasq.c @@ -308,52 +308,47 @@ hostsfileAdd(dnsmasqHostsfile *hostsfile, virSocketAddr *ip, const char *name, const char *id, + const char *leasetime, bool ipv6) { + int ret = -1; char *ipstr = NULL; + virBuffer hostbuf = VIR_BUFFER_INITIALIZER; + if (VIR_REALLOC_N(hostsfile->hosts, hostsfile->nhosts + 1) < 0) goto error; if (!(ipstr = virSocketAddrFormat(ip))) - return -1; + goto error; /* the first test determines if it is a dhcpv6 host */ if (ipv6) { - if (name && id) { - if (virAsprintf(&hostsfile->hosts[hostsfile->nhosts].host, - "id:%s,%s,[%s]", id, name, ipstr) < 0) - goto error; - } else if (name && !id) { - if (virAsprintf(&hostsfile->hosts[hostsfile->nhosts].host, - "%s,[%s]", name, ipstr) < 0) - goto error; - } else if (!name && id) { - if (virAsprintf(&hostsfile->hosts[hostsfile->nhosts].host, - "id:%s,[%s]", id, ipstr) < 0) - goto error; - } + if (name && id) + virBufferAsprintf(&hostbuf, "id:%s,%s,[%s]", id, name, ipstr); + else if (name && !id) + virBufferAsprintf(&hostbuf, "%s,[%s]", name, ipstr); + else if (!name && id) + virBufferAsprintf(&hostbuf, "id:%s,[%s]", id, ipstr); } else if (name && mac) { - if (virAsprintf(&hostsfile->hosts[hostsfile->nhosts].host, "%s,%s,%s", - mac, ipstr, name) < 0) - goto error; + virBufferAsprintf(&hostbuf, "%s,%s,%s", mac, ipstr, name); } else if (name && !mac) { - if (virAsprintf(&hostsfile->hosts[hostsfile->nhosts].host, "%s,%s", - name, ipstr) < 0) - goto error; + virBufferAsprintf(&hostbuf, "%s,%s", name, ipstr); } else { - if (virAsprintf(&hostsfile->hosts[hostsfile->nhosts].host, "%s,%s", - mac, ipstr) < 0) - goto error; + virBufferAsprintf(&hostbuf, "%s,%s", mac, ipstr); } - VIR_FREE(ipstr); - hostsfile->nhosts++; + /* The leasetime string already includes comma if there's any value at all */ + virBufferAsprintf(&hostbuf, "%s", leasetime); - return 0; + if (!(hostsfile->hosts[hostsfile->nhosts].host = virBufferContentAndReset (&hostbuf))) + goto error; + hostsfile->nhosts++; + ret = 0; error: + virBufferFreeAndReset(&hostbuf); VIR_FREE(ipstr); - return -1; + return ret; } static dnsmasqHostsfile * @@ -391,10 +386,9 @@ hostsfileWrite(const char *path, dnsmasqDhcpHost *hosts, unsigned int nhosts) { - char *tmp; + char *tmp, *content = NULL; FILE *f; bool istmp = true; - size_t i; int rc = 0; /* even if there are 0 hosts, create a 0 length file, to allow @@ -412,17 +406,21 @@ hostsfileWrite(const char *path, } } - for (i = 0; i < nhosts; i++) { - if (fputs(hosts[i].host, f) == EOF || fputc('\n', f) == EOF) { - rc = -errno; - VIR_FORCE_FCLOSE(f); + if (!(content = dnsmasqDhcpHostsToString(hosts, nhosts))) { + rc = -ENOMEM; + goto cleanup; + } - if (istmp) - unlink(tmp); + if (fputs(content, f) == EOF) { + rc = -errno; + VIR_FORCE_FCLOSE(f); + + if (istmp) + unlink(tmp); + + goto cleanup; + } - goto cleanup; - } - } if (VIR_FCLOSE(f) == EOF) { rc = -errno; @@ -436,6 +434,7 @@ hostsfileWrite(const char *path, } cleanup: + VIR_FREE(content); VIR_FREE(tmp); return rc; @@ -524,9 +523,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); } /* @@ -892,3 +892,31 @@ dnsmasqCapsGet(dnsmasqCapsPtr caps, dnsmasqCapsFlags flag) return caps && virBitmapIsBitSet(caps->flags, flag); } + +/** dnsmasqDhcpHostsToString: + * + * Turns a vector of dnsmasqDhcpHost into the string that is ought to be + * stored in the hostsfile, this functionality is split to make hostsfiles + * testable. Returs NULL if nhosts is 0. + */ +char * +dnsmasqDhcpHostsToString (dnsmasqDhcpHost *hosts, + unsigned int nhosts) +{ + int i; + char *result = NULL; + virBuffer hostsfilebuf = VIR_BUFFER_INITIALIZER; + + if (nhosts == 0) + goto cleanup; + + for (i = 0; i < nhosts; i++) { + virBufferAsprintf(&hostsfilebuf, "%s\n", hosts[i].host); + } + + result = virBufferContentAndReset(&hostsfilebuf); + +cleanup: + virBufferFreeAndReset(&hostsfilebuf); + return result; +} diff --git a/src/util/virdnsmasq.h b/src/util/virdnsmasq.h index f47bea3ab..1795bc83b 100644 --- a/src/util/virdnsmasq.h +++ b/src/util/virdnsmasq.h @@ -88,6 +88,7 @@ int dnsmasqAddDhcpHost(dnsmasqContext *ctx, virSocketAddr *ip, const char *name, const char *id, + const char *leastime, bool ipv6); int dnsmasqAddHost(dnsmasqContext *ctx, virSocketAddr *ip, @@ -105,6 +106,7 @@ int dnsmasqCapsRefresh(dnsmasqCapsPtr *caps, const char *binaryPath); bool dnsmasqCapsGet(dnsmasqCapsPtr caps, dnsmasqCapsFlags flag); const char *dnsmasqCapsGetBinaryPath(dnsmasqCapsPtr caps); unsigned long dnsmasqCapsGetVersion(dnsmasqCapsPtr caps); +char *dnsmasqDhcpHostsToString(dnsmasqDhcpHost *hosts, unsigned int nhosts); # define DNSMASQ_DHCPv6_MAJOR_REQD 2 # define DNSMASQ_DHCPv6_MINOR_REQD 64 diff --git a/tests/networkxml2confdata/dhcp6-nat-network.hostsfile b/tests/networkxml2confdata/dhcp6-nat-network.hostsfile new file mode 100644 index 000000000..de659b98c --- /dev/null +++ b/tests/networkxml2confdata/dhcp6-nat-network.hostsfile @@ -0,0 +1,7 @@ +00:16:3e:77:e2:ed,192.168.122.10,a.example.com +00:16:3e:3e:a9:1a,192.168.122.11,b.example.com +id:0:4:7e:7d:f0:7d:a8:bc:c5:d2:13:32:11:ed:16:ea:84:63,[2001:db8:ac10:fd01::1:20] +paul,[2001:db8:ac10:fd01::1:21] +id:0:3:0:1:0:16:3e:11:22:33,peter.xyz,[2001:db8:ac10:fd01::1:22] +id:0:3:0:1:0:16:3e:44:55:33,[2001:db8:ac10:fd01::1:23] +id:0:1:0:1:18:aa:62:fe:0:16:3e:44:55:66,badbob,[2001:db8:ac10:fd01::1:24] diff --git a/tests/networkxml2confdata/dhcp6-network.hostsfile b/tests/networkxml2confdata/dhcp6-network.hostsfile new file mode 100644 index 000000000..9dfb172ce --- /dev/null +++ b/tests/networkxml2confdata/dhcp6-network.hostsfile @@ -0,0 +1,5 @@ +id:0:4:7e:7d:f0:7d:a8:bc:c5:d2:13:32:11:ed:16:ea:84:63,[2001:db8:ac10:fd01::1:20] +paul,[2001:db8:ac10:fd01::1:21] +id:0:3:0:1:0:16:3e:11:22:33,peter.xyz,[2001:db8:ac10:fd01::1:22] +id:0:3:0:1:0:16:3e:44:55:33,[2001:db8:ac10:fd01::1:23] +id:0:1:0:1:18:aa:62:fe:0:16:3e:44:55:66,badbob,[2001:db8:ac10:fd01::1:24] diff --git a/tests/networkxml2confdata/dhcp6host-routed-network.hostsfile b/tests/networkxml2confdata/dhcp6host-routed-network.hostsfile new file mode 100644 index 000000000..de659b98c --- /dev/null +++ b/tests/networkxml2confdata/dhcp6host-routed-network.hostsfile @@ -0,0 +1,7 @@ +00:16:3e:77:e2:ed,192.168.122.10,a.example.com +00:16:3e:3e:a9:1a,192.168.122.11,b.example.com +id:0:4:7e:7d:f0:7d:a8:bc:c5:d2:13:32:11:ed:16:ea:84:63,[2001:db8:ac10:fd01::1:20] +paul,[2001:db8:ac10:fd01::1:21] +id:0:3:0:1:0:16:3e:11:22:33,peter.xyz,[2001:db8:ac10:fd01::1:22] +id:0:3:0:1:0:16:3e:44:55:33,[2001:db8:ac10:fd01::1:23] +id:0:1:0:1:18:aa:62:fe:0:16:3e:44:55:66,badbob,[2001:db8:ac10:fd01::1:24] diff --git a/tests/networkxml2confdata/leasetime-days.conf b/tests/networkxml2confdata/leasetime-days.conf new file mode 100644 index 000000000..d227be67a --- /dev/null +++ b/tests/networkxml2confdata/leasetime-days.conf @@ -0,0 +1,18 @@ +##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,86400 +dhcp-no-override +dhcp-authoritative +dhcp-range=2001:db8:ac10:fd01::1:10,2001:db8:ac10:fd01::1:ff,64,86400 +dhcp-lease-max=493 +dhcp-hostsfile=/var/lib/libvirt/dnsmasq/default.hostsfile +addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts +enable-ra diff --git a/tests/networkxml2confdata/leasetime-days.xml b/tests/networkxml2confdata/leasetime-days.xml new file mode 100644 index 000000000..b990b4d68 --- /dev/null +++ b/tests/networkxml2confdata/leasetime-days.xml @@ -0,0 +1,18 @@ +<network> + <name>default</name> + <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid> + <forward dev='eth1' mode='nat'/> + <bridge name='virbr0' stp='on' delay='0'/> + <ip address='192.168.122.1' netmask='255.255.255.0'> + <dhcp> + <leasetime unit="days">1</leasetime> + <range start='192.168.122.2' end='192.168.122.254'/> + </dhcp> + </ip> + <ip family='ipv6' address='2001:db8:ac10:fd01::1' prefix='64'> + <dhcp> + <leasetime unit="days">1</leasetime> + <range start='2001:db8:ac10:fd01::1:10' end='2001:db8:ac10:fd01::1:ff'/> + </dhcp> + </ip> +</network> diff --git a/tests/networkxml2confdata/leasetime-hours.conf b/tests/networkxml2confdata/leasetime-hours.conf new file mode 100644 index 000000000..e5e8c19cd --- /dev/null +++ b/tests/networkxml2confdata/leasetime-hours.conf @@ -0,0 +1,18 @@ +##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,3600 +dhcp-no-override +dhcp-authoritative +dhcp-range=2001:db8:ac10:fd01::1:10,2001:db8:ac10:fd01::1:ff,64,3600 +dhcp-lease-max=493 +dhcp-hostsfile=/var/lib/libvirt/dnsmasq/default.hostsfile +addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts +enable-ra diff --git a/tests/networkxml2confdata/leasetime-hours.xml b/tests/networkxml2confdata/leasetime-hours.xml new file mode 100644 index 000000000..3b9609601 --- /dev/null +++ b/tests/networkxml2confdata/leasetime-hours.xml @@ -0,0 +1,18 @@ +<network> + <name>default</name> + <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid> + <forward dev='eth1' mode='nat'/> + <bridge name='virbr0' stp='on' delay='0'/> + <ip address='192.168.122.1' netmask='255.255.255.0'> + <dhcp> + <leasetime unit="hours">1</leasetime> + <range start='192.168.122.2' end='192.168.122.254'/> + </dhcp> + </ip> + <ip family='ipv6' address='2001:db8:ac10:fd01::1' prefix='64'> + <dhcp> + <leasetime unit="hours">1</leasetime> + <range start='2001:db8:ac10:fd01::1:10' end='2001:db8:ac10:fd01::1:ff'/> + </dhcp> + </ip> +</network> diff --git a/tests/networkxml2confdata/leasetime-infinite.conf b/tests/networkxml2confdata/leasetime-infinite.conf new file mode 100644 index 000000000..52f4798e5 --- /dev/null +++ b/tests/networkxml2confdata/leasetime-infinite.conf @@ -0,0 +1,18 @@ +##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,infinite +dhcp-no-override +dhcp-authoritative +dhcp-range=2001:db8:ac10:fd01::1:10,2001:db8:ac10:fd01::1:ff,64,infinite +dhcp-lease-max=493 +dhcp-hostsfile=/var/lib/libvirt/dnsmasq/default.hostsfile +addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts +enable-ra diff --git a/tests/networkxml2confdata/leasetime-infinite.xml b/tests/networkxml2confdata/leasetime-infinite.xml new file mode 100644 index 000000000..d855978a1 --- /dev/null +++ b/tests/networkxml2confdata/leasetime-infinite.xml @@ -0,0 +1,18 @@ +<network> + <name>default</name> + <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid> + <forward dev='eth1' mode='nat'/> + <bridge name='virbr0' stp='on' delay='0'/> + <ip address='192.168.122.1' netmask='255.255.255.0'> + <dhcp> + <leasetime>0</leasetime> + <range start='192.168.122.2' end='192.168.122.254'/> + </dhcp> + </ip> + <ip family='ipv6' address='2001:db8:ac10:fd01::1' prefix='64'> + <dhcp> + <leasetime>0</leasetime> + <range start='2001:db8:ac10:fd01::1:10' end='2001:db8:ac10:fd01::1:ff'/> + </dhcp> + </ip> +</network> diff --git a/tests/networkxml2confdata/leasetime-minutes.conf b/tests/networkxml2confdata/leasetime-minutes.conf new file mode 100644 index 000000000..37da5702f --- /dev/null +++ b/tests/networkxml2confdata/leasetime-minutes.conf @@ -0,0 +1,18 @@ +##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,300 +dhcp-no-override +dhcp-authoritative +dhcp-range=2001:db8:ac10:fd01::1:10,2001:db8:ac10:fd01::1:ff,64,300 +dhcp-lease-max=493 +dhcp-hostsfile=/var/lib/libvirt/dnsmasq/default.hostsfile +addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts +enable-ra diff --git a/tests/networkxml2confdata/leasetime-minutes.xml b/tests/networkxml2confdata/leasetime-minutes.xml new file mode 100644 index 000000000..e7a27afe6 --- /dev/null +++ b/tests/networkxml2confdata/leasetime-minutes.xml @@ -0,0 +1,18 @@ +<network> + <name>default</name> + <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid> + <forward dev='eth1' mode='nat'/> + <bridge name='virbr0' stp='on' delay='0'/> + <ip address='192.168.122.1' netmask='255.255.255.0'> + <dhcp> + <leasetime unit="minutes">5</leasetime> + <range start='192.168.122.2' end='192.168.122.254'/> + </dhcp> + </ip> + <ip family='ipv6' address='2001:db8:ac10:fd01::1' prefix='64'> + <dhcp> + <leasetime unit="minutes">5</leasetime> + <range start='2001:db8:ac10:fd01::1:10' end='2001:db8:ac10:fd01::1:ff'/> + </dhcp> + </ip> +</network> diff --git a/tests/networkxml2confdata/leasetime-seconds.conf b/tests/networkxml2confdata/leasetime-seconds.conf new file mode 100644 index 000000000..ea0845e21 --- /dev/null +++ b/tests/networkxml2confdata/leasetime-seconds.conf @@ -0,0 +1,18 @@ +##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,125 +dhcp-no-override +dhcp-authoritative +dhcp-range=2001:db8:ac10:fd01::1:10,2001:db8:ac10:fd01::1:ff,64,125 +dhcp-lease-max=493 +dhcp-hostsfile=/var/lib/libvirt/dnsmasq/default.hostsfile +addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts +enable-ra diff --git a/tests/networkxml2confdata/leasetime-seconds.xml b/tests/networkxml2confdata/leasetime-seconds.xml new file mode 100644 index 000000000..56b07f8ae --- /dev/null +++ b/tests/networkxml2confdata/leasetime-seconds.xml @@ -0,0 +1,18 @@ +<network> + <name>default</name> + <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid> + <forward dev='eth1' mode='nat'/> + <bridge name='virbr0' stp='on' delay='0'/> + <ip address='192.168.122.1' netmask='255.255.255.0'> + <dhcp> + <leasetime unit="seconds">125</leasetime> + <range start='192.168.122.2' end='192.168.122.254'/> + </dhcp> + </ip> + <ip family='ipv6' address='2001:db8:ac10:fd01::1' prefix='64'> + <dhcp> + <leasetime unit="seconds">125</leasetime> + <range start='2001:db8:ac10:fd01::1:10' end='2001:db8:ac10:fd01::1:ff'/> + </dhcp> + </ip> +</network> diff --git a/tests/networkxml2confdata/leasetime.conf b/tests/networkxml2confdata/leasetime.conf new file mode 100644 index 000000000..b754c3ffb --- /dev/null +++ b/tests/networkxml2confdata/leasetime.conf @@ -0,0 +1,18 @@ +##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,122 +dhcp-no-override +dhcp-authoritative +dhcp-range=2001:db8:ac10:fd01::1:10,2001:db8:ac10:fd01::1:ff,64,121 +dhcp-lease-max=493 +dhcp-hostsfile=/var/lib/libvirt/dnsmasq/default.hostsfile +addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts +enable-ra diff --git a/tests/networkxml2confdata/leasetime.xml b/tests/networkxml2confdata/leasetime.xml new file mode 100644 index 000000000..fdbb15fc0 --- /dev/null +++ b/tests/networkxml2confdata/leasetime.xml @@ -0,0 +1,18 @@ +<network> + <name>default</name> + <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid> + <forward dev='eth1' mode='nat'/> + <bridge name='virbr0' stp='on' delay='0'/> + <ip address='192.168.122.1' netmask='255.255.255.0'> + <dhcp> + <leasetime>122</leasetime> + <range start='192.168.122.2' end='192.168.122.254'/> + </dhcp> + </ip> + <ip family='ipv6' address='2001:db8:ac10:fd01::1' prefix='64'> + <dhcp> + <leasetime>121</leasetime> + <range start='2001:db8:ac10:fd01::1:10' end='2001:db8:ac10:fd01::1:ff'/> + </dhcp> + </ip> +</network> diff --git a/tests/networkxml2confdata/nat-network-dns-srv-record-minimal.hostsfile b/tests/networkxml2confdata/nat-network-dns-srv-record-minimal.hostsfile new file mode 100644 index 000000000..deb3f00ac --- /dev/null +++ b/tests/networkxml2confdata/nat-network-dns-srv-record-minimal.hostsfile @@ -0,0 +1,2 @@ +00:16:3e:77:e2:ed,192.168.122.10,a.example.com +00:16:3e:3e:a9:1a,192.168.122.11,b.example.com diff --git a/tests/networkxml2confdata/nat-network-dns-srv-record.hostsfile b/tests/networkxml2confdata/nat-network-dns-srv-record.hostsfile new file mode 100644 index 000000000..deb3f00ac --- /dev/null +++ b/tests/networkxml2confdata/nat-network-dns-srv-record.hostsfile @@ -0,0 +1,2 @@ +00:16:3e:77:e2:ed,192.168.122.10,a.example.com +00:16:3e:3e:a9:1a,192.168.122.11,b.example.com diff --git a/tests/networkxml2confdata/nat-network-dns-txt-record.hostsfile b/tests/networkxml2confdata/nat-network-dns-txt-record.hostsfile new file mode 100644 index 000000000..deb3f00ac --- /dev/null +++ b/tests/networkxml2confdata/nat-network-dns-txt-record.hostsfile @@ -0,0 +1,2 @@ +00:16:3e:77:e2:ed,192.168.122.10,a.example.com +00:16:3e:3e:a9:1a,192.168.122.11,b.example.com diff --git a/tests/networkxml2confdata/nat-network-name-with-quotes.hostsfile b/tests/networkxml2confdata/nat-network-name-with-quotes.hostsfile new file mode 100644 index 000000000..deb3f00ac --- /dev/null +++ b/tests/networkxml2confdata/nat-network-name-with-quotes.hostsfile @@ -0,0 +1,2 @@ +00:16:3e:77:e2:ed,192.168.122.10,a.example.com +00:16:3e:3e:a9:1a,192.168.122.11,b.example.com diff --git a/tests/networkxml2confdata/nat-network.hostsfile b/tests/networkxml2confdata/nat-network.hostsfile new file mode 100644 index 000000000..deb3f00ac --- /dev/null +++ b/tests/networkxml2confdata/nat-network.hostsfile @@ -0,0 +1,2 @@ +00:16:3e:77:e2:ed,192.168.122.10,a.example.com +00:16:3e:3e:a9:1a,192.168.122.11,b.example.com diff --git a/tests/networkxml2confdata/ptr-domains-auto.hostsfile b/tests/networkxml2confdata/ptr-domains-auto.hostsfile new file mode 100644 index 000000000..deb3f00ac --- /dev/null +++ b/tests/networkxml2confdata/ptr-domains-auto.hostsfile @@ -0,0 +1,2 @@ +00:16:3e:77:e2:ed,192.168.122.10,a.example.com +00:16:3e:3e:a9:1a,192.168.122.11,b.example.com diff --git a/tests/networkxml2conftest.c b/tests/networkxml2conftest.c index ab3c13aa0..15f783fb1 100644 --- a/tests/networkxml2conftest.c +++ b/tests/networkxml2conftest.c @@ -19,9 +19,13 @@ #define VIR_FROM_THIS VIR_FROM_NONE static int -testCompareXMLToConfFiles(const char *inxml, const char *outconf, dnsmasqCapsPtr caps) +testCompareXMLToConfFiles(const char *inxml, + const char *outconf, + const char *outhostsfile, + dnsmasqCapsPtr caps) { - char *actual = NULL; + char *actualconf = NULL; + char *actualhosts = NULL; int ret = -1; virNetworkDefPtr dev = NULL; virNetworkObjPtr obj = NULL; @@ -41,7 +45,11 @@ testCompareXMLToConfFiles(const char *inxml, const char *outconf, dnsmasqCapsPtr if (dctx == NULL) goto fail; - if (networkDnsmasqConfContents(obj, pidfile, &actual, dctx, caps) < 0) + if (networkDnsmasqConfContents(obj, pidfile, &actualconf, &actualhosts, + dctx, caps) < 0) + goto fail; + + if (virTestCompareToFile(actualconf, outconf) < 0) goto fail; /* Any changes to this function ^^ should be reflected here too. */ @@ -57,13 +65,27 @@ testCompareXMLToConfFiles(const char *inxml, const char *outconf, dnsmasqCapsPtr tmp = NULL; #endif - if (virTestCompareToFile(actual, outconf) < 0) + if (virFileExists(outhostsfile)) { + if (!actualhosts) { + fprintf(stderr, + "%s: hostsfile exists but the configuration did not specify any host", + outhostsfile); + goto fail; + } else if (virTestCompareToFile(actualhosts, outhostsfile) < 0) { + goto fail; + } + } else if (actualhosts) { + fprintf(stderr, + "%s: file does not exist but actual data was expected", + outhostsfile); goto fail; + } ret = 0; fail: - VIR_FREE(actual); + VIR_FREE(actualconf); + VIR_FREE(actualhosts); VIR_FREE(pidfile); virCommandFree(cmd); virObjectUnref(obj); @@ -83,19 +105,23 @@ testCompareXMLToConfHelper(const void *data) const testInfo *info = data; char *inxml = NULL; char *outconf = NULL; + char *outhostsfile = NULL; if (virAsprintf(&inxml, "%s/networkxml2confdata/%s.xml", abs_srcdir, info->name) < 0 || virAsprintf(&outconf, "%s/networkxml2confdata/%s.conf", + abs_srcdir, info->name) < 0 || + virAsprintf(&outhostsfile, "%s/networkxml2confdata/%s.hostsfile", abs_srcdir, info->name) < 0) { goto cleanup; } - result = testCompareXMLToConfFiles(inxml, outconf, info->caps); + result = testCompareXMLToConfFiles(inxml, outconf, outhostsfile, info->caps); cleanup: VIR_FREE(inxml); VIR_FREE(outconf); + VIR_FREE(outhostsfile); return result; } @@ -143,6 +169,13 @@ mymain(void) DO_TEST("dhcp6-nat-network", dhcpv6); DO_TEST("dhcp6host-routed-network", dhcpv6); DO_TEST("ptr-domains-auto", dhcpv6); + DO_TEST("leasetime", dhcpv6); + DO_TEST("leasetime-seconds", dhcpv6); + DO_TEST("leasetime-hours", dhcpv6); + DO_TEST("leasetime-minutes", dhcpv6); + DO_TEST("leasetime-hours", dhcpv6); + DO_TEST("leasetime-days", dhcpv6); + DO_TEST("leasetime-infinite", dhcpv6); virObjectUnref(dhcpv6); virObjectUnref(full); -- 2.13.0
Hey Laine, Have some time to review this? 2017-06-23 1:44 GMT+01:00 <aruiz@gnome.org>:
From: Alberto Ruiz <aruiz@gnome.org>
Fixes #913446
This patch addresses a few problems found by the initial reviews:
* leaseTimeUnit RNG type renamed to timeUnit * virNetworkDHCPDefGetLeaseTime() renamed to virNetworkDHCPLeaseTimeParseXML() * consistent use of braces in if-else-if * use %lu instead of PRId64 * use 0 as infinite lease * add a leasetime_defined field to struct _virNetworkIPDef to describe whether the value was set in the xml configuration or not * use uint32_t for the leasetime instead of int64_t * fail on all invalid leasetime values * squash all patches into one
--- docs/schemas/basictypes.rng | 16 +++ docs/schemas/network.rng | 8 ++ src/conf/network_conf.c | 93 +++++++++++++++- src/conf/network_conf.h | 5 +- src/libvirt_private.syms | 1 + src/network/bridge_driver.c | 119 ++++++++++++++++----- src/network/bridge_driver.h | 1 + src/util/virdnsmasq.c | 106 +++++++++++------- src/util/virdnsmasq.h | 2 + .../dhcp6-nat-network.hostsfile | 7 ++ tests/networkxml2confdata/dhcp6-network.hostsfile | 5 + .../dhcp6host-routed-network.hostsfile | 7 ++ tests/networkxml2confdata/leasetime-days.conf | 18 ++++ tests/networkxml2confdata/leasetime-days.xml | 18 ++++ tests/networkxml2confdata/leasetime-hours.conf | 18 ++++ tests/networkxml2confdata/leasetime-hours.xml | 18 ++++ tests/networkxml2confdata/leasetime-infinite.conf | 18 ++++ tests/networkxml2confdata/leasetime-infinite.xml | 18 ++++ tests/networkxml2confdata/leasetime-minutes.conf | 18 ++++ tests/networkxml2confdata/leasetime-minutes.xml | 18 ++++ tests/networkxml2confdata/leasetime-seconds.conf | 18 ++++ tests/networkxml2confdata/leasetime-seconds.xml | 18 ++++ tests/networkxml2confdata/leasetime.conf | 18 ++++ tests/networkxml2confdata/leasetime.xml | 18 ++++ .../nat-network-dns-srv-record-minimal.hostsfile | 2 + .../nat-network-dns-srv-record.hostsfile | 2 + .../nat-network-dns-txt-record.hostsfile | 2 + .../nat-network-name-with-quotes.hostsfile | 2 + tests/networkxml2confdata/nat-network.hostsfile | 2 + .../networkxml2confdata/ptr-domains-auto.hostsfile | 2 + tests/networkxml2conftest.c | 45 ++++++-- 31 files changed, 571 insertions(+), 72 deletions(-) create mode 100644 tests/networkxml2confdata/dhcp6-nat-network.hostsfile create mode 100644 tests/networkxml2confdata/dhcp6-network.hostsfile create mode 100644 tests/networkxml2confdata/dhcp6host-routed-network.hostsfile 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 create mode 100644 tests/networkxml2confdata/nat-network-dns-srv-record-minimal.hostsfile create mode 100644 tests/networkxml2confdata/nat-network-dns-srv-record.hostsfile create mode 100644 tests/networkxml2confdata/nat-network-dns-txt-record.hostsfile create mode 100644 tests/networkxml2confdata/nat-network-name-with-quotes.hostsfile create mode 100644 tests/networkxml2confdata/nat-network.hostsfile create mode 100644 tests/networkxml2confdata/ptr-domains-auto.hostsfile
diff --git a/docs/schemas/basictypes.rng b/docs/schemas/basictypes.rng index 1ea667cdf..9db19c7f0 100644 --- a/docs/schemas/basictypes.rng +++ b/docs/schemas/basictypes.rng @@ -564,4 +564,20 @@ </element> </define>
+ <define name="timeUnit"> + <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 1048dabf3..a0d878e4a 100644 --- a/docs/schemas/network.rng +++ b/docs/schemas/network.rng @@ -357,6 +357,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="timeUnit"/></attribute> + </optional> + <ref name="leaseTime"/> + </element> + </optional> <interleave> <zeroOrMore> <element name="range"> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 3ebf67ff5..8431ee806 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -29,6 +29,8 @@ #include <sys/stat.h> #include <fcntl.h> #include <string.h> +#include <stdlib.h> +#include <inttypes.h>
#include "virerror.h" #include "datatypes.h" @@ -514,8 +516,92 @@ virNetworkDHCPHostDefParseXML(const char *networkName,
static int +virNetworkDHCPLeaseTimeParseXML (xmlNodePtr node, + xmlXPathContextPtr ctxt, + uint32_t *lease, + bool *defined) +{ + int ret = 0; + uint32_t multiplier; + char *leaseString, *leaseUnit; + xmlNodePtr save; + + *defined = 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) { + goto cleanup; + } + if (leaseString[0] == '-') { + virReportError(VIR_ERR_XML_ERROR, + _("<leasetime> value (%s) cannot be negative"), + leaseString); + } + + *defined = 1; + + 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); + ret = -1; + goto cleanup; + } + + errno = 0; + *lease = (uint32_t) strtoul((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); + ret = -1; + goto cleanup; + } + + if (*lease > (UINT32_MAX / multiplier)) { + virReportError (VIR_ERR_XML_ERROR, + _("<leasetime> value %lu %s exceeds the maximum of %lu seconds"), + (unsigned long)*lease, leaseUnit, (unsigned long)UINT32_MAX); + ret = -1; + goto cleanup; + } + + *lease = *lease * multiplier; + +cleanup: + VIR_FREE(leaseString); + VIR_FREE(leaseUnit); + ctxt->node = save; + return ret; +} + + +static int virNetworkDHCPDefParseXML(const char *networkName, xmlNodePtr node, + xmlXPathContextPtr ctxt, virNetworkIPDefPtr def) { int ret = -1; @@ -526,6 +612,11 @@ virNetworkDHCPDefParseXML(const char *networkName, memset(&range, 0, sizeof(range)); memset(&host, 0, sizeof(host));
+ if (virNetworkDHCPLeaseTimeParseXML (node, ctxt, + &def->leasetime, + &def->leasetime_defined)) + goto cleanup; + cur = node->children; while (cur != NULL) { if (cur->type == XML_ELEMENT_NODE && @@ -1104,7 +1195,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)) { diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index e9a5baf5b..466220e81 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -175,7 +175,10 @@ struct _virNetworkIPDef { char *tftproot; char *bootfile; virSocketAddr bootserver; - }; + + uint32_t leasetime; + bool leasetime_defined; +};
typedef struct _virNetworkForwardIfDef virNetworkForwardIfDef; typedef virNetworkForwardIfDef *virNetworkForwardIfDefPtr; diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c1e9471c5..5ef33dcfe 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1561,6 +1561,7 @@ dnsmasqCapsRefresh; dnsmasqContextFree; dnsmasqContextNew; dnsmasqDelete; +dnsmasqDhcpHostsToString; dnsmasqReload; dnsmasqSave;
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 3ba70180b..cdb0230ab 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 @@ -948,6 +950,62 @@ networkKillDaemon(pid_t pid, const char *daemonName, const char *networkName) return ret; }
+static int +networkBuildDnsmasqHostsList(dnsmasqContext *dctx, + virNetworkDNSDefPtr dnsdef) +{ + size_t i, j; + + if (dnsdef) { + for (i = 0; i < dnsdef->nhosts; i++) { + virNetworkDNSHostDefPtr host = &(dnsdef->hosts[i]); + if (VIR_SOCKET_ADDR_VALID(&host->ip)) { + for (j = 0; j < host->nnames; j++) + if (dnsmasqAddHost(dctx, &host->ip, host->names[j]) < 0) + return -1; + } + } + } + + return 0; +} + +/* translates the leasetime value into a dnsmasq configuration string + * for dhcp-range/host */ +static char * +networkDnsmasqConfLeaseValueToString (int64_t leasetime, bool defined) +{ + char *result = NULL; + virBuffer leasebuf = VIR_BUFFER_INITIALIZER; + + /* Leasetime parameter set on the XML */ + + /* defined = FALSE means we fallback to dnsmasq defaults*/ + if (defined == 0) { + virBufferAsprintf(&leasebuf, "%s", ""); + } + /* 0 means no expiration */ + else if (leasetime == 0) { + virBufferAsprintf(&leasebuf, ",infinite"); + } + /* DHCP value for lease time is a unsigned four octect integer */ + else if (leasetime <= UINT32_MAX) { + virBufferAsprintf(&leasebuf, ",%lu", (unsigned long)leasetime); + } + else { + if (leasetime < 120) + virReportError (VIR_ERR_CONFIG_UNSUPPORTED, + _("lease time must be greater than 120 seconds")); + goto cleanup; + } + + result = virBufferContentAndReset(&leasebuf); + +cleanup: + virBufferFreeAndReset (&leasebuf); + return result; +} + /* the following does not build a file, it builds a list * which is later saved into a file */ @@ -956,8 +1014,14 @@ static int networkBuildDnsmasqDhcpHostsList(dnsmasqContext *dctx, virNetworkIPDefPtr ipdef) { + int ret = -1; size_t i; bool ipv6 = false; + char *leasetime = networkDnsmasqConfLeaseValueToString(ipdef->leasetime, + ipdef->leasetime_defined); + + if (!leasetime) + goto cleanup;
if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET6)) ipv6 = true; @@ -965,31 +1029,14 @@ 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) - return -1; - } - - return 0; -} - -static int -networkBuildDnsmasqHostsList(dnsmasqContext *dctx, - virNetworkDNSDefPtr dnsdef) -{ - size_t i, j; - - if (dnsdef) { - for (i = 0; i < dnsdef->nhosts; i++) { - virNetworkDNSHostDefPtr host = &(dnsdef->hosts[i]); - if (VIR_SOCKET_ADDR_VALID(&host->ip)) { - for (j = 0; j < host->nnames; j++) - if (dnsmasqAddHost(dctx, &host->ip, host->names[j]) < 0) - return -1; - } - } + host->name, host->id, leasetime, ipv6) < 0) + goto cleanup; }
- return 0; + ret = 0; +cleanup: + VIR_FREE(leasetime); + return ret; }
@@ -1034,6 +1081,7 @@ int networkDnsmasqConfContents(virNetworkObjPtr network, const char *pidfile, char **configstr, + char **hostsfilestr, dnsmasqContext *dctx, dnsmasqCapsPtr caps ATTRIBUTE_UNUSED) { @@ -1362,6 +1410,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))) @@ -1369,12 +1418,23 @@ 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, + ipdef->leasetime_defined); + 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, @@ -1405,6 +1465,15 @@ networkDnsmasqConfContents(virNetworkObjPtr network, if (networkBuildDnsmasqDhcpHostsList(dctx, ipdef) < 0) goto cleanup;
+ /* Return the contents of the hostsfile if requested */ + if (hostsfilestr) { + *hostsfilestr = dnsmasqDhcpHostsToString (dctx->hostsfile->hosts, + dctx->hostsfile->nhosts); + + if (!hostsfilestr) + goto cleanup; + } + /* Note: the following is IPv4 only */ if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET)) { if (ipdef->nranges || ipdef->nhosts) { @@ -1506,7 +1575,7 @@ networkBuildDhcpDaemonCommandLine(virNetworkDriverStatePtr driver,
network->dnsmasqPid = -1;
- if (networkDnsmasqConfContents(network, pidfile, &configstr, + if (networkDnsmasqConfContents(network, pidfile, &configstr, NULL, dctx, dnsmasq_caps) < 0) goto cleanup; if (!configstr) diff --git a/src/network/bridge_driver.h b/src/network/bridge_driver.h index 7832b6031..56329eb59 100644 --- a/src/network/bridge_driver.h +++ b/src/network/bridge_driver.h @@ -53,6 +53,7 @@ int networkGetActualType(virDomainNetDefPtr iface) int networkDnsmasqConfContents(virNetworkObjPtr network, const char *pidfile, char **configstr, + char **hostsfilestr, dnsmasqContext *dctx, dnsmasqCapsPtr caps);
diff --git a/src/util/virdnsmasq.c b/src/util/virdnsmasq.c index 1b78c1fad..94c9a3bb1 100644 --- a/src/util/virdnsmasq.c +++ b/src/util/virdnsmasq.c @@ -308,52 +308,47 @@ hostsfileAdd(dnsmasqHostsfile *hostsfile, virSocketAddr *ip, const char *name, const char *id, + const char *leasetime, bool ipv6) { + int ret = -1; char *ipstr = NULL; + virBuffer hostbuf = VIR_BUFFER_INITIALIZER; + if (VIR_REALLOC_N(hostsfile->hosts, hostsfile->nhosts + 1) < 0) goto error;
if (!(ipstr = virSocketAddrFormat(ip))) - return -1; + goto error;
/* the first test determines if it is a dhcpv6 host */ if (ipv6) { - if (name && id) { - if (virAsprintf(&hostsfile->hosts[hostsfile->nhosts].host, - "id:%s,%s,[%s]", id, name, ipstr) < 0) - goto error; - } else if (name && !id) { - if (virAsprintf(&hostsfile->hosts[hostsfile->nhosts].host, - "%s,[%s]", name, ipstr) < 0) - goto error; - } else if (!name && id) { - if (virAsprintf(&hostsfile->hosts[hostsfile->nhosts].host, - "id:%s,[%s]", id, ipstr) < 0) - goto error; - } + if (name && id) + virBufferAsprintf(&hostbuf, "id:%s,%s,[%s]", id, name, ipstr); + else if (name && !id) + virBufferAsprintf(&hostbuf, "%s,[%s]", name, ipstr); + else if (!name && id) + virBufferAsprintf(&hostbuf, "id:%s,[%s]", id, ipstr); } else if (name && mac) { - if (virAsprintf(&hostsfile->hosts[hostsfile->nhosts].host, "%s,%s,%s", - mac, ipstr, name) < 0) - goto error; + virBufferAsprintf(&hostbuf, "%s,%s,%s", mac, ipstr, name); } else if (name && !mac) { - if (virAsprintf(&hostsfile->hosts[hostsfile->nhosts].host, "%s,%s", - name, ipstr) < 0) - goto error; + virBufferAsprintf(&hostbuf, "%s,%s", name, ipstr); } else { - if (virAsprintf(&hostsfile->hosts[hostsfile->nhosts].host, "%s,%s", - mac, ipstr) < 0) - goto error; + virBufferAsprintf(&hostbuf, "%s,%s", mac, ipstr); } - VIR_FREE(ipstr);
- hostsfile->nhosts++; + /* The leasetime string already includes comma if there's any value at all */ + virBufferAsprintf(&hostbuf, "%s", leasetime);
- return 0; + if (!(hostsfile->hosts[hostsfile->nhosts].host = virBufferContentAndReset (&hostbuf))) + goto error;
+ hostsfile->nhosts++; + ret = 0; error: + virBufferFreeAndReset(&hostbuf); VIR_FREE(ipstr); - return -1; + return ret; }
static dnsmasqHostsfile * @@ -391,10 +386,9 @@ hostsfileWrite(const char *path, dnsmasqDhcpHost *hosts, unsigned int nhosts) { - char *tmp; + char *tmp, *content = NULL; FILE *f; bool istmp = true; - size_t i; int rc = 0;
/* even if there are 0 hosts, create a 0 length file, to allow @@ -412,17 +406,21 @@ hostsfileWrite(const char *path, } }
- for (i = 0; i < nhosts; i++) { - if (fputs(hosts[i].host, f) == EOF || fputc('\n', f) == EOF) { - rc = -errno; - VIR_FORCE_FCLOSE(f); + if (!(content = dnsmasqDhcpHostsToString(hosts, nhosts))) { + rc = -ENOMEM; + goto cleanup; + }
- if (istmp) - unlink(tmp); + if (fputs(content, f) == EOF) { + rc = -errno; + VIR_FORCE_FCLOSE(f); + + if (istmp) + unlink(tmp); + + goto cleanup; + }
- goto cleanup; - } - }
if (VIR_FCLOSE(f) == EOF) { rc = -errno; @@ -436,6 +434,7 @@ hostsfileWrite(const char *path, }
cleanup: + VIR_FREE(content); VIR_FREE(tmp);
return rc; @@ -524,9 +523,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); }
/* @@ -892,3 +892,31 @@ dnsmasqCapsGet(dnsmasqCapsPtr caps, dnsmasqCapsFlags flag)
return caps && virBitmapIsBitSet(caps->flags, flag); } + +/** dnsmasqDhcpHostsToString: + * + * Turns a vector of dnsmasqDhcpHost into the string that is ought to be + * stored in the hostsfile, this functionality is split to make hostsfiles + * testable. Returs NULL if nhosts is 0. + */ +char * +dnsmasqDhcpHostsToString (dnsmasqDhcpHost *hosts, + unsigned int nhosts) +{ + int i; + char *result = NULL; + virBuffer hostsfilebuf = VIR_BUFFER_INITIALIZER; + + if (nhosts == 0) + goto cleanup; + + for (i = 0; i < nhosts; i++) { + virBufferAsprintf(&hostsfilebuf, "%s\n", hosts[i].host); + } + + result = virBufferContentAndReset(&hostsfilebuf); + +cleanup: + virBufferFreeAndReset(&hostsfilebuf); + return result; +} diff --git a/src/util/virdnsmasq.h b/src/util/virdnsmasq.h index f47bea3ab..1795bc83b 100644 --- a/src/util/virdnsmasq.h +++ b/src/util/virdnsmasq.h @@ -88,6 +88,7 @@ int dnsmasqAddDhcpHost(dnsmasqContext *ctx, virSocketAddr *ip, const char *name, const char *id, + const char *leastime, bool ipv6); int dnsmasqAddHost(dnsmasqContext *ctx, virSocketAddr *ip, @@ -105,6 +106,7 @@ int dnsmasqCapsRefresh(dnsmasqCapsPtr *caps, const char *binaryPath); bool dnsmasqCapsGet(dnsmasqCapsPtr caps, dnsmasqCapsFlags flag); const char *dnsmasqCapsGetBinaryPath(dnsmasqCapsPtr caps); unsigned long dnsmasqCapsGetVersion(dnsmasqCapsPtr caps); +char *dnsmasqDhcpHostsToString(dnsmasqDhcpHost *hosts, unsigned int nhosts);
# define DNSMASQ_DHCPv6_MAJOR_REQD 2 # define DNSMASQ_DHCPv6_MINOR_REQD 64 diff --git a/tests/networkxml2confdata/dhcp6-nat-network.hostsfile b/tests/networkxml2confdata/dhcp6-nat-network.hostsfile new file mode 100644 index 000000000..de659b98c --- /dev/null +++ b/tests/networkxml2confdata/dhcp6-nat-network.hostsfile @@ -0,0 +1,7 @@ +00:16:3e:77:e2:ed,192.168.122.10,a.example.com +00:16:3e:3e:a9:1a,192.168.122.11,b.example.com +id:0:4:7e:7d:f0:7d:a8:bc:c5:d2:13:32:11:ed:16:ea:84:63,[2001:db8:ac10:fd01::1:20] +paul,[2001:db8:ac10:fd01::1:21] +id:0:3:0:1:0:16:3e:11:22:33,peter.xyz,[2001:db8:ac10:fd01::1:22] +id:0:3:0:1:0:16:3e:44:55:33,[2001:db8:ac10:fd01::1:23] +id:0:1:0:1:18:aa:62:fe:0:16:3e:44:55:66,badbob,[2001:db8:ac10:fd01::1:24] diff --git a/tests/networkxml2confdata/dhcp6-network.hostsfile b/tests/networkxml2confdata/dhcp6-network.hostsfile new file mode 100644 index 000000000..9dfb172ce --- /dev/null +++ b/tests/networkxml2confdata/dhcp6-network.hostsfile @@ -0,0 +1,5 @@ +id:0:4:7e:7d:f0:7d:a8:bc:c5:d2:13:32:11:ed:16:ea:84:63,[2001:db8:ac10:fd01::1:20] +paul,[2001:db8:ac10:fd01::1:21] +id:0:3:0:1:0:16:3e:11:22:33,peter.xyz,[2001:db8:ac10:fd01::1:22] +id:0:3:0:1:0:16:3e:44:55:33,[2001:db8:ac10:fd01::1:23] +id:0:1:0:1:18:aa:62:fe:0:16:3e:44:55:66,badbob,[2001:db8:ac10:fd01::1:24] diff --git a/tests/networkxml2confdata/dhcp6host-routed-network.hostsfile b/tests/networkxml2confdata/dhcp6host-routed-network.hostsfile new file mode 100644 index 000000000..de659b98c --- /dev/null +++ b/tests/networkxml2confdata/dhcp6host-routed-network.hostsfile @@ -0,0 +1,7 @@ +00:16:3e:77:e2:ed,192.168.122.10,a.example.com +00:16:3e:3e:a9:1a,192.168.122.11,b.example.com +id:0:4:7e:7d:f0:7d:a8:bc:c5:d2:13:32:11:ed:16:ea:84:63,[2001:db8:ac10:fd01::1:20] +paul,[2001:db8:ac10:fd01::1:21] +id:0:3:0:1:0:16:3e:11:22:33,peter.xyz,[2001:db8:ac10:fd01::1:22] +id:0:3:0:1:0:16:3e:44:55:33,[2001:db8:ac10:fd01::1:23] +id:0:1:0:1:18:aa:62:fe:0:16:3e:44:55:66,badbob,[2001:db8:ac10:fd01::1:24] diff --git a/tests/networkxml2confdata/leasetime-days.conf b/tests/networkxml2confdata/leasetime-days.conf new file mode 100644 index 000000000..d227be67a --- /dev/null +++ b/tests/networkxml2confdata/leasetime-days.conf @@ -0,0 +1,18 @@ +##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,86400 +dhcp-no-override +dhcp-authoritative +dhcp-range=2001:db8:ac10:fd01::1:10,2001:db8:ac10:fd01::1:ff,64,86400 +dhcp-lease-max=493 +dhcp-hostsfile=/var/lib/libvirt/dnsmasq/default.hostsfile +addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts +enable-ra diff --git a/tests/networkxml2confdata/leasetime-days.xml b/tests/networkxml2confdata/leasetime-days.xml new file mode 100644 index 000000000..b990b4d68 --- /dev/null +++ b/tests/networkxml2confdata/leasetime-days.xml @@ -0,0 +1,18 @@ +<network> + <name>default</name> + <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid> + <forward dev='eth1' mode='nat'/> + <bridge name='virbr0' stp='on' delay='0'/> + <ip address='192.168.122.1' netmask='255.255.255.0'> + <dhcp> + <leasetime unit="days">1</leasetime> + <range start='192.168.122.2' end='192.168.122.254'/> + </dhcp> + </ip> + <ip family='ipv6' address='2001:db8:ac10:fd01::1' prefix='64'> + <dhcp> + <leasetime unit="days">1</leasetime> + <range start='2001:db8:ac10:fd01::1:10' end='2001:db8:ac10:fd01::1:ff'/> + </dhcp> + </ip> +</network> diff --git a/tests/networkxml2confdata/leasetime-hours.conf b/tests/networkxml2confdata/leasetime-hours.conf new file mode 100644 index 000000000..e5e8c19cd --- /dev/null +++ b/tests/networkxml2confdata/leasetime-hours.conf @@ -0,0 +1,18 @@ +##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,3600 +dhcp-no-override +dhcp-authoritative +dhcp-range=2001:db8:ac10:fd01::1:10,2001:db8:ac10:fd01::1:ff,64,3600 +dhcp-lease-max=493 +dhcp-hostsfile=/var/lib/libvirt/dnsmasq/default.hostsfile +addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts +enable-ra diff --git a/tests/networkxml2confdata/leasetime-hours.xml b/tests/networkxml2confdata/leasetime-hours.xml new file mode 100644 index 000000000..3b9609601 --- /dev/null +++ b/tests/networkxml2confdata/leasetime-hours.xml @@ -0,0 +1,18 @@ +<network> + <name>default</name> + <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid> + <forward dev='eth1' mode='nat'/> + <bridge name='virbr0' stp='on' delay='0'/> + <ip address='192.168.122.1' netmask='255.255.255.0'> + <dhcp> + <leasetime unit="hours">1</leasetime> + <range start='192.168.122.2' end='192.168.122.254'/> + </dhcp> + </ip> + <ip family='ipv6' address='2001:db8:ac10:fd01::1' prefix='64'> + <dhcp> + <leasetime unit="hours">1</leasetime> + <range start='2001:db8:ac10:fd01::1:10' end='2001:db8:ac10:fd01::1:ff'/> + </dhcp> + </ip> +</network> diff --git a/tests/networkxml2confdata/leasetime-infinite.conf b/tests/networkxml2confdata/leasetime-infinite.conf new file mode 100644 index 000000000..52f4798e5 --- /dev/null +++ b/tests/networkxml2confdata/leasetime-infinite.conf @@ -0,0 +1,18 @@ +##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,infinite +dhcp-no-override +dhcp-authoritative +dhcp-range=2001:db8:ac10:fd01::1:10,2001:db8:ac10:fd01::1:ff,64,infinite +dhcp-lease-max=493 +dhcp-hostsfile=/var/lib/libvirt/dnsmasq/default.hostsfile +addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts +enable-ra diff --git a/tests/networkxml2confdata/leasetime-infinite.xml b/tests/networkxml2confdata/leasetime-infinite.xml new file mode 100644 index 000000000..d855978a1 --- /dev/null +++ b/tests/networkxml2confdata/leasetime-infinite.xml @@ -0,0 +1,18 @@ +<network> + <name>default</name> + <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid> + <forward dev='eth1' mode='nat'/> + <bridge name='virbr0' stp='on' delay='0'/> + <ip address='192.168.122.1' netmask='255.255.255.0'> + <dhcp> + <leasetime>0</leasetime> + <range start='192.168.122.2' end='192.168.122.254'/> + </dhcp> + </ip> + <ip family='ipv6' address='2001:db8:ac10:fd01::1' prefix='64'> + <dhcp> + <leasetime>0</leasetime> + <range start='2001:db8:ac10:fd01::1:10' end='2001:db8:ac10:fd01::1:ff'/> + </dhcp> + </ip> +</network> diff --git a/tests/networkxml2confdata/leasetime-minutes.conf b/tests/networkxml2confdata/leasetime-minutes.conf new file mode 100644 index 000000000..37da5702f --- /dev/null +++ b/tests/networkxml2confdata/leasetime-minutes.conf @@ -0,0 +1,18 @@ +##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,300 +dhcp-no-override +dhcp-authoritative +dhcp-range=2001:db8:ac10:fd01::1:10,2001:db8:ac10:fd01::1:ff,64,300 +dhcp-lease-max=493 +dhcp-hostsfile=/var/lib/libvirt/dnsmasq/default.hostsfile +addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts +enable-ra diff --git a/tests/networkxml2confdata/leasetime-minutes.xml b/tests/networkxml2confdata/leasetime-minutes.xml new file mode 100644 index 000000000..e7a27afe6 --- /dev/null +++ b/tests/networkxml2confdata/leasetime-minutes.xml @@ -0,0 +1,18 @@ +<network> + <name>default</name> + <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid> + <forward dev='eth1' mode='nat'/> + <bridge name='virbr0' stp='on' delay='0'/> + <ip address='192.168.122.1' netmask='255.255.255.0'> + <dhcp> + <leasetime unit="minutes">5</leasetime> + <range start='192.168.122.2' end='192.168.122.254'/> + </dhcp> + </ip> + <ip family='ipv6' address='2001:db8:ac10:fd01::1' prefix='64'> + <dhcp> + <leasetime unit="minutes">5</leasetime> + <range start='2001:db8:ac10:fd01::1:10' end='2001:db8:ac10:fd01::1:ff'/> + </dhcp> + </ip> +</network> diff --git a/tests/networkxml2confdata/leasetime-seconds.conf b/tests/networkxml2confdata/leasetime-seconds.conf new file mode 100644 index 000000000..ea0845e21 --- /dev/null +++ b/tests/networkxml2confdata/leasetime-seconds.conf @@ -0,0 +1,18 @@ +##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,125 +dhcp-no-override +dhcp-authoritative +dhcp-range=2001:db8:ac10:fd01::1:10,2001:db8:ac10:fd01::1:ff,64,125 +dhcp-lease-max=493 +dhcp-hostsfile=/var/lib/libvirt/dnsmasq/default.hostsfile +addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts +enable-ra diff --git a/tests/networkxml2confdata/leasetime-seconds.xml b/tests/networkxml2confdata/leasetime-seconds.xml new file mode 100644 index 000000000..56b07f8ae --- /dev/null +++ b/tests/networkxml2confdata/leasetime-seconds.xml @@ -0,0 +1,18 @@ +<network> + <name>default</name> + <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid> + <forward dev='eth1' mode='nat'/> + <bridge name='virbr0' stp='on' delay='0'/> + <ip address='192.168.122.1' netmask='255.255.255.0'> + <dhcp> + <leasetime unit="seconds">125</leasetime> + <range start='192.168.122.2' end='192.168.122.254'/> + </dhcp> + </ip> + <ip family='ipv6' address='2001:db8:ac10:fd01::1' prefix='64'> + <dhcp> + <leasetime unit="seconds">125</leasetime> + <range start='2001:db8:ac10:fd01::1:10' end='2001:db8:ac10:fd01::1:ff'/> + </dhcp> + </ip> +</network> diff --git a/tests/networkxml2confdata/leasetime.conf b/tests/networkxml2confdata/leasetime.conf new file mode 100644 index 000000000..b754c3ffb --- /dev/null +++ b/tests/networkxml2confdata/leasetime.conf @@ -0,0 +1,18 @@ +##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,122 +dhcp-no-override +dhcp-authoritative +dhcp-range=2001:db8:ac10:fd01::1:10,2001:db8:ac10:fd01::1:ff,64,121 +dhcp-lease-max=493 +dhcp-hostsfile=/var/lib/libvirt/dnsmasq/default.hostsfile +addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts +enable-ra diff --git a/tests/networkxml2confdata/leasetime.xml b/tests/networkxml2confdata/leasetime.xml new file mode 100644 index 000000000..fdbb15fc0 --- /dev/null +++ b/tests/networkxml2confdata/leasetime.xml @@ -0,0 +1,18 @@ +<network> + <name>default</name> + <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid> + <forward dev='eth1' mode='nat'/> + <bridge name='virbr0' stp='on' delay='0'/> + <ip address='192.168.122.1' netmask='255.255.255.0'> + <dhcp> + <leasetime>122</leasetime> + <range start='192.168.122.2' end='192.168.122.254'/> + </dhcp> + </ip> + <ip family='ipv6' address='2001:db8:ac10:fd01::1' prefix='64'> + <dhcp> + <leasetime>121</leasetime> + <range start='2001:db8:ac10:fd01::1:10' end='2001:db8:ac10:fd01::1:ff'/> + </dhcp> + </ip> +</network> diff --git a/tests/networkxml2confdata/nat-network-dns-srv-record-minimal.hostsfile b/tests/networkxml2confdata/nat-network-dns-srv-record-minimal.hostsfile new file mode 100644 index 000000000..deb3f00ac --- /dev/null +++ b/tests/networkxml2confdata/nat-network-dns-srv-record-minimal.hostsfile @@ -0,0 +1,2 @@ +00:16:3e:77:e2:ed,192.168.122.10,a.example.com +00:16:3e:3e:a9:1a,192.168.122.11,b.example.com diff --git a/tests/networkxml2confdata/nat-network-dns-srv-record.hostsfile b/tests/networkxml2confdata/nat-network-dns-srv-record.hostsfile new file mode 100644 index 000000000..deb3f00ac --- /dev/null +++ b/tests/networkxml2confdata/nat-network-dns-srv-record.hostsfile @@ -0,0 +1,2 @@ +00:16:3e:77:e2:ed,192.168.122.10,a.example.com +00:16:3e:3e:a9:1a,192.168.122.11,b.example.com diff --git a/tests/networkxml2confdata/nat-network-dns-txt-record.hostsfile b/tests/networkxml2confdata/nat-network-dns-txt-record.hostsfile new file mode 100644 index 000000000..deb3f00ac --- /dev/null +++ b/tests/networkxml2confdata/nat-network-dns-txt-record.hostsfile @@ -0,0 +1,2 @@ +00:16:3e:77:e2:ed,192.168.122.10,a.example.com +00:16:3e:3e:a9:1a,192.168.122.11,b.example.com diff --git a/tests/networkxml2confdata/nat-network-name-with-quotes.hostsfile b/tests/networkxml2confdata/nat-network-name-with-quotes.hostsfile new file mode 100644 index 000000000..deb3f00ac --- /dev/null +++ b/tests/networkxml2confdata/nat-network-name-with-quotes.hostsfile @@ -0,0 +1,2 @@ +00:16:3e:77:e2:ed,192.168.122.10,a.example.com +00:16:3e:3e:a9:1a,192.168.122.11,b.example.com diff --git a/tests/networkxml2confdata/nat-network.hostsfile b/tests/networkxml2confdata/nat-network.hostsfile new file mode 100644 index 000000000..deb3f00ac --- /dev/null +++ b/tests/networkxml2confdata/nat-network.hostsfile @@ -0,0 +1,2 @@ +00:16:3e:77:e2:ed,192.168.122.10,a.example.com +00:16:3e:3e:a9:1a,192.168.122.11,b.example.com diff --git a/tests/networkxml2confdata/ptr-domains-auto.hostsfile b/tests/networkxml2confdata/ptr-domains-auto.hostsfile new file mode 100644 index 000000000..deb3f00ac --- /dev/null +++ b/tests/networkxml2confdata/ptr-domains-auto.hostsfile @@ -0,0 +1,2 @@ +00:16:3e:77:e2:ed,192.168.122.10,a.example.com +00:16:3e:3e:a9:1a,192.168.122.11,b.example.com diff --git a/tests/networkxml2conftest.c b/tests/networkxml2conftest.c index ab3c13aa0..15f783fb1 100644 --- a/tests/networkxml2conftest.c +++ b/tests/networkxml2conftest.c @@ -19,9 +19,13 @@ #define VIR_FROM_THIS VIR_FROM_NONE
static int -testCompareXMLToConfFiles(const char *inxml, const char *outconf, dnsmasqCapsPtr caps) +testCompareXMLToConfFiles(const char *inxml, + const char *outconf, + const char *outhostsfile, + dnsmasqCapsPtr caps) { - char *actual = NULL; + char *actualconf = NULL; + char *actualhosts = NULL; int ret = -1; virNetworkDefPtr dev = NULL; virNetworkObjPtr obj = NULL; @@ -41,7 +45,11 @@ testCompareXMLToConfFiles(const char *inxml, const char *outconf, dnsmasqCapsPtr if (dctx == NULL) goto fail;
- if (networkDnsmasqConfContents(obj, pidfile, &actual, dctx, caps) < 0) + if (networkDnsmasqConfContents(obj, pidfile, &actualconf, &actualhosts, + dctx, caps) < 0) + goto fail; + + if (virTestCompareToFile(actualconf, outconf) < 0) goto fail;
/* Any changes to this function ^^ should be reflected here too. */ @@ -57,13 +65,27 @@ testCompareXMLToConfFiles(const char *inxml, const char *outconf, dnsmasqCapsPtr tmp = NULL; #endif
- if (virTestCompareToFile(actual, outconf) < 0) + if (virFileExists(outhostsfile)) { + if (!actualhosts) { + fprintf(stderr, + "%s: hostsfile exists but the configuration did not specify any host", + outhostsfile); + goto fail; + } else if (virTestCompareToFile(actualhosts, outhostsfile) < 0) { + goto fail; + } + } else if (actualhosts) { + fprintf(stderr, + "%s: file does not exist but actual data was expected", + outhostsfile); goto fail; + }
ret = 0;
fail: - VIR_FREE(actual); + VIR_FREE(actualconf); + VIR_FREE(actualhosts); VIR_FREE(pidfile); virCommandFree(cmd); virObjectUnref(obj); @@ -83,19 +105,23 @@ testCompareXMLToConfHelper(const void *data) const testInfo *info = data; char *inxml = NULL; char *outconf = NULL; + char *outhostsfile = NULL;
if (virAsprintf(&inxml, "%s/networkxml2confdata/%s.xml", abs_srcdir, info->name) < 0 || virAsprintf(&outconf, "%s/networkxml2confdata/%s.conf", + abs_srcdir, info->name) < 0 || + virAsprintf(&outhostsfile, "%s/networkxml2confdata/%s.hostsfile", abs_srcdir, info->name) < 0) { goto cleanup; }
- result = testCompareXMLToConfFiles(inxml, outconf, info->caps); + result = testCompareXMLToConfFiles(inxml, outconf, outhostsfile, info->caps);
cleanup: VIR_FREE(inxml); VIR_FREE(outconf); + VIR_FREE(outhostsfile);
return result; } @@ -143,6 +169,13 @@ mymain(void) DO_TEST("dhcp6-nat-network", dhcpv6); DO_TEST("dhcp6host-routed-network", dhcpv6); DO_TEST("ptr-domains-auto", dhcpv6); + DO_TEST("leasetime", dhcpv6); + DO_TEST("leasetime-seconds", dhcpv6); + DO_TEST("leasetime-hours", dhcpv6); + DO_TEST("leasetime-minutes", dhcpv6); + DO_TEST("leasetime-hours", dhcpv6); + DO_TEST("leasetime-days", dhcpv6); + DO_TEST("leasetime-infinite", dhcpv6);
virObjectUnref(dhcpv6); virObjectUnref(full); -- 2.13.0
-- Cheers, Alberto Ruiz
Sorry this keeps falling off the radar... When posting a v2/v3/vX, please use [PATCH v2] prefix, and make each new posting a top level patch On 06/22/2017 08:44 PM, aruiz@gnome.org wrote:
From: Alberto Ruiz <aruiz@gnome.org>
Fixes #913446
Full bug link makes things slightly easier if the reviewer wants to look In the commit message [lease at least give an example of the added XML values, possibly what dnsmasq value it maps to. Makes grepping commit logs easier
This patch addresses a few problems found by the initial reviews:
* leaseTimeUnit RNG type renamed to timeUnit * virNetworkDHCPDefGetLeaseTime() renamed to virNetworkDHCPLeaseTimeParseXML() * consistent use of braces in if-else-if * use %lu instead of PRId64 * use 0 as infinite lease * add a leasetime_defined field to struct _virNetworkIPDef to describe whether the value was set in the xml configuration or not * use uint32_t for the leasetime instead of int64_t * fail on all invalid leasetime values * squash all patches into one
These types of bits should go after the --- break below: they don't need to be in the commit message but they are useful for reviewers There's lots of style issues: please run 'make syntax-check' and fix the warnings. Peter pointed out some of them already against your 6/21 posting but they weren't addressed in this version: https://www.redhat.com/archives/libvir-list/2017-June/msg00838.html Drop the strcmp usage, we have a STREQ macro we use ('make syntax-check' might warn but I'm not positive) CC me on the v3 and I'll do a full review Thanks, Cole
participants (5)
-
Alberto Ruiz -
aruiz＠gnome.org
-
Cole Robinson -
Laine Stump -
Peter Krempa