Return-Path: Content-Type: text/plain; charset=us-ascii Mime-Version: 1.0 (Mac OS X Mail 11.2 \(3445.5.20\)) Subject: Re: [PATCH] adapter: Add ConnectDevice method From: Marcel Holtmann In-Reply-To: <20180119145216.30032-1-szymon.janc@codecoup.pl> Date: Fri, 19 Jan 2018 18:38:18 +0100 Cc: linux-bluetooth@vger.kernel.org Message-Id: <2E0EB758-8B70-418A-9CEC-9348D4F9C870@holtmann.org> References: <20180119145216.30032-1-szymon.janc@codecoup.pl> To: Szymon Janc Sender: linux-bluetooth-owner@vger.kernel.org List-ID: Hi Szymon, > This allows to connect device without doing general discovery. This is > needed for some of qualification tests where there is no general > discovery upfront and we need to do connection to device with provided > address. > > Another usecase is for scenario where scanning happen on one controller but > connection handling on another. > > Implementation wide this results in new temporary object being created and > connect being called on it which behaves exactly the same as if Connect > method from Device1 interface was called on discovered device. If connection > fails, created object is removed. On success this method returns path to > created device object. > > This patch implements bare minimum properties needed for connection - address > and address type. If needed eg. for non-NFC based OOB it could be extended with > more options. > --- > doc/adapter-api.txt | 36 ++++++++++++++++++++++ > src/adapter.c | 89 +++++++++++++++++++++++++++++++++++++++++++++++++++++ > src/device.c | 64 +++++++++++++++++++++++++++++++------- > src/device.h | 2 ++ > 4 files changed, 180 insertions(+), 11 deletions(-) > > diff --git a/doc/adapter-api.txt b/doc/adapter-api.txt > index 0533b674a..67b7ca487 100644 > --- a/doc/adapter-api.txt > +++ b/doc/adapter-api.txt > @@ -145,6 +145,42 @@ Methods void StartDiscovery() > > Possible errors: None > > + object ConnectDevice(dict properties) [experimental] > + > + This method creates new temporary device with defined > + properties and issues connection to that device without > + need of performing General Discovery first. Connection > + mechanism is the same as if Connect method from Device1 > + interface was called. If connection failed, created > + device is removed and this method fails. If connection > + was successful this method returns object path to > + created device object. actually I would prefer if device is never announced if connection fails. Otherwise we might cause a bit of D-Bus object signaling overhead. So unless successfully connect, the device object should be a shallow object. It might exist internally, but is not yet exposed over D-Bus. Also we really need to talk about if the object manager update is triggered before the return message. So which message comes first. > + > + Parameters that may be set in the filter dictionary > + include the following: > + > + string Address > + > + The Bluetooth device address of the remote > + device. This parameter is mandatory. > + > + string AddressType > + > + The Bluetooth device Address Type. This is > + address type that should be used for initial > + connection. If this parameter is not present > + BR/EDR device is created. > + > + Possible values: > + "public" - Public address > + "random" - Random address > + > + Possible errors: org.bluez.Error.InvalidArguments > + org.bluez.Error.AlreadyExists > + org.bluez.Error.NotSupported > + org.bluez.Error.NotReady > + org.bluez.Error.Failed > + > Properties string Address [readonly] > > The Bluetooth device address. > diff --git a/src/adapter.c b/src/adapter.c > index 0a25ae27e..31686bc04 100644 > --- a/src/adapter.c > +++ b/src/adapter.c > @@ -3080,6 +3080,92 @@ static DBusMessage *get_discovery_filters(DBusConnection *conn, > return reply; > } > > +static DBusMessage *connect_device(DBusConnection *conn, > + DBusMessage *msg, void *user_data) > +{ > + struct btd_adapter *adapter = user_data; > + DBusMessageIter iter, subiter, dictiter, value; > + uint8_t addr_type = BDADDR_BREDR; > + bdaddr_t addr = *BDADDR_ANY; > + struct btd_device *device; > + > + DBG("sender %s", dbus_message_get_sender(msg)); > + > + if (!(adapter->current_settings & MGMT_SETTING_POWERED)) > + return btd_error_not_ready(msg); > + > + dbus_message_iter_init(msg, &iter); > + if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_ARRAY || > + dbus_message_iter_get_element_type(&iter) != DBUS_TYPE_DICT_ENTRY) > + return btd_error_invalid_args(msg); > + > + dbus_message_iter_recurse(&iter, &subiter); > + do { Why not while (true) { } here? Is there some affinity towards do-loops ;) > + int type = dbus_message_iter_get_arg_type(&subiter); > + char *key; > + char *str; > + > + if (type == DBUS_TYPE_INVALID) > + break; > + > + dbus_message_iter_recurse(&subiter, &dictiter); > + > + dbus_message_iter_get_basic(&dictiter, &key); > + if (!dbus_message_iter_next(&dictiter)) > + return btd_error_invalid_args(msg); > + > + if (dbus_message_iter_get_arg_type(&dictiter) != > + DBUS_TYPE_VARIANT) > + return btd_error_invalid_args(msg); > + > + dbus_message_iter_recurse(&dictiter, &value); > + > + if (!strcmp(key, "Address")) { > + if (dbus_message_iter_get_arg_type(&value) != > + DBUS_TYPE_STRING) > + return btd_error_invalid_args(msg); > + > + dbus_message_iter_get_basic(&value, &str); > + > + if (str2ba(str, &addr) < 0 ) > + return btd_error_invalid_args(msg); > + } else if (!strcmp(key, "AddressType")) { > + if (dbus_message_iter_get_arg_type(&value) != > + DBUS_TYPE_STRING) > + return btd_error_invalid_args(msg); > + > + dbus_message_iter_get_basic(&value, &str); > + > + > + if (!strcmp(str, "public")) > + addr_type = BDADDR_LE_PUBLIC; > + else if (!strcmp(str, "random")) > + addr_type = BDADDR_LE_RANDOM; > + else > + return btd_error_invalid_args(msg); > + } else { > + return btd_error_invalid_args(msg); > + } > + > + dbus_message_iter_next(&subiter); > + } while (true); > + > + if (!bacmp(&addr, BDADDR_ANY)) > + return btd_error_invalid_args(msg); > + > + device = btd_adapter_find_device(adapter, &addr, addr_type); > + if (device) > + return btd_error_already_exists(msg); > + > + device = adapter_create_device(adapter, &addr, addr_type); > + if (!device) > + return btd_error_failed(msg, "Failed to create new device"); > + > + device_update_last_seen(device, addr_type); > + > + return device_connect(conn, msg, device); > +} > + > static const GDBusMethodTable adapter_methods[] = { > { GDBUS_ASYNC_METHOD("StartDiscovery", NULL, NULL, start_discovery) }, > { GDBUS_METHOD("SetDiscoveryFilter", > @@ -3091,6 +3177,9 @@ static const GDBusMethodTable adapter_methods[] = { > { GDBUS_METHOD("GetDiscoveryFilters", NULL, > GDBUS_ARGS({ "filters", "as" }), > get_discovery_filters) }, > + { GDBUS_EXPERIMENTAL_ASYNC_METHOD("ConnectDevice", > + GDBUS_ARGS({ "properties", "a{sv}" }), NULL, > + connect_device) }, > { } > }; > > diff --git a/src/device.c b/src/device.c > index 1acecce33..5efde4b54 100644 > --- a/src/device.c > +++ b/src/device.c > @@ -624,6 +624,11 @@ static void browse_request_cancel(struct browse_req *req) > > attio_cleanup(device); > > + if (req->msg && dbus_message_is_method_call(req->msg, > + "org.bluez.Adapter1", "ConnectDevice")) > + g_dbus_send_message(dbus_conn, btd_error_failed(req->msg, > + strerror(ECANCELED))); > + > browse_request_free(req); > } > > @@ -1577,9 +1582,14 @@ done: > if (!dev->connect) > return; > > - if (!err && dbus_message_is_method_call(dev->connect, DEVICE_INTERFACE, > - "Connect")) > + if (!err) { > + if (dbus_message_is_method_call(dev->connect, DEVICE_INTERFACE, > + "Connect") || > + dbus_message_is_method_call(dev->connect, > + "org.bluez.Adapter1", "ConnectDevice")) > dev->general_connect = TRUE; > + btd_device_set_temporary(dev, false); > + } > > DBG("returning response to %s", dbus_message_get_sender(dev->connect)); > > @@ -1599,7 +1609,15 @@ done: > /* Start passive SDP discovery to update known services */ > if (dev->bredr && !dev->svc_refreshed) > device_browse_sdp(dev, NULL); > - g_dbus_send_reply(dbus_conn, dev->connect, DBUS_TYPE_INVALID); > + > + if (dbus_message_is_method_call(dev->connect, > + "org.bluez.Adapter1", "ConnectDevice")) > + g_dbus_send_reply(dbus_conn, dev->connect, > + DBUS_TYPE_OBJECT_PATH, > + &dev->path, DBUS_TYPE_INVALID); > + else > + g_dbus_send_reply(dbus_conn, dev->connect, > + DBUS_TYPE_INVALID); > } > > dbus_message_unref(dev->connect); > @@ -1773,11 +1791,15 @@ static DBusMessage *connect_profiles(struct btd_device *dev, uint8_t bdaddr_type > if (!btd_adapter_get_powered(dev->adapter)) > return btd_error_not_ready(msg); > > - btd_device_set_temporary(dev, false); > + if (!dbus_message_is_method_call(msg, "org.bluez.Adapter1", > + "ConnectDevice")) > + btd_device_set_temporary(dev, false); > > if (!state->svc_resolved) > goto resolve_services; > > + /* will never happen for ConnectDevice call */ > + > dev->pending = create_pending_list(dev, uuid); > if (!dev->pending) { > if (dev->svc_refreshed) { > @@ -1864,7 +1886,7 @@ static uint8_t select_conn_bearer(struct btd_device *dev) > return dev->bdaddr_type; > } > > -static DBusMessage *dev_connect(DBusConnection *conn, DBusMessage *msg, > +DBusMessage *device_connect(DBusConnection *conn, DBusMessage *msg, > void *user_data) > { > struct btd_device *dev = user_data; > @@ -1889,10 +1911,13 @@ static DBusMessage *dev_connect(DBusConnection *conn, DBusMessage *msg, > if (bdaddr_type != BDADDR_BREDR) { > int err; > > + /* This will never happen for ConnectDevice call */ > if (dev->le_state.connected) > return dbus_message_new_method_return(msg); > > - btd_device_set_temporary(dev, false); > + if (!dbus_message_is_method_call(msg, "org.bluez.Adapter1", > + "ConnectDevice")) > + btd_device_set_temporary(dev, false); > > if (dev->disable_auto_connect) { > dev->disable_auto_connect = FALSE; > @@ -2272,12 +2297,15 @@ static void browse_request_complete(struct browse_req *req, uint8_t type, > * device->browse is set and "InProgress" error is returned instead > * of actually connecting services > */ > + > msg = dbus_message_ref(req->msg); > browse_request_free(req); > req = NULL; > > - if (dbus_message_is_method_call(msg, DEVICE_INTERFACE, "Connect")) > - reply = dev_connect(dbus_conn, msg, dev); > + if (dbus_message_is_method_call(msg, DEVICE_INTERFACE, "Connect") || > + dbus_message_is_method_call(msg, "org.bluez.Adapter1", > + "ConnectDevice")) > + reply = device_connect(dbus_conn, msg, dev); > else if (dbus_message_is_method_call(msg, DEVICE_INTERFACE, > "ConnectProfile")) > reply = connect_profile(dbus_conn, msg, dev); > @@ -2624,7 +2652,7 @@ static DBusMessage *cancel_pairing(DBusConnection *conn, DBusMessage *msg, > > static const GDBusMethodTable device_methods[] = { > { GDBUS_ASYNC_METHOD("Disconnect", NULL, NULL, dev_disconnect) }, > - { GDBUS_ASYNC_METHOD("Connect", NULL, NULL, dev_connect) }, > + { GDBUS_ASYNC_METHOD("Connect", NULL, NULL, device_connect) }, Looks like you took an easy path here and hacked this into the same message handler. So no, do a clean separation and do that the right way. I really dislike conditional handling like above. And then especially separating the return messages based on how something is called. > { GDBUS_ASYNC_METHOD("ConnectProfile", GDBUS_ARGS({ "UUID", "s" }), > NULL, connect_profile) }, > { GDBUS_ASYNC_METHOD("DisconnectProfile", GDBUS_ARGS({ "UUID", "s" }), > @@ -4998,16 +5026,30 @@ done: > device_browse_gatt(device, NULL); > > if (device->connect) { > - if (err < 0) > + if (err < 0) { > reply = btd_error_failed(device->connect, > strerror(-err)); > - else > + } else { > reply = dbus_message_new_method_return(device->connect); > > + if (dbus_message_is_method_call(device->connect, > + "org.bluez.Adapter1", "ConnectDevice")) > + dbus_message_append_args(reply, > + DBUS_TYPE_OBJECT_PATH, > + &device->path, > + DBUS_TYPE_INVALID); > + } > + > g_dbus_send_message(dbus_conn, reply); > dbus_message_unref(device->connect); > device->connect = NULL; > } > + > + /* This is the case only for error in ConnectProfile, otherwise device > + * is not temporary > + */ > + if (err && device_is_temporary(device)) > + btd_adapter_remove_device(device->adapter, device); > } > > int device_connect_le(struct btd_device *dev) > diff --git a/src/device.h b/src/device.h > index bc046f780..608bc27d3 100644 > --- a/src/device.h > +++ b/src/device.h > @@ -151,6 +151,8 @@ void btd_device_set_pnpid(struct btd_device *device, uint16_t source, > uint16_t vendor, uint16_t product, uint16_t version); > > int device_connect_le(struct btd_device *dev); > +DBusMessage *device_connect(DBusConnection *conn, DBusMessage *msg, > + void *user_data); And I do not feel that DBusMessage or DBusConnection should bleed through into *.h here. Regards Marcel