On Wed, 2013-07-24 at 07:37 +0200, Jiri Pirko wrote:
> Wed, Jul 24, 2013 at 01:53:56AM CEST, d...@redhat.com wrote:
> >On Mon, 2013-07-15 at 09:46 +0200, Jiri Pirko wrote:
> >> Signed-off-by: Jiri Pirko <j...@resnulli.us>
> >> ---
> >> src/devices/nm-device-team.c | 283
> >> ++++++++++++++++++++++++++++++++++++++++++-
> >> 1 file changed, 282 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/src/devices/nm-device-team.c b/src/devices/nm-device-team.c
> >> index 320b659..ac0467c 100644
> >> --- a/src/devices/nm-device-team.c
> >> +++ b/src/devices/nm-device-team.c
> >> @@ -20,8 +20,13 @@
> >>
> >> #include "config.h"
> >>
> >> +#include <sys/types.h>
> >> +#include <unistd.h>
> >> +#include <signal.h>
> >> +#include <sys/wait.h>
> >> #include <glib.h>
> >> #include <glib/gi18n.h>
> >> +#include <gio/gio.h>
> >>
> >> #include <netinet/ether.h>
> >>
> >> @@ -34,6 +39,7 @@
> >> #include "nm-dbus-glib-types.h"
> >> #include "nm-dbus-manager.h"
> >> #include "nm-enum-types.h"
> >> +#include "nm-posix-signals.h"
> >>
> >> #include "nm-device-team-glue.h"
> >>
> >> @@ -45,7 +51,11 @@ G_DEFINE_TYPE (NMDeviceTeam, nm_device_team,
> >> NM_TYPE_DEVICE)
> >> #define NM_TEAM_ERROR (nm_team_error_quark ())
> >>
> >> typedef struct {
> >> - int dummy;
> >> + GPid teamd_pid;
> >> + guint teamd_process_watch;
> >> + guint teamd_timeout;
> >> + guint teamd_dbus_watch;
> >> + gboolean teamd_on_dbus;
> >> } NMDeviceTeamPrivate;
> >>
> >> enum {
> >> @@ -179,9 +189,267 @@ match_l2_config (NMDevice *self, NMConnection
> >> *connection)
> >>
> >> /******************************************************************/
> >>
> >> +typedef struct {
> >> + int pid;
> >> +} KillInfo;
> >> +
> >> +static gboolean
> >> +ensure_killed (gpointer data)
> >> +{
> >> + KillInfo *info = data;
> >> +
> >> + if (kill (info->pid, 0) == 0)
> >> + kill (info->pid, SIGKILL);
> >> +
> >> + /* ensure the child is reaped */
> >> + nm_log_dbg (LOGD_TEAM, "waiting for teamd pid %d to exit", info->pid);
> >> + waitpid (info->pid, NULL, 0);
> >> + nm_log_dbg (LOGD_TEAM, "teamd pid %d cleaned up", info->pid);
> >> +
> >> + g_free (info);
> >> + return FALSE;
> >> +}
> >> +
> >> +static void
> >> +service_kill (int pid)
> >> +{
> >> + if (kill (pid, SIGTERM) == 0) {
> >> + KillInfo *info;
> >> +
> >> + info = g_malloc0 (sizeof (KillInfo));
> >> + info->pid = pid;
> >
> >You don't actually need the 'info' thing here, since the pid will always
> >be a 32-bit integer (AFAIK?), you can use GINT_TO_POINTER (pid) and skip
> >all the allocation/free stuff. So it would really end up as:
> >
> >static gboolean
> >ensure_killed (gpointer data)
> >{
> > int pid = GPOINTER_TO_INT (data);
> >
> > kill (pid);
> > ...
> > return FALSE;
> >}
> >
> >static void
> >service_kill (...)
> >{
> > if (kill (pid, SIGTERM)) {
> > g_timeout_add_seconds (2, ensure_killed, GINT_TO_POINTER (pid));
> > }
> > ...
> >}
>
> You are right that structure is not needed per say. But even with int, I
> would have to do malloc/free anyway (because ensure_killed() is called
> when original pid variable is not available anymore)
Actually you still don't need to malloc/free, because the user_data
pointer is stored by glib. Since the PID here is just a value, and not
a pointer to anything, we don't need to worry about malloc/free at all.
Now, if we wanted to pass multiple values to ensure_killed(), yeah we'd
need a struct and we'd need to malloc/free it, but as long as we're
using a value that can fit into a pointer, malloc/free isn't required.
(more below)
> I always like to wrap this in struct, seems nicer to me.
>
>
> >
> >> + g_timeout_add_seconds (2, ensure_killed, info);
> >> + }
> >> + else {
> >> + kill (pid, SIGKILL);
> >> +
> >> + /* ensure the child is reaped */
> >> + nm_log_dbg (LOGD_TEAM, "waiting for teamd pid %d to exit", pid);
> >> + waitpid (pid, NULL, 0);
> >> + nm_log_dbg (LOGD_TEAM, "teamd pid %d cleaned up", pid);
> >> + }
> >> +}
> >> +
> >> +static void
> >> +teamd_timeout_remove (NMDevice *dev)
> >> +{
> >> + NMDeviceTeamPrivate *priv = NM_DEVICE_TEAM_GET_PRIVATE (dev);
> >> +
> >> + if (priv->teamd_timeout) {
> >> + g_source_remove (priv->teamd_timeout);
> >> + priv->teamd_timeout = 0;
> >> + }
> >> +}
> >> +
> >> +static void
> >> +teamd_cleanup (NMDevice *dev)
> >> +{
> >> + NMDeviceTeamPrivate *priv = NM_DEVICE_TEAM_GET_PRIVATE (dev);
> >> +
> >> + if (priv->teamd_dbus_watch) {
> >> + g_source_remove (priv->teamd_dbus_watch);
> >> + priv->teamd_dbus_watch = 0;
> >> + }
> >> +
> >> + if (priv->teamd_process_watch) {
> >> + g_source_remove (priv->teamd_process_watch);
> >> + priv->teamd_process_watch = 0;
> >> + }
> >> +
> >> + if (priv->teamd_pid > 0) {
> >> + service_kill (priv->teamd_pid);
> >> + priv->teamd_pid = 0;
> >> + }
> >> +
> >> + teamd_timeout_remove (dev);
> >> +
> >> + priv->teamd_on_dbus = FALSE;
> >> +}
> >> +
> >> +static gboolean
> >> +teamd_timeout_cb (gpointer user_data)
> >> +{
> >> + NMDevice *dev = NM_DEVICE (user_data);
> >> + NMDeviceTeamPrivate *priv = NM_DEVICE_TEAM_GET_PRIVATE (dev);
> >> +
> >> + if (priv->teamd_timeout) {
> >> + nm_log_info (LOGD_TEAM, "(%s): teamd timed out.",
> >> nm_device_get_iface (dev));
> >> + teamd_cleanup (dev);
> >> + }
> >> +
> >> + return FALSE;
> >> +}
> >> +
> >> +static void
> >> +teamd_dbus_appeared (GDBusConnection *connection,
> >> + const gchar *name,
> >> + const gchar *name_owner,
> >> + gpointer user_data)
> >> +{
> >> + NMDevice *dev = NM_DEVICE (user_data);
> >> + NMDeviceTeamPrivate *priv = NM_DEVICE_TEAM_GET_PRIVATE (dev);
> >> +
> >> + if (!priv->teamd_dbus_watch)
> >> + return;
> >> +
> >> + nm_log_info (LOGD_TEAM, "(%s): teamd appeared on D-Bus",
> >> nm_device_get_iface (dev));
> >> + priv->teamd_on_dbus = FALSE;
> >> + teamd_timeout_remove (dev);
> >> + nm_device_activate_schedule_stage2_device_config (dev);
> >> +}
> >> +
> >> +static void
> >> +teamd_dbus_vanished (GDBusConnection *connection,
> >> + const gchar *name,
> >> + gpointer user_data)
> >> +{
> >> + NMDevice *dev = NM_DEVICE (user_data);
> >> + NMDeviceTeamPrivate *priv = NM_DEVICE_TEAM_GET_PRIVATE (dev);
> >> + NMDeviceState state;
> >> +
> >> + if (!priv->teamd_dbus_watch || !priv->teamd_on_dbus)
> >> + return;
> >> + nm_log_info (LOGD_TEAM, "(%s): teamd vanished from D-Bus",
> >> nm_device_get_iface (dev));
> >> + teamd_cleanup (dev);
> >> +
> >> + state = nm_device_get_state (dev);
> >> + if (nm_device_is_activating (dev) || (state ==
> >> NM_DEVICE_STATE_ACTIVATED))
> >> + nm_device_state_changed (dev, NM_DEVICE_STATE_FAILED,
> >> NM_DEVICE_STATE_REASON_NONE);
> >> +}
> >> +
> >> +static void
> >> +teamd_process_watch_cb (GPid pid, gint status, gpointer user_data)
> >> +{
> >> + NMDevice *dev = NM_DEVICE (user_data);
> >> + NMDeviceTeamPrivate *priv = NM_DEVICE_TEAM_GET_PRIVATE (dev);
> >> + NMDeviceState state;
> >> +
> >> + nm_log_info (LOGD_TEAM, "(%s): teamd died", nm_device_get_iface (dev));
> >> + priv->teamd_pid = 0;
> >> + teamd_cleanup (dev);
> >> +
> >> + state = nm_device_get_state (dev);
> >> + if (nm_device_is_activating (dev) || (state ==
> >> NM_DEVICE_STATE_ACTIVATED))
> >> + nm_device_state_changed (dev, NM_DEVICE_STATE_FAILED,
> >> NM_DEVICE_STATE_REASON_NONE);
> >> +}
> >> +
> >> +static void
> >> +teamd_child_setup (gpointer user_data G_GNUC_UNUSED)
> >> +{
> >> + /* We are in the child process at this point.
> >> + * Give child it's own program group for signal
> >> + * separation.
> >> + */
> >> + pid_t pid = getpid ();
> >> + setpgid (pid, pid);
> >> +
> >> + /*
> >> + * We blocked signals in main(). We need to restore original signal
> >> + * mask for avahi-autoipd here so that it can receive signals.
> >> + */
> >> + nm_unblock_posix_signals (NULL);
> >> +}
> >> +
> >> +static gboolean
> >> +teamd_start (NMDevice *dev, NMSettingTeam *s_team, NMDeviceTeamPrivate
> >> *priv)
> >> +{
> >> + const char *iface = nm_device_get_ip_iface (dev);
> >> + char *tmp_str;
> >> + const char *config;
> >> + const char **teamd_binary = NULL;
> >> + static const char *teamd_paths[] = {
> >> + "/usr/bin/teamd",
> >> + "/usr/local/bin/teamd",
> >> + NULL
> >> + };
> >> + GPtrArray *argv;
> >> + GError *error = NULL;
> >> + gboolean ret;
> >> +
> >> + teamd_binary = teamd_paths;
> >> + while (*teamd_binary != NULL) {
> >> + if (g_file_test (*teamd_binary, G_FILE_TEST_EXISTS))
> >> + break;
> >> + teamd_binary++;
> >> + }
> >> +
> >> + if (!*teamd_binary) {
> >> + nm_log_warn (LOGD_TEAM,
> >> + "Activation (%s) to start teamd: not found",
> >> iface);
> >> + return FALSE;
> >> + }
> >> +
> >> + argv = g_ptr_array_new ();
> >> + g_ptr_array_add (argv, (gpointer) *teamd_binary);
> >> + g_ptr_array_add (argv, (gpointer) "-o");
> >> + g_ptr_array_add (argv, (gpointer) "-n");
> >> + g_ptr_array_add (argv, (gpointer) "-U");
> >> + g_ptr_array_add (argv, (gpointer) "-D");
> >> + g_ptr_array_add (argv, (gpointer) "-t");
> >> + g_ptr_array_add (argv, (gpointer) iface);
> >> +
> >> + config = nm_setting_team_get_config(s_team);
> >> + if (config) {
> >> + g_ptr_array_add (argv, (gpointer) "-c");
> >> + g_ptr_array_add (argv, (gpointer) config);
> >> + }
> >> +
> >> + if (nm_logging_level_enabled (LOGL_DEBUG))
> >> + g_ptr_array_add (argv, (gpointer) "-gg");
> >> + g_ptr_array_add (argv, NULL);
> >> +
> >> + tmp_str = g_strjoinv (" ", (gchar **) argv->pdata);
> >> + nm_log_dbg (LOGD_TEAM, "running: %s", tmp_str);
> >> + g_free (tmp_str);
> >> +
> >> + /* Start a timeout for teamd to appear at D-Bus */
> >> + priv->teamd_timeout = g_timeout_add_seconds (5, teamd_timeout_cb, dev);
> >> +
> >> + /* Register D-Bus name watcher */
> >> + tmp_str = g_strdup_printf ("org.libteam.teamd.%s", iface);
> >
> >Huh, so what if I run "ip link set dev team0 name foobar0"? Does teamd
> >drop its bus name and acquire a new name, which wouldn't play at all
> >well with clients, or does it just continue with the old bus name?
> >That's the problem with running a daemon-per-interface without some
> >central hub handling lookups for you...
> >
> >Or does the team driver simply prevent device renaming in the kernel?
> >
> >> + priv->teamd_dbus_watch = g_bus_watch_name (G_BUS_TYPE_SYSTEM,
> >> + tmp_str,
> >> +
> >> G_BUS_NAME_WATCHER_FLAGS_NONE,
> >> + teamd_dbus_appeared,
> >> + teamd_dbus_vanished,
> >> + dev,
> >> + NULL);
> >> + g_free (tmp_str);
> >> +
> >> + ret = g_spawn_async ("/", (char **) argv->pdata, NULL,
> >> G_SPAWN_DO_NOT_REAP_CHILD,
> >> + &teamd_child_setup, NULL, &priv->teamd_pid, &error);
> >> + g_ptr_array_free (argv, TRUE);
> >> + if (!ret) {
> >> + nm_log_warn (LOGD_TEAM,
> >> + "Activation (%s) failed to start teamd: %s",
> >> + iface, error->message);
> >> + g_clear_error (&error);
> >> + return FALSE;
> >> + }
> >> +
> >> + /* Monitor the child process so we know when it dies */
> >> + priv->teamd_process_watch = g_child_watch_add (priv->teamd_pid,
> >> + teamd_process_watch_cb,
> >> + dev);
> >> +
> >> + nm_log_info (LOGD_TEAM,
> >> + "Activation (%s) started teamd...", iface);
> >> + return TRUE;
> >> +}
> >> +
> >> +static void
> >> +teamd_stop (NMDevice *dev, NMDeviceTeamPrivate *priv)
> >> +{
> >> + g_return_if_fail (priv->teamd_pid > 0);
> >> + nm_log_info (LOGD_TEAM, "Deactivation (%s) stopping teamd...",
> >> + nm_device_get_ip_iface (dev));
> >> + teamd_cleanup (dev);
> >> +}
> >> +
> >> static NMActStageReturn
> >> act_stage1_prepare (NMDevice *dev, NMDeviceStateReason *reason)
> >> {
> >> + NMDeviceTeamPrivate *priv = NM_DEVICE_TEAM_GET_PRIVATE (dev);
> >> NMActStageReturn ret = NM_ACT_STAGE_RETURN_SUCCESS;
> >> NMConnection *connection;
> >> NMSettingTeam *s_team;
> >> @@ -194,10 +462,22 @@ act_stage1_prepare (NMDevice *dev,
> >> NMDeviceStateReason *reason)
> >> g_assert (connection);
> >> s_team = nm_connection_get_setting_team (connection);
> >> g_assert (s_team);
> >> + if (teamd_start (dev, s_team, priv))
> >> + ret = NM_ACT_STAGE_RETURN_POSTPONE;
> >> + else
> >> + ret = NM_ACT_STAGE_RETURN_FAILURE;
> >
> >So in the future, if teamd is already started for an interface and NM
> >wants to assume that interface non-destructively, is there a way to set
> >the options that NM usually runs teamd with, eg "-o -n -U -D -t" and
> >then also grab the configuration as a JSON blob over D-Bus? We'll want
> >to do that at some point this year I think. Also, same situation if NM
> >crashes for some reason, we'd want to take the interface over again when
> >NM gets restarted without doing anything destructive, and that would
> >involve finding the existing teamd process via D-Bus and reading the
> >configuration back from it.
> >
> >> }
> >> return ret;
> >> }
> >>
> >> +static void
> >> +deactivate (NMDevice *dev)
> >> +{
> >> + NMDeviceTeamPrivate *priv = NM_DEVICE_TEAM_GET_PRIVATE (dev);
> >> +
> >> + teamd_stop (dev, priv);
> >> +}
> >> +
> >> static gboolean
> >> enslave_slave (NMDevice *device, NMDevice *slave, NMConnection
> >> *connection)
> >> {
> >> @@ -334,6 +614,7 @@ nm_device_team_class_init (NMDeviceTeamClass *klass)
> >> parent_class->match_l2_config = match_l2_config;
> >>
> >> parent_class->act_stage1_prepare = act_stage1_prepare;
> >> + parent_class->deactivate = deactivate;
> >> parent_class->enslave_slave = enslave_slave;
> >> parent_class->release_slave = release_slave;
> >>
> >
> >Would be best to also call teamd_cleanup() from the dispose() method of
> >NMDeviceTeam just to ensure that if the device is freed, that everything
> >gets cleaned up.
This one should get done too, just to make sure we don't have any
use-after-free if the callbacks don't get removed for some reason.
Dan
> >Other than that, looks good. I'm still somewhat uncomfortable with the
> >JSON configuration blob, but I'm not sure there's much we can do about
> >it at this point. At least it's structured and can be validated, but it
> >makes that much more work for clients since they then have to go about
> >parsing the blob or linking to libteam or something, which means support
> >for libteam has to be a compile-time decision instead of a runtime
> >one...
> >
> >Dan
> >
_______________________________________________
networkmanager-list mailing list
networkmanager-list@gnome.org
https://mail.gnome.org/mailman/listinfo/networkmanager-list
