2013-12-18 15:15:16

by Andrei Emeltchenko

[permalink] [raw]
Subject: [PATCH 1/2] android/tester: Add Socket test close and listen

From: Andrei Emeltchenko <[email protected]>

This test the situation when Android close file descriptor we passed to
it and try to listen() again.
---
android/android-tester.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 55 insertions(+)

diff --git a/android/android-tester.c b/android/android-tester.c
index 8a2861e..cb8fb4e 100644
--- a/android/android-tester.c
+++ b/android/android-tester.c
@@ -1141,6 +1141,57 @@ clean:
close(sock_fd);
}

+static void test_listen_close(const void *test_data)
+{
+ struct test_data *data = tester_get_data();
+ const struct socket_data *test = data->test_data;
+ bt_status_t status;
+ int sock_fd = -1;
+
+ status = data->if_sock->listen(test->sock_type,
+ test->service_name, test->service_uuid,
+ test->channel, &sock_fd, test->flags);
+ if (status != test->expected_status) {
+ tester_test_failed();
+ goto clean;
+ }
+
+ /* Check that file descriptor is valid */
+ if (status == BT_STATUS_SUCCESS && fcntl(sock_fd, F_GETFD) == -1) {
+ tester_test_failed();
+ return;
+ }
+
+ tester_print("sock_fd: %d", sock_fd);
+
+ /* Now close sock_fd */
+ close(sock_fd);
+
+ /* Try to listen again */
+ status = data->if_sock->listen(test->sock_type,
+ test->service_name, test->service_uuid,
+ test->channel, &sock_fd, test->flags);
+ if (status != test->expected_status) {
+ tester_test_failed();
+ goto clean;
+ }
+
+ /* Check that file descriptor is valid */
+ if (status == BT_STATUS_SUCCESS && fcntl(sock_fd, F_GETFD) == -1) {
+ tester_test_failed();
+ return;
+ }
+
+ tester_print("sock_fd: %d", sock_fd);
+
+ tester_test_passed();
+
+clean:
+ if (sock_fd >= 0)
+ close(sock_fd);
+
+}
+
static void test_generic_connect(const void *test_data)
{
struct test_data *data = tester_get_data();
@@ -1363,6 +1414,10 @@ int main(int argc, char *argv[])
&btsock_success_check_chan,
setup_socket_interface, test_generic_listen, teardown);

+ test_bredrle("Socket Listen - Close and Listen again",
+ &btsock_success_check_chan,
+ setup_socket_interface, test_listen_close, teardown);
+
test_bredrle("Socket Connect - Invalid: sock_type 0",
&btsock_inv_param_socktype, setup_socket_interface,
test_generic_connect, teardown);
--
1.8.3.2



2013-12-19 09:09:06

by Johan Hedberg

[permalink] [raw]
Subject: Re: [PATCHv2 1/2] android/tester: Add Socket test close and listen

Hi Andrei,

On Thu, Dec 19, 2013, Andrei Emeltchenko wrote:
> This test the situation when Android close file descriptor we passed to
> it and try to listen() again.
> ---
> android/android-tester.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 60 insertions(+)

Both patches have been applied. Thanks.

Johan

2013-12-19 08:51:39

by Andrei Emeltchenko

[permalink] [raw]
Subject: [PATCHv2 1/2] android/tester: Add Socket test close and listen

From: Andrei Emeltchenko <[email protected]>

This test the situation when Android close file descriptor we passed to
it and try to listen() again.
---
android/android-tester.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 60 insertions(+)

diff --git a/android/android-tester.c b/android/android-tester.c
index 8a2861e..c24f5a6 100644
--- a/android/android-tester.c
+++ b/android/android-tester.c
@@ -1141,6 +1141,62 @@ clean:
close(sock_fd);
}

+static void test_listen_close(const void *test_data)
+{
+ struct test_data *data = tester_get_data();
+ const struct socket_data *test = data->test_data;
+ bt_status_t status;
+ int sock_fd = -1;
+
+ status = data->if_sock->listen(test->sock_type,
+ test->service_name, test->service_uuid,
+ test->channel, &sock_fd, test->flags);
+ if (status != test->expected_status) {
+ tester_warn("sock->listen() failed");
+ tester_test_failed();
+ goto clean;
+ }
+
+ /* Check that file descriptor is valid */
+ if (status == BT_STATUS_SUCCESS && fcntl(sock_fd, F_GETFD) < 0) {
+ tester_warn("sock_fd %d is not valid", sock_fd);
+ tester_test_failed();
+ return;
+ }
+
+ tester_print("Got valid sock_fd: %d", sock_fd);
+
+ /* Now close sock_fd */
+ close(sock_fd);
+ sock_fd = -1;
+
+ /* Try to listen again */
+ status = data->if_sock->listen(test->sock_type,
+ test->service_name, test->service_uuid,
+ test->channel, &sock_fd, test->flags);
+ if (status != test->expected_status) {
+ tester_warn("sock->listen() failed");
+ tester_test_failed();
+ goto clean;
+ }
+
+ /* Check that file descriptor is valid */
+ if (status == BT_STATUS_SUCCESS && fcntl(sock_fd, F_GETFD) < 0) {
+ tester_warn("sock_fd %d is not valid", sock_fd);
+ tester_test_failed();
+ return;
+ }
+
+ tester_print("Got valid sock_fd: %d", sock_fd);
+
+ tester_test_passed();
+
+clean:
+ if (sock_fd >= 0)
+ close(sock_fd);
+
+}
+
static void test_generic_connect(const void *test_data)
{
struct test_data *data = tester_get_data();
@@ -1363,6 +1419,10 @@ int main(int argc, char *argv[])
&btsock_success_check_chan,
setup_socket_interface, test_generic_listen, teardown);

+ test_bredrle("Socket Listen - Close and Listen again",
+ &btsock_success_check_chan,
+ setup_socket_interface, test_listen_close, teardown);
+
test_bredrle("Socket Connect - Invalid: sock_type 0",
&btsock_inv_param_socktype, setup_socket_interface,
test_generic_connect, teardown);
--
1.8.3.2


2013-12-19 08:51:40

by Andrei Emeltchenko

[permalink] [raw]
Subject: [PATCHv2 2/2] android/socket: Handle Android events for server socket

From: Andrei Emeltchenko <[email protected]>

Add watch for tracking events from Android framework for server socket.
Android might want to close server connection, in this case we close
our listening socket and cleaning up rfsock structure. Glib watch is
added with high priority to avoid races.
---
android/socket.c | 32 +++++++++++++++++++++++++++++++-
1 file changed, 31 insertions(+), 1 deletion(-)

diff --git a/android/socket.c b/android/socket.c
index 94a44fc..5a03a80 100644
--- a/android/socket.c
+++ b/android/socket.c
@@ -622,6 +622,24 @@ static bool sock_send_accept(struct rfcomm_sock *rfsock, bdaddr_t *bdaddr,
return true;
}

+static gboolean sock_server_stack_event_cb(GIOChannel *io, GIOCondition cond,
+ gpointer data)
+{
+ struct rfcomm_sock *rfsock = data;
+
+ DBG("sock %d cond %d", g_io_channel_unix_get_fd(io), cond);
+
+ if (cond & G_IO_NVAL)
+ return FALSE;
+
+ if (cond & (G_IO_ERR | G_IO_HUP )) {
+ servers = g_list_remove(servers, rfsock);
+ cleanup_rfsock(rfsock);
+ }
+
+ return FALSE;
+}
+
static void accept_cb(GIOChannel *io, GError *err, gpointer user_data)
{
struct rfcomm_sock *rfsock = user_data;
@@ -697,10 +715,12 @@ static void handle_listen(const void *buf, uint16_t len)
const struct profile_info *profile;
struct rfcomm_sock *rfsock = NULL;
BtIOSecLevel sec_level;
- GIOChannel *io;
+ GIOChannel *io, *io_stack;
+ GIOCondition cond;
GError *err = NULL;
int hal_fd = -1;
int chan;
+ guint id;

DBG("");

@@ -738,6 +758,16 @@ static void handle_listen(const void *buf, uint16_t len)
g_io_channel_set_close_on_unref(io, FALSE);
g_io_channel_unref(io);

+ /* Handle events from Android */
+ cond = G_IO_HUP | G_IO_ERR | G_IO_NVAL;
+ io_stack = g_io_channel_unix_new(rfsock->fd);
+ id = g_io_add_watch_full(io_stack, G_PRIORITY_HIGH, cond,
+ sock_server_stack_event_cb, rfsock,
+ NULL);
+ g_io_channel_unref(io_stack);
+
+ rfsock->stack_watch = id;
+
DBG("real_sock %d fd %d hal_fd %d", rfsock->real_sock, rfsock->fd,
hal_fd);

--
1.8.3.2


2013-12-19 08:18:48

by Johan Hedberg

[permalink] [raw]
Subject: Re: [PATCH 1/2] android/tester: Add Socket test close and listen

Hi Andrei,

On Wed, Dec 18, 2013, Andrei Emeltchenko wrote:
> This test the situation when Android close file descriptor we passed to
> it and try to listen() again.
> ---
> android/android-tester.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 55 insertions(+)
>
> diff --git a/android/android-tester.c b/android/android-tester.c
> index 8a2861e..cb8fb4e 100644
> --- a/android/android-tester.c
> +++ b/android/android-tester.c
> @@ -1141,6 +1141,57 @@ clean:
> close(sock_fd);
> }
>
> +static void test_listen_close(const void *test_data)
> +{
> + struct test_data *data = tester_get_data();
> + const struct socket_data *test = data->test_data;
> + bt_status_t status;
> + int sock_fd = -1;
> +
> + status = data->if_sock->listen(test->sock_type,
> + test->service_name, test->service_uuid,
> + test->channel, &sock_fd, test->flags);
> + if (status != test->expected_status) {
> + tester_test_failed();
> + goto clean;
> + }
> +
> + /* Check that file descriptor is valid */
> + if (status == BT_STATUS_SUCCESS && fcntl(sock_fd, F_GETFD) == -1) {

We usually check for failures like this with < 0 instead of == -1.

> + status = data->if_sock->listen(test->sock_type,
> + test->service_name, test->service_uuid,
> + test->channel, &sock_fd, test->flags);
> + if (status != test->expected_status) {
> + tester_test_failed();
> + goto clean;
> + }

You've got many different conditions that can trigger
tester_test_failed but no tester_warn() logs for each one to make
debugging easy if these new tests start failing. Please add those.
Otherwise once there's a regression it's going to be a real pain for you
to track down exactly what's broken.

Johan

2013-12-18 15:15:17

by Andrei Emeltchenko

[permalink] [raw]
Subject: [PATCH 2/2] android/socket: Handle Android events for server socket

From: Andrei Emeltchenko <[email protected]>

Add watch for tracking events from Android framework for server socket.
Android might want to close server connection, in this case we close
our listening socket and cleaning up rfsock structure. Glib watch is
added with high priority to avoid races.
---
android/socket.c | 32 +++++++++++++++++++++++++++++++-
1 file changed, 31 insertions(+), 1 deletion(-)

diff --git a/android/socket.c b/android/socket.c
index 7bc5b0b..de98ae4 100644
--- a/android/socket.c
+++ b/android/socket.c
@@ -622,6 +622,24 @@ static bool sock_send_accept(struct rfcomm_sock *rfsock, bdaddr_t *bdaddr,
return true;
}

+static gboolean sock_server_stack_event_cb(GIOChannel *io, GIOCondition cond,
+ gpointer data)
+{
+ struct rfcomm_sock *rfsock = data;
+
+ DBG("sock %d cond %d", g_io_channel_unix_get_fd(io), cond);
+
+ if (cond & G_IO_NVAL)
+ return FALSE;
+
+ if (cond & (G_IO_ERR | G_IO_HUP )) {
+ servers = g_list_remove(servers, rfsock);
+ cleanup_rfsock(rfsock);
+ }
+
+ return FALSE;
+}
+
static void accept_cb(GIOChannel *io, GError *err, gpointer user_data)
{
struct rfcomm_sock *rfsock = user_data;
@@ -697,10 +715,12 @@ static void handle_listen(const void *buf, uint16_t len)
const struct profile_info *profile;
struct rfcomm_sock *rfsock = NULL;
BtIOSecLevel sec_level;
- GIOChannel *io;
+ GIOChannel *io, *io_stack;
+ GIOCondition cond;
GError *err = NULL;
int hal_fd = -1;
int chan;
+ guint id;

DBG("");

@@ -738,6 +758,16 @@ static void handle_listen(const void *buf, uint16_t len)
g_io_channel_set_close_on_unref(io, FALSE);
g_io_channel_unref(io);

+ /* Handle events from Android */
+ cond = G_IO_HUP | G_IO_ERR | G_IO_NVAL;
+ io_stack = g_io_channel_unix_new(rfsock->fd);
+ id = g_io_add_watch_full(io_stack, G_PRIORITY_HIGH, cond,
+ sock_server_stack_event_cb, rfsock,
+ NULL);
+ g_io_channel_unref(io_stack);
+
+ rfsock->stack_watch = id;
+
DBG("real_sock %d fd %d hal_fd %d", rfsock->real_sock, rfsock->fd,
hal_fd);

--
1.8.3.2