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 783013 - [review] lr/bluetooth-nap: Add Bluetooth NAP support
[review] lr/bluetooth-nap: Add Bluetooth NAP support
Status: RESOLVED FIXED
Product: NetworkManager
Classification: Platform
Component: general
git master
Other Linux
: Normal normal
: ---
Assigned To: NetworkManager maintainer(s)
NetworkManager maintainer(s)
: 613598 733408 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2017-05-23 19:16 UTC by Lubomir Rintel
Modified: 2017-06-01 16:50 UTC
See Also:
GNOME target: ---
GNOME version: ---



Description Lubomir Rintel 2017-05-23 19:16:34 UTC
Usage:

nmcli c add type bluetooth bluetooth.type nap ifname bluetooth0 [addr 00:1A:7D:DA:71:0F]
Comment 1 Lubomir Rintel 2017-05-23 19:17:40 UTC
*** Bug 613598 has been marked as a duplicate of this bug. ***
Comment 2 Lubomir Rintel 2017-05-23 19:17:41 UTC
*** Bug 733408 has been marked as a duplicate of this bug. ***
Comment 3 Lubomir Rintel 2017-05-24 09:16:53 UTC
(In reply to Lubomir Rintel from comment #0)
> Usage:
> 
> nmcli c add type bluetooth bluetooth.type nap ifname bluetooth0 [addr
> 00:1A:7D:DA:71:0F]

Also, you probably want "ipv4.method shared" and "ipv6.method shared" too
Comment 4 Thomas Haller 2017-05-24 14:44:40 UTC
+              return nm_connection_get_setting_by_name (connection,
+                     nm_setting_connection_get_connection_type (s_con));

indentation


> core/connections: pick base setting from settings the connection actually has
    
the behavior of nm_connection_get_connection_type() and nm_connection_is_type() is well established. They are convenience methods that use connection.type property. That cannot be changed.

I think the handling of how to express such settings in a connection, needs more thought.


> device: register a bridge for Bluetooth NAP with Bluez
    
extern NMBtVTableNetworkServer *nm_bt_vtable_network_server;
       ^^const


+#include "bluetooth/nm-bluez-common.h"

This is wrong. The bluetooth plugin can use core parts, but not vice versa. Move NMBtVTableNetworkServer to core, for example src/device/nm-device-bridge.h.




> bluetooth: expose known HCI adapters to devices
    
network_server_added() leaks NMBtNetworkServer, if you receive Add events for the same path.

Can you use a CList to track the interfaces, instead of GHashTable? The only place where you use the O(1) API is added/removed. At other places, it just iterates over the hashtable (those places are much more frequently executed). At that point, you can just as well use a linked list.

NMBtNetworkServer should have no NM prefix, as it's an internal struct.
Comment 5 Lubomir Rintel 2017-05-25 16:59:26 UTC
(In reply to Thomas Haller from comment #4)
> +              return nm_connection_get_setting_by_name (connection,
> +                     nm_setting_connection_get_connection_type (s_con));
> 
> indentation

Changed into a slightly less offensive indentation.

> > core/connections: pick base setting from settings the connection actually has
>     
> the behavior of nm_connection_get_connection_type() and
> nm_connection_is_type() is well established. They are convenience methods
> that use connection.type property. That cannot be changed.

Note that this does not change the actual behavior in any of existing cases.

> I think the handling of how to express such settings in a connection, needs
> more thought.

Please, feel free to share your thoughts as long as they stay constructive.

Apart from the reasoning described in the commit message, the motivation was not to confuse the users. The other option seems to be to use con.type=bridge instead of con.type=bluetooth for bridges with Bluetooth NAP option which seems too confusing to the users (and for that reason a worst option).

> > device: register a bridge for Bluetooth NAP with Bluez
>     
> extern NMBtVTableNetworkServer *nm_bt_vtable_network_server;
>        ^^const
> 
> 
> +#include "bluetooth/nm-bluez-common.h"
> 
> This is wrong. The bluetooth plugin can use core parts, but not vice versa.
> Move NMBtVTableNetworkServer to core, for example
> src/device/nm-device-bridge.h.

Moved to nm-device. The whole thing might just be moved to nm-device-bridge; i have no real preferrence. This makes NMDevice even more cluttered than it is on the other idea BlueZ probably doesn't really care about the device actually being a bridge -- just something that you can enslave links to.

> > bluetooth: expose known HCI adapters to devices
>     
> network_server_added() leaks NMBtNetworkServer, if you receive Add events
> for the same path.

Fixed.

> Can you use a CList to track the interfaces, instead of GHashTable? The only
> place where you use the O(1) API is added/removed. At other places, it just
> iterates over the hashtable (those places are much more frequently
> executed). At that point, you can just as well use a linked list.

Done. It indeed looks much better now and dropped line count significantly. Thanks.

> NMBtNetworkServer should have no NM prefix, as it's an internal struct.

Fixed.

Pushed an updated branch now: https://cgit.freedesktop.org/NetworkManager/NetworkManager/log/?h=lr/bluetooth-nap

It is now perfect, which means I'll take offense at criticism :)
Comment 6 Thomas Haller 2017-05-29 10:05:49 UTC
+    /* NAP mode needs a bridge setting, and a bridge needs a name. */
+    if (!strcmp (priv->type, NM_SETTING_BLUETOOTH_TYPE_NAP)) {
+         if (!_nm_connection_verify_required_interface_name (connection, error))

connection may be NULL, when calling nm_setting_verify() without an entire connection.


>> device: retry autoactivation upon a component addition
    
component_added() only has one real implementation (NMDeviceBt). Which then, as first thing calls to the NMDevice's implementation, which then always returns FALSE. If the parent's component_added() would return TRUE, component_added(), then NMDeviceBt would return early with TRUE.

Instead, move the code from NMDevice:component_added() to nm_device_notify_component_added().

Also, only call component_added() for devices that implement it ("if (klass->component_added)"), contrary to a stub-parent-implementation (e.g. https://cgit.freedesktop.org/NetworkManager/NetworkManager/tree/src/devices/nm-device.c?id=c244cc9415cffa4fb9d432cb4b127b92d50bae32#n2176 ). The reason is that component_added()'s return value means "component consumed". That makes it hard to understand what it means to call the parent implementation (what does it mean, if the parent consumed it? Just leave it? If the parent didn't consume it, may we still?).




Could you rename the defines from src/devices/bluetooth/nm-bluez-common.h to have a NM_ prefix? Especially now, that the header is not only used in the plugin, but included by nm-device.h.




{
        nm_device_factory_emit_component_added (NM_DEVICE_FACTORY (user_data), NULL);

indentation




> bluetooth: emit component-added when a network server is added

commit uses signal NM_BLUEZ_MANAGER_NETWORK_SERVER_ADDED that is only introduced in the next commit
Comment 7 Thomas Haller 2017-05-29 10:21:49 UTC
+         /* Device and path matches are exact. */
+         if (   (path && !strcmp (network_server->path, path))
+             || (device && network_server->device != device))
                                                  ^^^^
this logic doesn't seem right.



could you add _NMLOG() macro to nm-bluez5-manager.c. It gives a consistent logging prefix ("bt5-mgr: "?).
Comment 8 Lubomir Rintel 2017-05-31 18:21:52 UTC
(In reply to Thomas Haller from comment #6)
> +    /* NAP mode needs a bridge setting, and a bridge needs a name. */
> +    if (!strcmp (priv->type, NM_SETTING_BLUETOOTH_TYPE_NAP)) {
> +         if (!_nm_connection_verify_required_interface_name (connection,
> error))
> 
> connection may be NULL, when calling nm_setting_verify() without an entire
> connection.

Uh, then it's wrong already in numerous places.

Added a fix.

> >> device: retry autoactivation upon a component addition
>     
> component_added() only has one real implementation (NMDeviceBt). Which then,
> as first thing calls to the NMDevice's implementation, which then always
> returns FALSE. If the parent's component_added() would return TRUE,
> component_added(), then NMDeviceBt would return early with TRUE.
> 
> Instead, move the code from NMDevice:component_added() to
> nm_device_notify_component_added().
> 
> Also, only call component_added() for devices that implement it ("if
> (klass->component_added)"), contrary to a stub-parent-implementation (e.g.
> https://cgit.freedesktop.org/NetworkManager/NetworkManager/tree/src/devices/
> nm-device.c?id=c244cc9415cffa4fb9d432cb4b127b92d50bae32#n2176 ). The reason
> is that component_added()'s return value means "component consumed". That
> makes it hard to understand what it means to call the parent implementation
> (what does it mean, if the parent consumed it? Just leave it? If the parent
> didn't consume it, may we still?).

Done.

> Could you rename the defines from src/devices/bluetooth/nm-bluez-common.h to
> have a NM_ prefix? Especially now, that the header is not only used in the
> plugin, but included by nm-device.h.

Done.

> {
>         nm_device_factory_emit_component_added (NM_DEVICE_FACTORY
> (user_data), NULL);
> 
> indentation

Fixed.

> > bluetooth: emit component-added when a network server is added
> 
> commit uses signal NM_BLUEZ_MANAGER_NETWORK_SERVER_ADDED that is only
> introduced in the next commit

Reordered.

(In reply to Thomas Haller from comment #7)
> +         /* Device and path matches are exact. */
> +         if (   (path && !strcmp (network_server->path, path))
> +             || (device && network_server->device != device))
>                                                   ^^^^
> this logic doesn't seem right.

Fixed.

> could you add _NMLOG() macro to nm-bluez5-manager.c. It gives a consistent
> logging prefix ("bt5-mgr: "?).

Done.
Comment 9 Thomas Haller 2017-05-31 21:32:17 UTC
pushed 3 patches.

It tests nicely! But I seem unable to connect a second time, after deactivating the connection + reactivating.
Comment 10 Beniamino Galvani 2017-06-01 07:32:51 UTC
Looks good to me.