[PATCH 00/10] Fix NSS plugin and net-dhcp-get-leases wrt to infinite leases
Some things are broken when using leases that don't expire. We don't store "expiry-time" in corresponding $brname.status file which sets off a spiral and we get errors from other places which expect it to be there always. These patches make sure that the attribute is always there. I've also implemented another approach, which puts "expiry-time" into the file only if not infinite and fixed the other places which expect it: https://gitlab.com/MichalPrivoznik/libvirt/-/commits/leases_docs/ but I like this version more. Michal Prívozník (10): docs: Document ability to configure lease time leaseshelper: Report errors on failure virlease: Rework virLeaseReadCustomLeaseFile() virlease: Use virTrimSpaces() instead of open coded alternative virlease: Allow infinite lease expiry time network: Drop @custom_lease_file_len variable from networkGetDHCPLeases() networkGetDHCPLeases: Use VIR_APPEND_ELEMENT() instead of VIR_INSERT_ELEMENT() network: Rework networkGetDHCPLeases() networkGetDHCPLeases: Handle leases with infinite expiry time nss: handle leases with infinite expiry time docs/formatnetwork.html.in | 21 ++++++++- src/network/bridge_driver.c | 79 +++++++++++++++++----------------- src/network/leaseshelper.c | 2 + src/util/virlease.c | 33 +++++++------- tests/nssdata/virbr0.status | 7 +++ tests/nsstest.c | 2 +- tools/nss/libvirt_nss_leases.c | 4 +- 7 files changed, 87 insertions(+), 61 deletions(-) -- 2.26.2
In v6.3.0-rc1~64 we've introduced ability to configure lease time, but forgot to document the feature. Let's fix that. Fixes: 97a0aa246799c97d0a9ca9ecd6b4fd932ae4756c Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1908631 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/formatnetwork.html.in | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in index f26909bec8..f5a48d9b92 100644 --- a/docs/formatnetwork.html.in +++ b/docs/formatnetwork.html.in @@ -930,8 +930,12 @@ </dns> <ip address="192.168.122.1" netmask="255.255.255.0" localPtr="yes"> <dhcp> - <range start="192.168.122.100" end="192.168.122.254"/> - <host mac="00:16:3e:77:e2:ed" name="foo.example.com" ip="192.168.122.10"/> + <range start="192.168.122.100" end="192.168.122.254"> + <lease expiry='1' unit='hours'/> + </range> + <host mac="00:16:3e:77:e2:ed" name="foo.example.com" ip="192.168.122.10"> + <lease expiry='30' unit='minutes'/> + </host> <host mac="00:16:3e:3e:a9:1a" name="bar.example.com" ip="192.168.122.11"/> </dhcp> </ip> @@ -1130,6 +1134,19 @@ <span class="since">since 0.7.3</span>) </dd> </dl> + + <p> + Optionally, <code>range</code> and <code>host</code> elements can + have <code>lease</code> child element which specifies the lease + time through it's attributes <code>expiry</code> and + <code>unit</code> (which accepts <code>seconds</code>, + <code>minutes</code> and <code>hours</code> and defaults to + <code>minutes</code> if omitted). The minimal lease time is 2 + minutes, except when setting an infinite lease time + (<code>expiry='0'</code>). + <span class="since">Since 6.3.0</span> + </p> + </dd> </dl> </dd> -- 2.26.2
If leasehelper fails all that we are left with is a simple error message produced by dnsmasq: lease-init script returned exit code 1 This is because the leasehelper did not write any message to stderr. According to dnsmasq's manpage, whenever it's invoking leasehelper the stderr is kept open: All file descriptors are closed except stdin, which is open to /dev/null, and stdout and stderr which capture output for logging by dnsmasq. As debugging leasehelper is not trivial (because dnsmasq invokes it with plenty of env vars set - that's how data is passed onto helper), let's print an error into stderr if exiting with an error. And since we are not calling public APIs, we have to call virDispatchError() explicitly and since we don't have any connection open, we have to pass NULL. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/network/leaseshelper.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/network/leaseshelper.c b/src/network/leaseshelper.c index 732dd09610..c20e63efa9 100644 --- a/src/network/leaseshelper.c +++ b/src/network/leaseshelper.c @@ -253,6 +253,8 @@ main(int argc, char **argv) rv = EXIT_SUCCESS; cleanup: + if (rv != EXIT_SUCCESS) + virDispatchError(NULL); if (pid_file_fd != -1) virPidFileReleasePath(pid_file, pid_file_fd); -- 2.26.2
There are some variables which are used only inside the single loop the function has. Let's declare them inside the loop body to make that obvious. Also, fix indendation. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virlease.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/util/virlease.c b/src/util/virlease.c index aeb605862f..f01cf5b9bd 100644 --- a/src/util/virlease.c +++ b/src/util/virlease.c @@ -50,11 +50,7 @@ virLeaseReadCustomLeaseFile(virJSONValuePtr leases_array_new, { g_autofree char *lease_entries = NULL; g_autoptr(virJSONValue) leases_array = NULL; - long long expirytime; int custom_lease_file_len = 0; - virJSONValuePtr lease_tmp = NULL; - const char *ip_tmp = NULL; - const char *server_duid_tmp = NULL; size_t i; /* Read entire contents */ @@ -83,7 +79,11 @@ virLeaseReadCustomLeaseFile(virJSONValuePtr leases_array_new, i = 0; while (i < virJSONValueArraySize(leases_array)) { - if (!(lease_tmp = virJSONValueArrayGet(leases_array, i))) { + virJSONValuePtr lease_tmp = virJSONValueArrayGet(leases_array, i); + long long expirytime; + const char *ip_tmp = NULL; + + if (!lease_tmp) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to parse json")); return -1; @@ -103,9 +103,10 @@ virLeaseReadCustomLeaseFile(virJSONValuePtr leases_array_new, } if (server_duid && strchr(ip_tmp, ':')) { + const char *server_duid_tmp = NULL; + /* This is an ipv6 lease */ - if ((server_duid_tmp - = virJSONValueObjectGetString(lease_tmp, "server-duid"))) { + if ((server_duid_tmp = virJSONValueObjectGetString(lease_tmp, "server-duid"))) { if (!*server_duid) *server_duid = g_strdup(server_duid_tmp); } else { -- 2.26.2
In virLeaseNew() we are trying to remove trailing space (per comment it may happen that older versions of dnsmasq put it into an env variable). Well, instead of open coding it, we can use virTrimSpaces(). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virlease.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/util/virlease.c b/src/util/virlease.c index f01cf5b9bd..edfdc6477a 100644 --- a/src/util/virlease.c +++ b/src/util/virlease.c @@ -226,8 +226,7 @@ virLeaseNew(virJSONValuePtr *lease_ret, /* Removed extraneous trailing space in DNSMASQ_LEASE_EXPIRES * (dnsmasq < 2.52) */ - if (exptime[strlen(exptime) - 1] == ' ') - exptime[strlen(exptime) - 1] = '\0'; + virTrimSpaces(exptime, NULL); } if (!exptime || -- 2.26.2
When adding a new lease by our leaseshelper then virLeaseNew() is called. Here, we check for DNSMASQ_LEASE_EXPIRES environment variable which is the expiration time for the lease. For infinite lease time the value is zero. However, our code is not prepared for that and adds "expiry-time" into the JSON file only if lease expiry time is non-zero. This breaks the assumption that the "expiry-time" attribute is always present (as can be seen in virLeaseReadCustomLeaseFile() and virLeasePrintLeases()). Store "expiry-time" always. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virlease.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/src/util/virlease.c b/src/util/virlease.c index edfdc6477a..3d68bb660c 100644 --- a/src/util/virlease.c +++ b/src/util/virlease.c @@ -227,14 +227,13 @@ virLeaseNew(virJSONValuePtr *lease_ret, /* Removed extraneous trailing space in DNSMASQ_LEASE_EXPIRES * (dnsmasq < 2.52) */ virTrimSpaces(exptime, NULL); - } - if (!exptime || - virStrToLong_ll(exptime, NULL, 10, &expirytime) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unable to convert lease expiry time to long long: %s"), - NULLSTR(exptime)); - return -1; + if (virStrToLong_ll(exptime, NULL, 10, &expirytime) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to convert lease expiry time to long long: %s"), + NULLSTR(exptime)); + return -1; + } } /* Create new lease */ @@ -252,7 +251,7 @@ virLeaseNew(virJSONValuePtr *lease_ret, return -1; if (server_duid && virJSONValueObjectAppendString(lease_new, "server-duid", server_duid) < 0) return -1; - if (expirytime && virJSONValueObjectAppendNumberLong(lease_new, "expiry-time", expirytime) < 0) + if (virJSONValueObjectAppendNumberLong(lease_new, "expiry-time", expirytime) < 0) return -1; *lease_ret = lease_new; -- 2.26.2
We don't need to track the lease file size. Instead, we can simply check if the file was empty by comparing the buffer the file was read into with an empty string. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/network/bridge_driver.c | 34 ++++++++++++++++++---------------- 1 file changed, 18 insertions(+), 16 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index fdad2191e6..191e429ea2 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -4035,7 +4035,6 @@ networkGetDHCPLeases(virNetworkPtr net, size_t nleases = 0; int rv = -1; size_t size = 0; - int custom_lease_file_len = 0; bool need_results = !!leases; long long currtime = 0; long long expirytime_tmp = -1; @@ -4071,9 +4070,9 @@ networkGetDHCPLeases(virNetworkPtr net, custom_lease_file = networkDnsmasqLeaseFileNameCustom(driver, def->bridge); /* Read entire contents */ - if ((custom_lease_file_len = virFileReadAllQuiet(custom_lease_file, - VIR_NETWORK_DHCP_LEASE_FILE_SIZE_MAX, - &lease_entries)) < 0) { + if (virFileReadAllQuiet(custom_lease_file, + VIR_NETWORK_DHCP_LEASE_FILE_SIZE_MAX, + &lease_entries) < 0) { /* Not all networks are guaranteed to have leases file. * Only those which run dnsmasq. Therefore, if we failed * to read the leases file, don't report error. Return 0 @@ -4088,20 +4087,23 @@ networkGetDHCPLeases(virNetworkPtr net, goto error; } - if (custom_lease_file_len) { - if (!(leases_array = virJSONValueFromString(lease_entries))) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("invalid json in file: %s"), custom_lease_file); - goto error; - } + if (STREQ(lease_entries, "")) { + rv = 0; + goto error; + } + + if (!(leases_array = virJSONValueFromString(lease_entries))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("invalid json in file: %s"), custom_lease_file); + goto error; + } - if (!virJSONValueIsArray(leases_array)) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Malformed lease_entries array")); - goto error; - } - size = virJSONValueArraySize(leases_array); + if (!virJSONValueIsArray(leases_array)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Malformed lease_entries array")); + goto error; } + size = virJSONValueArraySize(leases_array); currtime = (long long)time(NULL); -- 2.26.2
This function is misusing VIR_INSERT_ELEMENT() to behave like VIR_APPEND_ELEMENT(). Use the latter to make it explicit what we are trying to achieve. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/network/bridge_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 191e429ea2..43102a02c1 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -4179,7 +4179,7 @@ networkGetDHCPLeases(virNetworkPtr net, lease->clientid = g_strdup(virJSONValueObjectGetString(lease_tmp, "client-id")); lease->hostname = g_strdup(virJSONValueObjectGetString(lease_tmp, "hostname")); - if (VIR_INSERT_ELEMENT(leases_ret, nleases, nleases, lease) < 0) + if (VIR_APPEND_ELEMENT(leases_ret, nleases, lease) < 0) goto error; } else { -- 2.26.2
Firstly, bring variables that are used only within loops into their respective loops. Secondly, drop 'error' label which is redundant since we have @rv which holds the return value. Thirdly, fix indendation in one case, the rest is indented properly. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/network/bridge_driver.c | 47 +++++++++++++++++-------------------- 1 file changed, 22 insertions(+), 25 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 43102a02c1..fcc7e05ad8 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -4031,22 +4031,16 @@ networkGetDHCPLeases(virNetworkPtr net, unsigned int flags) { virNetworkDriverStatePtr driver = networkGetDriver(); - size_t i, j; + size_t i; size_t nleases = 0; int rv = -1; size_t size = 0; bool need_results = !!leases; long long currtime = 0; - long long expirytime_tmp = -1; - bool ipv6 = false; g_autofree char *lease_entries = NULL; g_autofree char *custom_lease_file = NULL; - const char *ip_tmp = NULL; - const char *mac_tmp = NULL; - virJSONValuePtr lease_tmp = NULL; g_autoptr(virJSONValue) leases_array = NULL; - virNetworkIPDefPtr ipdef_tmp = NULL; - virNetworkDHCPLeasePtr *leases_ret = NULL; + g_autofree virNetworkDHCPLeasePtr *leases_ret = NULL; virNetworkObjPtr obj; virNetworkDefPtr def; virMacAddr mac_addr; @@ -4084,34 +4078,38 @@ networkGetDHCPLeases(virNetworkPtr net, _("Unable to read leases file: %s"), custom_lease_file); } - goto error; + goto cleanup; } if (STREQ(lease_entries, "")) { rv = 0; - goto error; + goto cleanup; } if (!(leases_array = virJSONValueFromString(lease_entries))) { virReportError(VIR_ERR_INTERNAL_ERROR, _("invalid json in file: %s"), custom_lease_file); - goto error; + goto cleanup; } if (!virJSONValueIsArray(leases_array)) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Malformed lease_entries array")); - goto error; + goto cleanup; } size = virJSONValueArraySize(leases_array); currtime = (long long)time(NULL); for (i = 0; i < size; i++) { - if (!(lease_tmp = virJSONValueArrayGet(leases_array, i))) { + virJSONValuePtr lease_tmp = virJSONValueArrayGet(leases_array, i); + long long expirytime_tmp = -1; + const char *mac_tmp = NULL; + + if (!lease_tmp) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to parse json")); - goto error; + goto cleanup; } if (!(mac_tmp = virJSONValueObjectGetString(lease_tmp, "mac-address"))) { @@ -4119,7 +4117,7 @@ networkGetDHCPLeases(virNetworkPtr net, * mac-address is known otherwise not */ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("found lease without mac-address")); - goto error; + goto cleanup; } if (mac && virMacAddrCompare(mac, mac_tmp)) @@ -4129,7 +4127,7 @@ networkGetDHCPLeases(virNetworkPtr net, /* A lease cannot be present without expiry-time */ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("found lease without expiry-time")); - goto error; + goto cleanup; } /* Do not report expired lease */ @@ -4138,6 +4136,9 @@ networkGetDHCPLeases(virNetworkPtr net, if (need_results) { g_autoptr(virNetworkDHCPLease) lease = g_new0(virNetworkDHCPLease, 1); + const char *ip_tmp = NULL; + bool ipv6 = false; + size_t j; lease->expirytime = expirytime_tmp; @@ -4145,7 +4146,7 @@ networkGetDHCPLeases(virNetworkPtr net, /* A lease without ip-address makes no sense */ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("found lease without ip-address")); - goto error; + goto cleanup; } /* Unlike IPv4, IPv6 uses ':' instead of '.' as separator */ @@ -4154,7 +4155,7 @@ networkGetDHCPLeases(virNetworkPtr net, /* Obtain prefix */ for (j = 0; j < def->nips; j++) { - ipdef_tmp = &def->ips[j]; + virNetworkIPDefPtr ipdef_tmp = ipdef_tmp = &def->ips[j]; if (ipv6 && VIR_SOCKET_ADDR_IS_FAMILY(&ipdef_tmp->address, AF_INET6)) { @@ -4162,7 +4163,7 @@ networkGetDHCPLeases(virNetworkPtr net, break; } if (!ipv6 && VIR_SOCKET_ADDR_IS_FAMILY(&ipdef_tmp->address, - AF_INET)) { + AF_INET)) { lease->prefix = virSocketAddrGetIPPrefix(&ipdef_tmp->address, &ipdef_tmp->netmask, ipdef_tmp->prefix); @@ -4180,7 +4181,7 @@ networkGetDHCPLeases(virNetworkPtr net, lease->hostname = g_strdup(virJSONValueObjectGetString(lease_tmp, "hostname")); if (VIR_APPEND_ELEMENT(leases_ret, nleases, lease) < 0) - goto error; + goto cleanup; } else { nleases++; @@ -4197,15 +4198,11 @@ networkGetDHCPLeases(virNetworkPtr net, cleanup: virNetworkObjEndAPI(&obj); - return rv; - - error: if (leases_ret) { for (i = 0; i < nleases; i++) virNetworkDHCPLeaseFree(leases_ret[i]); - g_free(leases_ret); } - goto cleanup; + return rv; } -- 2.26.2
On 12/18/20 10:09 AM, Michal Privoznik wrote:
Firstly, bring variables that are used only within loops into their respective loops. Secondly, drop 'error' label which is redundant since we have @rv which holds the return value. Thirdly, fix indendation in one case, the rest is indented properly.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/network/bridge_driver.c | 47 +++++++++++++++++-------------------- 1 file changed, 22 insertions(+), 25 deletions(-)
[...]
@@ -4145,7 +4146,7 @@ networkGetDHCPLeases(virNetworkPtr net, /* A lease without ip-address makes no sense */ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("found lease without ip-address")); - goto error; + goto cleanup; }
/* Unlike IPv4, IPv6 uses ':' instead of '.' as separator */ @@ -4154,7 +4155,7 @@ networkGetDHCPLeases(virNetworkPtr net,
/* Obtain prefix */ for (j = 0; j < def->nips; j++) { - ipdef_tmp = &def->ips[j]; + virNetworkIPDefPtr ipdef_tmp = ipdef_tmp = &def->ips[j];
Coverity notes... Department of redundancy department Simple/trivial fix for someone I'm sure John
if (ipv6 && VIR_SOCKET_ADDR_IS_FAMILY(&ipdef_tmp->address, AF_INET6)) { @@ -4162,7 +4163,7 @@ networkGetDHCPLeases(virNetworkPtr net,
[...]
On 1/5/21 12:49 PM, John Ferlan wrote:
On 12/18/20 10:09 AM, Michal Privoznik wrote:
Firstly, bring variables that are used only within loops into their respective loops. Secondly, drop 'error' label which is redundant since we have @rv which holds the return value. Thirdly, fix indendation in one case, the rest is indented properly.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/network/bridge_driver.c | 47 +++++++++++++++++-------------------- 1 file changed, 22 insertions(+), 25 deletions(-)
[...]
@@ -4145,7 +4146,7 @@ networkGetDHCPLeases(virNetworkPtr net, /* A lease without ip-address makes no sense */ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("found lease without ip-address")); - goto error; + goto cleanup; }
/* Unlike IPv4, IPv6 uses ':' instead of '.' as separator */ @@ -4154,7 +4155,7 @@ networkGetDHCPLeases(virNetworkPtr net,
/* Obtain prefix */ for (j = 0; j < def->nips; j++) { - ipdef_tmp = &def->ips[j]; + virNetworkIPDefPtr ipdef_tmp = ipdef_tmp = &def->ips[j];
Coverity notes... Department of redundancy department
Simple/trivial fix for someone I'm sure
Huh, how I managed to write this? :-) I'll push the fix as trivial shortly. Thanks for noticing. Michal
After v6.3.0-rc1~64 a lease can have infinite expiry time. This means that the expiration time will appear as a value of zero. Do the expiration check only if the expiration time is not zero. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1908053 Fixes: 97a0aa246799c97d0a9ca9ecd6b4fd932ae4756c Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/network/bridge_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index fcc7e05ad8..22d7d330a3 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -4131,7 +4131,7 @@ networkGetDHCPLeases(virNetworkPtr net, } /* Do not report expired lease */ - if (expirytime_tmp < currtime) + if (expirytime_tmp > 0 && expirytime_tmp < currtime) continue; if (need_results) { -- 2.26.2
After v6.3.0-rc1~64 a lease can have infinite expiry time. This means that the expiration time will appear as a value of zero. Do the expiration check only if the expiration time is not zero. Fixes: 97a0aa246799c97d0a9ca9ecd6b4fd932ae4756c Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/nssdata/virbr0.status | 7 +++++++ tests/nsstest.c | 2 +- tools/nss/libvirt_nss_leases.c | 4 +++- 3 files changed, 11 insertions(+), 2 deletions(-) diff --git a/tests/nssdata/virbr0.status b/tests/nssdata/virbr0.status index 78afaf6200..354715bb1c 100644 --- a/tests/nssdata/virbr0.status +++ b/tests/nssdata/virbr0.status @@ -21,5 +21,12 @@ "ip-address": "192.168.122.2", "mac-address": "52:54:00:11:22:33", "expiry-time": 2000000000 + }, + { + "ip-address": "192.168.122.3", + "mac-address": "52:54:00:a4:6f:91", + "hostname": "fedora", + "client-id": "01:52:54:00:a4:6f:91", + "expiry-time": 0 } ] diff --git a/tests/nsstest.c b/tests/nsstest.c index 135f6b6c93..a4e8a4a37f 100644 --- a/tests/nsstest.c +++ b/tests/nsstest.c @@ -173,7 +173,7 @@ mymain(void) } while (0) # if !defined(LIBVIRT_NSS_GUEST) - DO_TEST("fedora", AF_INET, "192.168.122.197", "192.168.122.198", "192.168.122.199"); + DO_TEST("fedora", AF_INET, "192.168.122.197", "192.168.122.198", "192.168.122.199", "192.168.122.3"); DO_TEST("gentoo", AF_INET, "192.168.122.254"); DO_TEST("gentoo", AF_INET6, "2001:1234:dead:beef::2"); DO_TEST("gentoo", AF_UNSPEC, "192.168.122.254"); diff --git a/tools/nss/libvirt_nss_leases.c b/tools/nss/libvirt_nss_leases.c index 015bbc4ab6..536fcf8608 100644 --- a/tools/nss/libvirt_nss_leases.c +++ b/tools/nss/libvirt_nss_leases.c @@ -276,7 +276,8 @@ findLeasesParserEndMap(void *ctx) found = true; } DEBUG("Found %d", found); - if (parser->entry.expiry < parser->now) { + if (parser->entry.expiry != 0 && + parser->entry.expiry < parser->now) { DEBUG("Entry expired at %llu vs now %llu", parser->entry.expiry, parser->now); found = false; @@ -298,6 +299,7 @@ findLeasesParserEndMap(void *ctx) free(parser->entry.macaddr); free(parser->entry.ipaddr); free(parser->entry.hostname); + parser->entry.expiry = 0; parser->entry.macaddr = NULL; parser->entry.ipaddr = NULL; parser->entry.hostname = NULL; -- 2.26.2
On Fri, Dec 18, 2020 at 04:09:06PM +0100, Michal Privoznik wrote:
Some things are broken when using leases that don't expire. We don't store "expiry-time" in corresponding $brname.status file which sets off a spiral and we get errors from other places which expect it to be there always. These patches make sure that the attribute is always there. I've also implemented another approach, which puts "expiry-time" into the file only if not infinite and fixed the other places which expect it:
https://gitlab.com/MichalPrivoznik/libvirt/-/commits/leases_docs/
but I like this version more.
Michal Prívozník (10): docs: Document ability to configure lease time leaseshelper: Report errors on failure virlease: Rework virLeaseReadCustomLeaseFile() virlease: Use virTrimSpaces() instead of open coded alternative virlease: Allow infinite lease expiry time network: Drop @custom_lease_file_len variable from networkGetDHCPLeases() networkGetDHCPLeases: Use VIR_APPEND_ELEMENT() instead of VIR_INSERT_ELEMENT() network: Rework networkGetDHCPLeases() networkGetDHCPLeases: Handle leases with infinite expiry time nss: handle leases with infinite expiry time
docs/formatnetwork.html.in | 21 ++++++++- src/network/bridge_driver.c | 79 +++++++++++++++++----------------- src/network/leaseshelper.c | 2 + src/util/virlease.c | 33 +++++++------- tests/nssdata/virbr0.status | 7 +++ tests/nsstest.c | 2 +- tools/nss/libvirt_nss_leases.c | 4 +- 7 files changed, 87 insertions(+), 61 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
participants (3)
-
Daniel P. Berrangé -
John Ferlan -
Michal Privoznik