As confirmed via private email:
From: Florian Echtler <floe(at)butterbrot.org>
To: Thomas Haller <thaller(at)redhat.com>
Date: Fri, 22 May 2020 16:17:52 +0200
Iterating the hash gives the entries in undefined order. That
means, when validation would fail for more than one option,
then the error message arbitrarily points out one or the other.
Instead, process the entries in a defined order.
Note that nm_setting_ethtool_set_coalesce() used to coerce
"coalesce-adaptive-[rt]x" values to 0 or 1. The alternative
API doesn't do that. But so does nm_setting_option_set()
not tell you whether the value you set is valid. That is
not the options of the setters, for that we have verify().
More general purpose API for generic options of settings.
The predicate function is also nicely usable via bindings.
One question is about the form of the predicate. In this case,
it is convenient to pass nm_ethtool_optname_is_coalesce(). On the
other hand, it's not very flexible as it does not accept a user
data argument. Use NMUtilsPredicateStr here, which is not flexible
but convenient for where it's used.
NMSettingEthtool is implemented using "gendata", meaning a hash
of GVariant. This is different from most other settings that have
properties implemented as GObject properties. There are two reasons
for this approach:
- The setting is transferred via D-Bus as "a{sv}" dictionary.
By unpacking the dictionary into GObject properties, the setting
cannot handle unknown properties. To be forward compatible (and
due to sloppy programming), unknown dictionary keys are silently
ignored when parsing a NMSetting. That is error prone and also
prevents settings to be treated loss-less.
Instead, we should at first accept all values from the dictionary.
Only in a second step, nm_connection_verify() rejects invalid settings
with an error reason. This way, the user can create a NMSetting,
but in a separate step handle if the setting doesn't verify.
"gendata" solves this by tracking the full GVariant dictionary.
This is still not entirely lossless, because multiple keys are
combined.
This is for example interesting if an libnm client fetches a connection
from a newer NetworkManager version. Now the user can modify the
properties that she knows about, while leaving all unknown
properties (from newer versions) in place.
- the approach aims to reduce the necessary boiler plate to create
GObject properties. Adding a new property should require less new code.
This approach was always be intended to be suitable for all settings, not only
NMSettingEthtool. We should not once again try to add API like
nm_setting_ethtool_set_feature(), nm_setting_ethtool_set_coalesce(), etc.
Note that the option name already fully encodes whether it is a feature,
a coalesce option, or whatever. We should not have
"nm_setting_set_$SUB_GROUP (setting, $ONE_NAME_FROM_GROUP)" API, but
simply "nm_setting_option_set (setting, $OPTION)" accessors.
Also, when parsing a NMSettingEthtool from a GVariant, then a feature
option can be any kind of variant. Only nm_setting_verify() rejects
variants of the wrong type. As such, nm_setting_option_set*() also
doesn't validate whether the variant type matches the option. Of course,
if you set a value of wrong type, verify() will reject the setting.
Add new general purpose API for this and expose it for NMSetting.
We are going to expose some of this API in libnm.
The name "gendata" (for "generic data") is not very suited. Instead,
call the public API nm_setting_option_*(). This also brings no naming
conflict, because currently no API exists with such naming.
Rename the internal API, so that it matches the API that we are going
to expose next.
This was intended for when the gendata hash should be converted
to/from a GValue/GHashTable. This would have been used, if
we also would have added a GObject property that exposes
the hash. But that was never done (at least not for NMSettingEthtool
and not yet).
This code is not used. If you ever need it, revert the patch
or implement it anew.
This function is not used nor does it seem useful.
Either you only need the names (nm_setting_gendata_get_all_names())
or you need the names and values together (_nm_setting_gendata_get_all()).
Getting the values without knowing the corresponding name makes
little sense. If you need it, call _nm_setting_gendata_get_all()
instead.
Don't duplicate the code that maps the option to the variant type.
Also, only resolve the name to NMEthtoolID once. Multiple calls
to nm_ethtool_optname_is_*() unnecessarily need to convert the
string to the ethtool id multiple times.
We already have NMEthtoolID to handle coalesce options in a way that is
convenient programmatically. That is, we can iterate over all valid
coalesce options (it's just an integer) and use that in a more generic
way.
If NMEthtoolCoalesceState names all fields explicitly, we need explicit
code that names each coalesce option. Especially since NMEthtoolCoalesceState
is an internal and intermediate data structure, this is cumbersome
and unnecessary.
Thereby it also fixes the issue that nm_platform_ethtool_init_coalesce() has a
NMPlatform argument without actually needing it.
nm_platform_ethtool_init_coalesce() does not operate on a NMPlatform
instance, and should not have the appearance of being a method of
NMPlatform.
All other users name a similar variable "coalesce" (or "coalesce_new",
"coalesce_old"). Rename the variables, to don't use different names for
similar uses.
This will be used for nm_setting_option_clear_by_name(), to
filter based on a name. But it is a general purpose typedef
for a predicate, not tied to NMSetting or option.
As confirmed via private email:
From: Rafael Fontenelle <rafaelff(at)gnome.org>
To: Thomas Haller <thaller(at)redhat.com>
Date: Fri, 22 May 2020 10:28:59 -0300
As confirmed via private email:
From: Ikey Doherty <ikey.doherty(at)lispysnake.com>
To: Thomas Haller <thaller(at)redhat.com>
Date: Fri, 22 May 2020 14:43:18 +0100
As discussed via email:
From: Andrew Sinclair <andrew.sinclair(at)canonical.com>
To: Thomas Haller <thaller(at)redhat.com>
Cc: Legal(at)canonical.com <legal(at)canonical.com>, Loïc Minier <loic.minier(at)canonical.com>, Alfonso Sanchez-Beato <alfonso.sanchez-beato(at)canonical.com>
Date: Wed, 20 May 2020 12:08:20 -0400
nm-device now applies ethtool ring settings during stage 2 "device
config" of the connection activation.
ring settings will be then restored (according to what the state
was before the connection got activated on the device) when the
connection is deactivated during the device cleanup.
One thing to be noted is that unset ring settings (in the profile)
will not be touched at all by NetworkManager so that if the NIC driver
sets some default values these will be preserved unless specifically
overridden by the connection profile.
https://bugzilla.redhat.com/show_bug.cgi?id=1614700
The filter function in nm_setting_gendata_clear_all() is useful
for when you want to only clear values according to a predicate,
if no such function is supplied all values will be cleared.
https://bugzilla.redhat.com/show_bug.cgi?id=1614700
Only in one moment we need the old and requested settings together:
during _ethtool_coalesce_set(). But for that we shouldn't track both
states in "NMEthtoolCoalesceState".
Simplify "NMEthtoolCoalesceState" to only contain one set of options.
By tracking less state, the code becomes simpler, because you don't
need to wonder where the old and requested state is used.
