Re: [libvirt] [PATCH] Revert "utils: Remove the logging of errors from virNetDevSendEthtoolIoctl"

This reverts commit 6f2a0198e913c91a2ef8b99db79b7d3cc5396957. This commit removed error reporting from virNetDevSendEthtoolIoctl pushing responsibility onto the callers. This is wrong, however, since virNetDevSendEthtoolIoctl calls virNetDevSetupControl which can still report errors. So as a result virNetDevSendEthtoolIoctl may or may not report errors depending on which bit of it fails, and as a result callers now overwrite some errors. It also introduced a regression causing unprivileged libvirtd to spew error messages to the console due to inability to query the NIC features, an error which was previously ignored. virNetDevSetupControlFull:148 : Cannot open network interface control socket: Operation not permitted virNetDevFeatureAvailable:3062 : Cannot get device wlp3s0 flags: Operation not permitted virNetDevSetupControlFull:148 : Cannot open network interface control socket: Operation not permitted virNetDevFeatureAvailable:3062 : Cannot get device wlp3s0 flags: Operation not permitted virNetDevSetupControlFull:148 : Cannot open network interface control socket: Operation not permitted virNetDevFeatureAvailable:3062 : Cannot get device wlp3s0 flags: Operation not permitted virNetDevSetupControlFull:148 : Cannot open network interface control socket: Operation not permitted virNetDevFeatureAvailable:3062 : Cannot get device wlp3s0 flags: Operation not permitted Looking back at the original posting I see no explanation of why thsi refactoring was needed, so reverting the clearly broken error reporting logic looks like the best option.

Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com&gt; --- src/util/virnetdev.c | 56 +++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 40 insertions(+), 16 deletions(-) diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 9789e93..657df3a 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -3145,15 +3145,39 @@ static int virNetDevSendEthtoolIoctl(const char *ifname, void *cmd) { int ret = -1; - int fd; - struct ifreq ifr; + int sock = -1; + virIfreq ifr; - if ((fd = virNetDevSetupControl(ifname, &ifr)) < 0) - return ret; + sock = socket(AF_LOCAL, SOCK_DGRAM, 0); + if (sock < 0) { + virReportSystemError(errno, "%s", _("Cannot open control socket")); + goto cleanup; + } + + memset(&ifr, 0, sizeof(ifr)); + strcpy(ifr.ifr_name, ifname); ifr.ifr_data = cmd; - ret = ioctl(fd, SIOCETHTOOL, &ifr); + ret = ioctl(sock, SIOCETHTOOL, &ifr); + if (ret != 0) { + switch (errno) { + case EPERM: + VIR_DEBUG("ethtool ioctl: permission denied"); + break; + case EINVAL: + VIR_DEBUG("ethtool ioctl: invalid request"); + break; + case EOPNOTSUPP: + VIR_DEBUG("ethtool ioctl: request not supported"); + break; + default: + virReportSystemError(errno, "%s", _("ethtool ioctl error")); + goto cleanup; + } + } - VIR_FORCE_CLOSE(fd); + cleanup: + if (sock) + VIR_FORCE_CLOSE(sock); return ret; } @@ -3170,12 +3194,12 @@ virNetDevSendEthtoolIoctl(const char *ifname, void *cmd) static int virNetDevFeatureAvailable(const char *ifname, struct ethtool_value *cmd) { + int ret = -1; + cmd = (void*)cmd; - if (virNetDevSendEthtoolIoctl(ifname, cmd) < 0) { - virReportSystemError(errno, _("Cannot get device %s flags"), ifname); - return -1; - } - return cmd->data > 0 ? 1 : 0; + if (!virNetDevSendEthtoolIoctl(ifname, cmd)) + ret = cmd->data > 0 ? 1 : 0; + return ret; } @@ -3192,12 +3216,12 @@ virNetDevFeatureAvailable(const char *ifname, struct ethtool_value *cmd) static int virNetDevGFeatureAvailable(const char *ifname, struct ethtool_gfeatures *cmd) { + int ret = -1; + cmd = (void*)cmd; - if (virNetDevSendEthtoolIoctl(ifname, cmd) < 0) { - virReportSystemError(errno, _("Cannot get device %s generic features"), ifname); - return -1; - } - return FEATURE_BIT_IS_SET(cmd->features, TX_UDP_TNL, active); + if (!virNetDevSendEthtoolIoctl(ifname, cmd)) + ret = FEATURE_BIT_IS_SET(cmd->features, TX_UDP_TNL, active); + return ret; } # endif