Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210618001559.1284204-1-mcchou@chromium.org> <20210617170013.BlueZ.v1.1.If79c3f0720cc893be1770c6853fe2fbe626f2caa@changeid>
 <CABBYNZJZbc10FaOpXwqpZW9PTzXeTNc8ZpTU+KGwHbgUUMU_gQ@mail.gmail.com>
In-Reply-To: <CABBYNZJZbc10FaOpXwqpZW9PTzXeTNc8ZpTU+KGwHbgUUMU_gQ@mail.gmail.com>
From:   Miao-chen Chou <mcchou@chromium.org>
Date:   Fri, 25 Jun 2021 18:51:52 -0700
Message-ID: <CABmPvSFD5Zn1W-TpkpgtHLvKqp48BON5DcQm1F89or8zo1JY9g@mail.gmail.com>
Subject: Re: [BlueZ PATCH v1 1/3] error: BR/EDR and LE connection failure reasons
To:     Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Cc:     Bluetooth Kernel Mailing List <linux-bluetooth@vger.kernel.org>,
        Marcel Holtmann <marcel@holtmann.org>,
        Alain Michaud <alainm@chromium.org>,
        Luiz Augusto von Dentz <luiz.von.dentz@intel.com>,
        Howard Chung <howardchung@google.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Luiz,

On Fri, Jun 18, 2021 at 11:18 AM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Miao,
>
> On Thu, Jun 17, 2021 at 8:08 PM Miao-chen Chou <mcchou@chromium.org> wrote:
> >
> > The source of Connect() failures can be divided into the following
> > three.
> > - bluetoothd's device interface state transition and profile state
> >   transition
> > - Kernel's L2CAP layer state transition
> > - Potential HCI error codes returned by the remote device
> >
> > Reviewed-by: Alain Michaud <alainm@chromium.org>
> > Reviewed-by: Howard Chung <howardchung@google.com>
> > ---
> >
> >  src/error.c | 124 +++++++++++++++++++++++++++++++++++++
> >  src/error.h | 171 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 295 insertions(+)
> >
> > diff --git a/src/error.c b/src/error.c
> > index 89517075e..b72e3d764 100644
> > --- a/src/error.c
> > +++ b/src/error.c
> > @@ -27,6 +27,7 @@
> >  #include <config.h>
> >  #endif
> >
> > +#include <stdio.h>
> >  #include "gdbus/gdbus.h"
> >
> >  #include "error.h"
> > @@ -43,6 +44,15 @@ DBusMessage *btd_error_invalid_args_str(DBusMessage *msg, const char *str)
> >                                         "%s", str);
> >  }
> >
> > +DBusMessage *btd_error_invalid_args_err(DBusMessage *msg, uint16_t err)
> > +{
> > +       char str[16];
> > +
> > +       sprintf(str, "BtdError:0x%04X", err);
> > +       return g_dbus_create_error(msg, ERROR_INTERFACE ".InvalidArguments",
> > +                                       "%s", str);
> > +}
> > +
> >  DBusMessage *btd_error_busy(DBusMessage *msg)
> >  {
> >         return g_dbus_create_error(msg, ERROR_INTERFACE ".InProgress",
> > @@ -79,12 +89,30 @@ DBusMessage *btd_error_in_progress(DBusMessage *msg)
> >                                         "In Progress");
> >  }
> >
> > +DBusMessage *btd_error_in_progress_err(DBusMessage *msg, uint16_t err)
> > +{
> > +       char str[16];
> > +
> > +       sprintf(str, "BtdError:0x%04X", err);
> > +       return g_dbus_create_error(msg, ERROR_INTERFACE ".InProgress", "%s",
> > +                                       str);
>
> Was there a problem that you didn't use g_dbus_create_error to convert
> the error? I mean instead of using %s use BtdError:0x%04X, actually it
> might be enough to just have the code portion and skip BtdError as
> that can be assumed implicitly the our error messages would contain
> error codes.
I thought about dropping BtdError from the error message, but I ended
up with BtdError to make the meaning more explicit. I am happy to drop
it if the implicity is fine.
>
> > +}
> > +
> >  DBusMessage *btd_error_not_available(DBusMessage *msg)
> >  {
> >         return g_dbus_create_error(msg, ERROR_INTERFACE ".NotAvailable",
> >                                         "Operation currently not available");
> >  }
> >
> > +DBusMessage *btd_error_not_available_err(DBusMessage *msg, uint16_t err)
> > +{
> > +       char str[16];
> > +
> > +       sprintf(str, "BtdError:0x%04X", err);
> > +       return g_dbus_create_error(msg, ERROR_INTERFACE ".NotAvailable", "%s",
> > +                                       str);
> > +}
> > +
> >  DBusMessage *btd_error_does_not_exist(DBusMessage *msg)
> >  {
> >         return g_dbus_create_error(msg, ERROR_INTERFACE ".DoesNotExist",
> > @@ -121,8 +149,104 @@ DBusMessage *btd_error_not_ready(DBusMessage *msg)
> >                                         "Resource Not Ready");
> >  }
> >
> > +DBusMessage *btd_error_not_ready_err(DBusMessage *msg, uint16_t err)
> > +{
> > +       char str[16];
> > +
> > +       sprintf(str, "BtdError:0x%04X", err);
> > +       return g_dbus_create_error(msg, ERROR_INTERFACE ".NotReady", "%s", str);
> > +}
> > +
> >  DBusMessage *btd_error_failed(DBusMessage *msg, const char *str)
> >  {
> >         return g_dbus_create_error(msg, ERROR_INTERFACE
> >                                         ".Failed", "%s", str);
> >  }
> > +
> > +DBusMessage *btd_error_failed_err(DBusMessage *msg, uint16_t err)
> > +{
> > +       char str[16];
> > +
> > +       sprintf(str, "BtdError:0x%04X", err);
> > +       return g_dbus_create_error(msg, ERROR_INTERFACE ".Failed", "%s", str);
> > +}
> > +
> > +uint16_t btd_error_bredr_conn_from_errno(int errno_code)
> > +{
> > +       switch (-errno_code) {
> > +       case EALREADY:
> > +       case EISCONN: // Fall through
> > +               return BTD_ERR_BREDR_CONN_ALREADY_CONNECTED;
> > +       case EHOSTDOWN:
> > +               return BTD_ERR_BREDR_CONN_PAGE_TIMEOUT;
> > +       case ENOPROTOOPT:
> > +               return BTD_ERR_BREDR_CONN_PROFILE_UNAVAILABLE;
> > +       case EIO:
> > +               return BTD_ERR_BREDR_CONN_CREATE_SOCKET;
> > +       case EINVAL:
> > +               return BTD_ERR_BREDR_CONN_INVALID_ARGUMENTS;
> > +       case EHOSTUNREACH:
> > +               return BTD_ERR_BREDR_CONN_ADAPTER_NOT_POWERED;
> > +       case EOPNOTSUPP:
> > +       case EPROTONOSUPPORT: // Fall through
> > +               return BTD_ERR_BREDR_CONN_NOT_SUPPORTED;
> > +       case EBADFD:
> > +               return BTD_ERR_BREDR_CONN_BAD_SOCKET;
> > +       case ENOMEM:
> > +               return BTD_ERR_BREDR_CONN_MEMORY_ALLOC;
> > +       case EBUSY:
> > +               return BTD_ERR_BREDR_CONN_BUSY;
> > +       case EMLINK:
> > +               return BTD_ERR_BREDR_CONN_SYNC_CONNECT_LIMIT;
> > +       case ETIMEDOUT:
> > +               return BTD_ERR_BREDR_CONN_TIMEOUT;
> > +       case ECONNREFUSED:
> > +               return BTD_ERR_BREDR_CONN_REFUSED;
> > +       case ECONNRESET:
> > +               return BTD_ERR_BREDR_CONN_TERM_BY_REMOTE;
> > +       case ECONNABORTED:
> > +               return BTD_ERR_BREDR_CONN_TERM_BY_LOCAL;
> > +       case EPROTO:
> > +               return BTD_ERR_BREDR_CONN_PROTO_ERROR;
> > +       default:
> > +               return BTD_ERR_BREDR_CONN_UNKNOWN;
> > +       }
> > +}
> > +
> > +uint16_t btd_error_le_conn_from_errno(int errno_code)
> > +{
> > +       switch (-errno_code) {
> > +       case EINVAL:
> > +               return BTD_ERR_LE_CONN_INVALID_ARGUMENTS;
> > +       case EHOSTUNREACH:
> > +               return BTD_ERR_LE_CONN_ADAPTER_NOT_POWERED;
> > +       case EOPNOTSUPP:
> > +       case EPROTONOSUPPORT: // Fall through
> > +               return BTD_ERR_LE_CONN_NOT_SUPPORTED;
> > +       case EALREADY:
> > +       case EISCONN: // Fall through
> > +               return BTD_ERR_LE_CONN_ALREADY_CONNECTED;
> > +       case EBADFD:
> > +               return BTD_ERR_LE_CONN_BAD_SOCKET;
> > +       case ENOMEM:
> > +               return BTD_ERR_LE_CONN_MEMORY_ALLOC;
> > +       case EBUSY:
> > +               return BTD_ERR_LE_CONN_BUSY;
> > +       case ECONNREFUSED:
> > +               return BTD_ERR_LE_CONN_REFUSED;
> > +       case EIO:
> > +               return BTD_ERR_LE_CONN_CREATE_SOCKET;
> > +       case ETIMEDOUT:
> > +               return BTD_ERR_LE_CONN_TIMEOUT;
> > +       case EMLINK:
> > +               return BTD_ERR_LE_CONN_SYNC_CONNECT_LIMIT;
> > +       case ECONNRESET:
> > +               return BTD_ERR_LE_CONN_TERM_BY_REMOTE;
> > +       case ECONNABORTED:
> > +               return BTD_ERR_LE_CONN_TERM_BY_LOCAL;
> > +       case EPROTO:
> > +               return BTD_ERR_LE_CONN_PROTO_ERROR;
> > +       default:
> > +               return BTD_ERR_LE_CONN_UNKNOWN;
> > +       }
> > +}
> > diff --git a/src/error.h b/src/error.h
> > index 7c8cad066..f258cc4e7 100644
> > --- a/src/error.h
> > +++ b/src/error.h
> > @@ -24,22 +24,193 @@
> >   */
> >
> >  #include <dbus/dbus.h>
> > +#include <stdint.h>
> >
> >  #define ERROR_INTERFACE "org.bluez.Error"
> >
> > +/* BR/EDR connection failure reasons
> > + * BT_ERR_* should be used as one of the parameters to btd_error_*_err().
> > + */
> > +
> > +/* Either the profile is already connected or ACL connection is in
> > + * place.
> > + * errno: EALREADY, EISCONN
> > + */
> > +#define BTD_ERR_BREDR_CONN_ALREADY_CONNECTED   0x0001
> > +/* Failed due to page timeout.
> > + * errno: EHOSTDOWN
> > + */
> > +#define BTD_ERR_BREDR_CONN_PAGE_TIMEOUT                0x0002
> > +/* Failed to find connectable services or the target service.
> > + * errno: ENOPROTOOPT
> > + */
> > +#define BTD_ERR_BREDR_CONN_PROFILE_UNAVAILABLE 0x0003
> > +/* Failed to complete the SDP search.
> > + * errno: none
> > + */
> > +#define BTD_ERR_BREDR_CONN_SDP_SEARCH          0x0004
> > +/* Failed to create or connect to BT IO socket. This can also indicate
> > + * hardware failure in the controller.
> > + * errno: EIO
> > + */
> > +#define BTD_ERR_BREDR_CONN_CREATE_SOCKET       0x0005
> > +/* Failed due to invalid arguments.
> > + * errno: EINVAL
> > + */
> > +#define BTD_ERR_BREDR_CONN_INVALID_ARGUMENTS   0x0006
> > +/* Failed due to adapter not powered.
> > + * errno: EHOSTUNREACH
> > + */
> > +#define BTD_ERR_BREDR_CONN_ADAPTER_NOT_POWERED 0x0007
> > +/* Failed due to unsupported state transition of L2CAP channel or other
> > + * features either by the local host or the remote.
> > + * errno: EOPNOTSUPP, EPROTONOSUPPORT
> > + */
> > +#define BTD_ERR_BREDR_CONN_NOT_SUPPORTED       0x0008
> > +/* Failed due to the socket is in bad state.
> > + * errno: EBADFD
> > + */
> > +#define BTD_ERR_BREDR_CONN_BAD_SOCKET          0x0009
> > +/* Failed to allocate memory in either host stack or controller.
> > + * errno: ENOMEM
> > + */
> > +#define BTD_ERR_BREDR_CONN_MEMORY_ALLOC                0x000A
> > +/* Failed due to other ongoing operations, such as pairing, busy L2CAP
> > + * channel or the operation disallowed by the controller.
> > + * errno: EBUSY
> > + */
> > +#define BTD_ERR_BREDR_CONN_BUSY                        0x000B
> > +/* Failed due to reaching the synchronous connection limit to a device.
> > + * errno: EMLINK
> > + */
> > +#define BTD_ERR_BREDR_CONN_SYNC_CONNECT_LIMIT  0x000C
> > +/* Failed due to connection timeout
> > + * errno: ETIMEDOUT
> > + */
> > +#define BTD_ERR_BREDR_CONN_TIMEOUT             0x000D
> > +/* Refused by the remote device due to limited resource, security reason
> > + * or unacceptable address type.
> > + * errno: ECONNREFUSED
> > + */
> > +#define BTD_ERR_BREDR_CONN_REFUSED             0x000E
> > +/* Terminated by the remote device due to limited resource or power
> > + * off.
> > + * errno: ECONNRESET
> > + */
> > +#define BTD_ERR_BREDR_CONN_TERM_BY_REMOTE      0x000F
> > +/* Terminated by the local host.
> > + * errno: ECONNABORTED
> > + */
> > +#define BTD_ERR_BREDR_CONN_TERM_BY_LOCAL       0x0010
> > +/* Failed due to LMP protocol error.
> > + * errno: EPROTO
> > + */
> > +#define BTD_ERR_BREDR_CONN_PROTO_ERROR         0x0011
> > +/* Failed due to cancellation caused by adapter drop, unexpected device drop,
> > + * or incoming disconnection request before connection request is completed.
> > + * errno: none
> > + */
> > +#define BTD_ERR_BREDR_CONN_CANCELED            0x0012
> > +/* Failed due to unknown reason.
> > + * errno: ENOSYS
> > + */
> > +#define BTD_ERR_BREDR_CONN_UNKNOWN             0x0013
> > +
> > +/* LE connection failure reasons
> > + * BT_ERR_* should be used as one of the parameters to btd_error_*_err().
> > + */
> > +
> > +/* Failed due to invalid arguments.
> > + * errno: EINVAL
> > + */
> > +#define BTD_ERR_LE_CONN_INVALID_ARGUMENTS      0x0101
> > +/* Failed due to adapter not powered.
> > + * errno: EHOSTUNREACH
> > + */
> > +#define BTD_ERR_LE_CONN_ADAPTER_NOT_POWERED    0x0102
> > +/* Failed due to unsupported state transition of L2CAP channel or other
> > + * features (e.g. LE features) either by the local host or the remote.
> > + * errno: EOPNOTSUPP, EPROTONOSUPPORT
> > + */
> > +#define BTD_ERR_LE_CONN_NOT_SUPPORTED          0x0103
> > +/* Either the BT IO is already connected or LE link connection in place.
> > + * errno: EALREADY, EISCONN
> > + */
> > +#define BTD_ERR_LE_CONN_ALREADY_CONNECTED      0x0104
> > +/* Failed due to the socket is in bad state.
> > + * errno: EBADFD
> > + */
> > +#define BTD_ERR_LE_CONN_BAD_SOCKET             0x0105
> > +/* Failed to allocate memory in either host stack or controller.
> > + * errno: ENOMEM
> > + */
> > +#define BTD_ERR_LE_CONN_MEMORY_ALLOC           0x0106
> > +/* Failed due to other ongoing operations, such as pairing, connecting, busy
> > + * L2CAP channel or the operation disallowed by the controller.
> > + * errno: EBUSY
> > + */
> > +#define BTD_ERR_LE_CONN_BUSY                   0x0107
> > +/* Failed due to that LE is not enabled or the attempt is refused by the remote
> > + * device due to limited resource, security reason or unacceptable address type.
> > + * errno: ECONNREFUSED
> > + */
> > +#define BTD_ERR_LE_CONN_REFUSED                        0x0108
> > +/* Failed to create or connect to BT IO socket. This can also indicate
> > + * hardware failure in the controller.
> > + * errno: EIO
> > + */
> > +#define BTD_ERR_LE_CONN_CREATE_SOCKET          0x0109
> > +/* Failed due to connection timeout
> > + * errno: ETIMEDOUT
> > + */
> > +#define BTD_ERR_LE_CONN_TIMEOUT                        0x010A
> > +/* Failed due to reaching the synchronous connection limit to a device.
> > + * errno: EMLINK
> > + */
> > +#define BTD_ERR_LE_CONN_SYNC_CONNECT_LIMIT     0x010B
> > +/* Terminated by the remote device due to limited resource or power
> > + * off.
> > + * errno: ECONNRESET
> > + */
> > +#define BTD_ERR_LE_CONN_TERM_BY_REMOTE         0x010C
> > +/* Terminated by the local host.
> > + * errno: ECONNABORTED
> > + */
> > +#define BTD_ERR_LE_CONN_TERM_BY_LOCAL          0x010D
> > +/* Failed due to LL protocol error.
> > + * errno: EPROTO
> > + */
> > +#define BTD_ERR_LE_CONN_PROTO_ERROR            0x010E
> > +/* Failed to complete the GATT browsing.
> > + * errno: none
> > + */
> > +#define BTD_ERR_LE_CONN_GATT_BROWSE            0x010F
> > +/* Failed due to unknown reason.
> > + * errno: ENOSYS
> > + */
> > +#define BTD_ERR_LE_CONN_UNKNOWN                        0x0110
> > +
> >  DBusMessage *btd_error_invalid_args(DBusMessage *msg);
> >  DBusMessage *btd_error_invalid_args_str(DBusMessage *msg, const char *str);
> > +DBusMessage *btd_error_invalid_args_err(DBusMessage *msg, uint16_t err);
> >  DBusMessage *btd_error_busy(DBusMessage *msg);
> >  DBusMessage *btd_error_already_exists(DBusMessage *msg);
> >  DBusMessage *btd_error_not_supported(DBusMessage *msg);
> >  DBusMessage *btd_error_not_connected(DBusMessage *msg);
> >  DBusMessage *btd_error_already_connected(DBusMessage *msg);
> >  DBusMessage *btd_error_not_available(DBusMessage *msg);
> > +DBusMessage *btd_error_not_available_err(DBusMessage *msg, uint16_t err);
> >  DBusMessage *btd_error_in_progress(DBusMessage *msg);
> > +DBusMessage *btd_error_in_progress_err(DBusMessage *msg, uint16_t err);
> >  DBusMessage *btd_error_does_not_exist(DBusMessage *msg);
> >  DBusMessage *btd_error_not_authorized(DBusMessage *msg);
> >  DBusMessage *btd_error_not_permitted(DBusMessage *msg, const char *str);
> >  DBusMessage *btd_error_no_such_adapter(DBusMessage *msg);
> >  DBusMessage *btd_error_agent_not_available(DBusMessage *msg);
> >  DBusMessage *btd_error_not_ready(DBusMessage *msg);
> > +DBusMessage *btd_error_not_ready_err(DBusMessage *msg, uint16_t err);
> >  DBusMessage *btd_error_failed(DBusMessage *msg, const char *str);
> > +DBusMessage *btd_error_failed_err(DBusMessage *msg, uint16_t err);
> > +
> > +uint16_t btd_error_bredr_conn_from_errno(int errno_code);
> > +uint16_t btd_error_le_conn_from_errno(int errno_code);
> > --
> > 2.32.0.288.g62a8d224e6-goog
> >
>
>
> --
> Luiz Augusto von Dentz
Thanks,
Miao