Bug 1055628

Update ... after cleaning everything and reinstalling the stuff I can't even see the Vodafone entry in the NetworkManager menu (even when previously re-created). Is that a NetworkManager issue?

I am going to re-assign it to Network Manager to let them make sure it isn't the transition to the bluez5 stack, that is the problem.

Cheers,
Don

NOTE: I successfully paired the device with bluetooth-wizard / bluedevil-wizard and the phone changed the bluetooth icon from white to blue (that means the PC is interconnected with the mobile phone), but it seems that the NetworkManager is unable to dial via the new API. It doesn't offer the mobile phone in the list of devices when I try to create the mobile broadband connection.

Correct, F20 does not support Bluetooth DUN because bluez5 completely changes how DUN is done, and that was too large of a task to port NetworkManager to that new interface for F20.  We are working on updating NM to support DUN with bluez5, but that work is not yet complete.

(In reply to Dan Williams from comment #4)
> Correct, F20 does not support Bluetooth DUN because bluez5 completely
> changes how DUN is done, and that was too large of a task to port
> NetworkManager to that new interface for F20.  We are working on updating NM
> to support DUN with bluez5, but that work is not yet complete.

Dan, could you please show where one can check DUN support status, some tracker or code repository.

*** Bug 1030673 has been marked as a duplicate of this bug. ***

(In reply to ZiN from comment #5)
> (In reply to Dan Williams from comment #4)
> > Correct, F20 does not support Bluetooth DUN because bluez5 completely
> > changes how DUN is done, and that was too large of a task to port
> > NetworkManager to that new interface for F20.  We are working on updating NM
> > to support DUN with bluez5, but that work is not yet complete.
> 
> Dan, could you please show where one can check DUN support status, some
> tracker or code repository.

We're working on it upstream at:

http://cgit.freedesktop.org/NetworkManager/NetworkManager/log/?h=dcbw/dun

It's about 10 months the problem was reported (bug 1030673). Any ETA? This gives bad user experience and renders f20 unusable for mobile users.

Hello Dan.

I see the following two lines on that gitweb:
2014-06-26	bluez: re-add DUN support for Bluez5
2014-06-26	bluez: implement DUN connect/disconnect for Bluez5

Does it mean the DUN is already supported upstream?
If so, is there a chance of upgrading to that version and provide people with testing packages for Fedora 20?

Thanks,
Jaromir.

(In reply to Jaromír Cápík from comment #9)
> Hello Dan.
> 
> I see the following two lines on that gitweb:
> 2014-06-26	bluez: re-add DUN support for Bluez5
> 2014-06-26	bluez: implement DUN connect/disconnect for Bluez5
> 
> Does it mean the DUN is already supported upstream?
> If so, is there a chance of upgrading to that version and provide people
> with testing packages for Fedora 20?

No, it's not completely done yet upstream.  I had to pause working on it for a couple reasons, but I'm hoping to pick that work back up again in the next couple weeks.  Some things left to do are to run the SDP search asynchronously to ensure we don't block NetworkManager while searching for the DUN channel number, and do a lot of testing to ensure that bad Bluetooth drivers or devices don't block NetworkManager since all the BT DUN stuff is now run in-process instead of in Bluez.

A further enhancement would be to get all the pieces in place so that pre-configuring the DUN connection is not required, but it can be done as part of the first connection attempt.

I'm working on this.

Ready for review: https://github.com/lkundrak/NetworkManager/commits/lr-bluez5-dun

Please note this won't work on F20, as the ModemManager there is too old and attempts to do 3GPP registration checks even though the modem is connected and already talks PPP. F21 seems to correctly notice that there's no separated data and command connections for RFCOMM ttys and ceases to intermix them.

Also, SELinux is going to deny SDP discovery. Fix here: https://bugzilla.redhat.com/show_bug.cgi?id=1148364

So I don't know anything about bluez's APIs, so I'm mostly only reviewing the style of the code, not the substance...

> bluez: re-add DUN support for Bluez5
> 
> This adds service discovery via SDP and RFCOMM tty management do
> NerworkManager, as it was dropped from Bluez.

"to NetworkManager" not "do NerworkManager" (two typos)

>+	AC_SUBST(BLUEZ5_CFLAGS)
>+	AC_SUBST(BLUEZ5_LIBS)

You don't need that; PKG_CHECK_MODULES's outputs are automatically AB_SUBST()ed.

Also, either as part of this commit or as a separate commit, you should update contrib/fedora/rpm/NetworkManager.spec's BuildRequires.

>-	/* only expect the supported capabilities set. */

You removed that comment, but I'd leave it in; it still applies even to the simplified assertion.

>+			priv->b5_context = nm_bluez5_dun_new (
>+				priv->adapter_address,
>+				priv->address,
>+				simple);

code in NM is generally wrapped around the 100th column or so, so I'd put priv->adapter_address on the same line as the "(" (and re-indent what follows).

It's also really non-idiomatic to pass a GSimpleAsyncResult to an API, rather than having the API take a GAsyncReadyCallback and user_data and creating its own GSimpleAsyncResult...

It's also really weird that the GSimpleAsyncResult is passed to nm_bluez5_dun_new(), when it's nm_bluez5_dun_connect() that is the async op.

And also, that nm_bluez5_dun_connect() can return an error, in which case nm_bluez_device_connect_async() calls g_simple_async_result_complete_in_idle() itself. nm_bluez5_dun_connect() should do that, for consistency.

Except really, you should just make nm_bluez5_dun_connect() be nm_bluez5_dun_connect_async(), and take a GAsyncReadyCallback and user_data, and have nm_bluez5_dun_connect_finish() to get the result.

>+	bdaddr_t src;
>+	bdaddr_t dst;
>+	char *source;
>+	char *dest;

That's a pretty subtle naming distinction... "src" and "src_str" (or "source" and "source_str") would be clearer and more like other NM code.

>+		g_simple_async_result_set_error (context->simple, NM_BT_ERROR,
>+		                                 NM_BT_ERROR_DUN_CONNECT_FAILED,
>+		                                 "Failed to create RFCOMM socket: (%d) %s",
>+		                                 errno, strerror (errno));

We're probably getting this wrong in lots of existing code, but you really need to do:

    int errsv = errno;
    g_simple_async_result_set_error (..., errsv, strerror (errsv));

because NM_BT_ERROR expands to a function call, and evaluating it might change the value of errno before the "errno" and "strerror (errno)" arguments are evaluated...

>+	while ((context->rfcomm_tty_fd = open (tty, O_RDONLY | O_NOCTTY)) < 0 && try--) {

I think there's a missing "return;" at the end of this loop

>+	g_simple_async_result_complete_in_idle (context->simple);

You only need to use complete_in_idle() if it's still the same iteration of the main loop as you started in. From a GSource callback or from the callback of any other async op, you can just use g_simple_async_result_complete().

Thank you, Dan. I've attempted to address all discovered issues.

Updated branch: https://github.com/lkundrak/NetworkManager/commits/lr-bluez5-dun

I do see some warnings on NM startup:

NetworkManager[32029]: <info>  use BlueZ version 5

(NetworkManager:32029): NetworkManager-bluetooth-CRITICAL **: set_adapter_address: assertion 'address' failed

which are coming from nm_bluez_device_new().  In the Bluez4 case we know the adapter address already, so we should set it, but in the Bluez5 case we don't know that until later (when we read the Bluez device properties) so it's going to be NULL initially.

I think the right fix is just to do:

if (adapter_address)
    set_adapter_address ();

in nm_bluez_device_new().

----

bluez5_connect_cb() - indentation issue for the g_simple_async_result_take_error() call, it uses spaces not tabs

Next we should also cache the channel # into NMBluezDevicePrivate in bluez5_connect_cb() and re-use it the next time so we dont' have to do another SDP search on a subsequent connect.  The channel # will never change over the life of the device so we only need to search the first time we activate the DUN connection.

nm_bluez5_dun_connect() should probably just take the channel # as an argument and if (channel >= 0) just proceed to the DUN connect instead of doing a search, and NMBluez5DunFunc should pass it back along with 'device' so the caller can cache it and provide it next time.

Also, with nm_bluez5_dun_connect() the arguments should be aligned with GLib-style, so either all arguments on the same line (if there's room) or every argument on a separate line.  I guess we should really put this into the coding style docs :)

----

Looks pretty good though, and seems to work fine for me!  (minus the kernel panics of course)

(In reply to Dan Williams from comment #16)
> I do see some warnings on NM startup:
> 
> NetworkManager[32029]: <info>  use BlueZ version 5
> 
> (NetworkManager:32029): NetworkManager-bluetooth-CRITICAL **:
> set_adapter_address: assertion 'address' failed

And obviously this was a bug in my original patch, not created by you :)

A few more...

The changes to nm_bluez_device_get_connected() look un-necessary...

nm_bluez_device_connect_async() - the rename of bluez_connect_cb -> bluez4_connect_cb isn't quite accurate, since the function is still used for Bluez5 NAP.  Maybe keep it bluez_connect_cb() and rename bluez5_connect_cb() -> bluez5_dun_connect_cb()?

Thank you for the review.

(In reply to Dan Williams from comment #16)
> I do see some warnings on NM startup:
> 
> NetworkManager[32029]: <info>  use BlueZ version 5
> 
> (NetworkManager:32029): NetworkManager-bluetooth-CRITICAL **:
> set_adapter_address: assertion 'address' failed

Fixed.

> bluez5_connect_cb() - indentation issue for the
> g_simple_async_result_take_error() call, it uses spaces not tabs

Fixed.

> Next we should also cache the channel # into NMBluezDevicePrivate in
> bluez5_connect_cb() and re-use it the next time so we dont' have to do
> another SDP search on a subsequent connect.  The channel # will never change
> over the life of the device so we only need to search the first time we
> activate the DUN connection.
> 
> nm_bluez5_dun_connect() should probably just take the channel # as an
> argument and if (channel >= 0) just proceed to the DUN connect instead of
> doing a search, and NMBluez5DunFunc should pass it back along with 'device'
> so the caller can cache it and provide it next time.

Solved differently. I'm now caching the NMBluez5DunContext (just cleanup it upon disconnect, not free it yet). I believe it keeps the interface saner, not needing an extra argument that would expose a DUN implementation detail such as the port number.

Your original code passed the port number, but also provided separate cleanup and free routines suggesting that it's intended for reconnection.

> Also, with nm_bluez5_dun_connect() the arguments should be aligned with
> GLib-style, so either all arguments on the same line (if there's room) or
> every argument on a separate line.  I guess we should really put this into
> the coding style docs :)

Well, does GLib have a coding style document?

> Looks pretty good though, and seems to work fine for me!  (minus the kernel
> panics of course)

Have you hit a panic conditions with recent kernels? With 3.17 snapshots I'm able to lock RFCOMM up, but never seen a panic. It panicks easily on el7's 3.10 though.

(In reply to Dan Williams from comment #17)
> (In reply to Dan Williams from comment #16)
> > I do see some warnings on NM startup:
> > 
> > NetworkManager[32029]: <info>  use BlueZ version 5
> > 
> > (NetworkManager:32029): NetworkManager-bluetooth-CRITICAL **:
> > set_adapter_address: assertion 'address' failed
> 
> And obviously this was a bug in my original patch, not created by you :)

Well, actually not. I've added the problem as well as the assert when changing it from guint8[ETH_ALEN] addresses to char *.

(In reply to Dan Williams from comment #18)
> A few more...
> 
> The changes to nm_bluez_device_get_connected() look un-necessary...
> 
> nm_bluez_device_connect_async() - the rename of bluez_connect_cb ->
> bluez4_connect_cb isn't quite accurate, since the function is still used for
> Bluez5 NAP.  Maybe keep it bluez_connect_cb() and rename bluez5_connect_cb()
> -> bluez5_dun_connect_cb()?

Done.

Updated branch: https://github.com/lkundrak/NetworkManager/commits/lr-bluez5-dun

Looks good now!

branch merged to master on behalf of Lubomir:

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

NetworkManager-0.9.10.0-8.git20140704.fc21 has been submitted as an update for Fedora 21.
https://admin.fedoraproject.org/updates/NetworkManager-0.9.10.0-8.git20140704.fc21

Package NetworkManager-0.9.10.0-8.git20140704.fc21:
* should fix your issue,
* was pushed to the Fedora 21 testing repository,
* should be available at your local mirror within two days.
Update it with:
# su -c 'yum update --enablerepo=updates-testing NetworkManager-0.9.10.0-8.git20140704.fc21'
as soon as you are able to.
Please go to the following url:
https://admin.fedoraproject.org/updates/FEDORA-2014-12941/NetworkManager-0.9.10.0-8.git20140704.fc21
then log in and leave karma (feedback).

NetworkManager-0.9.10.0-10.git20140704.fc21 has been submitted as an update for Fedora 21.
https://admin.fedoraproject.org/updates/FEDORA-2014-13679/NetworkManager-0.9.10.0-10.git20140704.fc21

NetworkManager-0.9.10.0-10.git20140704.fc21 has been pushed to the Fedora 21 stable repository.  If problems still persist, please make note of it in this bug report.

I am glad, that this bug is finally solved, but anyway, how do I connect Fedora to internet using Bluetooth? I don't see such option anywhere.

Reopening, it still doesn't work for me. I have NetworkManager-bluetooth package, I can setup broadband, but I cannot link it with DUN nor initate it from NetworkManager.

The nm-connection-editor update that has some basic ability to add a Bluetooth connection was submitted for Fedora 22:

https://admin.fedoraproject.org/updates/NetworkManager-1.0.2-1.fc22,network-manager-applet-1.0.2-1.fc22,NetworkManager-openconnect-1.0.2-1.fc22,NetworkManager-openvpn-1.0.2-1.fc22,NetworkManager-vpnc-1.0.2-1.fc22,NetworkManager-openswan-1.0.2-1.fc22

Please open a separate ticket for any other UI functionality you miss (e.g. against gnome-bluetooth).