GNOME Bugzilla – Bug 701078
Port NM to BlueZ 5
Last modified: 2013-09-25 21:19:12 UTC
We need to port NM to BlueZ 5.
I'm looking into this
Created attachment 246257 [details] [review] bluez: listen to Connected changes in NMBluezDevice
Created attachment 246258 [details] [review] bluez: listen to Connected changes through NMBluezDevice
Created attachment 246259 [details] [review] bluez: move org.bluez Connection() handling to NMBluezDevice
Created attachment 246260 [details] [review] bluez: create the NMDevice immediately Instead of waiting for the connections to be defined.
Created attachment 246261 [details] [review] bluez: add configure switch for BlueZ 5
Created attachment 246262 [details] [review] bluez: add support for BlueZ 5 At this moment we only support one of BlueZ 4 and 5, which has to be defined at build time.
Created attachment 246263 [details] [review] bluez: add basic dun Profile1 implementation
Created attachment 246264 [details] [review] bluez: check for bluez when building for BlueZ 5 We don't link against it though as we only need the headers at build time to get some definitions (such as rfcomm_dev_req and RFCOMM_REUSE_DLC).
Created attachment 246265 [details] [review] bluez: add code to create the rfcomm device
Created attachment 246266 [details] [review] bluez: hook DUN signals to the devices
Created attachment 246267 [details] [review] bluez: use ConnectProfile for DUN org.bluez.Serial is gone in BlueZ 5. What we do now is call ConnectProfile on org.bluez.Device1. That calls NewConnection on the DUN Profile implementation in NMBluezDUNManager, which creates the rfcomm device for us. We will probably just use the fd in the future, perhaps when we don't have to worry about BlueZ 4 compatibility anymore.
Created attachment 246268 [details] [review] bluez: get adapter address asynchronously
Created attachment 246269 [details] [review] bluez: connect to PAN device asynchronously
Working with PAN and DUN on bluez4 and with PAN on bluez5. DUN currently fails on getsockname() on bluez5, need to investigate that issue. Branch up for review though, I think it's in a pretty good state.
Some additional comments: This needs a fix for bug 701715 for which there is a patch. It also needs a fix in bluez5 for the bluez5 codepath to work, as it currently doesn't update org.bluez.Device1's Connected property. Gustavo was working on a fix and I got PAN to work with a preliminar fix/workaround to bluez.
Created attachment 246418 [details] [review] bluez: add support for BlueZ 5 At this moment we only support one of BlueZ 4 and 5, which has to be defined at build time.
I just attached ad new version of the bluez 5 support patch, this patch deprecate all other bluez 5 patches, except the one for building system. In this patch only PAN is supported, DUN is still in discussion and will be added later. All patches are ready for upstreaming.
Created attachment 247010 [details] [review] bluez: pass the NMBluezDevice down to the NMDeviceBt So that the latter can use the former instead of listening for changes over dbus.
Created attachment 247015 [details] [review] bluez: listen to Connected changes in NMBluezDevice
Created attachment 247016 [details] [review] bluez: listen to Connected changes through NMBluezDevic
Created attachment 247017 [details] [review] bluez: move org.bluez Connection() handling to NMBluezDevice
Created attachment 247018 [details] [review] bluez: create the NMDevice immediately Instead of waiting for the connections to be defined.
Created attachment 247019 [details] [review] bluez: add configure switch for BlueZ 5
Created attachment 247020 [details] [review] bluez: pass the NMBluezDevice down to the NMDeviceBt
Created attachment 247021 [details] [review] bluez: check for bluez when building for BlueZ 5
Created attachment 247022 [details] [review] bluez: add support for BlueZ 5 At this moment we only support one of BlueZ 4 and 5, which has to be defined at build time.
Review of attachment 247015 [details] [review]: Looks good.
Review of attachment 247020 [details] [review]: I think the NMBluezDevice should get assigned with g_value_dup_object() in nm-device-bt.c, since we can't be 100% sure we won't get a use-after-free without a lot of pointless verification work. That also implies a corresponding g_clear_object() in NMDeviceBt's dispose(). Once we throw Bluez5 into the mix, it's likely safer to just ensure the NMBluezDevice is refed by NMDeviceBt.
Review of attachment 247016 [details] [review]: Similar to the passing of the bluez device down NMDeviceBt, the patches should probably save the connected signal ID and disconnect that ID when the BluezDevice is unrefed in dispose()
Review of attachment 247017 [details] [review]: ::: src/bluez-manager/nm-bluez-device.c @@ +291,3 @@ + G_TYPE_INVALID) == FALSE + || !device || !strlen (device)) { + g_simple_async_result_take_error (result, error); If the call was successful for some reason, but device was NULL/zero-length, then 'error' could be set to NULL here and the simply async result wouldn't have an error. Perhaps: else if (!device || !strlen (device)) { g_simple_async_result_set_error (...); }
Review of attachment 247018 [details] [review]: Looks good.
Review of attachment 247022 [details] [review]: ::: src/bluez-manager/nm-bluez5-device.c @@ +204,3 @@ + g_strdup (device), + g_free); + priv->rfcomm_iface = device; We should probably priv->rfcomm_iface to something like priv->bt_iface, since it's used for both DUN and PAN. @@ +333,3 @@ + if ( (!priv->name && str) + || (priv->name && !str) + || (priv->name && str && strcmp (priv->name, str))) { g_strcmp0() could reduce these 3 lines down to one. ::: src/bluez-manager/nm-bluez5-manager.c @@ +305,3 @@ + g_hash_table_iter_init (&iter, priv->devices); + while (g_hash_table_iter_next (&iter, NULL, (gpointer) &device)) + g_signal_emit (self, signals[BDADDR_REMOVED], 0, Should probably use do_signal here to suppress emitting BDADDR_REMOVED; I forget why the original bluez4 code had that switch, but it's likely to ensure that a whole bunch of signals don't get emitted both internally and via D-Bus when NM is quitting.
Review of attachment 247021 [details] [review]: This one is obsolete, right? Since we don't link to the bluez libraries anymore.
Review of attachment 247019 [details] [review]: Looks fine.
(In reply to comment #35) > Review of attachment 247021 [details] [review]: > > This one is obsolete, right? Since we don't link to the bluez libraries > anymore. Should still be needed to get the headers at runtime for some definitions. We don't link against libbluetooth though, and so we do BLUEZ_LIBS= (or we could remove BLUEZ_LIBS from LDADD in src/Makefile.am)
(In reply to comment #37) > (In reply to comment #35) > > Review of attachment 247021 [details] [review] [details]: > > > > This one is obsolete, right? Since we don't link to the bluez libraries > > anymore. > > Should still be needed to get the headers at runtime for some definitions. We > don't link against libbluetooth though, and so we do BLUEZ_LIBS= (or we could > remove BLUEZ_LIBS from LDADD in src/Makefile.am) Oh but we don't have the DUN code and so don't even need the bluez headers anymore - I guess that's what you meant. Shouldn't be needed anymore, indeed. Sorry for the noise!
Created attachment 247173 [details] [review] bluez: pass the NMBluezDevice down to the NMDeviceBt So that the latter can use the former instead of listening for changes over dbus.
Created attachment 247174 [details] [review] bluez: listen to Connected changes in NMBluezDevice
Created attachment 247175 [details] [review] bluez: listen to Connected changes through NMBluezDevice
Created attachment 247176 [details] [review] bluez: move org.bluez Connection() handling to NMBluezDevice
Created attachment 247177 [details] [review] bluez: create the NMDevice immediately
Created attachment 247179 [details] [review] bluez: add configure switch for BlueZ 5
Created attachment 247180 [details] [review] bluez: add support for BlueZ 5 At this moment we only support one of BlueZ 4 and 5, which has to be defined at build time.
Just a note to myself, since the devices are now being created immediately without a defined connection, we'll need to add some support to the GUI applets to handle asking the user for APN and other information during the connection attempt. That's blocking on some secrets changes, much of which got merged recently as part of the agent secrets rework.
Could Gustavo's patches get a review?
another ping here - we need to get clarity on whether this will be feasible for gnome 3.10 or not.
(In reply to comment #47) > Could Gustavo's patches get a review? The patches are actually fine. But we need more work in core NM as described before they can go in, otherwise they break Bluetooth connection setup, which is something we agreed on. The core issue is that now *all* capable Bluetooth devices are shown even before they have configuration defined for them. This change was made in the core NM code in the patches, and affects both Bluez4 and Bluez5, so it's not just a Bluez5 thing. This was a change we agreed to implement because it's the right thing to do. This breaks things because now the device is exposed to clients, but it cannot be started using the current NM D-Bus API because there is no configuration for the device and thus NM has no idea how to connect it. This isn't as much of an issue for PAN, but for DUN you need setup information like the APN and possibly a username and a password. So the solution here is to: 1) add a Device.Activate(connection path, specific object) => active connection path method. If the connection path is empty, NM picks the "best" connection for the device and starts it. If there are no connections defined for the device, NM creates a skeleton NMConnection (in the case of BT, always use PAN if the device is PAN capable, otherwise DUN with empty APN if it's not). If the connection path is not empty, then NM just starts that connection. 2) NM proceeds to start the new connection; if it needs more details, like APN and password, then NM would request that from agents via the Agent.NeedSecrets method call and give the appropriate hints and setting name. The NMDevice object path should be passed in the hints with a format of "x-nm-device-path:<path>" so the agent knows which device the information is being requested for. 3) The agent then presents some UI (like the mobile broadband config wizard or whatever) to the user to request the additional information, and returns that information to NM along with any secrets. 4) NM is then modified not to reject non-secrets in the NeedSecrets reply, but if the user is privileged (via PolicyKit) then NM updates the connection with the information from the reply and continues connecting with the new stuff like APN. This would also eventually be used to kill the Dialog of WPA Enterprise Doom such that you can just connect to the AP with one click, and then NM retrieves the supported EAP methods from the supplicant halfway through the connection, and then only asks the user for the stuff it needs, instead of making the user choose between a number of irrelevent options.
Given that we don't have the resources/time to ensure there aren't functionality regressions when porting to bluez5 (eg, DUN) here's a new proposal: 1) revert the changes in the bluez5 patches that always add a NMDeviceBt for any Bluez device that is capable of PAN & DUN, which solves the problem of having to update clients for configuration during activation as described in the above comment 2) merge the Bluez5 PAN support, and if no existing PAN connection for the Bluez device exists, create an PAN connection for that Bluez device and export it via D-Bus. This has the effect of always exposing any PAN-capable device like the bluez5 patches currently do. 3) ensure that when bluez5 is enabled, DUN connections are hidden in NMSettings 4) don't build the gnome-bluetooth plugin when bluez5 is enabled, since it's not ported to bluez5, and is mostly useless for PAN The core problem here was that we chose (correctly, given enough time) to always expose the Bluetooth device whenever Bluez exposed it. This is still the right course of action once we can get the DUN stuff ported over, since we want this functionality for better WPA Enterprise setup flow as well. However, that requires changes in NM and the clients, and that clearly isn't going to happen in time. Sound like a plan? I think that's do-able before mid September.
So what is the exact set of regressions relative to bluez 4 following that plan?
(In reply to comment #51) > So what is the exact set of regressions relative to bluez 4 following that > plan? DUN doesn't work *at all* (can't even manually configure it) if Bluez5 support is enabled. I think that's it, and it's acceptable to me if I hold my nose so that we don't block releases.
This is also helped by the fact that disconnecting a DUN connection has caused a kernel panic since 3.9 or 3.8, so it's not like DUN works right now anyway, even with bluez4. Which is sad.
So to revert the change that makes the Bluez devices available to NetworkManager immediately, we need to revert the commit "bluez: create the NMDevice immediately" and then fix up the following commits. That will return to the behavior that a Bluetooth connection must be pre-configured for the device to show up in NetworkManager's device list. Next, the bluez5 code (nm-bluez5-device.c) needs to do the same thing as the bluez4 code and only consider the NMBluezDevice 'usable' when there's a connection for it. Finally, NMManager should create a new Bluetooth PAN connection for any Bluez PAN-capable device that does not yet have one. This preserves the same behavior that Gustavo's patchset has, but also remains compatible with older clients that wouldn't be able to handle the configure-while-connecting capability that we'd need to properly support DUN and immediately-available devices.
(In reply to comment #54) I pushed the branch thaller/bluez: http://cgit.freedesktop.org/NetworkManager/NetworkManager/log/?h=thaller/bluez (currently at f9cff559655c060de622f015f3751688281ede85). As of now, this implements the first two points from comment #54. It takes dcbw/bluez and leaves the original patches untouched so that it is easier to see what I changed (later they can be squashed together). I merged nm-bluez-device.c and nm-bluez5-device.c into one file because they are mostly identical. IMO, having them in one file side-by-side makes it easier to see the differences between BlueZ 4 and 5.
(In reply to comment #54 and comment #55) Updated branch http://cgit.freedesktop.org/NetworkManager/NetworkManager/log/?h=thaller/bluez This now creates an NMConnection for BlueZ5. Please review. The did not modify the patches that I cherry-picked from the original branch "dcbw/bluez5", so they are the same as you know them.
> bluez: listen to Connected changes through NMBluezDevice in dispose(): >+ if (priv->connected_id) { >+ g_source_remove (priv->connected_id); >+ priv->connected_id = 0; >+ } but connected_id is a signal id, not a source id. But you can just get rid of priv->connected_id, and instead do: g_signal_handlers_disconnect_by_func (priv->bt_device, G_CALLBACK (bluez_connected_changed), object); > bluez: move org.bluez Connection() handling to NMBluezDevice It's not clear to me why "type_proxy" is called "type_proxy", as opposed to something like "connection_proxy". (I guess it was copied from the old code, but it didn't make sense before either.) And connect_async() should probably return an error if it's already connected. >+#define BLUETOOTH_DUN_UUID "dun" >+#define BLUETOOTH_NAP_UUID "nap" those aren't UUIDs... >+nm_bluez_device_connect_async (NMBluezDevice *self, >+ gboolean dun, an enum type would be better than a gboolean here >+nm_bluez_device_call_disconnect (NMBluezDevice *self, >+ gboolean dun); and it seems like you shouldn't have to respecify dun/nap here; the NMBluezDevice should just remember it when you connect. > trivial: fix whitespace errors there's also a missing space before the paren in the g_simple_async_result_set_error() call that you fixed the tabbing of. > bluez: create the NMDevice immediately > Revert "bluez: create the NMDevice immediately" Just remove both patches from the branch. > bluez: add configure switch for BlueZ 5 This is weird... I guess if we assume that BlueZ 4 support will eventually go away, then it doesn't make sense to try to be all spiffy and support either of them at runtime. But in that case, I think we should (a) have 5 be the default, with the configure option being to specify 4, (b) rename nm-bluez-manager and nm-bluez-adapter to "nm-bluez4-manager" and "nm-bluez4-adapter", and let the BlueZ 5 manager be just "nm-bluez-manager", and (c) have the two bluez-manager implementations check if they get back a "no such interface" error when calling the methods, and log a warning saying something about it probably being the wrong bluez version if so. > bluez: fixup add support for BlueZ 5 > trivial: change nm-bluez*-device.c to be more similar > trivial: merge file nm-bluez5-device.c into nm-bluez-device.c > bluez: connections must be pre-configured for BlueZ 5 devices I think this would end up a lot cleaner if you added the NMConnectionProvider stuff to nm-bluez5-device first, and then merged the two files after. > trivial: merge file nm-bluez5-device.c into nm-bluez-device.c It's not really "trivial"... (Most of the comments below also apply to the original code in nm-bluez5-device. You might decide it makes more sense to first apply a fixup commit to fix them there, and then merge the already-fixed version.) >+ if (dun) >+ return; it should return an error >+ priv->adapter = g_dbus_proxy_new_for_bus_finish (res, &error); >+ >+ if (!priv->adapter) { >+ nm_log_warn (LOGD_BT, "failed to acquire adapter proxy: %s.", >+ error && error->message ? error->message : "(unknown)"); Some internal NM code is bad about setting GErrors when it's supposed to (or at least, it used to be), so there's a lot of double-checking like that in existing code. But you don't have to do that with glib functions. If priv->adapter is NULL, then error is set (and error->message is non-NULL). (Likewise in other places.) >+ v = g_dbus_proxy_get_cached_property (priv->proxy5, "Adapter"); >+ if (v) { >+ g_dbus_proxy_new_for_bus (G_BUS_TYPE_SYSTEM, you should g_object_ref (self) here, and unref it in on_adapter_acquired(), to ensure that the device doesn't get destroyed while the D-Bus call is outstanding. Likewise with the async gdbus calls in nm_bluez_device_new(). > bluez: create NMConnection for BlueZ 5 NAP devices Any reason it's only for Bluez5? >+ id = g_strdup_printf (priv->name ? priv->name : (_("%s Network"), priv->address ? priv->address : "(Unknown)")); The device won't emit usable until priv->name is set... although it may not be set yet at this point. But then, you don't want to add the connection to the connection provider if the device isn't usable yet anyway. So I think there needs to be some reorganization of what happens when. But then after that you can just set the id to priv->name unconditionally. >+ setting = nm_setting_ip4_config_new (); >+ g_object_set (G_OBJECT (setting), >+ NM_SETTING_IP4_CONFIG_METHOD, NM_SETTING_IP4_CONFIG_METHOD_AUTO, >+ NM_SETTING_IP6_CONFIG_MAY_FAIL, FALSE, "IP6" should be "IP4"
(In reply to comment #57) I pushed an update of branch thaller/bluez (the previous version was 6772414a016ae29c6bc6a07d2fdc25dd62157526, thaller/bluez-1) (I implemented the points from comment #57, that I do not explicitly mention here.) > > bluez: add configure switch for BlueZ 5 > > This is weird... > > I guess if we assume that BlueZ 4 support will eventually go away, then it > doesn't make sense to try to be all spiffy and support either of them at > runtime. But in that case, I think we should (a) have 5 be the default, with > the configure option being to specify 4, (b) rename nm-bluez-manager and > nm-bluez-adapter to "nm-bluez4-manager" and "nm-bluez4-adapter", and let the > BlueZ 5 manager be just "nm-bluez-manager", and (c) have the two bluez-manager > implementations check if they get back a "no such interface" error when calling > the methods, and log a warning saying something about it probably being the > wrong bluez version if so. Not yet done. > >+ id = g_strdup_printf (priv->name ? priv->name : (_("%s Network"), priv->address ? priv->address : "(Unknown)")); > > The device won't emit usable until priv->name is set... although it may not be > set yet at this point. But then, you don't want to add the connection to the > connection provider if the device isn't usable yet anyway. So I think there > needs to be some reorganization of what happens when. But then after that you > can just set the id to priv->name unconditionally. True, this is not yet done (will follow soon).
(You'd said on IRC that the only thing still missing was the configure changes, but you didn't do the automatic-NAP-connection-creation fixes yet either.) > bluez: pass NMBluetoothCapabilities to nm_bluez_device It's fine to reuse NMBluetoothCapabilities as the enum type I guess, but you shouldn't call it "device_capability", since it doesn't represent a capability here. NMDeviceBt uses "NMBluetoothCapabilities bt_type" for this, so you could just use that name here too. The nm_bluez_device_connect_async() call in nm-device-bt.c is still passing TRUE/FALSE. Hm... should have noticed this before, but get rid of the word "call" in "nm_bluez_device_call_disconnect()". It's inconsistent with the name of the connect function. > trivial: change nm-bluez*-device.c to be more similar > As a first step, make some trvial renaming to make the two ^^^^^^ > bluez: also enable creation of NMConnection for NAP devices for BlueZ 4 Just squash this with the previous commit.
ok, wrong branch... >+ /* Clear the no_autocreate flag, so that when the last connection gets removed, >+ * a new one can be created. */ >+ priv->nap_connection_no_autocreate = FALSE; with ethernet default devices, we actually remember forever to not recreate the device. So probably you should just leave it TRUE here.
pushed an update. The branch is still called thaller/bluez. Implemented everything except making bluez5 default + renaming of files
Updated branch again and tested (as far as I reasonably could). I think all issues so far are addressed there. Please see branch http://cgit.freedesktop.org/NetworkManager/NetworkManager/log/?h=thaller/bluez (b77392b53e88e558590aa0b21a8ed71d873d7320).
Tested with both Bluez4 (PAN, DUN) and Bluez5 (PAN) and pushed a few fixes to dcbw/bluez, along with two further patches. The first consolidates the bluez4 and bluez5 connect/disconnect code, and the second follows the adapter's 'powered' value to determine if the adapter is rfkilled or not. If the adapter is rfkilled, then clearly we can't use it; just something I found during testing. I can't get PAN to work either, same failures as you ("bnep failed") with both bluez4 *and* bluez5. AFAIK that's a kernel issue though. > bluez: create NMConnection for NAP devices I think the functions here should be "pan" not "nap" to remain consistent with the other code. We're not actually creating a NAP connection at all, we're creating a "PAN" connection. NAP = Network Access Point (ie, the server), PAN = Personal Area Network (ie, the client), and NM isn't the server here, it's the client. Also, instead of using priv->nap_connection_is_creating, it should be sufficient to just disconnect the "cp_connection_added" handler with g_signal_handlers_block_by_func() and unblock it after adding the connection, like so: g_signal_handlers_block_by_func (priv->provider, cp_connection_added, self); added = nm_connection_provider_add_connection (priv->provider, connection, FALSE, &error); g_signal_handlers_unblock_by_func (priv->provider, cp_connection_added, self); Besides that, the rest look good, so check out my fixes and additional patches and squash down the fixups. Thanks!
Hi, I reordered the commits (and fixed the conflicts). Also, I added few additional fixup!, please check them ("thaller/bluez", 4a83a6f92b3b5108c9c020a485b4b1ee89d5ba76). I did not squash the fixup! commits yet, so that you can easier review them.
BlueZ 5 support now pushed to master. See http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=a84fc3169a18cf06769d5f005feaf8d6cd462a34 Thank you all for your help!!