On Mon, Aug 17, 2015 at 05:47:56PM +0000, Moshe Levi wrote:
-----Original Message----- From: Martin Kletzander [mailto:mkletzan@xxxxxxxxxx] Sent: Monday, August 17, 2015 8:30 PM To: Moshe Levi <moshele@xxxxxxxxxxxx> Cc: libvir-list@xxxxxxxxxx Subject: Re: [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@xxxxxxxxxx >https://www.redhat.com/mailman/listinfo/libvir-list
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list