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

[Moshe Levi]

> -----Original Message-----
> From: Martin Kletzander [mailto:mkletzan at redhat.com]
> Sent: Monday, August 17, 2015 8:30 PM
> To: Moshe Levi <moshele at mellanox.com>
> Cc: libvir-list at 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.

Yes because this is a critical error, but the ioctl api can return error which are sometime not an actual ERROR.
So the intention of the patch is to let the caller decide what is really an error just for the ioctl api. 
> 
> >     }
> >@@ -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.
This is some kind of refactoring that John Ferlan suggested and I agree with him (or at least that was my understanding  :) )
See https://www.redhat.com/archives/libvir-list/2015-August/msg00567.html
In this patch I am adding a function to determine the GFFATURE SIZE from the kernel 
And there are some error (like EOPNOTSUPP or  EINVAL ) from ioctl which mean the GFFATURE is not supported by the kernel 
So in this case I need to filter them out and log an error only for the other errors.

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