[libvirt] [PATCH 0/4] Report less errors
Some are distracting and not really helpful. Ján Tomko (4): Track privileged state in udev nodedev driver Only detect PCI Express devices as root in udev nodedev driver Introduce virFileReadAllQuiet Report one error less when getting net dev speed src/libvirt_private.syms | 1 + src/node_device/node_device_udev.c | 10 ++++++++-- src/util/virfile.c | 15 +++++++++++++++ src/util/virfile.h | 2 ++ src/util/virnetdev.c | 7 ++++--- 5 files changed, 30 insertions(+), 5 deletions(-) -- 1.8.5.5
Remember if libvirtd is running as root or not. --- src/node_device/node_device_udev.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index bb6a0b9..50bb952 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -53,6 +53,7 @@ VIR_LOG_INIT("node_device.node_device_udev"); struct _udevPrivate { struct udev_monitor *udev_monitor; int watch; + bool privileged; }; static virNodeDeviceDriverStatePtr driverState = NULL; @@ -1712,7 +1713,7 @@ static int udevSetupSystemDev(void) return ret; } -static int nodeStateInitialize(bool privileged ATTRIBUTE_UNUSED, +static int nodeStateInitialize(bool privileged, virStateInhibitCallback callback ATTRIBUTE_UNUSED, void *opaque ATTRIBUTE_UNUSED) { @@ -1746,6 +1747,7 @@ static int nodeStateInitialize(bool privileged ATTRIBUTE_UNUSED, } priv->watch = -1; + priv->privileged = privileged; if (VIR_ALLOC(driverState) < 0) { VIR_FREE(priv); -- 1.8.5.5
This stops the error message spam when running unprivileged libvirtd: 2014-06-30 12:38:47.990+0000: 631: error : virPCIDeviceConfigOpen:300 : Failed to open config space file '/sys/bus/pci/devices/0000:00:00.0/config': Permission denied Reported by Daniel Berrange: https://www.redhat.com/archives/libvir-list/2014-June/msg01082.html --- src/node_device/node_device_udev.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 50bb952..fe3dd26 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -429,10 +429,13 @@ static int udevProcessPCI(struct udev_device *device, virPCIDeviceAddress addr; virPCIEDeviceInfoPtr pci_express = NULL; virPCIDevicePtr pciDev = NULL; + udevPrivate *priv = NULL; int tmpGroup, ret = -1; char *p; int rc; + priv = driverState->privateData; + syspath = udev_device_get_syspath(device); if (udevGetUintProperty(device, @@ -544,7 +547,8 @@ static int udevProcessPCI(struct udev_device *device, data->pci_dev.function))) goto out; - if (virPCIDeviceIsPCIExpress(pciDev) > 0) { + /* We need to be root to read PCI device configs */ + if (priv->privileged && virPCIDeviceIsPCIExpress(pciDev) > 0) { if (VIR_ALLOC(pci_express) < 0) goto out; -- 1.8.5.5
On 30.06.2014 15:38, Ján Tomko wrote:
This stops the error message spam when running unprivileged libvirtd: 2014-06-30 12:38:47.990+0000: 631: error : virPCIDeviceConfigOpen:300 : Failed to open config space file '/sys/bus/pci/devices/0000:00:00.0/config': Permission denied
Reported by Daniel Berrange: https://www.redhat.com/archives/libvir-list/2014-June/msg01082.html --- src/node_device/node_device_udev.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 50bb952..fe3dd26 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -429,10 +429,13 @@ static int udevProcessPCI(struct udev_device *device, virPCIDeviceAddress addr; virPCIEDeviceInfoPtr pci_express = NULL; virPCIDevicePtr pciDev = NULL; + udevPrivate *priv = NULL; int tmpGroup, ret = -1; char *p; int rc;
+ priv = driverState->privateData; +
I personally prefer the initialization to be done in the declaration. But I can live with this too.
syspath = udev_device_get_syspath(device);
if (udevGetUintProperty(device, @@ -544,7 +547,8 @@ static int udevProcessPCI(struct udev_device *device, data->pci_dev.function))) goto out;
- if (virPCIDeviceIsPCIExpress(pciDev) > 0) { + /* We need to be root to read PCI device configs */ + if (priv->privileged && virPCIDeviceIsPCIExpress(pciDev) > 0) { if (VIR_ALLOC(pci_express) < 0) goto out;
Michal
Just like virFileReadAll, but returns -errno instead of reporting errors. Useful for ignoring some errors. --- src/libvirt_private.syms | 1 + src/util/virfile.c | 15 +++++++++++++++ src/util/virfile.h | 2 ++ 3 files changed, 18 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 1e1dd84..ed56103 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1288,6 +1288,7 @@ virFileOpenAs; virFileOpenTty; virFilePrintf; virFileReadAll; +virFileReadAllQuiet; virFileReadHeaderFD; virFileReadLimFD; virFileRelLinkPointsTo; diff --git a/src/util/virfile.c b/src/util/virfile.c index 9e17504..463064c 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -1301,6 +1301,21 @@ virFileReadAll(const char *path, int maxlen, char **buf) return len; } +int +virFileReadAllQuiet(const char *path, int maxlen, char **buf) +{ + int fd = open(path, O_RDONLY); + if (fd < 0) + return -errno; + + int len = virFileReadLimFD(fd, maxlen, buf); + VIR_FORCE_CLOSE(fd); + if (len < 0) + return -errno; + + return len; +} + /* Truncate @path and write @str to it. If @mode is 0, ensure that @path exists; otherwise, use @mode if @path must be created. Return 0 for success, nonzero for failure. diff --git a/src/util/virfile.h b/src/util/virfile.h index 5fee407..36d3fe7 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -129,6 +129,8 @@ int virFileReadLimFD(int fd, int maxlen, char **buf) ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NONNULL(3); int virFileReadAll(const char *path, int maxlen, char **buf) ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3); +int virFileReadAllQuiet(const char *path, int maxlen, char **buf) + ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3); int virFileWriteStr(const char *path, const char *str, mode_t mode) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; -- 1.8.5.5
virFileReadAll already logs an error. If reading the 'speed' file fails with EINVAL, we log an error even though we ignore it. If it fails with other errors, we log two errors. Use virFileReadAllQuiet - ignore EINVAL and report just one error in other cases. Fixes this error on libvirtd startup: 2014-06-30 12:47:14.583+0000: 20971: error : virFileReadAll:1297 : Failed to read file '/sys/class/net/wlan0/speed': Invalid argument --- src/util/virnetdev.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index a551f98..c3a7384 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -1844,6 +1844,7 @@ virNetDevGetLinkInfo(const char *ifname, char *tmp; int tmp_state; unsigned int tmp_speed; + int rc; if (virNetDevSysfsFile(&path, ifname, "operstate") < 0) goto cleanup; @@ -1891,13 +1892,13 @@ virNetDevGetLinkInfo(const char *ifname, if (virNetDevSysfsFile(&path, ifname, "speed") < 0) goto cleanup; - if (virFileReadAll(path, 1024, &buf) < 0) { + if ((rc = virFileReadAllQuiet(path, 1024, &buf)) < 0) { /* Some devices doesn't report speed, in which case we get EINVAL */ - if (errno == EINVAL) { + if (-rc == EINVAL) { ret = 0; goto cleanup; } - virReportSystemError(errno, + virReportSystemError(-rc, _("unable to read: %s"), path); goto cleanup; -- 1.8.5.5
On 30.06.2014 15:38, Ján Tomko wrote:
virFileReadAll already logs an error. If reading the 'speed' file fails with EINVAL, we log an error even though we ignore it. If it fails with other errors, we log two errors.
Use virFileReadAllQuiet - ignore EINVAL and report just one error in other cases.
Fixes this error on libvirtd startup: 2014-06-30 12:47:14.583+0000: 20971: error : virFileReadAll:1297 : Failed to read file '/sys/class/net/wlan0/speed': Invalid argument --- src/util/virnetdev.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index a551f98..c3a7384 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -1844,6 +1844,7 @@ virNetDevGetLinkInfo(const char *ifname, char *tmp; int tmp_state; unsigned int tmp_speed; + int rc;
There's no need for this new variable, virFileReadAAllQuiet will set errno too.
if (virNetDevSysfsFile(&path, ifname, "operstate") < 0) goto cleanup; @@ -1891,13 +1892,13 @@ virNetDevGetLinkInfo(const char *ifname, if (virNetDevSysfsFile(&path, ifname, "speed") < 0) goto cleanup;
- if (virFileReadAll(path, 1024, &buf) < 0) {
In fact, simple s/virFileReadAll/virFileReadAllQuiet/ would suffice as well.
+ if ((rc = virFileReadAllQuiet(path, 1024, &buf)) < 0) { /* Some devices doesn't report speed, in which case we get EINVAL */ - if (errno == EINVAL) { + if (-rc == EINVAL) { ret = 0; goto cleanup; } - virReportSystemError(errno, + virReportSystemError(-rc, _("unable to read: %s"), path); goto cleanup;
Michal
On 06/30/2014 03:37 PM, Ján Tomko wrote:
Some are distracting and not really helpful.
Ján Tomko (4): Track privileged state in udev nodedev driver Only detect PCI Express devices as root in udev nodedev driver Introduce virFileReadAllQuiet Report one error less when getting net dev speed
src/libvirt_private.syms | 1 + src/node_device/node_device_udev.c | 10 ++++++++-- src/util/virfile.c | 15 +++++++++++++++ src/util/virfile.h | 2 ++ src/util/virnetdev.c | 7 ++++--- 5 files changed, 30 insertions(+), 5 deletions(-)
This would be nice to have in 1.2.6: The first two patches fix noisy startup of unprivileged daemon and the last two silence an error that gets logged on startup with some network card drivers. Jan
On 30.06.2014 15:37, Ján Tomko wrote:
Some are distracting and not really helpful.
Ján Tomko (4): Track privileged state in udev nodedev driver Only detect PCI Express devices as root in udev nodedev driver Introduce virFileReadAllQuiet Report one error less when getting net dev speed
src/libvirt_private.syms | 1 + src/node_device/node_device_udev.c | 10 ++++++++-- src/util/virfile.c | 15 +++++++++++++++ src/util/virfile.h | 2 ++ src/util/virnetdev.c | 7 ++++--- 5 files changed, 30 insertions(+), 5 deletions(-)
ACK to all the patches and safe for 1.2.6. Michal
On 07/01/2014 04:15 PM, Michal Privoznik wrote:
On 30.06.2014 15:37, Ján Tomko wrote:
Some are distracting and not really helpful.
Ján Tomko (4): Track privileged state in udev nodedev driver Only detect PCI Express devices as root in udev nodedev driver Introduce virFileReadAllQuiet Report one error less when getting net dev speed
src/libvirt_private.syms | 1 + src/node_device/node_device_udev.c | 10 ++++++++-- src/util/virfile.c | 15 +++++++++++++++ src/util/virfile.h | 2 ++ src/util/virnetdev.c | 7 ++++--- 5 files changed, 30 insertions(+), 5 deletions(-)
ACK to all the patches and safe for 1.2.6.
Thanks, I fixed the small issues you raised and pushed it.
Michal
Jan
participants (2)
-
Ján Tomko -
Michal Privoznik