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