On 11/04/2015 04:39 AM, Ján Tomko wrote: > On Tue, Nov 03, 2015 at 07:18:10PM -0500, John Ferlan wrote: >> Since virNetDevSetupControl can generate a virReportSystemError, rather >> than message in the caller for any errors. Do all the messaging in the >> virNetDevSendEthtoolIoctl. >> >> This change partially reverts commit id '6f2a0198' by moving the error >> message in the virNetDev[G]FeatureAvailable to where the ioctl fails. >> However, the ioctl will report any error rather than filtering some. >>
The shorter version - ditch patch 2 & 3 of this series, revert as Dan suggests, then add new series to 1. Avoid calling when not privileged from udevProcessNetworkInterface (node_device_udev.c), but not from update_caps (e.g. called from nodeDeviceGetXMLDesc) 2. Use virNetDevSetupControl instead of socket as well as adding a couple of comments regarding the skipped errno values. However, this could cause issues for the XMLDesc path for unprivileged daemons. > > I think we should not pollute the logs for some error codes and just > VIR_DEBUG the error, even in privileged mode. > I think a question I never got answered when posed during review of other related changes is why we were filtering certain error codes: http://www.redhat.com/archives/libvir-list/2015-August/msg00584.html However, based on the unprivileged daemon I think I now know part of the reason. Digging further back into the patch series that added support for the ethtool ioctl, there's another hint: http://www.redhat.com/archives/libvir-list/2015-February/msg00506.html When v2 was posted - the ReportSystemError's were changed to VIR_DEBUG's: http://www.redhat.com/archives/libvir-list/2015-February/msg00881.html when pushed those VIR_DEBUG's were slightly adjusted. So, it seems EPERM is to avoid the unprivileged daemon run, EINVAL seems to be because a kernel doesn't support SIOCETHTOOL, and EOPNOTSUPP is when a requested feature check is not supported in the kernel. As an aside - this is one of the reasons why it's "nice" to have either in the comments or the commit message a reason why decisions to filter certain messages were made. The code isn't self documenting. Future changes to the code then won't have to assume, wonder, and/or adjust the code "incorrectly" from initial design. John > On the other hand, the proposed revert unnecessarily reintroduces > the use of AF_LOCAL, but that's because the orignal commit > does two things at once. > > Jan > >> Signed-off-by: John Ferlan <jfer...@redhat.com> >> --- >> src/util/virnetdev.c | 24 +++++++++++++----------- >> 1 file changed, 13 insertions(+), 11 deletions(-) >> >> diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c >> index 12faf51..d47859e 100644 >> --- a/src/util/virnetdev.c >> +++ b/src/util/virnetdev.c >> @@ -3157,7 +3157,8 @@ virNetDevSendEthtoolIoctl(const char *ifname, void >> *cmd) >> if ((fd = virNetDevSetupControl(ifname, &ifr)) < 0) >> return ret; >> ifr.ifr_data = cmd; >> - ret = ioctl(fd, SIOCETHTOOL, &ifr); >> + if ((ret = ioctl(fd, SIOCETHTOOL, &ifr)) < 0) >> + virReportSystemError(errno, "%s", _("ethtool ioctl error")); >> >> VIR_FORCE_CLOSE(fd); >> return ret; >> @@ -3176,12 +3177,13 @@ 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) < 0) { >> - virReportSystemError(errno, _("Cannot get device %s flags"), >> ifname); >> - return -1; >> - } >> - return cmd->data > 0 ? 1 : 0; >> + if (!virNetDevSendEthtoolIoctl(ifname, cmd)) >> + ret = cmd->data > 0 ? 1 : 0; >> + return ret; >> + >> } >> >> >> @@ -3198,12 +3200,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) < 0) { >> - virReportSystemError(errno, _("Cannot get device %s generic >> features"), ifname); >> - return -1; >> - } >> - return FEATURE_BIT_IS_SET(cmd->features, TX_UDP_TNL, active); >> + if (!virNetDevSendEthtoolIoctl(ifname, cmd)) >> + ret = FEATURE_BIT_IS_SET(cmd->features, TX_UDP_TNL, active); >> + return ret; >> } >> # endif >> >> -- >> 2.1.0 >> >> -- >> libvir-list mailing list >> libvir-list@redhat.com >> https://www.redhat.com/mailman/listinfo/libvir-list -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
