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

-----Original Message----- From: Martin Kletzander [mailto:mkletzan@redhat.com] Sent: Monday, August 17, 2015 8:30 PM To: Moshe Levi <moshele(a)mellanox.com&gt; Cc: libvir-list(a)redhat.com Subject: Re: [libvirt] [PATCH] utils: Remove the logging of errors from virNetDevSendEthtoolIoctl On Sat, Aug 15, 2015 at 12:04:04PM +0300, Moshe Levi wrote: >This patch remove the logging of errors of ioctl api and instead let >the caller to choose what errors to log >--- > src/util/virnetdev.c | 44 +++++++++++++------------------------------- > 1 files changed, 13 insertions(+), 31 deletions(-) > >diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index >2f3690e..cf79e8d 100644 >--- a/src/util/virnetdev.c >+++ b/src/util/virnetdev.c >@@ -3032,11 +3032,10 @@ static int > virNetDevSendEthtoolIoctl(const char *ifname, void *cmd) { > int ret = -1; >- int sock = -1; >+ int sock; > virIfreq ifr; > >- sock = socket(AF_LOCAL, SOCK_DGRAM, 0); >- if (sock < 0) { >+ if ((sock = socket(AF_LOCAL, SOCK_DGRAM, 0)) < 0) { > virReportSystemError(errno, "%s", _("Cannot open control socket")); > goto cleanup; Here you still report error from this function.

> } >@@ -3045,26 +3044,9 @@ virNetDevSendEthtoolIoctl(const char *ifname, void *cmd) > strcpy(ifr.ifr_name, ifname); > ifr.ifr_data = cmd; > 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; >- } >- } > > cleanup: >- if (sock) >- VIR_FORCE_CLOSE(sock); >+ VIR_FORCE_CLOSE(sock); > return ret; > } > >@@ -3081,12 +3063,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)) >- ret = cmd->data > 0 ? 1 : 0; >- return ret; >+ if (virNetDevSendEthtoolIoctl(ifname, cmd) < 0) { >+ virReportSystemError(errno, _("Cannot get device %s flags"), ifname); >+ return -1; >+ } >+ return cmd->data > 0 ? 1 : 0; > } > > >@@ -3103,12 +3085,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)) >- ret = FEATURE_BIT_IS_SET(cmd->features, TX_UDP_TNL, active); >- return ret; >+ if (virNetDevSendEthtoolIoctl(ifname, cmd) < 0) { >+ virReportSystemError(errno, _("Cannot get device %s generic features"), ifname); >+ return -1; And here and above you rewrite it. I suggest just using virReportError in the virNetDevSendEthtoolIoctl() function based on what happened and don't rewrite it in callers.

>+ } >+ return FEATURE_BIT_IS_SET(cmd->features, TX_UDP_TNL, active); > } > # endif > >-- >1.7.1 > >-- >libvir-list mailing list &gt;libvir-list(a)redhat.com >https://www.redhat.com/mailman/listinfo/libvir-list