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 Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list