[libvirt] [PATCH] networkGetDHCPLeases: Don't always report error if unable to read leases file
https://bugzilla.redhat.com/show_bug.cgi?id=1600468 If we are unable to read leases file (no matter what the reason is), we return 0 - just like if there were no leases. However, because we use virFileReadAll() an error is printed into the log. Note that not all networks have leases file - only those for which we start dnsmasq. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/network/bridge_driver.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index a7826e9b63..5839021caf 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -4235,13 +4235,20 @@ networkGetDHCPLeases(virNetworkPtr net, custom_lease_file = networkDnsmasqLeaseFileNameCustom(driver, def->bridge); /* Read entire contents */ - if ((custom_lease_file_len = virFileReadAll(custom_lease_file, - VIR_NETWORK_DHCP_LEASE_FILE_SIZE_MAX, - &lease_entries)) < 0) { - /* Even though src/network/leaseshelper.c guarantees the existence of - * leases file (even if no leases are present), and the control reaches - * here, instead of reporting error, return 0 leases */ - rv = 0; + if ((custom_lease_file_len = 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 + * leases instead. */ + if (errno != ENOENT) { + virReportSystemError(errno, + _("Unable to read leases file: %s"), + custom_lease_file); + } else { + rv = 0; + } goto error; } -- 2.16.4
On Thu, Jul 26, 2018 at 09:55:17AM +0200, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1600468
If we are unable to read leases file (no matter what the reason is), we return 0 - just like if there were no leases. However, because we use virFileReadAll() an error is printed into the log. Note that not all networks have leases file - only those for which we start dnsmasq.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/network/bridge_driver.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index a7826e9b63..5839021caf 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -4235,13 +4235,20 @@ networkGetDHCPLeases(virNetworkPtr net, custom_lease_file = networkDnsmasqLeaseFileNameCustom(driver, def->bridge);
/* Read entire contents */ - if ((custom_lease_file_len = virFileReadAll(custom_lease_file, - VIR_NETWORK_DHCP_LEASE_FILE_SIZE_MAX, - &lease_entries)) < 0) { - /* Even though src/network/leaseshelper.c guarantees the existence of - * leases file (even if no leases are present), and the control reaches - * here, instead of reporting error, return 0 leases */ - rv = 0; + if ((custom_lease_file_len = 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 + * leases instead. */ + if (errno != ENOENT) { + virReportSystemError(errno, + _("Unable to read leases file: %s"), + custom_lease_file);
^This is essentially a 'single-line' body, so the curly braces could be dropped. I'd also invert the condition, so as to comply with the guidelines and make the 'else' clause, in this case only optically, bigger. Regardless: Reviewed-by: Erik Skultety <eskultet@redhat.com>
+ } else { + rv = 0; + } goto error; }
-- 2.16.4
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 07/26/2018 10:55 AM, Erik Skultety wrote:
On Thu, Jul 26, 2018 at 09:55:17AM +0200, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1600468
If we are unable to read leases file (no matter what the reason is), we return 0 - just like if there were no leases. However, because we use virFileReadAll() an error is printed into the log. Note that not all networks have leases file - only those for which we start dnsmasq.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/network/bridge_driver.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index a7826e9b63..5839021caf 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -4235,13 +4235,20 @@ networkGetDHCPLeases(virNetworkPtr net, custom_lease_file = networkDnsmasqLeaseFileNameCustom(driver, def->bridge);
/* Read entire contents */ - if ((custom_lease_file_len = virFileReadAll(custom_lease_file, - VIR_NETWORK_DHCP_LEASE_FILE_SIZE_MAX, - &lease_entries)) < 0) { - /* Even though src/network/leaseshelper.c guarantees the existence of - * leases file (even if no leases are present), and the control reaches - * here, instead of reporting error, return 0 leases */ - rv = 0; + if ((custom_lease_file_len = 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 + * leases instead. */ + if (errno != ENOENT) { + virReportSystemError(errno, + _("Unable to read leases file: %s"), + custom_lease_file);
^This is essentially a 'single-line' body, so the curly braces could be dropped.
While essentially it is one line, in fact it's split into three so I rather keep the brackets, if you don't mind.
I'd also invert the condition, so as to comply with the guidelines and make the 'else' clause, in this case only optically, bigger.
Oh, for some reason I though that the rule is reversed. Huh, whatever.
Regardless: Reviewed-by: Erik Skultety <eskultet@redhat.com>
Pushed, thanks. Michal
participants (2)
-
Erik Skultety -
Michal Privoznik