On Wed, Nov 04, 2015 at 10:18:27AM -0500, John Ferlan wrote: > > > 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) > Both code paths should be avoided for unprivileged daemons. > 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. > Yes. Jan
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list