9:09 a.m.
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
9:09 a.m.
New subject: [PATCH 01/10] docs: Document ability to configure lease time
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(a)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
9:09 a.m.
New subject: [PATCH 02/10] leaseshelper: Report errors on failure
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(a)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
9:09 a.m.
New subject: [PATCH 03/10] virlease: Rework virLeaseReadCustomLeaseFile()
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(a)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
9:09 a.m.
New subject: [PATCH 04/10] virlease: Use virTrimSpaces() instead of open coded alternative
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(a)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
9:09 a.m.
New subject: [PATCH 05/10] virlease: Allow infinite lease expiry time
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(a)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
9:09 a.m.
New subject: [PATCH 06/10] network: Drop @custom_lease_file_len variable from networkGetDHCPLeases()
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(a)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
9:09 a.m.
New subject: [PATCH 07/10] networkGetDHCPLeases: Use VIR_APPEND_ELEMENT() instead of VIR_INSERT_ELEMENT()
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(a)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
9:09 a.m.
New subject: [PATCH 08/10] network: Rework networkGetDHCPLeases()
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(a)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
5:49 a.m.
New subject: [PATCH 08/10] network: Rework networkGetDHCPLeases()
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(a)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,
[...]
5:54 a.m.
New subject: [PATCH 08/10] network: Rework networkGetDHCPLeases()
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(a)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
9:09 a.m.
New subject: [PATCH 09/10] networkGetDHCPLeases: Handle leases with infinite expiry time
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(a)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
9:09 a.m.
New subject: [PATCH 10/10] nss: handle leases with infinite expiry time
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(a)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
7:28 a.m.
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(a)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 :|
1452
days inactive
1470
days old
13 comments
3 participants
participants (3)
-
Daniel P. Berrangé -
John Ferlan -
Michal Privoznik