Re: [PATCH 6/9] device: Postpone auto-activation of a slave until its master is present

[Dan Williams <dcbw redhat com>]

On Fri, 2011-09-23 at 14:52 +0200, Thomas Graf wrote:
> When a slave is activated, it waits for the master device to appear. Once
> the master is present, it acquires a reference which is released again
> when the slave is deactivated.

So here, can you explain the logic a bit more?  As far as I can tell,
this happens before the slave is actually activated?  So to activate a
slave requires that the master interface is created and in the
NMManager's device list?

If that's the case, I'd rather we do the check in the NMDevice
subclass' (NMDeviceEthernet for the slave, right?)
get_best_auto_connection() virtual method instead of in the policy.
Yes, that would require grabbing the manager with nm_manager_get() like
the OLPC Mesh subclass does, but whatever.  In the future I think we
should make NMManager implement a DeviceProvider GInterface and pass
that interface to each device as we create it so they have access to the
device list right when they are created.  The device subclasses don't
need all the manager stuff, and it would be good to create a clearer
separation there.  But that's an easy cleanup patch for later.  Now we
can just use nm_manager_get().

Also the ACT_STAGE_RETURN_XX stuff should probably only be used during
activation in nm-device files instead of elsewhere, I have grand plans
for that going away in the future to make the device activation flow a
lot more flexible.

Dan


> Signed-off-by: Thomas Graf <tgraf redhat com>
> ---
>  src/nm-device.c  |    3 +++
>  src/nm-manager.c |   18 ++++++++++++++++++
>  src/nm-manager.h |    4 ++++
>  src/nm-policy.c  |   50 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 75 insertions(+), 0 deletions(-)
> 
> diff --git a/src/nm-device.c b/src/nm-device.c
> index 6142243..89c5000 100644
> --- a/src/nm-device.c
> +++ b/src/nm-device.c
> @@ -3388,6 +3388,9 @@ finalize (GObject *object)
>  	if (priv->dhcp_anycast_address)
>  		g_byte_array_free (priv->dhcp_anycast_address, TRUE);
>  
> +	/* release master reference it still exists */
> +	nm_device_set_master (self, NULL);
> +
>  	G_OBJECT_CLASS (nm_device_parent_class)->finalize (object);
>  }
>  
> diff --git a/src/nm-manager.c b/src/nm-manager.c
> index b930f17..83a20dd 100644
> --- a/src/nm-manager.c
> +++ b/src/nm-manager.c
> @@ -367,6 +367,24 @@ nm_manager_get_device_by_path (NMManager *manager, const char *path)
>  	return NULL;
>  }
>  
> +NMDevice *
> +nm_manager_get_device_by_master (NMManager *manager, const char *master, const char *driver)
> +{
> +	GSList *iter;
> +
> +	g_return_val_if_fail (master != NULL, NULL);
> +
> +	for (iter = NM_MANAGER_GET_PRIVATE (manager)->devices; iter; iter = iter->next) {
> +		NMDevice *device = NM_DEVICE (iter->data);
> +
> +		if (!strcmp (nm_device_get_iface (device), master) &&
> +		    (!driver || !strcmp (nm_device_get_driver (device), driver)))
> +			return device;
> +	}
> +
> +	return NULL;
> +}
> +
>  static gboolean
>  manager_sleeping (NMManager *self)
>  {
> diff --git a/src/nm-manager.h b/src/nm-manager.h
> index 22bfca9..ccf5364 100644
> --- a/src/nm-manager.h
> +++ b/src/nm-manager.h
> @@ -83,6 +83,10 @@ void nm_manager_start (NMManager *manager);
>  
>  GSList *nm_manager_get_devices (NMManager *manager);
>  
> +NMDevice *nm_manager_get_device_by_master (NMManager *manager,
> +					   const char *master,
> +					   const char *driver);
> +
>  const char * nm_manager_activate_connection (NMManager *manager,
>                                               NMConnection *connection,
>                                               const char *specific_object,
> diff --git a/src/nm-policy.c b/src/nm-policy.c
> index 02292f7..69ab0ef 100644
> --- a/src/nm-policy.c
> +++ b/src/nm-policy.c
> @@ -708,6 +708,33 @@ activate_data_free (ActivateData *data)
>  	g_free (data);
>  }
>  
> +static NMActStageReturn
> +check_master_dependency (NMManager *manager, NMDevice *device, NMConnection *connection)
> +{
> +	NMSettingConnection *s_con;
> +	NMDevice *master_device;
> +	const char *master;
> +
> +	s_con = (NMSettingConnection *) nm_connection_get_setting (connection, NM_TYPE_SETTING_CONNECTION);
> +	g_assert (s_con);
> +
> +	master = nm_setting_connection_get_master (s_con);
> +
> +	/* no master defined, proceed with activation */
> +	if (!master)
> +		return NM_ACT_STAGE_RETURN_SUCCESS;
> +
> +	master_device = nm_manager_get_device_by_master (manager, master, NULL);
> +
> +	/* If master device is not yet present, postpone activation until later */
> +	if (!master_device)
> +		return NM_ACT_STAGE_RETURN_POSTPONE;
> +
> +	nm_device_set_master (device, master_device);
> +
> +	return NM_ACT_STAGE_RETURN_SUCCESS;
> +}
> +
>  static gboolean
>  auto_activate_device (gpointer user_data)
>  {
> @@ -763,6 +790,28 @@ auto_activate_device (gpointer user_data)
>  	best_connection = nm_device_get_best_auto_connection (data->device, connections, &specific_object);
>  	if (best_connection) {
>  		GError *error = NULL;
> +		NMActStageReturn ret;
> +
> +		ret = check_master_dependency (data->policy->manager, data->device, best_connection);
> +		switch (ret) {
> +		case NM_ACT_STAGE_RETURN_SUCCESS:
> +			break;
> +
> +		case NM_ACT_STAGE_RETURN_POSTPONE:
> +			nm_log_info (LOGD_DEVICE, "Connection '%s' auto-activation postponed: master not available",
> +			             nm_connection_get_id (best_connection));
> +			goto postpone;
> +
> +		case NM_ACT_STAGE_RETURN_STOP:
> +		case NM_ACT_STAGE_RETURN_FAILURE:
> +			/* Avoid further retries and postpone activation */
> +			set_connection_auto_retries (best_connection, 0);
> +
> +			nm_log_info (LOGD_DEVICE, "Connection '%s' auto-activation not allowed due to master dependency",
> +			             nm_connection_get_id (best_connection));
> +
> +			goto postpone;
> +		}
>  
>  		nm_log_info (LOGD_DEVICE, "Auto-activating connection '%s'.",
>  		             nm_connection_get_id (best_connection));
> @@ -778,6 +827,7 @@ auto_activate_device (gpointer user_data)
>  		}
>  	}
>  
> + postpone:
>  	g_slist_free (connections);
>  
>   out: