Re: [patch NetworkManager v2 6/6] nm-device-team: spawn teamd for team device connection
Wed, Jul 24, 2013 at 07:23:51PM CEST, d...@redhat.com wrote:
>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
>> >> ---
>> >> 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
>> >> +#include
>> >> +#include
>> >> +#include
>> >> #include
>> >> #include
>> >> +#include
>> >>
>> >> #include
>> >>
>> >> @@ -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.
Okay. Yeah, you are right. I misread the code. Will fix this.
>
>(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 =
Re: [patch NetworkManager v2 6/6] nm-device-team: spawn teamd for team device connection
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
> >> ---
> >> 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
> >> +#include
> >> +#include
> >> +#include
> >> #include
> >> #include
> >> +#include
> >>
> >> #include
> >>
> >> @@ -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;
> >> +}
Re: [patch NetworkManager v2 6/6] nm-device-team: spawn teamd for team device connection
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
>> ---
>> 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
>> +#include
>> +#include
>> +#include
>> #include
>> #include
>> +#include
>>
>> #include
>>
>> @@ -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)
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_D
Re: [patch NetworkManager v2 6/6] nm-device-team: spawn teamd for team device connection
On Mon, 2013-07-15 at 09:46 +0200, Jiri Pirko wrote:
> Signed-off-by: Jiri Pirko
> ---
> 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
> +#include
> +#include
> +#include
> #include
> #include
> +#include
>
> #include
>
> @@ -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));
}
...
}
> + 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,
> + cons
