Opened 10 years ago

Last modified 6 years ago

#8694 new patch

Support NetworkManager 0.7 multiple devices

Reported by: pedric Owned by:
Milestone: Patches Needing Improvement Component: libpurple
Version: 2.5.5 Keywords: NetworkManager
Cc: elreydetodo

Description

As shipped with Ubuntu 8.10, NetworkManager 0.7 supports multiple active devices, such as wired and wireless networking at the same time. If one interface is disconnected or fails, the other should still provide networking without the need for user interaction. However, this does not currently work with Pidgin.

How to reproduce:

  1. Login with pidgin and networkmanager 0.7 autostarting, with wired networking plugged and wireless configured.
  2. See pidgin start nicely
  3. Send and receive some messages
  4. Pull the plug
  5. See NetworkManager tray icon go "wireless"

Expected behaviour:

  1. Send and receive some more messages without problems

Observed behaviour:

  1. Pidgin stalls and is neither able to send nor receive messages and status updates (until some timeout?).
  2. User needs to manually reconnect

Attachments (4)

libpurple-networkc-nm07-multiple-devices.patch (17.7 KB) - added by pedric 10 years ago.
second draft of a patch that reconnects if the number of active devices decreases
libpurple-networkc-nm07-multiple-devices-libnm_glib.patch (9.9 KB) - added by pedric 10 years ago.
third draft of the patch, a rewrite using libnm_glib instead of dbus directly, minor cleanups
libpurple-networkc-nm07-multiple-devices-libnm_glib_ac.patch (8.7 KB) - added by pedric 10 years ago.
fourth draft of the patch, rewritten using libnm_glib, using NMActiveConnections to determine critical devices
libpurple-networkc-nm07-multiple-devices-libnm_glib-base.patch (7.0 KB) - added by pedric 10 years ago.
5th version of the patch. Always reconnects if number of active devices changes.

Download all attachments as: .zip

Change History (14)

comment:1 Changed 10 years ago by pedric

  • Summary changed from Support NetworkManager 0.7 multiple device to Support NetworkManager 0.7 multiple devices

Changed 10 years ago by pedric

second draft of a patch that reconnects if the number of active devices decreases

comment:2 Changed 10 years ago by darkrain42

  • Milestone set to Patches Needing Review

comment:3 Changed 10 years ago by darkrain42

  • Type changed from defect to patch

comment:4 follow-up: Changed 10 years ago by QuLogic

This code needs some cleanup:

  • No comments, please.
  • I'm no D-Bus expert, but why do we need the marshallers file when we never had it before?
  • You should use PKG_CHECK_EXISTS to look for NM >= 0.7; PKG_CHECK_MODULES will print an error for NM < 0.7 instead.
  • You shouldn't g_strdup the input to g_hash_table_lookup.
  • There are some incorrect casts like to a gpointer * where a gpointer is required.
  • Why do you need a new proxy for each device?
  • You g_strdup key for the hash table, but didn't specify a destroy function for it.
  • nm_device_proxy_tmp is leaked when you malloc it and then immediately replace the value.
  • It appears that nothing is ever removed from the hash table (assuming you have something like a USB-Ethernet dongle that you unplugged).

comment:5 Changed 10 years ago by rekkanoryo

  • Milestone changed from Patches Needing Review to Patches Needing Improvement

comment:6 in reply to: ↑ 4 Changed 10 years ago by pedric

Replying to QuLogic:

This code needs some cleanup:

  • No comments, please.

okay.

  • I'm no D-Bus expert, but why do we need the marshallers file when we never had it before?

NetworkManager does not (yet) provide the marshallers required for its signals. Using dbus directly, this is currently the only, however ugly way to get the callbacks to trigger.

  • You should use PKG_CHECK_EXISTS to look for NM >= 0.7; PKG_CHECK_MODULES will print an error for NM < 0.7 instead.

I'll have a look at this.

  • Why do you need a new proxy for each device?

The dbus path of each device differs, and you cannot set a proxy to a dbus path using wildcards.

  • It appears that nothing is ever removed from the hash table (assuming you have something like a USB-Ethernet dongle that you unplugged).

That was left out on purpose, and is something that will be added as the patch matures.

I plan to rewrite this patch using libnm_glib (that was rewritten with NM 0.7 and hopefully is more stable than previous versions).

Changed 10 years ago by pedric

third draft of the patch, a rewrite using libnm_glib instead of dbus directly, minor cleanups

Changed 10 years ago by pedric

fourth draft of the patch, rewritten using libnm_glib, using NMActiveConnections to determine critical devices

comment:7 Changed 10 years ago by rlaager

Is it your intention to replace all the NetworkManager code with calls to libnm_glib? Are we going to require NetworkManager 0.7 or higher for NM support in Pidgin?

comment:8 Changed 10 years ago by pedric

No, I think for the time being, it's better to keep backwards compatibility (at least I was told to do so on IRC/XMPP conf). Currently, it falls back to what it was before without NetworkManager 0.7, and the "global" state changes are still detected the old fashioned way. However, making Pidgin use libnm_glib for that as well if NM 0.7 is detected is easy.

Changed 10 years ago by pedric

5th version of the patch. Always reconnects if number of active devices changes.

comment:9 Changed 10 years ago by pedric

The 5th version that I uploaded adresses a number of issues I was having with detecting if a reconnect was really needed. It now always reconnects if the number of active devices changes. I decided to remove the code that attempted to minimize the number of unnecessary reconnects, mainly because of the fact that information about default connections for DNS and routes provided by libnm_glib have been unreliable, and also because I found Pidgin to stall not only if one of multiple devices is disconnected, but sometimes also if a device is connected. This 5th version basically provides the functionality as if the user was using an earlier version of NetworkManager.

comment:10 Changed 6 years ago by slightlyrandom

This bug is still in effect, and the patch is woefully outdated.

My current workaround is to disable the Active connection first, going offline, then enabling the to-be Active device after. This forces Pidgin to notice things have changed by kicking it off the internet entirely, sidestepping the reported issue.

Note: See TracTickets for help on using tickets.
All information, including names and email addresses, entered onto this website or sent to mailing lists affiliated with this website will be public. Do not post confidential information, especially passwords!