----- Original Message -----
From: "Ondrej Lichtner" <olichtne(a)redhat.com>
To: csfakian(a)redhat.com
Cc: lnst-developers(a)lists.fedorahosted.org
Sent: Wednesday, February 27, 2019 12:39:08 PM
Subject: Re: [PATCH-next 1/2] lnst.Devices.Device: handle coalesce
On Tue, Feb 26, 2019 at 04:34:43PM +0100, csfakian(a)redhat.com wrote:
> From: Christos Sfakianakis <csfakian(a)redhat.com>
>
> Add methods for handling coalesce settings. Raise errors
> when these settings cannot be read or modified correctly.
>
> Signed-off-by: Christos Sfakianakis <csfakian(a)redhat.com>
> ---
> lnst/Devices/Device.py | 47 ++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 47 insertions(+)
>
> diff --git a/lnst/Devices/Device.py b/lnst/Devices/Device.py
> index 20cddcf..6a04d1b 100644
> --- a/lnst/Devices/Device.py
> +++ b/lnst/Devices/Device.py
> @@ -17,10 +17,12 @@ import pyroute2
> import logging
> import pprint
> from abc import ABCMeta
> +from itertools import product
> from pyroute2.netlink.rtnl import ifinfmsg
> from lnst.Common.Logs import log_exc_traceback
> from lnst.Common.NetUtils import normalize_hwaddr
> from lnst.Common.ExecCmd import exec_cmd
> +from lnst.Common.LnstError import LnstError
> from lnst.Common.DeviceError import DeviceError, DeviceDeleted,
> DeviceDisabled
> from lnst.Common.DeviceError import DeviceConfigError,
> DeviceConfigValueError
> from lnst.Common.IpAddress import ipaddress
> @@ -58,6 +60,7 @@ class Device(object):
> self._deleted = False
>
> self._ip_addrs = []
> + self._coalesce_settings = ['', '']
>
> self._nl_update = {}
> self._bulk_enabled = False
> @@ -603,6 +606,50 @@ class Device(object):
> """disable automatic negotiation of speed for this
device"""
> exec_cmd("ethtool -s %s autoneg off" % self.name)
>
> + def _read_coalesce(self):
> + try:
> + res = exec_cmd("ethtool -c %s" % self.name)
> + except:
> + raise DeviceError("Could not fetch coalesce settings of "
> + "%s." % self.name)
> + res = re.findall('\wX: (o[n|f]*)', res[0])
^^^^^^^
i think this can just be (on|off)
> + if not tuple(res) in product(['off', 'on'], repeat=2):
> + raise LnstError("Invalid values for coalesce settings of "
> + "{} : {}".format(self.name, res))
^^^
use DeviceError for exceptions from the Device class
So if I understand this correctly, the condition checks if all the
strings found by the re.findall are "on" or "off", and that
there's
exactly two of them. But the regex already ensures that it's only "on"
or "off" (with my modification at least). And you can ensure the
"exactly two values" by modifying the regex a bit more:
"Adaptive RX: (on|off) TX: (on|off)"
then I'd just use re.search and return the result.groups().
In case the regex search fails, then throw the exception.
Agree, will apply these changes.
> + return res
> +
> + def _store_coalesce(self):
> + coal = self._read_coalesce()
> + self._coalesce_settings = coal
> +
> + def _write_coalesce(self, rx_val, tx_val):
> + if self._coalesce_settings == [rx_val, tx_val]:
Depending on if you store a tuple or a list into the _coalesce_settings,
this comparison might always fail, tuple never equals a list... just a
note in case you rewrite the read method with match.groups() which
returns a tuple...
> + return
> + try:
> + exec_cmd("ethtool -C %s adaptive-rx %s adaptive-tx %s" %
> + (self.name, rx_val, tx_val))
> + except:
> + raise DeviceConfigError("Not allowed to modify coalesce "
> + "settings for %s." % self.name)
> + if self._read_coalesce != [rx_val, tx_val]:
^^^
this is just a reference to a method, you're missing ()
to actually call it so this condition is always True
> + raise LnstError("Could not apply coalesce settings for "
> + "%s effectively." % self.name)
DeviceError instead of LnstError, DeviceConfigError
is also appropriate here I think
> +
> + def _disable_coalesce(self):
> + self._store_coalesce()
> + self._write_coalesce('off', 'off')
> +
> + def _restore_coalesce(self):
> + rx_val = self._coalesce_settings[0]
> + tx_val = self._coalesce_settings[1]
> + self._write_coalesce(rx_val, tx_val)
> +
> + def disable_coalesce(self):
> + self._disable_coalesce()
Does it make sense to also have an enable_coalesce method for
completeness?
Adding an enable() method. I will assume the initial values need to be
stored only once prior to the first change (and then restored in the end
of the test), so modifying the disable method slightly as well.
-Christos
> > +
> > + def restore_coalesce(self):
> > + self._restore_coalesce()
> > +
> > #TODO implement proper Route objects
> > #consider the same as with tc?
> > # def route_add(self, dest):
> > --
> > 2.17.1
> > _______________________________________________
> > LNST-developers mailing list -- lnst-developers(a)lists.fedorahosted.org
> > To unsubscribe send an email to
> > lnst-developers-leave(a)lists.fedorahosted.org
> > Fedora Code of Conduct:
https://getfedora.org/code-of-conduct.html
> > List Guidelines:
https://fedoraproject.org/wiki/Mailing_list_guidelines
> > List Archives:
> >
https://lists.fedorahosted.org/archives/list/lnst-developers@lists.fedora...
> _______________________________________________
> LNST-developers mailing list -- lnst-developers(a)lists.fedorahosted.org
> To unsubscribe send an email to lnst-developers-leave(a)lists.fedorahosted.org
> Fedora Code of Conduct:
https://getfedora.org/code-of-conduct.html
> List Guidelines:
https://fedoraproject.org/wiki/Mailing_list_guidelines
> List Archives:
>
https://lists.fedorahosted.org/archives/list/lnst-developers@lists.fedora...
>