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

[John Ferlan]

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