Hi Luiz,
On Wednesday 20 of August 2014 13:38:28 Luiz Augusto von Dentz wrote:
> Hi Szymon,
>
> On Tue, Aug 19, 2014 at 10:51 PM, Szymon Janc <szymon.janc@xxxxxxxxx> wrote:
> > Move all error handling to notification thread. This makes single
> > point of error (ie exit()) and is a preparation for custom
> > disconnect callback.
>
> Does this imply every ipc must have a notification thread? iirc this
> was not the case for audio.
The idea is to use notification thread for detecting IPC shutdown.
But you are right that on audio we don't connect second socket. Both sockets
could be always connected (even when notifications are not used) or hal-ipc
could have flag for notification support (similar to daemon ipc part) and
use correct socket for watching.
I'll see which one fits better.
>
> > ---
> >
> > android/hal-ipc.c | 134
> > +++++++++++++++++++++++++++++++++++++----------------- 1 file changed,
> > 93 insertions(+), 41 deletions(-)
> >
> > diff --git a/android/hal-ipc.c b/android/hal-ipc.c
> > index 363072c..1f5cc52 100644
> > --- a/android/hal-ipc.c
> > +++ b/android/hal-ipc.c
> > @@ -39,7 +39,7 @@ static int listen_sk = -1;
> >
> > static int cmd_sk = -1;
> > static int notif_sk = -1;
> >
> > -static pthread_mutex_t cmd_sk_mutex = PTHREAD_MUTEX_INITIALIZER;
> > +static pthread_mutex_t ipc_mutex = PTHREAD_MUTEX_INITIALIZER;
> >
> > static pthread_t notif_th = 0;
> >
> > @@ -133,6 +133,7 @@ static void *notification_handler(void *data)
> >
> > struct iovec iv;
> > struct cmsghdr *cmsg;
> > char cmsgbuf[CMSG_SPACE(sizeof(int))];
> >
> > + bool failed = false;
> >
> > char buf[IPC_MTU];
> > ssize_t ret;
> > int fd;
> >
> > @@ -157,20 +158,22 @@ static void *notification_handler(void *data)
> >
> > if (ret < 0) {
> >
> > error("Receiving notifications failed: %s",
> >
> > strerror(errno));
> >
> > - goto failed;
> > + failed = true;
> > + break;
> >
> > }
> >
> > /* socket was shutdown */
> > if (ret == 0) {
> >
> > - pthread_mutex_lock(&cmd_sk_mutex);
> > + pthread_mutex_lock(&ipc_mutex);
> >
> > if (cmd_sk == -1) {
> >
> > - pthread_mutex_unlock(&cmd_sk_mutex);
> > + pthread_mutex_unlock(&ipc_mutex);
> >
> > break;
> >
> > }
> >
> > - pthread_mutex_unlock(&cmd_sk_mutex);
> > + pthread_mutex_unlock(&ipc_mutex);
> >
> > - error("Notification socket closed");
> > - goto failed;
> > + error("Notification socket closed unexpectedly");
> > + failed = true;
> > + break;
> >
> > }
> >
> > fd = -1;
> >
> > @@ -185,21 +188,25 @@ static void *notification_handler(void *data)
> >
> > }
> >
> > }
> >
> > - if (!handle_msg(buf, ret, fd))
> > - goto failed;
> > + if (!handle_msg(buf, ret, fd)) {
> > + failed = true;
> > + break;
> > + }
> >
> > }
> >
> > + pthread_mutex_lock(&ipc_mutex);
> >
> > close(notif_sk);
> > notif_sk = -1;
> >
> > + pthread_mutex_unlock(&ipc_mutex);
> >
> > bt_thread_disassociate();
> >
> > DBG("exit");
> >
> > - return NULL;
> > + if (failed)
> > + exit(EXIT_FAILURE);
> >
> > -failed:
> > - exit(EXIT_FAILURE);
> > + return NULL;
> >
> > }
> >
> > static int accept_connection(int sk)
> >
> > @@ -238,32 +245,67 @@ bool hal_ipc_accept(void)
> >
> > {
> >
> > int err;
> >
> > + pthread_mutex_lock(&ipc_mutex);
> > + if (cmd_sk >= 0) {
> > + close(cmd_sk);
> > + cmd_sk = -1;
> > + }
> > +
> > + if (notif_sk >= 0) {
> > + close(notif_sk);
> > + notif_sk = -1;
> > + }
> > +
> > + /*
> > + * If notification thread is running this means accept was called
> > + * from notification thread context . We need to detach thread to
> > not + * leak resources.
> > + *
> > + * TODO should we verify if context is really notification thread?
> > + * calling accept from other thread when IPC is running is an
> > illegal + * usage anyway..
> > + */
> > + if (notif_th) {
> > + pthread_detach(notif_th);
> > + notif_th = 0;
> > + }
> > +
> >
> > cmd_sk = accept_connection(listen_sk);
> > if (cmd_sk < 0)
> >
> > - return false;
> > + goto failed;
> >
> > notif_sk = accept_connection(listen_sk);
> >
> > - if (notif_sk < 0) {
> > - close(cmd_sk);
> > - cmd_sk = -1;
> > - return false;
> > - }
> > + if (notif_sk < 0)
> > + goto failed;
> >
> > err = pthread_create(¬if_th, NULL, notification_handler, NULL);
> > if (err) {
> >
> > notif_th = 0;
> > error("Failed to start notification thread: %d (%s)", err,
> >
> > strerror(err));
> >
> > + goto failed;
> > + }
> > +
> > + pthread_mutex_unlock(&ipc_mutex);
> > +
> > + info("IPC connected");
> > +
> > + return true;
> > +
> > +failed:
> > + if (cmd_sk >= 0) {
> >
> > close(cmd_sk);
> > cmd_sk = -1;
> >
> > + }
> > +
> > + if (notif_sk >= 0) {
> >
> > close(notif_sk);
> > notif_sk = -1;
> >
> > - return false;
> >
> > }
> >
> > - info("IPC connected");
> > + pthread_mutex_unlock(&ipc_mutex);
> >
> > - return true;
> > + return false;
> >
> > }
> >
> > bool hal_ipc_init(const char *path, size_t size)
> >
> > @@ -307,23 +349,28 @@ bool hal_ipc_init(const char *path, size_t size)
> >
> > void hal_ipc_cleanup(void)
> > {
> >
> > + pthread_t th;
> > +
> > + pthread_mutex_lock(&ipc_mutex);
> > +
> >
> > close(listen_sk);
> > listen_sk = -1;
> >
> > - pthread_mutex_lock(&cmd_sk_mutex);
> >
> > if (cmd_sk >= 0) {
> >
> > close(cmd_sk);
> > cmd_sk = -1;
> >
> > }
> >
> > - pthread_mutex_unlock(&cmd_sk_mutex);
> > -
> > - if (notif_sk < 0)
> > - return;
> >
> > - shutdown(notif_sk, SHUT_RD);
> > + if (notif_sk >= 0)
> > + shutdown(notif_sk, SHUT_RD);
> >
> > - pthread_join(notif_th, NULL);
> > + th = notif_th;
> >
> > notif_th = 0;
> >
> > +
> > + pthread_mutex_unlock(&ipc_mutex);
> > +
> > + if (th)
> > + pthread_join(th, NULL);
> >
> > }
> >
> > int hal_ipc_cmd(uint8_t service_id, uint8_t opcode, uint16_t len, void
> > *param,>
> > @@ -337,11 +384,6 @@ int hal_ipc_cmd(uint8_t service_id, uint8_t opcode,
> > uint16_t len, void *param,>
> > struct ipc_status s;
> > size_t s_len = sizeof(s);
> >
> > - if (cmd_sk < 0) {
> > - error("Invalid cmd socket passed to hal_ipc_cmd");
> > - goto failed;
> > - }
> > -
> >
> > if (!rsp || !rsp_len) {
> >
> > memset(&s, 0, s_len);
> > rsp_len = &s_len;
> >
> > @@ -358,26 +400,29 @@ int hal_ipc_cmd(uint8_t service_id, uint8_t opcode,
> > uint16_t len, void *param,>
> > iv[0].iov_base = &cmd;
> > iv[0].iov_len = sizeof(cmd);
> >
> > - iv[1].iov_base = param;
> > + iv[1].iov_base = (void *) param;
>
> Does this change is really necessary?
>
> > iv[1].iov_len = len;
> >
> > msg.msg_iov = iv;
> > msg.msg_iovlen = 2;
> >
> > - pthread_mutex_lock(&cmd_sk_mutex);
> > + pthread_mutex_lock(&ipc_mutex);
> > +
> > + if (cmd_sk < 0) {
> > + error("Invalid cmd socket passed to hal_ipc_cmd");
> > + goto failed_locked;
> > + }
> >
> > ret = sendmsg(cmd_sk, &msg, 0);
> > if (ret < 0) {
> >
> > error("Sending command failed:%s", strerror(errno));
> >
> > - pthread_mutex_unlock(&cmd_sk_mutex);
> > - goto failed;
> > + goto failed_locked;
> >
> > }
> >
> > /* socket was shutdown */
> > if (ret == 0) {
> >
> > error("Command socket closed");
> >
> > - pthread_mutex_unlock(&cmd_sk_mutex);
> > - goto failed;
> > + goto failed_locked;
> >
> > }
> >
> > memset(&msg, 0, sizeof(msg));
> >
> > @@ -400,7 +445,7 @@ int hal_ipc_cmd(uint8_t service_id, uint8_t opcode,
> > uint16_t len, void *param,>
> > ret = recvmsg(cmd_sk, &msg, 0);
> >
> > - pthread_mutex_unlock(&cmd_sk_mutex);
> > + pthread_mutex_unlock(&ipc_mutex);
> >
> > if (ret < 0) {
> >
> > error("Receiving command response failed: %s",
> > strerror(errno));
> >
> > @@ -467,5 +512,12 @@ int hal_ipc_cmd(uint8_t service_id, uint8_t opcode,
> > uint16_t len, void *param,>
> > return BT_STATUS_SUCCESS;
> >
> > failed:
> > - exit(EXIT_FAILURE);
> > + pthread_mutex_lock(&ipc_mutex);
> > +
> > +failed_locked:
> > + if (notif_sk >= 0)
> > + shutdown(notif_sk, SHUT_RD);
> > + pthread_mutex_unlock(&ipc_mutex);
>
> Perhaps having ipc_shutdown would be a good idea here, it should
> probably only be called with the lock being held, also if notif_sk is
> not set perhaps it should shutdown the cmd_sk otherwise it can have no
> effect.
>
> > +
> > + return BT_STATUS_FAIL;
> >
> > }
> >
> > --
> > 1.9.3
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-bluetooth"
> > in the body of a message to majordomo@xxxxxxxxxxxxxxx
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
--
BR
Szymon Janc
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
