Re: [patch NetworkManager 0/6] add support for network team devices

[Dan Winship <danw gnome org>]

On 07/11/2013 11:31 AM, Jiri Pirko wrote:
> I heavily reused existing code, mainly bonding parts.

Makes sense (and makes it easy to review...)

> The only thing not
> implemented yet is dbus communication with teamd (used particulary for
> port config setup).

Will that allow you to shut down teamd via d-bus too? The current
teamd-handling is kind of a weird mix of direct spawn/kill vs dbus... (I
guess it's not possible to use dbus activation to start teamd, because
you need a separate teamd process for each interface? Although the
initscripts seem to do this via some systemd magic... I wonder if that
magic is available to us programmatically somehow?)


I don't really like having a single string-valued "config" property on
NMSettingTeam... we did basically that with "options" in NMSettingBond,
and later decided it was a bad idea (which is why we didn't do the same
thing with bridges).

But I guess if it's possible to add new runners and/or link_watches then
there's no way NMSettingTeam could define properties for all possible
options, and so we have no choice but to use the json, right?

In that case we will probably eventually want to have NMSettingTeam
parse the config, and ensure that it's valid json, and contains the
mandatory properties. Maybe libteam has a function for that?

Is it expected/allowed for the NMSettingTeam's config to contain "port"
elements, and if so, what happens if they don't match up with the
available NMConnections for the port devices? I think we want to say
"no" here and pass "-n" to teamd. (Or if we're parsing the json in
NMSettingTeam we could strip out the ports, or reject the config if it
contains ports.) (Any of these would be inconsistent with the current
behavior of the Fedora initscripts, but maybe they should change too? I
don't think there's any other situation where one device's ifcfg file
can cause another device to be configured.)


Other than the possible config issues, the code looks good. Two minor
bugs I noticed:

You added an '#include "nm-setting-team.h"' to src/nm-device-ethernet.c,
but it's not actually used. (Nor is the nm-setting-bond.h include above
it... probably leftover from old code.)

write_team_port_setting() mistakenly writes out "TEAM_CONFIG=..."
instead of "TEAM_PORT_CONFIG=..."