On 11/03/2015 11:15 AM, Daniel P. Berrange wrote: > On Tue, Nov 03, 2015 at 10:58:58AM -0500, John Ferlan wrote: >> >> >> On 11/03/2015 09:17 AM, Daniel P. Berrange wrote: >>> This reverts commit 6f2a0198e913c91a2ef8b99db79b7d3cc5396957. >>> >>> This commit removed error reporting from virNetDevSendEthtoolIoctl >>> pushing responsibility onto the callers. This is wrong, however, >>> since virNetDevSendEthtoolIoctl calls virNetDevSetupControl >>> which can still report errors. So as a result virNetDevSendEthtoolIoctl >>> may or may not report errors depending on which bit of it fails, and as >>> a result callers now overwrite some errors. >>> >>> It also introduced a regression causing unprivileged libvirtd to >>> spew error messages to the console due to inability to query the >>> NIC features, an error which was previously ignored. >>> >>> virNetDevSetupControlFull:148 : Cannot open network interface control socket: Operation not permitted >>> virNetDevFeatureAvailable:3062 : Cannot get device wlp3s0 flags: Operation not permitted >>> virNetDevSetupControlFull:148 : Cannot open network interface control socket: Operation not permitted >>> virNetDevFeatureAvailable:3062 : Cannot get device wlp3s0 flags: Operation not permitted >>> virNetDevSetupControlFull:148 : Cannot open network interface control socket: Operation not permitted >>> virNetDevFeatureAvailable:3062 : Cannot get device wlp3s0 flags: Operation not permitted >>> virNetDevSetupControlFull:148 : Cannot open network interface control socket: Operation not permitted >>> virNetDevFeatureAvailable:3062 : Cannot get device wlp3s0 flags: Operation not permitted >>> >>> Looking back at the original posting I see no explanation of why >>> thsi refactoring was needed, so reverting the clearly broken >>> error reporting logic looks like the best option. >> >> A bit of history... >> >> Going back through the timeline... Moshe Levi proposes patches to add >> support for new feature/bits: >> >> http://www.redhat.com/archives/libvir-list/2015-June/msg00921.html >> >> reviewed and posted v2 in July: >> >> http://www.redhat.com/archives/libvir-list/2015-July/msg00548.html >> >> Changes get pushed; however, they lead to an issue: >> >> http://www.redhat.com/archives/libvir-list/2015-August/msg00162.html >> >> Moshe provided a patch to resolve that: >> >> http://www.redhat.com/archives/libvir-list/2015-August/msg00263.html >> >> Laine has a different proposal: >> >> http://www.redhat.com/archives/libvir-list/2015-August/msg00382.html >> >> which after review gets shortened a bit: >> >> http://www.redhat.com/archives/libvir-list/2015-August/msg00471.html >> >> and pushed >> >> Moshe as an update to his v1 msg00263 >> >> http://www.redhat.com/archives/libvir-list/2015-August/msg00567.html >> >> Comments from there lead to posting changes just for the ioctl errors: >> >> http://www.redhat.com/archives/libvir-list/2015-August/msg00625.html >> >> Which leads to the v2 of that series (which you're proposing to revert): >> >> http://www.redhat.com/archives/libvir-list/2015-August/msg00720.html >> >> >> I suppose when I reviewed I just saw using virNetDevSetupControl as way >> to use existing functions as opposed to calling socket() directly. It >> was something different between v2 and v1 of that change, but other than >> the additional virSetInherit call - it was essentially the same. >> >> Even with the revert, in the unpriv'd libvirt, wouldn't the error: >> >> virReportSystemError(errno, "%s", _("Cannot open control socket")); >> >> still still be spewed? Since virNetDevSetupControlFull:148 is the socket >> call failure and not the ioctl failure. Sure the caller wouldn't spew a >> second one, but why would the first one not be spewed? > > Empirically I don't see any errors after reverting it. It appears the > reason for this is that the original code uses AF_LOCAL while the > virNetDevSetupControllFull uses AF_PACKET, and the latter is restricted > to only privileged users. If I change the original code to use AF_PACKET > then it shows errors too. The reason for using AF_PACKET was that it > was substaintially faster than AF_LOCAL > Ah - I see VIR_NETDEV_FAMILY used by virNetDevSetupControl is what hides this... #ifdef __linux__ # include <linux/sockios.h> # include <linux/if_vlan.h> # define VIR_NETDEV_FAMILY AF_PACKET #elif defined(HAVE_STRUCT_IFREQ) && defined(AF_LOCAL) # define VIR_NETDEV_FAMILY AF_LOCAL #else # undef HAVE_STRUCT_IFREQ #endif Well hopefully no one falls into this rabbit hole in the future as it's not well marked ;-) ACK to the revert John -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list