| Message ID | 20200419211058.31987-1-pali@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | profile: Fix reporting error message when connection failed | expand |
Hi Pali, On Sun, Apr 19, 2020 at 2:14 PM Pali Rohár <pali@kernel.org> wrote: > > Some bluetooth headsets do not support connecting more then one bluetooth > profile (e.g. in parallel A2DP and HSP, or HSP and HFP) and issuing > connect() syscall for second profile returns just ECONNREFUSED. > > Prior this patch bluetooth daemon for such situation reported following > message to log: > > Unable to get connect data for Headset Voice gateway: getpeername: Transport endpoint is not connected (107) > > Message is incorrect as connect() syscall failed, not getpeername(). This > patch fixes this problem and logs correct error message: > > Headset Voice gateway failed connect to XX:XX:XX:XX:XX:XX: Connection refused (111) > > Main problem was in ext_connect() function which called bt_io_get() for > retrieving remote address (BT_IO_OPT_DEST) and if it failed then original > error from connect() syscall was masked. Because it is not possible to > retrieve BT_IO_OPT_DEST for unconnected socket, original destination > address for error message is propagated via connect_add() function in btio. > > -- > > Having correct error message in logs is important. Due to this mangled > error message I was not able to easily debug why particular bluetooth > headset sometimes connection with nonsense error that Transport endpoint > was not connected. Looks good, but lets have the btio changes as a separate patch. > --- > btio/btio.c | 19 ++++++++++++++----- > src/profile.c | 5 +++-- > 2 files changed, 17 insertions(+), 7 deletions(-) > > diff --git a/btio/btio.c b/btio/btio.c > index e7b4db16b..3ea73faea 100644 > --- a/btio/btio.c > +++ b/btio/btio.c > @@ -85,6 +85,7 @@ struct connect { > BtIOConnect connect; > gpointer user_data; > GDestroyNotify destroy; > + bdaddr_t dst; > }; > > struct accept { > @@ -214,6 +215,7 @@ static gboolean connect_cb(GIOChannel *io, GIOCondition cond, > GError *gerr = NULL; > int err, sk_err, sock; > socklen_t len = sizeof(sk_err); > + char addr[18]; > > /* If the user aborted this connect attempt */ > if ((cond & G_IO_NVAL) || check_nval(io)) > @@ -226,8 +228,11 @@ static gboolean connect_cb(GIOChannel *io, GIOCondition cond, > else > err = -sk_err; > > - if (err < 0) > - ERROR_FAILED(&gerr, "connect error", -err); > + if (err < 0) { > + ba2str(&conn->dst, addr); > + g_set_error(&gerr, BT_IO_ERROR, err, > + "connect to %s: %s (%d)", addr, strerror(-err), -err); > + } > > conn->connect(io, gerr, conn->user_data); > > @@ -286,7 +291,7 @@ static void server_add(GIOChannel *io, BtIOConnect connect, > (GDestroyNotify) server_remove); > } > > -static void connect_add(GIOChannel *io, BtIOConnect connect, > +static void connect_add(GIOChannel *io, BtIOConnect connect, bdaddr_t dst, > gpointer user_data, GDestroyNotify destroy) > { > struct connect *conn; > @@ -296,6 +301,7 @@ static void connect_add(GIOChannel *io, BtIOConnect connect, > conn->connect = connect; > conn->user_data = user_data; > conn->destroy = destroy; > + conn->dst = dst; > > cond = G_IO_OUT | G_IO_ERR | G_IO_HUP | G_IO_NVAL; > g_io_add_watch_full(io, G_PRIORITY_DEFAULT, cond, connect_cb, conn, > @@ -1671,6 +1677,7 @@ GIOChannel *bt_io_connect(BtIOConnect connect, gpointer user_data, > struct set_opts opts; > int err, sock; > gboolean ret; > + char addr[18]; > > va_start(args, opt1); > ret = parse_set_opts(&opts, gerr, opt1, args); > @@ -1710,12 +1717,14 @@ GIOChannel *bt_io_connect(BtIOConnect connect, gpointer user_data, > } > > if (err < 0) { > - ERROR_FAILED(gerr, "connect", -err); > + ba2str(&opts.dst, addr); > + g_set_error(gerr, BT_IO_ERROR, err, > + "connect to %s: %s (%d)", addr, strerror(-err), -err); > g_io_channel_unref(io); > return NULL; > } > > - connect_add(io, connect, user_data, destroy); > + connect_add(io, connect, opts.dst, user_data, destroy); > > return io; > } > diff --git a/src/profile.c b/src/profile.c > index c2992e795..6961a107b 100644 > --- a/src/profile.c > +++ b/src/profile.c > @@ -1085,12 +1085,13 @@ static void ext_connect(GIOChannel *io, GError *err, gpointer user_data) > if (!bt_io_get(io, &io_err, > BT_IO_OPT_DEST, addr, > BT_IO_OPT_INVALID)) { > - error("Unable to get connect data for %s: %s", ext->name, > - io_err->message); > if (err) { > + error("%s failed %s", ext->name, err->message); > g_error_free(io_err); > io_err = NULL; > } else { > + error("Unable to get connect data for %s: %s", > + ext->name, io_err->message); > err = io_err; > } > goto drop; > -- > 2.20.1 >
On Monday 20 April 2020 15:38:43 Luiz Augusto von Dentz wrote:
> Looks good, but lets have the btio changes as a separate patch.
Hello! Do you need from me to resent this change again split to two
separate patches? Or will you apply it directly?
Hi Pali, On Mon, Apr 20, 2020 at 3:43 PM Pali Rohár <pali@kernel.org> wrote: > > On Monday 20 April 2020 15:38:43 Luiz Augusto von Dentz wrote: > > Looks good, but lets have the btio changes as a separate patch. > > Hello! Do you need from me to resent this change again split to two > separate patches? Or will you apply it directly? Please do the split, it is always a good practice to have the author do these changes since you are more familiar with the changes, it also makes it simple in terms of authorship since then I don't have to fix the author manually and risk having a typo there.
diff --git a/btio/btio.c b/btio/btio.c index e7b4db16b..3ea73faea 100644 --- a/btio/btio.c +++ b/btio/btio.c @@ -85,6 +85,7 @@ struct connect { BtIOConnect connect; gpointer user_data; GDestroyNotify destroy; + bdaddr_t dst; }; struct accept { @@ -214,6 +215,7 @@ static gboolean connect_cb(GIOChannel *io, GIOCondition cond, GError *gerr = NULL; int err, sk_err, sock; socklen_t len = sizeof(sk_err); + char addr[18]; /* If the user aborted this connect attempt */ if ((cond & G_IO_NVAL) || check_nval(io)) @@ -226,8 +228,11 @@ static gboolean connect_cb(GIOChannel *io, GIOCondition cond, else err = -sk_err; - if (err < 0) - ERROR_FAILED(&gerr, "connect error", -err); + if (err < 0) { + ba2str(&conn->dst, addr); + g_set_error(&gerr, BT_IO_ERROR, err, + "connect to %s: %s (%d)", addr, strerror(-err), -err); + } conn->connect(io, gerr, conn->user_data); @@ -286,7 +291,7 @@ static void server_add(GIOChannel *io, BtIOConnect connect, (GDestroyNotify) server_remove); } -static void connect_add(GIOChannel *io, BtIOConnect connect, +static void connect_add(GIOChannel *io, BtIOConnect connect, bdaddr_t dst, gpointer user_data, GDestroyNotify destroy) { struct connect *conn; @@ -296,6 +301,7 @@ static void connect_add(GIOChannel *io, BtIOConnect connect, conn->connect = connect; conn->user_data = user_data; conn->destroy = destroy; + conn->dst = dst; cond = G_IO_OUT | G_IO_ERR | G_IO_HUP | G_IO_NVAL; g_io_add_watch_full(io, G_PRIORITY_DEFAULT, cond, connect_cb, conn, @@ -1671,6 +1677,7 @@ GIOChannel *bt_io_connect(BtIOConnect connect, gpointer user_data, struct set_opts opts; int err, sock; gboolean ret; + char addr[18]; va_start(args, opt1); ret = parse_set_opts(&opts, gerr, opt1, args); @@ -1710,12 +1717,14 @@ GIOChannel *bt_io_connect(BtIOConnect connect, gpointer user_data, } if (err < 0) { - ERROR_FAILED(gerr, "connect", -err); + ba2str(&opts.dst, addr); + g_set_error(gerr, BT_IO_ERROR, err, + "connect to %s: %s (%d)", addr, strerror(-err), -err); g_io_channel_unref(io); return NULL; } - connect_add(io, connect, user_data, destroy); + connect_add(io, connect, opts.dst, user_data, destroy); return io; } diff --git a/src/profile.c b/src/profile.c index c2992e795..6961a107b 100644 --- a/src/profile.c +++ b/src/profile.c @@ -1085,12 +1085,13 @@ static void ext_connect(GIOChannel *io, GError *err, gpointer user_data) if (!bt_io_get(io, &io_err, BT_IO_OPT_DEST, addr, BT_IO_OPT_INVALID)) { - error("Unable to get connect data for %s: %s", ext->name, - io_err->message); if (err) { + error("%s failed %s", ext->name, err->message); g_error_free(io_err); io_err = NULL; } else { + error("Unable to get connect data for %s: %s", + ext->name, io_err->message); err = io_err; } goto drop;