| Message ID | 20250520162621.190769-1-frederic.danis@collabora.com |
|---|---|
| Headers | show |
| Series | Propagate disconnection reason | expand |
Hi Frédéric, On Tue, May 20, 2025 at 12:26 PM Frédéric Danis <frederic.danis@collabora.com> wrote: > > Currently a client application is informed of the disconnection by the > update of the Connected property to false. > This sends a Disconnected signal with the disconnection reason before > the property is updated. > > This helps client application to know the reason for the disconnection > and to take appropriate action. > --- > v1->v2: Propagate numerical reason instead of text one > > src/adapter.c | 13 ++++++++----- > src/device.c | 16 ++++++++++++++-- > src/device.h | 3 ++- > 3 files changed, 24 insertions(+), 8 deletions(-) > > diff --git a/src/adapter.c b/src/adapter.c > index fd425e6d2..a10721489 100644 > --- a/src/adapter.c > +++ b/src/adapter.c > @@ -7549,7 +7549,8 @@ struct agent *adapter_get_agent(struct btd_adapter *adapter) > > static void adapter_remove_connection(struct btd_adapter *adapter, > struct btd_device *device, > - uint8_t bdaddr_type) > + uint8_t bdaddr_type, > + uint8_t reason) > { > bool remove_device = false; > > @@ -7560,7 +7561,7 @@ static void adapter_remove_connection(struct btd_adapter *adapter, > return; > } > > - device_remove_connection(device, bdaddr_type, &remove_device); > + device_remove_connection(device, bdaddr_type, &remove_device, reason); > > device_cancel_authentication(device, TRUE); > > @@ -7601,9 +7602,11 @@ static void adapter_stop(struct btd_adapter *adapter) > struct btd_device *device = adapter->connections->data; > uint8_t addr_type = btd_device_get_bdaddr_type(device); > > - adapter_remove_connection(adapter, device, BDADDR_BREDR); > + adapter_remove_connection(adapter, device, BDADDR_BREDR, > + MGMT_DEV_DISCONN_UNKNOWN); > if (addr_type != BDADDR_BREDR) > - adapter_remove_connection(adapter, device, addr_type); > + adapter_remove_connection(adapter, device, addr_type, > + MGMT_DEV_DISCONN_UNKNOWN); > } > > g_dbus_emit_property_changed(dbus_conn, adapter->path, > @@ -8551,7 +8554,7 @@ static void dev_disconnected(struct btd_adapter *adapter, > > device = btd_adapter_find_device(adapter, &addr->bdaddr, addr->type); > if (device) { > - adapter_remove_connection(adapter, device, addr->type); > + adapter_remove_connection(adapter, device, addr->type, reason); > disconnect_notify(device, reason); > } > > diff --git a/src/device.c b/src/device.c > index d230af0a8..00a0fbfc7 100644 > --- a/src/device.c > +++ b/src/device.c > @@ -3417,6 +3417,12 @@ static const GDBusMethodTable device_methods[] = { > { } > }; > > +static const GDBusSignalTable device_signals[] = { > + { GDBUS_SIGNAL("Disconnected", > + GDBUS_ARGS({ "reason", "y" })) }, Ive changed my mind regarding this, this should actually have the same format as Device.Connect error reply, so we use the same domain of errors org.bluez.Error.{Name} followed by its message. > + { } > +}; > + > static gboolean > dev_property_get_prefer_bearer(const GDBusPropertyTable *property, > DBusMessageIter *iter, void *data) > @@ -3638,7 +3644,8 @@ static void set_temporary_timer(struct btd_device *dev, unsigned int timeout) > } > > void device_remove_connection(struct btd_device *device, uint8_t bdaddr_type, > - bool *remove) > + bool *remove, > + uint8_t reason) > { > struct bearer_state *state = get_state(device, bdaddr_type); > DBusMessage *reply; > @@ -3708,6 +3715,11 @@ void device_remove_connection(struct btd_device *device, uint8_t bdaddr_type, > g_slist_free_full(device->eir_uuids, g_free); > device->eir_uuids = NULL; > > + g_dbus_emit_signal(dbus_conn, device->path, DEVICE_INTERFACE, > + "Disconnected", > + DBUS_TYPE_BYTE, &reason, > + DBUS_TYPE_INVALID); > + > g_dbus_emit_property_changed(dbus_conn, device->path, > DEVICE_INTERFACE, "Connected"); > > @@ -4611,7 +4623,7 @@ static struct btd_device *device_new(struct btd_adapter *adapter, > > if (g_dbus_register_interface(dbus_conn, > device->path, DEVICE_INTERFACE, > - device_methods, NULL, > + device_methods, device_signals, > device_properties, device, > device_free) == FALSE) { > error("Unable to register device interface for %s", address); > diff --git a/src/device.h b/src/device.h > index a35bb1386..4eebcebe9 100644 > --- a/src/device.h > +++ b/src/device.h > @@ -134,7 +134,8 @@ gboolean device_is_authenticating(struct btd_device *device); > void device_add_connection(struct btd_device *dev, uint8_t bdaddr_type, > uint32_t flags); > void device_remove_connection(struct btd_device *device, uint8_t bdaddr_type, > - bool *remove); > + bool *remove, > + uint8_t reason); > void device_request_disconnect(struct btd_device *device, DBusMessage *msg); > bool device_is_disconnecting(struct btd_device *device); > void device_set_ltk(struct btd_device *device, const uint8_t val[16], > -- > 2.43.0 > >
Hi Luiz, On 20/05/2025 18:41, Luiz Augusto von Dentz wrote: > Hi Frédéric, > > On Tue, May 20, 2025 at 12:26 PM Frédéric Danis > <frederic.danis@collabora.com> wrote: >> Currently a client application is informed of the disconnection by the >> update of the Connected property to false. >> This sends a Disconnected signal with the disconnection reason before >> the property is updated. >> >> This helps client application to know the reason for the disconnection >> and to take appropriate action. >> --- >> v1->v2: Propagate numerical reason instead of text one >> >> src/adapter.c | 13 ++++++++----- >> src/device.c | 16 ++++++++++++++-- >> src/device.h | 3 ++- >> 3 files changed, 24 insertions(+), 8 deletions(-) >> >> diff --git a/src/adapter.c b/src/adapter.c >> index fd425e6d2..a10721489 100644 >> --- a/src/adapter.c >> +++ b/src/adapter.c >> @@ -7549,7 +7549,8 @@ struct agent *adapter_get_agent(struct btd_adapter *adapter) >> >> static void adapter_remove_connection(struct btd_adapter *adapter, >> struct btd_device *device, >> - uint8_t bdaddr_type) >> + uint8_t bdaddr_type, >> + uint8_t reason) >> { >> bool remove_device = false; >> >> @@ -7560,7 +7561,7 @@ static void adapter_remove_connection(struct btd_adapter *adapter, >> return; >> } >> >> - device_remove_connection(device, bdaddr_type, &remove_device); >> + device_remove_connection(device, bdaddr_type, &remove_device, reason); >> >> device_cancel_authentication(device, TRUE); >> >> @@ -7601,9 +7602,11 @@ static void adapter_stop(struct btd_adapter *adapter) >> struct btd_device *device = adapter->connections->data; >> uint8_t addr_type = btd_device_get_bdaddr_type(device); >> >> - adapter_remove_connection(adapter, device, BDADDR_BREDR); >> + adapter_remove_connection(adapter, device, BDADDR_BREDR, >> + MGMT_DEV_DISCONN_UNKNOWN); >> if (addr_type != BDADDR_BREDR) >> - adapter_remove_connection(adapter, device, addr_type); >> + adapter_remove_connection(adapter, device, addr_type, >> + MGMT_DEV_DISCONN_UNKNOWN); >> } >> >> g_dbus_emit_property_changed(dbus_conn, adapter->path, >> @@ -8551,7 +8554,7 @@ static void dev_disconnected(struct btd_adapter *adapter, >> >> device = btd_adapter_find_device(adapter, &addr->bdaddr, addr->type); >> if (device) { >> - adapter_remove_connection(adapter, device, addr->type); >> + adapter_remove_connection(adapter, device, addr->type, reason); >> disconnect_notify(device, reason); >> } >> >> diff --git a/src/device.c b/src/device.c >> index d230af0a8..00a0fbfc7 100644 >> --- a/src/device.c >> +++ b/src/device.c >> @@ -3417,6 +3417,12 @@ static const GDBusMethodTable device_methods[] = { >> { } >> }; >> >> +static const GDBusSignalTable device_signals[] = { >> + { GDBUS_SIGNAL("Disconnected", >> + GDBUS_ARGS({ "reason", "y" })) }, > Ive changed my mind regarding this, this should actually have the same > format as Device.Connect error reply, so we use the same domain of > errors org.bluez.Error.{Name} followed by its message. I'm not sure to understand this point as some of the possible reason are not errors like MGMT_DEV_DISCONN_LOCAL_HOST or MGMT_DEV_DISCONN_REMOTE, and so org.bluez.Error.{Name} seems inappropriate to me here. To be more similar to other methods, the Disconnected signal may provide the reason as text and numerical value, which can be useful in case of 'disconnection-unknown' text.
Hi Frédéric, On Wed, May 21, 2025 at 4:49 AM Frédéric Danis <frederic.danis@collabora.com> wrote: > > Hi Luiz, > > On 20/05/2025 18:41, Luiz Augusto von Dentz wrote: > > Hi Frédéric, > > On Tue, May 20, 2025 at 12:26 PM Frédéric Danis > <frederic.danis@collabora.com> wrote: > > Currently a client application is informed of the disconnection by the > update of the Connected property to false. > This sends a Disconnected signal with the disconnection reason before > the property is updated. > > This helps client application to know the reason for the disconnection > and to take appropriate action. > --- > v1->v2: Propagate numerical reason instead of text one > > src/adapter.c | 13 ++++++++----- > src/device.c | 16 ++++++++++++++-- > src/device.h | 3 ++- > 3 files changed, 24 insertions(+), 8 deletions(-) > > diff --git a/src/adapter.c b/src/adapter.c > index fd425e6d2..a10721489 100644 > --- a/src/adapter.c > +++ b/src/adapter.c > @@ -7549,7 +7549,8 @@ struct agent *adapter_get_agent(struct btd_adapter *adapter) > > static void adapter_remove_connection(struct btd_adapter *adapter, > struct btd_device *device, > - uint8_t bdaddr_type) > + uint8_t bdaddr_type, > + uint8_t reason) > { > bool remove_device = false; > > @@ -7560,7 +7561,7 @@ static void adapter_remove_connection(struct btd_adapter *adapter, > return; > } > > - device_remove_connection(device, bdaddr_type, &remove_device); > + device_remove_connection(device, bdaddr_type, &remove_device, reason); > > device_cancel_authentication(device, TRUE); > > @@ -7601,9 +7602,11 @@ static void adapter_stop(struct btd_adapter *adapter) > struct btd_device *device = adapter->connections->data; > uint8_t addr_type = btd_device_get_bdaddr_type(device); > > - adapter_remove_connection(adapter, device, BDADDR_BREDR); > + adapter_remove_connection(adapter, device, BDADDR_BREDR, > + MGMT_DEV_DISCONN_UNKNOWN); > if (addr_type != BDADDR_BREDR) > - adapter_remove_connection(adapter, device, addr_type); > + adapter_remove_connection(adapter, device, addr_type, > + MGMT_DEV_DISCONN_UNKNOWN); > } > > g_dbus_emit_property_changed(dbus_conn, adapter->path, > @@ -8551,7 +8554,7 @@ static void dev_disconnected(struct btd_adapter *adapter, > > device = btd_adapter_find_device(adapter, &addr->bdaddr, addr->type); > if (device) { > - adapter_remove_connection(adapter, device, addr->type); > + adapter_remove_connection(adapter, device, addr->type, reason); > disconnect_notify(device, reason); > } > > diff --git a/src/device.c b/src/device.c > index d230af0a8..00a0fbfc7 100644 > --- a/src/device.c > +++ b/src/device.c > @@ -3417,6 +3417,12 @@ static const GDBusMethodTable device_methods[] = { > { } > }; > > +static const GDBusSignalTable device_signals[] = { > + { GDBUS_SIGNAL("Disconnected", > + GDBUS_ARGS({ "reason", "y" })) }, > > Ive changed my mind regarding this, this should actually have the same > format as Device.Connect error reply, so we use the same domain of > errors org.bluez.Error.{Name} followed by its message. > > I'm not sure to understand this point as some of the possible reason are not errors > like MGMT_DEV_DISCONN_LOCAL_HOST or MGMT_DEV_DISCONN_REMOTE, and so org.bluez.Error.{Name} > seems inappropriate to me here. We can do something like org.bluez.Error.None for these, or use a different domain like org.bluez.Reason.Local, that still is a fixed string that client can match, rather than a free format string, note that sometimes an error can cause a clean disconnect but in that case we want to capture the error not the disconnect reason. > To be more similar to other methods, the Disconnected signal may provide the reason as text > and numerical value, which can be useful in case of 'disconnection-unknown' text. But we are already masking the numerical reason, besides for D-Bus it sort of makes more sense to use a domain name to describe errors.