Re: Talking to NM 0.7 with dbus vs. libnm_glib

[Christian Huff <christian huff silur de>]

Dan Williams schrieb:
> On Wed, 2009-05-06 at 22:42 +0200, Christian Huff wrote:
>> Hi,
>>
>> I am currently working at the libpurple/pidgin NetworkManager
>> integration code to improve to user experience with roaming with
>> multiple devices. Currently, libpurple only reacts to the global
>> StateChange signal, which does not change if one device goes offline or
>> online and there is another active device. In effect, this often results
>> in the inability to send or receive messages - the apps stalls until a
>> timeout occurs.
>>
>> libpurple gets the current global NM state through Dbus, so, being kinda
>> new to glib programming, I tried to get NM's devices, connect to device
>> StateChanged, DeviceAdded and DeviceRemoved signals using
>> dbus_g_proxy_connect_signal. Doing so, I found I needed to register
>> custom marshallers as NetworkManager.h didn't provide them (this was
>> with 0.7.0, I haven't checked 0.7.1 yet). Also, I had to store a proxy
>> for each device, effectively producing some overly complicated code that
>> wouldn't make it into libpurple...
> 
> libnm-glib makes this easy for you, as it will proxy these signals into
> GObject signals.  You shouldn't really need to touch dbus-glib at all
> here.  Using libnm-glib, you could have created a new NMClient object,
> then queried that object for the list of devices, or attached to the
> 'device-added' / 'device-removed' signals or 'state' properties.  I can
> explain more if you like.

Yes, please! How do you attach to properties? e.g. how do I listen to
the default property of a NMActiveConnection?

> But the approach you took was wrong.  Not a problem though, given you
> hadn't done this before :)
> 
>> So I read that libnm_glib was rewritten with 0.7, and rewrote my patch
>> using it. The code turned out much cleaner, yet I found some problems
>> that I want to share with you. One of the goals I had set myself was to
>> minimize the number of unnecessary reconnects. A scheme to do so would
>> be to track the devices that are active at a connect action, and
>> reconnect only if such a "critical" device goes down. Another would be
>> to see if the device currently going down is part of an Active
>> Connection (NMActiveConnection) that NM reports as being the/a default
>> connection for DNS and routes.
> 
> Yeah, I'd expect that whenever the 'default' property on an active
> connection changes, that's when you'd want to see if you should
> reconnect.
> 
>> While the former scheme was not perfect, the latter wasn't reliable,
>> either. I found nm_active_connection_get_default() to often return true
>> for multiple active connections, and sometimes the value from before and
>> sometimes the predicted value from after the device StateChanged event.
>> That kind of synchronisation problems currently prevent me from
>> implementing that scheme.
> 
> That behavior seems wrong.  The NM code should ensure that only one
> active connection object has the default property at the same time, but
> there may be some latency issues in libnm-glib's proxying of properties.
> Or, you may have been checking things at the wrong time.  Can you post
> some code, or ideally a distilled testcase?

This often occurs when looking at the active connections at the time a
device's state-changed signal is caught by the code, which looks like this:

static NMClient *nm_client = NULL;

static void
device_state_changed_cb(
	NMDevice *device,
	NMDeviceState new_state,
	NMDeviceState old_state,
	NMDeviceStateReason reason,
	gpointer user_data)
{
	const GPtrArray nm_active_connections =
		nm_client_get_active_connections(nm_client);

	int i = 0;
	int default_ac_count = 0;
    	NMActiveConnection *nm_ac = NULL;

	/* entering or leaving the activated state? */
	if (new_state != NM_DEVICE_STATE_ACTIVATED
		|| old_state != NM_DEVICE_STATE_ACTIVATED)
		return;
	
	for (i = 0; i < nm_active_connections->len; i++)
	{
        	nm_ac = g_ptr_array_index (nm_active_connections, i);
        	
	        if (nm_active_connection_get_default (nm_ac))
			default_ac_count++;
	}

	if (default_ac_count != 1)
		sprintf("wtf: more or less than one default active connection:
detected %d instead!\n", default_ac_count);
}

static void
init_device (NMDevice *device, gpointer user_data)
{
	/*connect state-changed signal*/
	g_signal_connect(G_OBJECT(device),
		"state-changed",
		G_CALLBACK(device_state_changed_cb),
		NULL);
}

static void
device_added_cb(
	NMClient *client,
	NMDevice *device,
	gpointer user_data)
{
	/*new device, initialize it!*/
	init_device(device);
}

void
init()
{
	nm_client = nm_client_new();
	const GPtrArray nm_devices =
		nm_client_get_devices(nm_client);

	/*get new devices being added to the system*/
	g_signal_connect(G_OBJECT(nm_client),
		"device-added",
		G_CALLBACK(device_added_cb),
		NULL);

	/*initialize each device*/
	g_ptr_array_foreach((GPtrArray*)nm_devices,
		(GFunc)init_device,
		NULL);

	....g_main_loop and stuff...
}

> If libnm-glib is reporting two NMActiveConnection objects having
> default=True at the same time, that's something that should get
> identified and fixed.
> 
>> Also, I tried to entirely replace the dbus functionality, but
>> libnm_glib_get_network_state() does not return values equivalent to the
>> StateChanged signal, and I didn't find the signal in the libnm_glib docs.
> 
> No, it doesn't.  libnm_glib_get_network_state() is the "old" interface
> from NM 0.6 that was preserved for minimal compatibility.  You'd want to
> create a new NMClient object and attach signals to it:
> 
> static void
> state_changed (NMClient *client,
>                GParamSpec *pspec,
>                gpointer user_data)
> {
>     NMState state;
> 
>     state = nm_client_get_state (client);
>     <do something here>
> }
> 
> static void
> active_connections_changed (NMClient *client,
>                             GParamSpec *pspec,
>                             gpointer user_data)
> {
>     const GPtrArray *acs = nm_client_get_active_connections (client);
>     int i;
> 
>     for (i = 0; i < acs->len; i++) {
>         NMActiveConnection *ac = g_ptr_array_index (acs, i);
> 
>         if (nm_active_connection_get_default (ac))
>             <its default>
>     }
> }
>                            
> 
> int main (...)
> {
>     NMClient *client;
> 
>     client = nm_client_new ();
> 
>     /* Monitor state */
>     g_signal_connect (client, "notify::state",
>                       G_CALLBACK (state_changed), my_data);
> 
>     g_signal_connect (client, "notify::active-connections",
>                       G_CALLBACK (active_connections_changed), my_data);
> 
>     g_main_loop_run (loop);
> 
>     g_object_unref (client);
> }
> 
> Dan

That's great, thank you! The notify::state signal works fine as a
replacement for libpurple's old dbus code. However,
"notify::active-connections" isn't emitted if an active connection's
property (e.g. "default") changes, see my question above on how to
connect to property changes. So I see the change in active connections
if I plug in e.g. eth0, but too early, as it hasn't finished connecting
yet and default is still false...

Christian

> 
>> The current version of my patch reconnects any time a device enters or
>> leaves the NM_DEVICE_STATE_ACTIVATED state, and can be found at:
>>
>> http://developer.pidgin.im/ticket/8694
>>
>> thanks for your feedback,
>>
>> Christian Huff (Pedric)
>> _______________________________________________
>> NetworkManager-list mailing list
>> NetworkManager-list gnome org
>> http://mail.gnome.org/mailman/listinfo/networkmanager-list
>