Bug 1398925

Hi,

This is a medium sized effort and will not be able to make it in the 7.4 time frame. We will consider this for inclusion in the 7.5 release.

Thanks,
Sushil

Exposed all team properties but the link_watch ones.
The original team.config and team-port.config properties are in sync with the new explicit single properties: this works only when jansson library is available.
If jansson is not available, explicit properties will be ignored, the sync with the .config properties will not be performed: in this case the only way to configure team connections is to change directly the .config properties.

Please review branch fg/team_abstraction_rh1398925

> cli: add team property aliases

aliases mostly were added to accompensate for the historically different syntaxes for `nmcli connection add` and `nmcli connection modify`. I don't think we should add aliases. Having multiple ways to express the same, is not great, just confusing.

> libnm-core: team, after a key is deleted, delete also the parent if needed
    
is there a reason not to merge the commit into the parent commit?


+    g_object_class_install_property
+         (object_class, PROP_MCASTREJOIN_INTERVAL,
+          g_param_spec_int (NM_SETTING_TEAM_MCASTREJOIN_INTERVAL, "", "",
+                            G_MININT32, G_MAXINT32, 0,
+                            G_PARAM_READWRITE |
+                            G_PARAM_CONSTRUCT |
+                            G_PARAM_STATIC_STRINGS));

A property with G_PARAM_CONSTRUCT is always set, during construction of the object. There is some overhead associated with doing that.

It might be justified, if you want to set a default value or force that a complex setter() is always run. But if the default value is 0 anyway, and you don't need the setter to run, you can save the G_PARAM_CONSTRUCT.


>> libnm-core: add tx-hash methods in nm-setting-team

  if (!strcmp (txhash, priv->runner_txhash->pdata[i])) {

I personally dislike strcmp(), but it seems other people prefer it. Ok, that's how it is.
but in the same commit you mix nm_streq() with strcmp() for the same purpose. Could we not ditch this strcmp() use?


If there is a nm_setting_team_get_num_runner_txhash() API, it needs a nm_setting_team_get_get_runner_txhash(self, idx) API as well. Otherwise, you cannot really use this information.



>> libnm-core: take advantage of the new _nm_utils_team_config_set function

 
+/* Keep aligned with team properties enum */
+static struct {

"const"



+   { "runner", "tx_balancer", "interval" }, /* PROP_RUNNER_TXBALANCER_INTER

[PROP_RUNNER_TXBALANCER_INTERVAL] = { "runner", "tx_balancer", "interval" }, 


>> cli: add team properties

+static gboolean
+_set_fcn_team_runner (ARGS_SET_FCN)
+{


let's avoid special implementations of the accessor functions. Why is it not enough to use
  .property_type =                &_pt_gobject_string,

TODO: get rid of check_and_set_string().


> cli: add team.runner-txhash property

this duplicates again behavior. The property type should implement the functionality, and possibly the metadata should control how it behaves (like values_static affects _pt_gobject_string()).

TODO: get rid of DEFINE_REMOVER_INDEX_OR_VALUE() et.al.



> libnm-core: add supported values for team.runner-hwpolicy
> libnm-core: add explicit team properties to NMSettingTeamPort
> libnm-core: share team macros and struct to update json config

merge commits into previous commits??


> libnm-core: share team macros and struct to update json config

nm-core-internal.h is shared between libnm-core and src/. I don't think this should be used from src, so move it to libnm-core/nm-utils-private.h, or one of the *-private.h headers.

    

    

> libnm-core: add functions to align team json config to exposed properties

at this point, does it even make sense to support libnm without libjansson?

It's not only missing nmcli functionality. It's also that the D-Bus API either supports the new team properties or doesn't. Behaving so differently, seems bad to me.

> libnm-core: add explicit team properties to NMSettingTeam

+#define NM_SETTING_TEAM_NOTIFYPEERS_COUNT          "notifypeers-count"
+#define NM_SETTING_TEAM_NOTIFYPEERS_INTERVAL       "notifypeers-interval"
+#define NM_SETTING_TEAM_MCASTREJOIN_COUNT          "mcastrejoin-count"
+#define NM_SETTING_TEAM_MCASTREJOIN_INTERVAL       "mcastrejoin-interval"

In my opinion it's more readable if names have a underscore to split
between words:

teamd notify_peers.count    -> NM_SETTING_TEAM_NOTIFY_PEERS_COUNT
teamd mcast_rejoin.count    -> NM_SETTING_TEAM_MCAST_REJOIN_COUNT

etc...

> libnm-core: add functions to align team json config to exposed properties

+GValue *_nm_utils_team_config_get (const char *conf,
+                                          const char *key,
+                                          const char *key2,
+                                          const char *key3,
+                                          gboolean port_config);

Indentation.


+#define JSON_ADD_OBJ(json, key, key2, key3, value) \

Can this be turned into a regular function?


> libnm-core: take advantage of the new _nm_utils_team_config_get function

+#define JSON_EXTRACT_VAL(var, conf, key, key2, key3) \

+#define JSON_EXTRACT_STRING(var, conf, key, key2, key3) \

+#define JSON_EXTRACT_STRV(var, conf, key, key2, key3, func_add) \

Can these be inline functions? I find a function more readable than a
macro.

(In reply to Thomas Haller from comment #4)
> > cli: add team property aliases
> 
> aliases mostly were added to accompensate for the historically different
> syntaxes for `nmcli connection add` and `nmcli connection modify`. I don't
> think we should add aliases. Having multiple ways to express the same, is
> not great, just confusing.

The original idea was to allow users familiar with teamd properties to edit quickly the connection. But, yes, seems to bring in just more confusion.
Commit dropped.

> 
> > libnm-core: team, after a key is deleted, delete also the parent if needed
>     
> is there a reason not to merge the commit into the parent commit?

Merged

> 
> 
> +    g_object_class_install_property
> +         (object_class, PROP_MCASTREJOIN_INTERVAL,
> +          g_param_spec_int (NM_SETTING_TEAM_MCASTREJOIN_INTERVAL, "", "",
> +                            G_MININT32, G_MAXINT32, 0,
> +                            G_PARAM_READWRITE |
> +                            G_PARAM_CONSTRUCT |
> +                            G_PARAM_STATIC_STRINGS));
> 
> A property with G_PARAM_CONSTRUCT is always set, during construction of the
> object. There is some overhead associated with doing that.
> 
> It might be justified, if you want to set a default value or force that a
> complex setter() is always run. But if the default value is 0 anyway, and
> you don't need the setter to run, you can save the G_PARAM_CONSTRUCT.
> 

Dropped G_PARAM_CONSTRUCT from all properties, thanks.


> 
> >> libnm-core: add tx-hash methods in nm-setting-team
> 
>   if (!strcmp (txhash, priv->runner_txhash->pdata[i])) {
> 
> I personally dislike strcmp(), but it seems other people prefer it. Ok,
> that's how it is.
> but in the same commit you mix nm_streq() with strcmp() for the same
> purpose. Could we not ditch this strcmp() use?
> 

thanks for noticing this, dropped strcmp() in favor of nm_streq()

> 
> If there is a nm_setting_team_get_num_runner_txhash() API, it needs a
> nm_setting_team_get_get_runner_txhash(self, idx) API as well. Otherwise, you
> cannot really use this information.

right, added m_setting_team_get_runner_txhash(self, idx)

> 
> 
> 
> >> libnm-core: take advantage of the new _nm_utils_team_config_set function
> 
>  
> +/* Keep aligned with team properties enum */
> +static struct {
> 
> "const"
> 
> 
> 
> +   { "runner", "tx_balancer", "interval" }, /* PROP_RUNNER_TXBALANCER_INTER
> 
> [PROP_RUNNER_TXBALANCER_INTERVAL] = { "runner", "tx_balancer", "interval" }, 
> 

good advice, changed code, thanks

> 
> >> cli: add team properties
> 
> +static gboolean
> +_set_fcn_team_runner (ARGS_SET_FCN)
> +{
> 
> 
> let's avoid special implementations of the accessor functions. Why is it not
> enough to use
>   .property_type =                &_pt_gobject_string,
> 

done

> 
> 
> > cli: add team.runner-txhash property
> 
> this duplicates again behavior. The property type should implement the
> functionality, and possibly the metadata should control how it behaves (like
> values_static affects _pt_gobject_string()).
> 
> TODO: get rid of DEFINE_REMOVER_INDEX_OR_VALUE() et.al.

well, not found an easy way to do this... if I got it correctly we need to extend NMMetaPropertyTypData to deal with the generic case of a string containing more elements... I would like to address this in a dedicated branch, so leaving as is for now

> 
> 
> 
> > libnm-core: add supported values for team.runner-hwpolicy
> > libnm-core: add explicit team properties to NMSettingTeamPort
> > libnm-core: share team macros and struct to update json config
> 
> merge commits into previous commits??

merged and reorganized the commits: more clean and symmetric history

> 
> 
> > libnm-core: share team macros and struct to update json config
> 
> nm-core-internal.h is shared between libnm-core and src/. I don't think this
> should be used from src, so move it to libnm-core/nm-utils-private.h, or one
> of the *-private.h headers.
> 

libnm-core/nm-utils-private.h seems the best fit among the existing *-private.h ones. Moved, thanks.

>     
> 
>     
> 
> > libnm-core: add functions to align team json config to exposed properties
> 
> at this point, does it even make sense to support libnm without libjansson?
> 
> It's not only missing nmcli functionality. It's also that the D-Bus API
> either supports the new team properties or doesn't. Behaving so differently,
> seems bad to me.

Yes, that's far from being great.
Another option could be to provide some minimal alignment between config and properties when jansson is not available.
Not sure we want to enforce jansson lib dependency at this point (anyway it is true that anyone that needs team functionality will likely need jansson installed). So, not sure, not dealing with this yet in the branch update.

(In reply to Beniamino Galvani from comment #5)
> > libnm-core: add explicit team properties to NMSettingTeam
> 
> +#define NM_SETTING_TEAM_NOTIFYPEERS_COUNT          "notifypeers-count"
> +#define NM_SETTING_TEAM_NOTIFYPEERS_INTERVAL       "notifypeers-interval"
> +#define NM_SETTING_TEAM_MCASTREJOIN_COUNT          "mcastrejoin-count"
> +#define NM_SETTING_TEAM_MCASTREJOIN_INTERVAL       "mcastrejoin-interval"
> 
> In my opinion it's more readable if names have a underscore to split
> between words:
> 
> teamd notify_peers.count    -> NM_SETTING_TEAM_NOTIFY_PEERS_COUNT
> teamd mcast_rejoin.count    -> NM_SETTING_TEAM_MCAST_REJOIN_COUNT
> 
> etc...

I squeezed NOTIFY-PEERS and MACAST-REJOIN because I wanted to reflect there the json nesting level of the property: removed the base suffix:
NM_SETTING_TEAM_
then comes the json property in a "squeezed" form where each underscore hints for a nested property, e.g.:
RUNNER_TXBALANCER_INTERVAL   -> runner.tx_balancer.balancing_interval

the only exception is for "name" properties which are dropped, e.g.:
RUNNER            -> runner.name
RUNNER_TXBALANCER -> runner.tx_balancer.name

I would prefer to keep the nesting info there. But if complaint is strong, I can change it...

> 
> > libnm-core: add functions to align team json config to exposed properties
> 
> +GValue *_nm_utils_team_config_get (const char *conf,
> +                                          const char *key,
> +                                          const char *key2,
> +                                          const char *key3,
> +                                          gboolean port_config);
> 
> Indentation.

Ops... thanks, fixed!

> 
> 
> +#define JSON_ADD_OBJ(json, key, key2, key3, value) \
> 
> Can this be turned into a regular function?
> 
> 
> > libnm-core: take advantage of the new _nm_utils_team_config_get function
> 
> +#define JSON_EXTRACT_VAL(var, conf, key, key2, key3) \
> 
> +#define JSON_EXTRACT_STRING(var, conf, key, key2, key3) \
> 
> +#define JSON_EXTRACT_STRV(var, conf, key, key2, key3, func_add) \
> 
> Can these be inline functions? I find a function more readable than a
> macro.

Done: changed all the above macros to inline functions.


Updated branch fg/team_abstraction_rh1398925 with the changes reported in this and the above comments.

> I squeezed NOTIFY-PEERS and MACAST-REJOIN because I wanted to reflect
> there the json nesting level of the property: removed the base suffix:
> NM_SETTING_TEAM_ then comes the json property in a "squeezed" form
> where each underscore hints for a nested property, e.g.:
> RUNNER_TXBALANCER_INTERVAL -> runner.tx_balancer.balancing_interval
>
> the only exception is for "name" properties which are dropped, e.g.:
> RUNNER            -> runner.name
> RUNNER_TXBALANCER -> runner.tx_balancer.name

I understand the reason, but in my opinion exposed properties should
not necessarily hint at how teamd splits object names. I think the
current convention for NM properties is to split with
dashes/underscores at word boundaries.


> I would prefer to keep the nesting info there. But if complaint is strong, I can change it...


> libnm-core: add functions to align team json config to exposed properties

+#define JSON_ADD_OBJ(json, key, key2, key3, value) \
+       G_STMT_START { \

Can you turn this into a function? (in a later commit/branch is fine
too).

> libnm-core: add team macros and struct to update json config

static inline int
+_nm_utils_json_extract_int (char *conf,
+                            _nm_utils_team_property_keys key)
+{
+       gs_free GValue *t_value = NULL;
+
+       t_value = _nm_utils_team_config_get (conf, key.key1, key.key2, key.key3, FALSE);
+       if (!t_value)
+               return 0;
+
+       return g_value_get_int (t_value);
+}

Leaks the resources associated to t_value; you should call
g_value_unset() before freeing the gvalue (can be a follow-up).

The rest LGTM!

(In reply to Beniamino Galvani from comment #8)
> > I squeezed NOTIFY-PEERS and MACAST-REJOIN because I wanted to reflect
> > there the json nesting level of the property: removed the base suffix:
> > NM_SETTING_TEAM_ then comes the json property in a "squeezed" form
> > where each underscore hints for a nested property, e.g.:
> > RUNNER_TXBALANCER_INTERVAL -> runner.tx_balancer.balancing_interval
> >
> > the only exception is for "name" properties which are dropped, e.g.:
> > RUNNER            -> runner.name
> > RUNNER_TXBALANCER -> runner.tx_balancer.name
> 
> I understand the reason, but in my opinion exposed properties should
> not necessarily hint at how teamd splits object names. I think the
> current convention for NM properties is to split with
> dashes/underscores at word boundaries.
> 

Done.
Changed all the property and function names to have all words split with an underscore.

> 
> > I would prefer to keep the nesting info there. But if complaint is strong, I can change it...
> 
> 
> > libnm-core: add functions to align team json config to exposed properties
> 
> +#define JSON_ADD_OBJ(json, key, key2, key3, value) \
> +       G_STMT_START { \
> 
> Can you turn this into a function? (in a later commit/branch is fine
> too).

Sorry, I missed to change it in the previous rebase.
Done. In the meanwhile I found also that it was buggy: if key3 would have been specified it would have behaved incorrectly (anyway, in current code key3 is always NULL).
So, reworked a bit and changed in static function.

> 
> > libnm-core: add team macros and struct to update json config
> 
> static inline int
> +_nm_utils_json_extract_int (char *conf,
> +                            _nm_utils_team_property_keys key)
> +{
> +       gs_free GValue *t_value = NULL;
> +
> +       t_value = _nm_utils_team_config_get (conf, key.key1, key.key2,
> key.key3, FALSE);
> +       if (!t_value)
> +               return 0;
> +
> +       return g_value_get_int (t_value);
> +}
> 
> Leaks the resources associated to t_value; you should call
> g_value_unset() before freeing the gvalue (can be a follow-up).

Well, I totally forgot this.. thanks, done!

branch fg/team_abstraction_rh1398925 updated (with also few fixes).

> The rest LGTM!

Thanks, going to merge soon

branch fg/team_abstraction_rh1398925 merged on master upstream:

https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=9022b14478a9224ed0cd9e8f3b5a9997597abc46

All properties for team and team-port but the link_watch one are exposed.

Added 'team.link-watchers' and 'team-port.link-watchers' properties in branch:
fg/team_abstraction_link_watchers_rh1398925

Please review

> libnm-core: team: add NMTeamLinkWatcher boxed type

+NMTeamLinkWatcher *
+nm_team_link_watcher_new (const char *name,
+                          gint val1,
+                          gint val2,
+                          gint val3,
+                          const char *target_host,
+                          const char *source_host,
+                          NMTeamLinkWatcherArpPingFlags flags,
+                          GError **error)

Instead of having generic arguments, maybe we can define instead separate constructors:

  nm_team_link_watcher_ethtool_new (delay_up, delay_down)
  nm_team_link_watcher_nsna_ping_new (init_wait, interval, missed_max, target_host)
  nm_team_link_watcher_arp_ping_new (init_wait, interval, missed_max, target_host, source_host, flags)

? This seems a nicer API, but it's less generic. I don't think new
link watchers type will appear in the future, and even if they do
probably we'll have to extend the API anyway.

+	nm_team_link_watcher_set_delay_down;
+	nm_team_link_watcher_set_delay_up;
+	nm_team_link_watcher_set_flags;
+	nm_team_link_watcher_set_init_wait;
+	nm_team_link_watcher_set_interval;
+	nm_team_link_watcher_set_name;
+	nm_team_link_watcher_set_missed_max;
+	nm_team_link_watcher_set_source_host;
+	nm_team_link_watcher_set_target_host;

I wonder if we need setters or if the watchers should be immutable
once constructed...

> libnm-core: add backend for GVariant de/serialization of link_watchers.

+	nm_utils_team_link_watchers_from_variant;
+	nm_utils_team_link_watchers_to_variant;

Do these need to be exported or they can be internal-only?

> libnm-core: synchronize team.config when team.link_watchers is set

@@ -4645,48 +4715,87 @@ _nm_utils_team_config_set (char **conf,
...
-		if (strv) {
+		if (nm_streq (key, "link_watch")) {
+			array = g_value_get_boxed (value);
+			if (!array || !array->len)
+				return FALSE;

Leaks @json.

> libnm-core: synchronize team.link_watchers when team.config is set.

+static NMTeamLinkWatcher *
+_nm_utils_team_link_watcher_from_json (json_t *json_element)
...
+		if (nm_streq (j_key, "name"))
+			name = strdup (json_string_value (j_val));

Leaks @name.

> cli: add support to Team link watchers

Why are some options space-separated and some comma-separated?

 name=ethtool,delay-up=200; name=arp_ping target-host=1.1.1.1 source-host=1.2.3.4

> ifcfg-rh: tests: align json team configuration format to jansson one


-         V1 ("'{ \"device\": \"team0\", \"link_watch\": { \"name\": \"ethtool\" } }'",
-           "{ \"device\": \"team0\", \"link_watch\": { \"name\": \"ethtool\" } }"),
+         V1 ("'{\"device\": \"team0\", \"link_watch\": {\"name\": \"ethtool\"}}'",
+           "{\"device\": \"team0\", \"link_watch\": {\"name\": \"ethtool\"}}"),

This test checks how unescape behaves. If you want to test for a different string, add a new test without dropping the old one.

The tests pass before, why are you changing them? It's fine that the used format in the tests is not normalized like libjansson would do.


> I wonder if we need setters or if the watchers should be immutable
once constructed...

Immutable +1, especially in combination with ref-counting.
I can't remember why we didn't do that for NMIPRoute, maybe because it would be a bit inconvenient with the attributes dictionary (you would have to provide a string list, instead of constructing them one-by-one).

Also, +1 for specialized constructors.




+    if (   !nm_streq (name, NM_TEAM_LINK_WATCHER_ETHTOOL)
+        && !nm_streq (name, NM_TEAM_LINK_WATCHER_ARP_PING)
+
+
+    watcher->name = g_strdup (name);

since the names are from a fixed set, don't clone it, just use the static C-string, one of NM_TEAM_LINK_WATCHER_*.

Also, I dislike how nm_team_link_watcher_new() does 

   initialize_union_type_1()
   if (is_type_1)
       return
   initialize_union_type_2()
   if (is_type_2)
       return
   ...

especially, with this pattern you have leaks for

+    watcher->nsna_ping.target_host = g_strdup (target_host);


it should be

   if (nm_streq (name, NM_TEAM_LINK_WATCHER_ETHTOOL)) {
       watcher = _new_watcher (NM_TEAM_LINK_WATCHER_ETHTOOL);
       watcher->ethtool... = ;
   } else if (nm_streq (name, ...)

or, actually, specialized constructors.



If you have an union, you often need to know which type the union is, and thus often switch on that. It's inconvinient, that the type is a string, require you to use nm_streq() and prevents switch(priv->type).

Don't record the name in the private data at all, have a enum WatcherTypes, which indexes a static array, and a guint8 "type" field:

+nm_team_link_watcher_get_name (NMTeamLinkWatcher *watcher)
+{
+    g_return_val_if_fail (watcher != NULL, NULL);
+    g_return_val_if_fail (watcher->refcount > 0, NULL);
+
+    return watcher_names[watcher->type - 1];
+}


+    g_return_val_if_fail (watcher != NULL, NULL);
+    g_return_val_if_fail (watcher->refcount > 0, NULL);

also, replace this with a macro that does all the entry check you care about. Like the _CHECK_SELF() for platform. Also, add nm_assert (watcher->type > 0 && watcher->type <= WATCHER_TYPE_LAST);

> cli: change printable representation of team/team-port.link_watchers
    
please squash commits that modify a patch that you just did on the same branch.

(In reply to Beniamino Galvani from comment #12)

> [...]
> 
> Instead of having generic arguments, maybe we can define instead separate
> constructors:
> 
>   nm_team_link_watcher_ethtool_new (delay_up, delay_down)
>   nm_team_link_watcher_nsna_ping_new (init_wait, interval, missed_max,
> target_host)
>   nm_team_link_watcher_arp_ping_new (init_wait, interval, missed_max,
> target_host, source_host, flags)
> [...]

Done, looks nicer thanks

> 
> I wonder if we need setters or if the watchers should be immutable
> once constructed...

You are right, setters for link watchers removed

> 
> > libnm-core: add backend for GVariant de/serialization of link_watchers.
> 
> +	nm_utils_team_link_watchers_from_variant;
> +	nm_utils_team_link_watchers_to_variant;
> 
> Do these need to be exported or they can be internal-only?

Moved to internal only

> 
> > libnm-core: synchronize team.config when team.link_watchers is set
> 
> @@ -4645,48 +4715,87 @@ _nm_utils_team_config_set (char **conf,
> ...
> -		if (strv) {
> +		if (nm_streq (key, "link_watch")) {
> +			array = g_value_get_boxed (value);
> +			if (!array || !array->len)
> +				return FALSE;
> 
> Leaks @json.

Nice catch! There were other couples of "return FALSE" in the function, all leaking json... changed to "goto done" where the json is unref before returning... thanks

> 
> > libnm-core: synchronize team.link_watchers when team.config is set.
> 
> +static NMTeamLinkWatcher *
> +_nm_utils_team_link_watcher_from_json (json_t *json_element)
> ...
> +		if (nm_streq (j_key, "name"))
> +			name = strdup (json_string_value (j_val));
> 
> Leaks @name.

Missed it! The function also leaked target_host and source_host... fixed all three, thanks!

> 
> > cli: add support to Team link watchers
> 
> Why are some options space-separated and some comma-separated?
> 
>  name=ethtool,delay-up=200; name=arp_ping target-host=1.1.1.1
> source-host=1.2.3.4

Well, clearly the change in the format (space->comma) was incomplete. After discussing a bit the correct format is to have options space separated. Aligned everywhere.

(In reply to Thomas Haller from comment #13)
> > ifcfg-rh: tests: align json team configuration format to jansson one
> 
> 
> -         V1 ("'{ \"device\": \"team0\", \"link_watch\": { \"name\":
> \"ethtool\" } }'",
> -           "{ \"device\": \"team0\", \"link_watch\": { \"name\":
> \"ethtool\" } }"),
> +         V1 ("'{\"device\": \"team0\", \"link_watch\": {\"name\":
> \"ethtool\"}}'",
> +           "{\"device\": \"team0\", \"link_watch\": {\"name\":
> \"ethtool\"}}"),
> 
> This test checks how unescape behaves. If you want to test for a different
> string, add a new test without dropping the old one.
> 
> The tests pass before, why are you changing them? It's fine that the used
> format in the tests is not normalized like libjansson would do.

The problem here is that we are going to understand the "link_watch" property from json config: so when setting the team.config property, we will process it for the "link_watch" part and we will end up updating the link_watch property, dumping the config via jansson in the team.config parameter: the differences in spaces would trigger the failure in the string comparison:
g_assert_cmpstr (nm_setting_team_get_config (s_team), ==, expected_config);

The only needed change is in the "expected_config" and not in the ifcfg file. But looked nicer to me to update both.

Without link_watch support, no property triggered the processing of the config so it was left untouched.

> 
> 
> > I wonder if we need setters or if the watchers should be immutable
> once constructed...
> 
> Immutable +1, especially in combination with ref-counting.
> I can't remember why we didn't do that for NMIPRoute, maybe because it would
> be a bit inconvenient with the attributes dictionary (you would have to
> provide a string list, instead of constructing them one-by-one).
> 
> Also, +1 for specialized constructors.
> 

Done both

> 
> 
> 
> +    if (   !nm_streq (name, NM_TEAM_LINK_WATCHER_ETHTOOL)
> +        && !nm_streq (name, NM_TEAM_LINK_WATCHER_ARP_PING)
> +
> +
> +    watcher->name = g_strdup (name);
> 
> since the names are from a fixed set, don't clone it, just use the static
> C-string, one of NM_TEAM_LINK_WATCHER_*.

done (in the way you suggested later)

> 
> Also, I dislike how nm_team_link_watcher_new() does 
> 
>    initialize_union_type_1()
>    if (is_type_1)
>        return
>    initialize_union_type_2()
>    if (is_type_2)
>        return
>    ...

Dropped and added specialized constructors, as suggested


> 
> especially, with this pattern you have leaks for
> 
> +    watcher->nsna_ping.target_host = g_strdup (target_host);

Well, in this case there was no leak, as in the union watcher->nsna_ping.target_host matched watcher->arp_ping.target_host which was indeed freed. But anyway, with specialized constructors now it is much more clear.

> [...]
> 
> If you have an union, you often need to know which type the union is, and
> thus often switch on that. It's inconvinient, that the type is a string,
> require you to use nm_streq() and prevents switch(priv->type).
> 
> Don't record the name in the private data at all, have a enum WatcherTypes,
> which indexes a static array, and a guint8 "type" field:
> 
> +nm_team_link_watcher_get_name (NMTeamLinkWatcher *watcher)
> +{
> +    g_return_val_if_fail (watcher != NULL, NULL);
> +    g_return_val_if_fail (watcher->refcount > 0, NULL);
> +
> +    return watcher_names[watcher->type - 1];
> +}

Done... also if I used the "0" in the enum as the ETHTOOL watcher: the all 0 union representing the a watcher would then match the default watcher for team. While I did not used it in any way, looked right to me as a watcher may exist only if the type is specified.

> 
> 
> +    g_return_val_if_fail (watcher != NULL, NULL);
> +    g_return_val_if_fail (watcher->refcount > 0, NULL);
> 
> also, replace this with a macro that does all the entry check you care
> about. Like the _CHECK_SELF() for platform. Also, add nm_assert
> (watcher->type > 0 && watcher->type <= WATCHER_TYPE_LAST);

Done. Thanks

(In reply to Thomas Haller from comment #14)
> > cli: change printable representation of team/team-port.link_watchers
>     
> please squash commits that modify a patch that you just did on the same
> branch.

I left that on top to highlight a change of which I was not 100% sure... in fact, I dropped that commit as we agreed would be better to resort to the "original" spacing. Sorry, was meant to be a way to highlight a change, not to make the review harder. Next time I will add a note in the bugzilla comment instead.

Branch updated: all the above changes and few minor ones added.
Most relevant is that team link watcher "ethtool" with delay-up and delay-down not set (which is the default link watcher used in teamd if no other one is specified) has no more special treatment: before was removed from the json config and never displayed for team master while instead was always displayed and added in team port connections.

Updated  branch fg/team_abstraction_link_watchers_rh1398925
Please have a look, I'm going to test it a little bit more before merging upstream.

Reworked the defaults for runner properties, added some tests and proposed a change in the Makefile for jansson lib inclusion requirements.
Pushed on top of fg/team_abstraction_link_watchers_rh1398925, please review comprehensive branch:
fg/team_abstraction_tests_and_fixes_rh1398925

Thanks

>  libnm-core: team: add NMTeamLinkWatcher boxed type

+typedef enum { /*< flags >*/
+       NM_TEAM_LINK_WATCHER_ARP_PING_FLAG_NONE              = 0, /*< skip >*/
+       NM_TEAM_LINK_WATCHER_ARP_PING_FLAG_VALIDATE_ACTIVE   = (1 << 1),
+       NM_TEAM_LINK_WATCHER_ARP_PING_FLAG_VALIDATE_INACTIVE = (1 << 2),
+       NM_TEAM_LINK_WATCHER_ARP_PING_FLAG_SEND_ALWAYS       = (1 << 3)
+} NMTeamLinkWatcherArpPingFlags;

Can you document these?


> libnm-core: add backend for GVariant de/serialization of link_watchers

+       while (g_variant_iter_next (&iter, "{&sv}", &key, &val1)) {
+               if (!g_variant_lookup (value2, key, "v", &val2)) {
+                       g_variant_unref (val1);
+                       return -1;
+               }
+               if (!g_variant_equal (val1, val2)) {
+                       g_variant_unref (val1);
+                       g_variant_unref (val2);
+                       return -1;
+               }
+       }

I think this leaks @val1 and @val2.


> cli: add support to Team link watchers

+static NMTeamLinkWatcher *
+_parse_team_link_watcher (const char *str,
+                          GError **error)
...
+               pair = nmc_strsplit_set (watcherv[i], "=:", 0);

IPv6 addresses can contain the ':' character. Maybe, can we split only
at '='? Otherwise the following fails:

 $ nmcli c mod t2+ team.link-watchers "name=nsna_ping target-host=2001::1"
 $ Error: failed to modify team.link-watchers: 'target-host=2001::1' is not valid: properties should be specified as 'key=value'.


> libnm-core: add keyfile writer for team link watcher

Is this only needed for tests? I don't think we should add duplicate
information to keyfile.


> libnm-core: use proper writer for team-port link-watchers

Here too.

Pushed a couple of trivial fixups. The rest LGTM.

+nm_team_link_watcher_ethtool_new (gint delay_up,
+                                  gint delay_down,
+                                  GError **error)
+{
+    NMTeamLinkWatcher *watcher;
+
+    watcher = g_slice_new0 (NMTeamLinkWatcher);
+    watcher->refcount = 1;
+
+    watcher->type = LINK_WATCHER_ETHTOOL;
+    watcher->ethtool.delay_up = delay_up;

what's the meaning of setting delay_up to -1 or 0? I think the function should validate the input and error out.


There are some string arguments, like target_host. Later on, nmcli's parsing does not provision any way for escaping/quoting. You must either:
  - make it impossible to create target hosts that mess up the parsing.
    Basically, error out in nm_team_link_watcher_nsna_ping_new() if 
      - target_host is empty
      - target_host contains any delimiter character
      - target_host contains a backslash (so that we later could introduce a
        escaping mechanism without breaking existing configurations.
  - properly escape/unescape special character whereever necessary. Especially,
    take care of '\'.





nm_team_link_watcher_unref()
+    if (watcher->refcount == 0) {
+         g_free (watcher->arp_ping.target_host);

Let's not access union-members without checking the type first. This makes non-obvious assumptions about the memory layout.

same in nm_team_link_watcher_equal().




+int
+nm_team_link_watcher_get_delay_up (NMTeamLinkWatcher *watcher)
+{
+    return nm_team_link_watcher_get_init_wait (watcher);
+}

This is confusing

I think calling nm_team_link_watcher_get_delay_up() on an instance that is not of type LINK_WATCHER_ETHTOOL should either return a default or signal an error.
Either:

int
nm_team_link_watcher_get_delay_up (NMTeamLinkWatcher *watcher)
{
     if (watcher->type != LINK_WATCHER_ETHTOOL)
         return -1;
     return watcher->ethtool.delay_up;
}

or

int
nm_team_link_watcher_get_delay_up (NMTeamLinkWatcher *watcher, int *out_delay_up)
{
     if (watcher->type != LINK_WATCHER_ETHTOOL)
         return -EINVAL;
     NM_SET_OUT (out_delay_up, watcher->ethtool.delay_up);
     return 0;
}

or

gboolean
nm_team_link_watcher_get_delay_up (NMTeamLinkWatcher *watcher, int *out_delay_up)
{
     if (watcher->type != LINK_WATCHER_ETHTOOL)
         return FALSE;
     NM_SET_OUT (out_delay_up, watcher->ethtool.delay_up);
     return TRUE;
}


Since nm_team_link_watcher_get_delay_up() only works for "ethtool", maybe it should be called nm_team_link_watcher_ethtool_get_delay_up(). That way, it resonates with nm_team_link_watcher_ethtool_new().

The same on the D-Bus API, where it's called "delay-up", instead of "ethtool-delay-up". What is the reason for this? Sorry, did we discuss this already? :)

Otherwise, I would nm_team_link_watcher_*_new() rather call nm_team_link_watcher_new_*().



Beniamino, what do you think?



since the objects are immutable and ref-counted, I don't think there is any need for nm_team_link_watcher_dup().







+         if (!NM_IN_STRSET (name,
+                            NM_TEAM_LINK_WATCHER_ETHTOOL,
+                            NM_TEAM_LINK_WATCHER_ARP_PING,
+                            NM_TEAM_LINK_WATCHER_NSNA_PING)) {
+              g_warning ("Ignoring link watcher: unknow name '%s'", name);

Logging is no appropriate reaction for the library.
When the server receives an bogus name, it should fail and return an error. Otherwise, the input error is just silently ignored without the user noticing.

On the other hand, if server introduces a new link-watcher type, all old libnm clients must silently ignore it. Warning can even crash with G_DEBUG=fatal-warnings and is annoying at best.

You must differenciate whether you are parsing as the server or as the client.
See _nm_utils_is_manager_process.




+    if (len1 != len2)
+         return len1 < len2 ? -1 : len1 > len2;
+
+    g_variant_iter_init (&iter, value1);
+    while (g_variant_iter_next (&iter, "{&sv}", &key, &val1)) {
+         if (!g_variant_lookup (value2, key, "v", &val2)) {

a GVariant "dictionary" is not really a dictionary. It's a list of key-value pairs and lookup just runs of the list searching for the first match.

That means, you can construct a dictionary with duplicate keys. and it means, that it is not enough to 
  - compare both lengths
  - compare that every key in "value1" is also in "value2"
because the keys it would return equal for
  value1 = { 'a': 'value1', 'a': 'value1' }
  value2 = { 'a': 'value1', 'b': 'value2' }



Yes, _nm_property_compare_strdict() should be fixed as well.





+         int_val = nm_team_link_watcher_get_missed_max (watcher);
+         if (int_val != 3) {

just always serialize.




+              if (!g_variant_lookup (watcher_var, "delay-down", "i", &val2))
+                   val2 = 0;

"i" is 32 bit. If the D-Bus serialization format explicitly can contain only 32 bit integers, then the libnm API should have the exact same type.

int nm_team_link_watcher_get_delay_down (NMTeamLinkWatcher *watcher)
NMTeamLinkWatcher *nm_team_link_watcher_ethtool_new (gint delay_up,
                                                     gint delay_down,

or alternatively, nm_team_link_watcher_ethtool_new() should check that delay_up <= G_MAXINT32 (in addition to > 0).



if (g_variant_lookup (watcher_var, "send-always", "b"))

g_variant_lookup() expects an output boolean argument. This overwrites the stack.



> cli: add support to Team link watchers

Is it not possible to convert the watcher to a GVariant of a GHashTable and then use  NMVariantAttributeSpec and nm_utils_parse_variant_attributes()? Instead, of implementing a new parser (that lacks escaping). Also, the parsing code is not usable form outside libnm, which would be nice-to-have.

(In reply to Beniamino Galvani from comment #20)
> >  libnm-core: team: add NMTeamLinkWatcher boxed type
> 
> +typedef enum { /*< flags >*/
> +       NM_TEAM_LINK_WATCHER_ARP_PING_FLAG_NONE              = 0, /*< skip
> >*/
> +       NM_TEAM_LINK_WATCHER_ARP_PING_FLAG_VALIDATE_ACTIVE   = (1 << 1),
> +       NM_TEAM_LINK_WATCHER_ARP_PING_FLAG_VALIDATE_INACTIVE = (1 << 2),
> +       NM_TEAM_LINK_WATCHER_ARP_PING_FLAG_SEND_ALWAYS       = (1 << 3)
> +} NMTeamLinkWatcherArpPingFlags;
> 
> Can you document these?

Done

> 
> 
> > libnm-core: add backend for GVariant de/serialization of link_watchers
> 
> +       while (g_variant_iter_next (&iter, "{&sv}", &key, &val1)) {
> +               if (!g_variant_lookup (value2, key, "v", &val2)) {
> +                       g_variant_unref (val1);
> +                       return -1;
> +               }
> +               if (!g_variant_equal (val1, val2)) {
> +                       g_variant_unref (val1);
> +                       g_variant_unref (val2);
> +                       return -1;
> +               }
> +       }
> 
> I think this leaks @val1 and @val2.

Good catch... fixed.

> 
> 
> > cli: add support to Team link watchers
> 
> +static NMTeamLinkWatcher *
> +_parse_team_link_watcher (const char *str,
> +                          GError **error)
> ...
> +               pair = nmc_strsplit_set (watcherv[i], "=:", 0);
> 
> IPv6 addresses can contain the ':' character. Maybe, can we split only
> at '='? Otherwise the following fails:
> 
>  $ nmcli c mod t2+ team.link-watchers "name=nsna_ping target-host=2001::1"
>  $ Error: failed to modify team.link-watchers: 'target-host=2001::1' is not
> valid: properties should be specified as 'key=value'.

Right. I did that because looked me smart to let use the ':' separator too as it is used in json config... but it was not. Removed the ':' separator.

> 
> 
> > libnm-core: add keyfile writer for team link watcher
> 
> Is this only needed for tests? I don't think we should add duplicate
> information to keyfile.

Changed with a NULL writer.

> 
> 
> > libnm-core: use proper writer for team-port link-watchers
> 
> Here too.
> 
> Pushed a couple of trivial fixups. The rest LGTM.

fixups merged, thanks!

(In reply to Thomas Haller from comment #21)
> I think calling nm_team_link_watcher_get_delay_up() on an instance that is
> not of type LINK_WATCHER_ETHTOOL should either return a default or signal an
> error.
> Either:
> 
> int
> nm_team_link_watcher_get_delay_up (NMTeamLinkWatcher *watcher)
> {
>      if (watcher->type != LINK_WATCHER_ETHTOOL)
>          return -1;
>      return watcher->ethtool.delay_up;
> }

I like this one.

> Since nm_team_link_watcher_get_delay_up() only works for "ethtool", maybe it
> should be called nm_team_link_watcher_ethtool_get_delay_up(). That way, it
> resonates with nm_team_link_watcher_ethtool_new().

I don't have a preference... the property is specific to a single watcher and so it would be more clear if it had the prefix.  OTOH, there are properties that are common to multiple watchers (but not all) and they can't have a prefix; so I think it's also fine if we avoid the prefix because it would be only for some properties and not others.

> 
> The same on the D-Bus API, where it's called "delay-up", instead of
> "ethtool-delay-up". What is the reason for this? Sorry, did we discuss this
> already? :)


> Otherwise, I would nm_team_link_watcher_*_new() rather call
> nm_team_link_watcher_new_*().

Sounds good.

(In reply to Beniamino Galvani from comment #23)
> (In reply to Thomas Haller from comment #21)
> > I think calling nm_team_link_watcher_get_delay_up() on an instance that is
> > not of type LINK_WATCHER_ETHTOOL should either return a default or signal an
> > error.
> > Either:
> > 
> > int
> > nm_team_link_watcher_get_delay_up (NMTeamLinkWatcher *watcher)
> > {
> >      if (watcher->type != LINK_WATCHER_ETHTOOL)
> >          return -1;
> >      return watcher->ethtool.delay_up;
> > }
> 
> I like this one.

Agreed.

> 
> > Since nm_team_link_watcher_get_delay_up() only works for "ethtool", maybe it
> > should be called nm_team_link_watcher_ethtool_get_delay_up(). That way, it
> > resonates with nm_team_link_watcher_ethtool_new().
> 
> I don't have a preference... the property is specific to a single watcher
> and so it would be more clear if it had the prefix.  OTOH, there are
> properties that are common to multiple watchers (but not all) and they can't
> have a prefix; so I think it's also fine if we avoid the prefix because it
> would be only for some properties and not others.
> 

Yeah, this is also my point. I would leave it as generic... see following comment...

> > 
> > The same on the D-Bus API, where it's called "delay-up", instead of
> > "ethtool-delay-up". What is the reason for this? Sorry, did we discuss this
> > already? :)
> 
> 
> > Otherwise, I would nm_team_link_watcher_*_new() rather call
> > nm_team_link_watcher_new_*().
> 
> Sounds good.

Agreed

(In reply to Thomas Haller from comment #21)
> +nm_team_link_watcher_ethtool_new (gint delay_up,
> +                                  gint delay_down,
> +                                  GError **error)
> +{
> +    NMTeamLinkWatcher *watcher;
> +
> +    watcher = g_slice_new0 (NMTeamLinkWatcher);
> +    watcher->refcount = 1;
> +
> +    watcher->type = LINK_WATCHER_ETHTOOL;
> +    watcher->ethtool.delay_up = delay_up;
> 
> what's the meaning of setting delay_up to -1 or 0? I think the function
> should validate the input and error out.

Added checks on the integer values for all the three specialized constructors

> 
> 
> There are some string arguments, like target_host. Later on, nmcli's parsing
> does not provision any way for escaping/quoting. You must either:
>   - make it impossible to create target hosts that mess up the parsing.
>     Basically, error out in nm_team_link_watcher_nsna_ping_new() if 
>       - target_host is empty
>       - target_host contains any delimiter character
>       - target_host contains a backslash (so that we later could introduce a
>         escaping mechanism without breaking existing configurations.
>   - properly escape/unescape special character whereever necessary.
> Especially,
>     take care of '\'.
> 

Checked in the constructors that escape/quoting characters like:
' ', ':', '\', '/', '\t', '=', '"', ''' are not present in the strings.

> 
> 
> 
> 
> nm_team_link_watcher_unref()
> +    if (watcher->refcount == 0) {
> +         g_free (watcher->arp_ping.target_host);
> 
> Let's not access union-members without checking the type first. This makes
> non-obvious assumptions about the memory layout.

But this is exactly the aim: leverage the way in which the union is constructed.
I added a comment just before the union to warn about changing it as the memory layout should be preserved as some functions rely on it.

I find more convenient coding the api leveraging the union, and the way the memory layout is used is an implementation detail invisible when using the APIs.
Besides the _unref(), also the _get_init_wait() or _get_interval() and all the getters for nsna_ping and arp_ping watchers are shared and don't need to check the watcher type to retrieve the correct struct.
I would prefer to keep it in this way, but of course we can change it if needed.

> 
> same in nm_team_link_watcher_equal().

This makes the code shorter... I like it as is, but, as said above, I can change it if you find it too bad.

> 
> 
> 
> 
> +int
> +nm_team_link_watcher_get_delay_up (NMTeamLinkWatcher *watcher)
> +{
> +    return nm_team_link_watcher_get_init_wait (watcher);
> +}
> 
> This is confusing
> 
> I think calling nm_team_link_watcher_get_delay_up() on an instance that is
> not of type LINK_WATCHER_ETHTOOL should either return a default or signal an
> error.
> Either:
> 
> int
> nm_team_link_watcher_get_delay_up (NMTeamLinkWatcher *watcher)
> {
>      if (watcher->type != LINK_WATCHER_ETHTOOL)
>          return -1;
>      return watcher->ethtool.delay_up;
> }
> 
> or
> 
> int
> nm_team_link_watcher_get_delay_up (NMTeamLinkWatcher *watcher, int
> *out_delay_up)
> {
>      if (watcher->type != LINK_WATCHER_ETHTOOL)
>          return -EINVAL;
>      NM_SET_OUT (out_delay_up, watcher->ethtool.delay_up);
>      return 0;
> }
> 
> or
> 
> gboolean
> nm_team_link_watcher_get_delay_up (NMTeamLinkWatcher *watcher, int
> *out_delay_up)
> {
>      if (watcher->type != LINK_WATCHER_ETHTOOL)
>          return FALSE;
>      NM_SET_OUT (out_delay_up, watcher->ethtool.delay_up);
>      return TRUE;
> }

Agreed. Added checks on watcher type for those properties that are mapped in the same memory location but have different names: "delay-up", "delay-down", "init-wait", "interval" and "missed-max" (this last one for coherence)

> 
> 
> Since nm_team_link_watcher_get_delay_up() only works for "ethtool", maybe it
> should be called nm_team_link_watcher_ethtool_get_delay_up(). That way, it
> resonates with nm_team_link_watcher_ethtool_new().
> 
> The same on the D-Bus API, where it's called "delay-up", instead of
> "ethtool-delay-up". What is the reason for this? Sorry, did we discuss this
> already? :)

This makes sense but... in order to be coherent we should do the same with all the properties, i.e., "interval", "init-wait", "missed-max", ...  seems to me better to have just one _get_interval () instead of two functions: nsna_ping_get_interval() and arp_ping_get_interval(). Same with all the properties that appear both in nsna_ping and arp_ping: I would not like this.
So, I would not change this...

> 
> Otherwise, I would nm_team_link_watcher_*_new() rather call
> nm_team_link_watcher_new_*().

Agreed. Changed the specialized constructor names in this way.

> 
> Beniamino, what do you think?
> 
> 
> 
> since the objects are immutable and ref-counted, I don't think there is any
> need for nm_team_link_watcher_dup().

Well, I think could be useful after all and does not harm... moreover, is currently used as a generic copy functions for link-watchers in a couple of places (G_BOXED DEFINE, "set" and "get" of PROP_LINK_WATCHERS). I would leave it.

> 
> 
> 
> 
> 
> 
> 
> +         if (!NM_IN_STRSET (name,
> +                            NM_TEAM_LINK_WATCHER_ETHTOOL,
> +                            NM_TEAM_LINK_WATCHER_ARP_PING,
> +                            NM_TEAM_LINK_WATCHER_NSNA_PING)) {
> +              g_warning ("Ignoring link watcher: unknow name '%s'", name);
> 
> Logging is no appropriate reaction for the library.
> When the server receives an bogus name, it should fail and return an error.
> Otherwise, the input error is just silently ignored without the user
> noticing.

I did this as we already did it for many other functions:
nm_utils_ip4_addresses_from_variant()
nm_utils_ip4_routes_from_variant()
nm_utils_ip6_dns_from_variant()
...

But it is ok for me to remove the g_warnings: all removed.


> 
> On the other hand, if server introduces a new link-watcher type, all old
> libnm clients must silently ignore it. Warning can even crash with
> G_DEBUG=fatal-warnings and is annoying at best.
> 
> You must differenciate whether you are parsing as the server or as the
> client.
> See _nm_utils_is_manager_process.
> 
> 
> 
> 
> +    if (len1 != len2)
> +         return len1 < len2 ? -1 : len1 > len2;
> +
> +    g_variant_iter_init (&iter, value1);
> +    while (g_variant_iter_next (&iter, "{&sv}", &key, &val1)) {
> +         if (!g_variant_lookup (value2, key, "v", &val2)) {
> 
> a GVariant "dictionary" is not really a dictionary. It's a list of key-value
> pairs and lookup just runs of the list searching for the first match.
> 
> That means, you can construct a dictionary with duplicate keys. and it
> means, that it is not enough to 
>   - compare both lengths
>   - compare that every key in "value1" is also in "value2"
> because the keys it would return equal for
>   value1 = { 'a': 'value1', 'a': 'value1' }
>   value2 = { 'a': 'value1', 'b': 'value2' }
> 
> 
> 
> Yes, _nm_property_compare_strdict() should be fixed as well.

You are right. Anyway, which is the meaning of having multiple equal keys in a configuration? They should be... keys! While I agree this is something we may want to take care of as we don't enforce keys are unique, I would leave it as a future improvement, as not critical.
> 
> 
> 
> 
> 
> +         int_val = nm_team_link_watcher_get_missed_max (watcher);
> +         if (int_val != 3) {
> 
> just always serialize.
In this case I think would be better not... when missed-max is not present the assumption is that it has the default value, which is 3. Seems fine to skip it from the variant dictionary. We skip it everywhere when it gets the default value.

> 
> 
> 
> 
> +              if (!g_variant_lookup (watcher_var, "delay-down", "i", &val2))
> +                   val2 = 0;
> 
> "i" is 32 bit. If the D-Bus serialization format explicitly can contain only
> 32 bit integers, then the libnm API should have the exact same type.
> 
> int nm_team_link_watcher_get_delay_down (NMTeamLinkWatcher *watcher)
> NMTeamLinkWatcher *nm_team_link_watcher_ethtool_new (gint delay_up,
>                                                      gint delay_down,
> 
> or alternatively, nm_team_link_watcher_ethtool_new() should check that
> delay_up <= G_MAXINT32 (in addition to > 0).

Ok, added the check that all int values in the specialized constructors are in [0, G_MAXINT32].

> 
> 
> 
> if (g_variant_lookup (watcher_var, "send-always", "b"))
> 
> g_variant_lookup() expects an output boolean argument. This overwrites the
> stack.

Fixed: now the value is put in a var. This fixes also the retrieval of the value when it is found with the default value "FALSE".

> 
> 
> 
> > cli: add support to Team link watchers
> 
> Is it not possible to convert the watcher to a GVariant of a GHashTable and
> then use  NMVariantAttributeSpec and nm_utils_parse_variant_attributes()?
> Instead, of implementing a new parser (that lacks escaping). Also, the
> parsing code is not usable form outside libnm, which would be nice-to-have.

I saw NMVariantAttributeSpec but I did not take it into account as I saw the 2 bool in the structure mapping v4 and v6 and I thought was for ip properties only... I was wrong. I agree that it could be leveraged for team parsing just ignoring v4 and v6... but at this point, as I have also added the checks for the quoting characters in the link-watchers constructors, I would let it as a further improvement.

Merged the fixups, reworked as per review requests as detailed in above comments and repushed:

fg/team_abstraction_tests_and_fixes_rh1398925

LGTM

+    strv = g_strsplit (target_host, " :\\/\t=\"\'", 0);
+    if (strv[1]) {

if (strpbrk (target_host, " :\\/\t=\"\'"))




nm_team_link_watcher_dup:

+         g_assert_not_reached ();
+         return NULL;


g_return_val_if_reached (NULL);



+NMTeamLinkWatcherArpPingFlags
+nm_team_link_watcher_get_flags (NMTeamLinkWatcher *watcher)
+{
+    _CHECK_WATCHER (watcher, 0);
+
+    return watcher->arp_ping.flags;

This is clearly only relevant for arp-ping, as also the NMTeamLinkWatcherArpPingFlags type indicates. At least this function should be called get_arp_ping_flags().
With nm_team_link_watcher_get_delay_up() you could at least argue, that one future (still non-existing) watcher could have a delay-up property. Although currently it is also just valid for LINK_WATCHER_ETHTOOL.

I dislike that calling these functions on a watcher of an unexpected type is actually a bug, but the function won't signal any error. For nm_team_link_watcher_get_delay_up() it returns -1 (ok, at least, that it out for range to be a value for this -- although, it's not documented either).

Ok, whatever. But some careful considerations are in order here, because this is public API that is gonna stay with us, basically forever.



> Well, I think could be useful after all and does not harm... moreover, is 
> currently used as a generic copy functions for link-watchers in a couple of 
> places (G_BOXED DEFINE, "set" and "get" of PROP_LINK_WATCHERS). I would leave 
> it.

A small harm is that it increases up the API and required documentation and adds code.
Why do you want to actually clone the memory (in G_BOXED_DEFINE/PROP_LINK_WATCHERS)? As these are immutable+refcounted objects, you can just take an additional reference?


> I did this as we already did it for many other functions:
> nm_utils_ip4_addresses_from_variant()
> nm_utils_ip4_routes_from_variant()
> nm_utils_ip6_dns_from_variant()
> ...
> 
> But it is ok for me to remove the g_warnings: all removed.

The existing functions are bugs, or at least limited API (TODO).
Since these functions cannot fail (except g_warn() -- which is essentially an assertion failure!), they are not supposed to be used against untrusted data. But commonly you get the GVariant from some other application via D-Bus, thus untrusted by design.

As this is public API, you need two more arguments arguments:

  gboolean best_effort /* whether to ignore all unknown/invalid attributes */
  GError **error /* return a message when something fails. Especially with 
                    !best_effort, there are failure cases. */

server side, would then parse GVariants with !best_effort. On the other hand, client side, you want to make the best of what the server sends you. Since client needs to be forward compatible with server, it must forgive unknown fields -- the could be from a newer version.

> nm_setting_team_port_get_link_watcher (NMSettingTeamPort *setting, int idx)

can you make the idx type guint (other public API functions as well)? It indexes GPtrArray, whose index is guint. The types should be identical.

Also simplifies:

  g_return_val_if_fail (idx >= 0 && idx < priv->link_watchers->len, NULL);

(In reply to Thomas Haller from comment #30)
> > nm_setting_team_port_get_link_watcher (NMSettingTeamPort *setting, int idx)
> 
> can you make the idx type guint (other public API functions as well)? It
> indexes GPtrArray, whose index is guint. The types should be identical.
> 
> Also simplifies:
> 
>   g_return_val_if_fail (idx >= 0 && idx < priv->link_watchers->len, NULL);

done.

Also added some other changes/fixes, most notable:
- link-watchers strings (target-host, source-host) are checked for escape characters in constructors
- exposed team and team-port properties are not written to keyfiles
- moved tests to shared test-setting file (cherry-picked commit from Lubo and added there team and team-port tests)

Rebased on master and updated branch.

Merged last part (fg/team_abstraction_tests_and_fixes_rh1398925) upstream:
https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=4d1796b2c7caf67805704218e554fc39ceb94614

Cherry-picked both branches on nm-1-10:
https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=f5e358eb46c70d51b0a218f0fb76b564aa610e29
https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=f092cc0f94bef57c6aa7b5637ef40d5c18cf84d4

Working well, few details filed into bugzilla (1533830, 1533847, 1533813, 1533926)

Current doctext is not clear on whether this applies to libteam or NetworkManager. Please improve in a way that it's also clear on what is being changed, and not just on how. Thanks

(In reply to Marcelo Ricardo Leitner from comment #44)
> Current doctext is not clear on whether this applies to libteam or
> NetworkManager. Please improve in a way that it's also clear on what is
> being changed, and not just on how. Thanks

Tried to improve a bit the DocText.
Ioanna, please review it.

Thanks

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHBA-2018:0778