Hi Bastien,
On Wed, May 21, 2025 at 5:34 AM Bastien Nocera <hadess@xxxxxxxxxx> wrote:
>
> On Tue, 2025-05-20 at 10:36 -0400, Luiz Augusto von Dentz wrote:
> > Hi Bastien,
> >
> > On Tue, May 20, 2025 at 9:27 AM Bastien Nocera <hadess@xxxxxxxxxx>
> > wrote:
> > >
> > > ---
> > > src/device.c | 10 ++++++----
> > > 1 file changed, 6 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/src/device.c b/src/device.c
> > > index 0797e5ff5bb8..d1023f762474 100644
> > > --- a/src/device.c
> > > +++ b/src/device.c
> > > @@ -1922,9 +1922,9 @@ void device_request_disconnect(struct
> > > btd_device *device, DBusMessage *msg)
> > > DBusMessage *reply;
> > >
> > > if (device->bonding_status ==
> > > MGMT_STATUS_AUTH_FAILED)
> > > - err_str = ERR_BREDR_CONN_KEY_MISSING;
> > > + err_str = ":" ERR_BREDR_CONN_KEY_MISSING
> > > ":Link key missing";
> > > else
> > > - err_str = ERR_BREDR_CONN_CANCELED;
> > > + err_str = ":" ERR_BREDR_CONN_CANCELED
> > > ":Connection canceled";
> > > reply = btd_error_failed(device->connect, err_str);
> > > g_dbus_send_message(dbus_conn, reply);
> > > dbus_message_unref(device->connect);
> > > @@ -2545,7 +2545,8 @@ static DBusMessage *connect_profiles(struct
> > > btd_device *dev, uint8_t bdaddr_type
> > >
> > > if (!btd_adapter_get_powered(dev->adapter)) {
> > > return btd_error_not_ready_str(msg,
> > > -
> > > ERR_BREDR_CONN_ADAPTER_NOT_POWERED);
> > > + ":"
> > > ERR_BREDR_CONN_ADAPTER_NOT_POWERED
> > > + ":Adapter not powered");
> > > }
> > >
> > > btd_device_set_temporary(dev, false);
> > > @@ -2564,7 +2565,8 @@ static DBusMessage *connect_profiles(struct
> > > btd_device *dev, uint8_t bdaddr_type
> > > return
> > > dbus_message_new_method_return(msg);
> > > } else {
> > > return
> > > btd_error_not_available_str(msg,
> > > -
> > > ERR_BREDR_CONN_PROFILE_UNAVAILABLE);
> > > + ":"
> > > ERR_BREDR_CONN_PROFILE_UNAVAILABLE ":"
> > > + "Exhausted the list of
> > > BR/EDR profiles to connect to");
> > > }
> > > }
> > >
> > > --
> > > 2.49.0
> >
> > I was afraid we would need to change these string again, also if we
> > need to encode more than human readable errors on on the message
> > something is already quite wrong, so I wonder if we shall instead
> > expand the error code itself e.g.:
> >
> > DBusMessage *btd_error_profile_unavailable_str(DBusMessage *msg,
> > const
> > char *str)
> > {
> > return g_dbus_create_error(msg, ERROR_INTERFACE
> > ".ProfileUnavailable", str);
> > }
> >
> > That in my opinion uses the error reply better since we can isolate
> > what is considered a fixed string, and therefore part of the stable
> > API, and what could be considered a free format which is the error
> > message.
>
> So, for this example, a new error code
> org.bluez.Error.ProfileUnavailable to be added to
> doc/org.bluez.Device.rst right?
>
> Or would org.bluez.Error.Failed.ProfileUnavailable make it easier for
> front-ends to parse? gnome-bluetooth doesn't (currently) have any code
> to differentiate those, so it's not a problem on my side.
Don't think it adds anything to include a subdomain as
org.bluez.Error.Failed, except if you are saying that is going to
break if application don't except errors other than what is currently
documented, I would consider that a bug since otherwise we can't
really add any new errors.
> What about the human-readable strings, would you want bluetoothctl to
> translate the error code to something human-readable, or should we have
> a human-readable message right in src/device.c in this case.
>
> In short, does this work for you?
>
> DBusMessage *btd_error_failed_profile_unavailable(DBusMessage *msg)
> {
> return g_dbus_create_error(msg, ERROR_INTERFACE
> ".Failed.ProfileUnavailable", "Exhausted the list of
> BR/EDR profiles to connect to");
> }
Except for the error just being .ProfileUnavailable that sounds good to me.
> >
> > >
> >
--
Luiz Augusto von Dentz
