> From: "Jiri Pirko" <j...@resnulli.us> > > Fri, Jul 12, 2013 at 10:08:49PM CEST, d...@gnome.org wrote: > >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?) > > Not possible to start or stop teamd via d-bus. Standard process api is > used for that work. Also, it is possible to use systemd to spawn teamd > for NM but I think that NM should be doing such things himself to take > control over all needed processes. Plus systemd approach would be > limited only for systems based on systemd.
+1 NetworkManager currently doesn't require systemd. > >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). > > teamd config is non-trivial tree structure (JSON). At this point, there is > really no point to have NM to be aware of the teamd config structure. > Once NM will need that, this can be changed. > > > > >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? > > At this point, I strongly believe so. I have the same feeling here. > >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? > > No function for that. Teamd will scream when you pass non-valid JSON to > it. Also will scream in case some configuration is not done correctly. > At this point, I would make NM non-aware at all of the config structure. > Just leave it as string and NM to blindly pass it through... Connection take over will require a bit more love but for now we can just consider it unsupported for team. > >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.) > > When config of a port is set over Dbus, the original config of this port > is overwritten in case it have existed. > > Yes you are probably correct. Teamd could be rather executed with > -n / --no-ports. Good point. How do you store the port config then? > >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.) > > Yep, I forgave to strip this out. Noted. > > > > >write_team_port_setting() mistakenly writes out "TEAM_CONFIG=..." > >instead of "TEAM_PORT_CONFIG=..." > > Noted. > > Will fix these and repost. Nice. Cheers, Pavel _______________________________________________ networkmanager-list mailing list networkmanager-list@gnome.org https://mail.gnome.org/mailman/listinfo/networkmanager-list
