GNOME Bugzilla – Bug 783013
[review] lr/bluetooth-nap: Add Bluetooth NAP support
Last modified: 2017-06-01 16:50:44 UTC
Usage: nmcli c add type bluetooth bluetooth.type nap ifname bluetooth0 [addr 00:1A:7D:DA:71:0F]
*** Bug 613598 has been marked as a duplicate of this bug. ***
*** Bug 733408 has been marked as a duplicate of this bug. ***
(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
+ 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.
(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 :)
+ /* 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
+ /* 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: "?).
(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.
pushed 3 patches. It tests nicely! But I seem unable to connect a second time, after deactivating the connection + reactivating.
Looks good to me.
merged: https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=f9b1bc16e9e691ab89caf883f33d94be72364671