2014-01-24 14:56:41

by Andrei Emeltchenko

Subject: [PATCH 1/3] bnep: Calculate ifindex after NULL check

From: Andrei Emeltchenko <[email protected]>

---
profiles/network/bnep.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/profiles/network/bnep.c b/profiles/network/bnep.c
index 2a74016..0ef85d5 100644
--- a/profiles/network/bnep.c
+++ b/profiles/network/bnep.c
@@ -518,13 +518,15 @@ static int bnep_add_to_bridge(const char *devname, const char *bridge)

static int bnep_del_from_bridge(const char *devname, const char *bridge)
{
- int ifindex = if_nametoindex(devname);
+ int ifindex;
struct ifreq ifr;
int sk, err;

if (!devname || !bridge)
return -EINVAL;

+ ifindex = if_nametoindex(devname);
+
sk = socket(AF_INET, SOCK_STREAM, 0);
if (sk < 0)
return -1;
--
1.8.3.2

2014-01-27 18:41:41

by Luiz Augusto von Dentz

Subject: Re: [PATCHv2] avctp: Fix unchecked return value

Hi Andrei,

On Mon, Jan 27, 2014 at 7:59 AM, Andrei Emeltchenko
<[email protected]> wrote:
> From: Andrei Emeltchenko <[email protected]>
>
> 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 6fd1454..bf44f9c 100644
> --- a/profiles/audio/avctp.c
> +++ b/profiles/audio/avctp.c
> @@ -1027,27 +1027,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.8.3.2

I start to wonder if there is a better way to set those bits...


--
Luiz Augusto von Dentz

2014-01-27 15:59:35

by Andrei Emeltchenko

Subject: [PATCHv2] avctp: Fix unchecked return value

From: Andrei Emeltchenko <[email protected]>

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 6fd1454..bf44f9c 100644
--- a/profiles/audio/avctp.c
+++ b/profiles/audio/avctp.c
@@ -1027,27 +1027,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.8.3.2

2014-01-26 04:29:00

by Luiz Augusto von Dentz

Subject: Re: [PATCH 2/3] android/avdtp: Retry send on EAGAIN as well

Hi Szymon,

On Sat, Jan 25, 2014 at 12:16 AM, Szymon Janc <[email protected]> wrote:
> Hi Andrei,
>
> On Friday 24 January 2014 16:56:42 Andrei Emeltchenko wrote:
>> From: Andrei Emeltchenko <[email protected]>
>>
>> ---
>> android/avdtp.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/android/avdtp.c b/android/avdtp.c
>> index 5da1206..1055bc1 100644
>> --- a/android/avdtp.c
>> +++ b/android/avdtp.c
>> @@ -444,7 +444,7 @@ static gboolean try_send(int sk, void *data, size_t len)
>>
>> do {
>> err = send(sk, data, len, 0);
>> - } while (err < 0 && errno == EINTR);
>> + } while (err < 0 && (errno == EINTR || errno == EAGAIN));
>>
>
> This makes try_send() blocking, if this is ok then at least some explanation
> in commit message would be welcome.

And if we want to block we should not set the socket as non-blocking
in first place, Im not even sure this is easy to currently reproduce
any case that would block since the command are quite small we would
have to schedule several in a sequence but normally we can only have
one command outstanding.


--
Luiz Augusto von Dentz

2014-01-24 22:16:39

by Szymon Janc

Subject: Re: [PATCH 2/3] android/avdtp: Retry send on EAGAIN as well

Hi Andrei,

On Friday 24 January 2014 16:56:42 Andrei Emeltchenko wrote:
> From: Andrei Emeltchenko <[email protected]>
>
> ---
> android/avdtp.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/android/avdtp.c b/android/avdtp.c
> index 5da1206..1055bc1 100644
> --- a/android/avdtp.c
> +++ b/android/avdtp.c
> @@ -444,7 +444,7 @@ static gboolean try_send(int sk, void *data, size_t len)
>
> do {
> err = send(sk, data, len, 0);
> - } while (err < 0 && errno == EINTR);
> + } while (err < 0 && (errno == EINTR || errno == EAGAIN));
>

This makes try_send() blocking, if this is ok then at least some explanation
in commit message would be welcome.

> if (err < 0) {
> error("send: %s (%d)", strerror(errno), errno);

--
Szymon K. Janc
[email protected]

2014-01-24 15:00:58

by Anderson Lizardo

Subject: Re: [PATCH 3/3] avctp: Fix unchecked return value

Hi Andrei,

On Fri, Jan 24, 2014 at 10:56 AM, Andrei Emeltchenko
<[email protected]> wrote:

> - for (i = 0; key_map[i].name != NULL; i++)
> + for (i = 0; key_map[i].name != NULL; i++) {
> ioctl(fd, UI_SET_KEYBIT, key_map[i].uinput);
> + err = -errno;
> + error("ioctl UI_SET_KEYBIT: %s (%d)", strerror(-err), -err);
> + goto fail;
> + }

You seem to have missed the "if (ioctl(...) < 0)" above.

Best Regards,
--
Anderson Lizardo
http://www.indt.org/?lang=en
INdT - Manaus - Brazil

2014-01-24 14:56:43

by Andrei Emeltchenko

Subject: [PATCH 3/3] avctp: Fix unchecked return value

From: Andrei Emeltchenko <[email protected]>

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

diff --git a/profiles/audio/avctp.c b/profiles/audio/avctp.c
index 6669ddc..4d0f906 100644
--- a/profiles/audio/avctp.c
+++ b/profiles/audio/avctp.c
@@ -1026,27 +1026,52 @@ 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;
+ }
+
+ if (ioctl(fd, UI_SET_EVBIT, EV_REL) < 0) {
+ err = -errno;
+ error("ioctl UI_SET_EVBIT: %s (%d)", strerror(-err), -err);
+ goto fail;
+ }
+
+ if (ioctl(fd, UI_SET_EVBIT, EV_REP) < 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_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++)
+ for (i = 0; key_map[i].name != NULL; i++) {
ioctl(fd, UI_SET_KEYBIT, key_map[i].uinput);
+ 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.8.3.2

2014-01-24 14:56:42

by Andrei Emeltchenko

Subject: [PATCH 2/3] android/avdtp: Retry send on EAGAIN as well

From: Andrei Emeltchenko <[email protected]>

---
android/avdtp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/android/avdtp.c b/android/avdtp.c
index 5da1206..1055bc1 100644
--- a/android/avdtp.c
+++ b/android/avdtp.c
@@ -444,7 +444,7 @@ static gboolean try_send(int sk, void *data, size_t len)

do {
err = send(sk, data, len, 0);
- } while (err < 0 && errno == EINTR);
+ } while (err < 0 && (errno == EINTR || errno == EAGAIN));

if (err < 0) {
error("send: %s (%d)", strerror(errno), errno);
--
1.8.3.2