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

Reply via email to