[libvirt] [PATCH] Revert "utils: Remove the logging of errors from virNetDevSendEthtoolIoctl"

[Daniel P. Berrange]

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 :|