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=..." _______________________________________________ networkmanager-list mailing list [email protected] https://mail.gnome.org/mailman/listinfo/networkmanager-list
