Re: [libvirt] [PATCH 2/3] virnetdev: Message in virNetDevSendEthtoolIoctl rather than caller.

On 11/04/2015 04:39 AM, Ján Tomko wrote: > On Tue, Nov 03, 2015 at 07:18:10PM -0500, John Ferlan wrote: >> Since virNetDevSetupControl can generate a virReportSystemError, rather >> than message in the caller for any errors. Do all the messaging in the >> virNetDevSendEthtoolIoctl. >> >> This change partially reverts commit id '6f2a0198' by moving the error >> message in the virNetDev[G]FeatureAvailable to where the ioctl fails. >> However, the ioctl will report any error rather than filtering some. >> The shorter version - ditch patch 2 & 3 of this series, revert as Dan suggests, then add new series to 1. Avoid calling when not privileged from udevProcessNetworkInterface (node_device_udev.c), but not from update_caps (e.g. called from nodeDeviceGetXMLDesc)

2. Use virNetDevSetupControl instead of socket as well as adding a couple of comments regarding the skipped errno values. However, this could cause issues for the XMLDesc path for unprivileged daemons. > > I think we should not pollute the logs for some error codes and just > VIR_DEBUG the error, even in privileged mode. > I think a question I never got answered when posed during review of other related changes is why we were filtering certain error codes: http://www.redhat.com/archives/libvir-list/2015-August/msg00584.html However, based on the unprivileged daemon I think I now know part of the reason. Digging further back into the patch series that added support for the ethtool ioctl, there's another hint: http://www.redhat.com/archives/libvir-list/2015-February/msg00506.html When v2 was posted - the ReportSystemError's were changed to VIR_DEBUG's: http://www.redhat.com/archives/libvir-list/2015-February/msg00881.html when pushed those VIR_DEBUG's were slightly adjusted. So, it seems EPERM is to avoid the unprivileged daemon run, EINVAL seems to be because a kernel doesn't support SIOCETHTOOL, and EOPNOTSUPP is when a requested feature check is not supported in the kernel. As an aside - this is one of the reasons why it's "nice" to have either in the comments or the commit message a reason why decisions to filter certain messages were made. The code isn't self documenting. Future changes to the code then won't have to assume, wonder, and/or adjust the code "incorrectly" from initial design.