[PATCH] android/hal-health: Clear NONBLOCK flag from fd

[PATCH] android/hal-health: Clear NONBLOCK flag from fd

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Java expects file descriptor passed with channel_state_cb() to be
blocking.
---
 android/hal-health.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/android/hal-health.c b/android/hal-health.c
index 858d499..2ff19ab 100644
--- a/android/hal-health.c
+++ b/android/hal-health.c
@@ -19,6 +19,8 @@
 #include <stddef.h>
 #include <string.h>
 #include <stdlib.h>
+#include <fcntl.h>
+#include <unistd.h>
 
 #include "hal-log.h"
 #include "hal.h"
@@ -44,6 +46,13 @@ static void handle_app_registration_state(void *buf, uint16_t len, int fd)
 static void handle_channel_state(void *buf, uint16_t len, int fd)
 {
 	struct hal_ev_health_channel_state *ev = buf;
+	int flags;
+
+	if ((flags = fcntl(fd, F_GETFL, 0)) < 0)
+		flags = 0;
+
+	fcntl(fd, F_SETFL, flags & ~O_NONBLOCK);
+
 
 	if (cbacks->channel_state_cb)
 		cbacks->channel_state_cb(ev->app_id, (bt_bdaddr_t *) ev->bdaddr,
-- 
1.9.1

Re: [PATCH] android/hal-health: Clear NONBLOCK flag from fd

[Marcel Holtmann]

Hi Andrei,

> Java expects file descriptor passed with channel_state_cb() to be
> blocking.
> ---
> android/hal-health.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
> 
> diff --git a/android/hal-health.c b/android/hal-health.c
> index 858d499..2ff19ab 100644
> --- a/android/hal-health.c
> +++ b/android/hal-health.c
> @@ -19,6 +19,8 @@
> #include <stddef.h>
> #include <string.h>
> #include <stdlib.h>
> +#include <fcntl.h>
> +#include <unistd.h>
> 
> #include "hal-log.h"
> #include "hal.h"
> @@ -44,6 +46,13 @@ static void handle_app_registration_state(void *buf, uint16_t len, int fd)
> static void handle_channel_state(void *buf, uint16_t len, int fd)
> {
> 	struct hal_ev_health_channel_state *ev = buf;
> +	int flags;
> +
> +	if ((flags = fcntl(fd, F_GETFL, 0)) < 0)
> +		flags = 0;

I do not like the if ((x = x) < x) syntax. Don't use that.

Also what is the point of setting it to 0 here. Just skip the F_SETFL. I rather print a proper error here then trying to outsmart ourselves.

> +
> +	fcntl(fd, F_SETFL, flags & ~O_NONBLOCK);
> +

Same here. Might check the return error and print an error if it fails. At least then someone can debug it.

> 
> 	if (cbacks->channel_state_cb)
> 		cbacks->channel_state_cb(ev->app_id, (bt_bdaddr_t *) ev->bdaddr,

Regards

Marcel

Re: [PATCH] android/hal-health: Clear NONBLOCK flag from fd

[Andrei Emeltchenko]

Hi Marcel,

On Thu, Jul 03, 2014 at 12:38:29PM +0200, Marcel Holtmann wrote:
> Hi Andrei,
> 
> > Java expects file descriptor passed with channel_state_cb() to be
> > blocking.
> > ---
> > android/hal-health.c | 9 +++++++++
> > 1 file changed, 9 insertions(+)
> > 
> > diff --git a/android/hal-health.c b/android/hal-health.c
> > index 858d499..2ff19ab 100644
> > --- a/android/hal-health.c
> > +++ b/android/hal-health.c
> > @@ -19,6 +19,8 @@
> > #include <stddef.h>
> > #include <string.h>
> > #include <stdlib.h>
> > +#include <fcntl.h>
> > +#include <unistd.h>
> > 
> > #include "hal-log.h"
> > #include "hal.h"
> > @@ -44,6 +46,13 @@ static void handle_app_registration_state(void *buf, uint16_t len, int fd)
> > static void handle_channel_state(void *buf, uint16_t len, int fd)
> > {
> > 	struct hal_ev_health_channel_state *ev = buf;
> > +	int flags;
> > +
> > +	if ((flags = fcntl(fd, F_GETFL, 0)) < 0)
> > +		flags = 0;
> 
> I do not like the if ((x = x) < x) syntax. Don't use that.
> 
> Also what is the point of setting it to 0 here. Just skip the F_SETFL. I rather print a proper error here then trying to outsmart ourselves.
> 
> > +
> > +	fcntl(fd, F_SETFL, flags & ~O_NONBLOCK);
> > +
> 
> Same here. Might check the return error and print an error if it fails. At least then someone can debug it.
> 

OK, I will send updated version ASAP.

Best regards 
Andrei Emeltchenko 

> > 
> > 	if (cbacks->channel_state_cb)
> > 		cbacks->channel_state_cb(ev->app_id, (bt_bdaddr_t *) ev->bdaddr,
> 
> Regards
> 
> Marcel
> 

Re: [PATCH] android/hal-health: Clear NONBLOCK flag from fd

[Andrei Emeltchenko]

Hi Marcel,

On Thu, Jul 03, 2014 at 12:38:29PM +0200, Marcel Holtmann wrote:
> > +
> > +	fcntl(fd, F_SETFL, flags & ~O_NONBLOCK);
> > +
> 
> Same here. Might check the return error and print an error if it fails.
> At least then someone can debug it.

I have some other patch hanging in my tree checking return from ioctl()
for avctp. I will send it in a moment.

Best regards 
Andrei Emeltchenko 

[PATCH] avctp: Fix unchecked return value

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Refactor code so that ioctl() return value is checked.
---
 profiles/audio/avctp.c | 47 +++++++++++++++++++++++++++++++++++++----------
 1 file changed, 37 insertions(+), 10 deletions(-)

diff --git a/profiles/audio/avctp.c b/profiles/audio/avctp.c
index 74d3512..695f0f1 100644
--- a/profiles/audio/avctp.c
+++ b/profiles/audio/avctp.c
@@ -1054,27 +1054,54 @@ static int uinput_create(char *name)
 		err = -errno;
 		error("Can't write device information: %s (%d)",
 						strerror(-err), -err);
-		close(fd);
-		return err;
+		goto fail;
+	}
+
+	if (ioctl(fd, UI_SET_EVBIT, EV_KEY) < 0) {
+		err = -errno;
+		error("ioctl UI_SET_EVBIT: %s (%d)", strerror(-err), -err);
+		goto fail;
 	}
 
-	ioctl(fd, UI_SET_EVBIT, EV_KEY);
-	ioctl(fd, UI_SET_EVBIT, EV_REL);
-	ioctl(fd, UI_SET_EVBIT, EV_REP);
-	ioctl(fd, UI_SET_EVBIT, EV_SYN);
+	if (ioctl(fd, UI_SET_EVBIT, EV_REL) < 0) {
+		err = -errno;
+		error("ioctl UI_SET_EVBIT: %s (%d)", strerror(-err), -err);
+		goto fail;
+	}
 
-	for (i = 0; key_map[i].name != NULL; i++)
-		ioctl(fd, UI_SET_KEYBIT, key_map[i].uinput);
+	if (ioctl(fd, UI_SET_EVBIT, EV_REP) < 0) {
+		err = -errno;
+		error("ioctl UI_SET_EVBIT: %s (%d)", strerror(-err), -err);
+		goto fail;
+	}
+
+	if (ioctl(fd, UI_SET_EVBIT, EV_SYN) < 0) {
+		err = -errno;
+		error("ioctl UI_SET_EVBIT: %s (%d)", strerror(-err), -err);
+		goto fail;
+	}
+
+	for (i = 0; key_map[i].name != NULL; i++) {
+		if (ioctl(fd, UI_SET_KEYBIT, key_map[i].uinput) < 0) {
+			err = -errno;
+			error("ioctl UI_SET_KEYBIT: %s (%d)", strerror(-err),
+									-err);
+			goto fail;
+		}
+	}
 
 	if (ioctl(fd, UI_DEV_CREATE, NULL) < 0) {
 		err = -errno;
 		error("Can't create uinput device: %s (%d)",
 						strerror(-err), -err);
-		close(fd);
-		return err;
+		goto fail;
 	}
 
 	return fd;
+
+fail:
+	close(fd);
+	return err;
 }
 
 static void init_uinput(struct avctp *session)
-- 
1.9.1