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

[Martin Kletzander]

On Mon, Aug 17, 2015 at 05:47:56PM +0000, Moshe Levi wrote:
>
>
>> -----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.
>

Well, but no caller doesn anything else than compare it to 0.  If you
need different error messages, then I'm very OK with that, but then
report all the messages from callers and none from the function
itself.

>> >+    }
>> >+    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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150817/e53a92d8/attachment-0001.sig>