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

[John Ferlan]

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.

John

> On the other hand, the proposed revert unnecessarily reintroduces
> the use of AF_LOCAL, but that's because the orignal commit
> does two things at once.
> 
> Jan
> 
>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>> ---
>>  src/util/virnetdev.c | 24 +++++++++++++-----------
>>  1 file changed, 13 insertions(+), 11 deletions(-)
>>
>> diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
>> index 12faf51..d47859e 100644
>> --- a/src/util/virnetdev.c
>> +++ b/src/util/virnetdev.c
>> @@ -3157,7 +3157,8 @@ virNetDevSendEthtoolIoctl(const char *ifname, void *cmd)
>>      if ((fd = virNetDevSetupControl(ifname, &ifr)) < 0)
>>          return ret;
>>      ifr.ifr_data = cmd;
>> -    ret = ioctl(fd, SIOCETHTOOL, &ifr);
>> +    if ((ret = ioctl(fd, SIOCETHTOOL, &ifr)) < 0)
>> +        virReportSystemError(errno, "%s", _("ethtool ioctl error"));
>>  
>>      VIR_FORCE_CLOSE(fd);
>>      return ret;
>> @@ -3176,12 +3177,13 @@ virNetDevSendEthtoolIoctl(const char *ifname, void *cmd)
>>  static int
>>  virNetDevFeatureAvailable(const char *ifname, struct ethtool_value *cmd)
>>  {
>> +    int ret = -1;
>> +
>>      cmd = (void*)cmd;
>> -    if (virNetDevSendEthtoolIoctl(ifname, cmd) < 0) {
>> -        virReportSystemError(errno, _("Cannot get device %s flags"), ifname);
>> -        return -1;
>> -    }
>> -    return cmd->data > 0 ? 1 : 0;
>> +    if (!virNetDevSendEthtoolIoctl(ifname, cmd))
>> +        ret = cmd->data > 0 ? 1 : 0;
>> +    return ret;
>> +
>>  }
>>  
>>  
>> @@ -3198,12 +3200,12 @@ virNetDevFeatureAvailable(const char *ifname, struct ethtool_value *cmd)
>>  static int
>>  virNetDevGFeatureAvailable(const char *ifname, struct ethtool_gfeatures *cmd)
>>  {
>> +    int ret = -1;
>> +
>>      cmd = (void*)cmd;
>> -    if (virNetDevSendEthtoolIoctl(ifname, cmd) < 0) {
>> -        virReportSystemError(errno, _("Cannot get device %s generic features"), ifname);
>> -        return -1;
>> -    }
>> -    return FEATURE_BIT_IS_SET(cmd->features, TX_UDP_TNL, active);
>> +    if (!virNetDevSendEthtoolIoctl(ifname, cmd))
>> +        ret = FEATURE_BIT_IS_SET(cmd->features, TX_UDP_TNL, active);
>> +    return ret;
>>  }
>>  # endif
>>  
>> -- 
>> 2.1.0
>>
>> --
>> libvir-list mailing list
>> libvir-list at redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list