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

[Ján Tomko]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20151106/f4458bf9/attachment-0001.sig>