After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 701078 - Port NM to BlueZ 5
Port NM to BlueZ 5
Status: RESOLVED FIXED
Product: NetworkManager
Classification: Platform
Component: general
git master
Other Linux
: Normal normal
: ---
Assigned To: NetworkManager maintainer(s)
NetworkManager maintainer(s)
Depends on:
Blocks: 703059 706995
 
 
Reported: 2013-05-27 10:30 UTC by Emilio Pozuelo Monfort
Modified: 2013-09-25 21:19 UTC
See Also:
GNOME target: 3.10
GNOME version: 3.9/3.10


Attachments
bluez: listen to Connected changes in NMBluezDevice (3.21 KB, patch)
2013-06-07 16:32 UTC, Emilio Pozuelo Monfort
none Details | Review
bluez: listen to Connected changes through NMBluezDevice (4.04 KB, patch)
2013-06-07 16:32 UTC, Emilio Pozuelo Monfort
none Details | Review
bluez: move org.bluez Connection() handling to NMBluezDevice (12.98 KB, patch)
2013-06-07 16:32 UTC, Emilio Pozuelo Monfort
none Details | Review
bluez: create the NMDevice immediately (12.58 KB, patch)
2013-06-07 16:32 UTC, Emilio Pozuelo Monfort
none Details | Review
bluez: add configure switch for BlueZ 5 (1.26 KB, patch)
2013-06-07 16:32 UTC, Emilio Pozuelo Monfort
none Details | Review
bluez: add support for BlueZ 5 (33.73 KB, patch)
2013-06-07 16:32 UTC, Emilio Pozuelo Monfort
none Details | Review
bluez: add basic dun Profile1 implementation (14.58 KB, patch)
2013-06-07 16:32 UTC, Emilio Pozuelo Monfort
none Details | Review
bluez: check for bluez when building for BlueZ 5 (1.02 KB, patch)
2013-06-07 16:32 UTC, Emilio Pozuelo Monfort
none Details | Review
bluez: add code to create the rfcomm device (4.98 KB, patch)
2013-06-07 16:32 UTC, Emilio Pozuelo Monfort
none Details | Review
bluez: hook DUN signals to the devices (2.23 KB, patch)
2013-06-07 16:32 UTC, Emilio Pozuelo Monfort
none Details | Review
bluez: use ConnectProfile for DUN (7.47 KB, patch)
2013-06-07 16:32 UTC, Emilio Pozuelo Monfort
none Details | Review
bluez: get adapter address asynchronously (5.02 KB, patch)
2013-06-07 16:32 UTC, Emilio Pozuelo Monfort
none Details | Review
bluez: connect to PAN device asynchronously (6.91 KB, patch)
2013-06-07 16:33 UTC, Emilio Pozuelo Monfort
none Details | Review
bluez: add support for BlueZ 5 (34.20 KB, patch)
2013-06-10 16:39 UTC, Gustavo Padovan
none Details | Review
bluez: pass the NMBluezDevice down to the NMDeviceBt (6.93 KB, patch)
2013-06-17 11:12 UTC, Gustavo Padovan
none Details | Review
bluez: listen to Connected changes in NMBluezDevice (3.22 KB, patch)
2013-06-17 11:38 UTC, Gustavo Padovan
accepted-commit_now Details | Review
bluez: listen to Connected changes through NMBluezDevic (4.05 KB, patch)
2013-06-17 11:38 UTC, Gustavo Padovan
needs-work Details | Review
bluez: move org.bluez Connection() handling to NMBluezDevice (12.99 KB, patch)
2013-06-17 11:39 UTC, Gustavo Padovan
needs-work Details | Review
bluez: create the NMDevice immediately (12.58 KB, patch)
2013-06-17 11:39 UTC, Gustavo Padovan
reviewed Details | Review
bluez: add configure switch for BlueZ 5 (1.27 KB, patch)
2013-06-17 11:40 UTC, Gustavo Padovan
reviewed Details | Review
bluez: pass the NMBluezDevice down to the NMDeviceBt (7.19 KB, patch)
2013-06-17 11:43 UTC, Gustavo Padovan
needs-work Details | Review
bluez: check for bluez when building for BlueZ 5 (911 bytes, patch)
2013-06-17 11:43 UTC, Gustavo Padovan
none Details | Review
bluez: add support for BlueZ 5 (34.21 KB, patch)
2013-06-17 11:43 UTC, Gustavo Padovan
reviewed Details | Review
bluez: pass the NMBluezDevice down to the NMDeviceBt (7.45 KB, patch)
2013-06-18 15:41 UTC, Gustavo Padovan
none Details | Review
bluez: listen to Connected changes in NMBluezDevice (3.22 KB, patch)
2013-06-18 15:42 UTC, Gustavo Padovan
none Details | Review
bluez: listen to Connected changes through NMBluezDevice (4.44 KB, patch)
2013-06-18 15:43 UTC, Gustavo Padovan
none Details | Review
bluez: move org.bluez Connection() handling to NMBluezDevice (13.07 KB, patch)
2013-06-18 15:43 UTC, Gustavo Padovan
none Details | Review
bluez: create the NMDevice immediately (12.58 KB, patch)
2013-06-18 15:43 UTC, Gustavo Padovan
none Details | Review
bluez: add configure switch for BlueZ 5 (1.27 KB, patch)
2013-06-18 15:44 UTC, Gustavo Padovan
none Details | Review
bluez: add support for BlueZ 5 (34.12 KB, patch)
2013-06-18 15:45 UTC, Gustavo Padovan
none Details | Review

Description Emilio Pozuelo Monfort 2013-05-27 10:30:20 UTC
We need to port NM to BlueZ 5.
Comment 1 Emilio Pozuelo Monfort 2013-05-27 18:41:22 UTC
I'm looking into this
Comment 2 Emilio Pozuelo Monfort 2013-06-07 16:32:16 UTC
Created attachment 246257 [details] [review]
bluez: listen to Connected changes in NMBluezDevice
Comment 3 Emilio Pozuelo Monfort 2013-06-07 16:32:20 UTC
Created attachment 246258 [details] [review]
bluez: listen to Connected changes through NMBluezDevice
Comment 4 Emilio Pozuelo Monfort 2013-06-07 16:32:24 UTC
Created attachment 246259 [details] [review]
bluez: move org.bluez Connection() handling to NMBluezDevice
Comment 5 Emilio Pozuelo Monfort 2013-06-07 16:32:27 UTC
Created attachment 246260 [details] [review]
bluez: create the NMDevice immediately

Instead of waiting for the connections to be defined.
Comment 6 Emilio Pozuelo Monfort 2013-06-07 16:32:31 UTC
Created attachment 246261 [details] [review]
bluez: add configure switch for BlueZ 5
Comment 7 Emilio Pozuelo Monfort 2013-06-07 16:32:35 UTC
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.
Comment 8 Emilio Pozuelo Monfort 2013-06-07 16:32:39 UTC
Created attachment 246263 [details] [review]
bluez: add basic dun Profile1 implementation
Comment 9 Emilio Pozuelo Monfort 2013-06-07 16:32:42 UTC
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).
Comment 10 Emilio Pozuelo Monfort 2013-06-07 16:32:46 UTC
Created attachment 246265 [details] [review]
bluez: add code to create the rfcomm device
Comment 11 Emilio Pozuelo Monfort 2013-06-07 16:32:50 UTC
Created attachment 246266 [details] [review]
bluez: hook DUN signals to the devices
Comment 12 Emilio Pozuelo Monfort 2013-06-07 16:32:54 UTC
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.
Comment 13 Emilio Pozuelo Monfort 2013-06-07 16:32:57 UTC
Created attachment 246268 [details] [review]
bluez: get adapter address asynchronously
Comment 14 Emilio Pozuelo Monfort 2013-06-07 16:33:02 UTC
Created attachment 246269 [details] [review]
bluez: connect to PAN device asynchronously
Comment 15 Emilio Pozuelo Monfort 2013-06-07 16:34:44 UTC
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.
Comment 16 Emilio Pozuelo Monfort 2013-06-07 16:57:46 UTC
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.
Comment 17 Gustavo Padovan 2013-06-10 16:39:39 UTC
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.
Comment 18 Gustavo Padovan 2013-06-10 16:46:38 UTC
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.
Comment 19 Gustavo Padovan 2013-06-17 11:12:45 UTC
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.
Comment 20 Gustavo Padovan 2013-06-17 11:38:23 UTC
Created attachment 247015 [details] [review]
 bluez: listen to Connected changes in NMBluezDevice
Comment 21 Gustavo Padovan 2013-06-17 11:38:56 UTC
Created attachment 247016 [details] [review]
bluez: listen to Connected changes through NMBluezDevic
Comment 22 Gustavo Padovan 2013-06-17 11:39:19 UTC
Created attachment 247017 [details] [review]
bluez: move org.bluez Connection() handling to NMBluezDevice
Comment 23 Gustavo Padovan 2013-06-17 11:39:53 UTC
Created attachment 247018 [details] [review]
bluez: create the NMDevice immediately

Instead of waiting for the connections to be defined.
Comment 24 Gustavo Padovan 2013-06-17 11:40:40 UTC
Created attachment 247019 [details] [review]
bluez: add configure switch for BlueZ 5
Comment 25 Gustavo Padovan 2013-06-17 11:43:07 UTC
Created attachment 247020 [details] [review]
bluez: pass the NMBluezDevice down to the NMDeviceBt
Comment 26 Gustavo Padovan 2013-06-17 11:43:28 UTC
Created attachment 247021 [details] [review]
bluez: check for bluez when building for BlueZ 5
Comment 27 Gustavo Padovan 2013-06-17 11:43:55 UTC
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.
Comment 28 Dan Williams 2013-06-17 19:02:12 UTC
Review of attachment 247015 [details] [review]:

Looks good.
Comment 29 Dan Williams 2013-06-17 19:15:50 UTC
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.
Comment 30 Dan Williams 2013-06-17 19:16:57 UTC
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()
Comment 31 Dan Williams 2013-06-17 19:23:39 UTC
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 (...);
}
Comment 32 Dan Williams 2013-06-17 19:25:41 UTC
Review of attachment 247018 [details] [review]:

Looks good.
Comment 33 Dan Williams 2013-06-17 19:25:55 UTC
Review of attachment 247018 [details] [review]:

Looks good.
Comment 34 Dan Williams 2013-06-17 19:57:19 UTC
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.
Comment 35 Dan Williams 2013-06-17 19:57:42 UTC
Review of attachment 247021 [details] [review]:

This one is obsolete, right?  Since we don't link to the bluez libraries anymore.
Comment 36 Dan Williams 2013-06-17 19:58:06 UTC
Review of attachment 247019 [details] [review]:

Looks fine.
Comment 37 Emilio Pozuelo Monfort 2013-06-17 20:04:35 UTC
(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)
Comment 38 Emilio Pozuelo Monfort 2013-06-17 20:05:39 UTC
(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!
Comment 39 Gustavo Padovan 2013-06-18 15:41:57 UTC
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.
Comment 40 Gustavo Padovan 2013-06-18 15:42:46 UTC
Created attachment 247174 [details] [review]
bluez: listen to Connected changes in NMBluezDevice
Comment 41 Gustavo Padovan 2013-06-18 15:43:06 UTC
Created attachment 247175 [details] [review]
bluez: listen to Connected changes through NMBluezDevice
Comment 42 Gustavo Padovan 2013-06-18 15:43:26 UTC
Created attachment 247176 [details] [review]
bluez: move org.bluez Connection() handling to NMBluezDevice
Comment 43 Gustavo Padovan 2013-06-18 15:43:59 UTC
Created attachment 247177 [details] [review]
 bluez: create the NMDevice immediately
Comment 44 Gustavo Padovan 2013-06-18 15:44:36 UTC
Created attachment 247179 [details] [review]
bluez: add configure switch for BlueZ 5
Comment 45 Gustavo Padovan 2013-06-18 15:45:26 UTC
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.
Comment 46 Dan Williams 2013-07-11 15:24:44 UTC
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.
Comment 47 André Klapper 2013-08-02 12:10:06 UTC
Could Gustavo's patches get a review?
Comment 48 Matthias Clasen 2013-08-10 00:42:18 UTC
another ping here - we need to get clarity on whether this will be feasible for gnome 3.10 or not.
Comment 49 Dan Williams 2013-08-16 16:41:09 UTC
(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.
Comment 50 Dan Williams 2013-08-27 20:36:45 UTC
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.
Comment 51 Dan Winship 2013-08-27 21:18:16 UTC
So what is the exact set of regressions relative to bluez 4 following that plan?
Comment 52 Dan Williams 2013-08-30 14:05:11 UTC
(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.
Comment 53 Dan Williams 2013-08-31 00:18:18 UTC
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.
Comment 54 Dan Williams 2013-09-04 12:49:41 UTC
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.
Comment 55 Thomas Haller 2013-09-09 14:53:00 UTC
(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.
Comment 56 Thomas Haller 2013-09-17 11:57:51 UTC
(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.
Comment 57 Dan Winship 2013-09-18 16:23:11 UTC
> 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"
Comment 58 Thomas Haller 2013-09-19 16:14:46 UTC
(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).
Comment 59 Dan Winship 2013-09-20 13:02:53 UTC
(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.
Comment 60 Dan Winship 2013-09-20 13:19:32 UTC
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.
Comment 61 Thomas Haller 2013-09-20 15:54:01 UTC
pushed an update. The branch is still called thaller/bluez.

Implemented everything except making bluez5 default + renaming of files
Comment 62 Thomas Haller 2013-09-20 19:21:19 UTC
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).
Comment 63 Dan Williams 2013-09-23 22:44:42 UTC
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!
Comment 64 Thomas Haller 2013-09-24 11:18:02 UTC
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.
Comment 65 Thomas Haller 2013-09-25 21:19:12 UTC
BlueZ 5 support now pushed to master.

See http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=a84fc3169a18cf06769d5f005feaf8d6cd462a34

Thank you all for your help!!