[libvirt] [PATCH] utils: Remove the logging of errors from virNetDevSendEthtoolIoctl
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; } @@ -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; + } + return FEATURE_BIT_IS_SET(cmd->features, TX_UDP_TNL, active); } # endif -- 1.7.1
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 libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-----Original Message----- From: Martin Kletzander [mailto:mkletzan@redhat.com] Sent: Monday, August 17, 2015 8:30 PM To: Moshe Levi <moshele@mellanox.com> Cc: libvir-list@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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Aug 17, 2015 at 05:47:56PM +0000, Moshe Levi wrote:
-----Original Message----- From: Martin Kletzander [mailto:mkletzan@redhat.com] Sent: Monday, August 17, 2015 8:30 PM To: Moshe Levi <moshele@mellanox.com> Cc: libvir-list@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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Aug 17, 2015 at 05:47:56PM +0000, Moshe Levi wrote:
-----Original Message----- From: Martin Kletzander [mailto:mkletzan@redhat.com] Sent: Monday, August 17, 2015 8:30 PM To: Moshe Levi <moshele@mellanox.com> Cc: libvir-list@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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
participants (2)
-
Martin Kletzander -
Moshe Levi