[PATCH 1/3] sco-tester: Update ECONNABORTED to EOPNOTSUPP

[PATCH 1/3] sco-tester: Update ECONNABORTED to EOPNOTSUPP

[Frédéric Dalleau]

Kernel interface has evolved in between.
---
 tools/sco-tester.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/sco-tester.c b/tools/sco-tester.c
index 70b8a5d..1e8351f 100644
--- a/tools/sco-tester.c
+++ b/tools/sco-tester.c
@@ -237,7 +237,7 @@ static const struct sco_client_data connect_success = {
 };
 
 static const struct sco_client_data connect_failure = {
-	.expect_err = ECONNABORTED
+	.expect_err = EOPNOTSUPP
 };
 
 static void client_connectable_complete(uint16_t opcode, uint8_t status,
-- 
1.7.9.5

[PATCH 2/3] sco-tester: Introduce adapter features

[Frédéric Dalleau]

---
 tools/sco-tester.c |   43 ++++++++++++++++++++++++-------------------
 1 file changed, 24 insertions(+), 19 deletions(-)

diff --git a/tools/sco-tester.c b/tools/sco-tester.c
index 1e8351f..e826d9c 100644
--- a/tools/sco-tester.c
+++ b/tools/sco-tester.c
@@ -43,6 +43,10 @@
 #include "src/shared/mgmt.h"
 #include "src/shared/hciemu.h"
 
+struct adapter_features {
+	bool disable_esco;
+};
+
 struct test_data {
 	const void *test_data;
 	struct mgmt *mgmt;
@@ -50,7 +54,7 @@ struct test_data {
 	struct hciemu *hciemu;
 	enum hciemu_type hciemu_type;
 	unsigned int io_id;
-	bool disable_esco;
+	struct adapter_features features;
 };
 
 struct sco_client_data {
@@ -164,7 +168,7 @@ static void read_index_list_callback(uint8_t status, uint16_t length,
 
 	tester_print("New hciemu instance created");
 
-	if (data->disable_esco) {
+	if (data->features.disable_esco) {
 		uint8_t *features;
 
 		tester_print("Disabling eSCO packet type support");
@@ -211,7 +215,7 @@ static void test_data_free(void *test_data)
 	free(data);
 }
 
-#define test_sco_full(name, data, setup, func, _disable_esco) \
+#define test_sco(name, data, setup, func, feat) \
 	do { \
 		struct test_data *user; \
 		user = malloc(sizeof(struct test_data)); \
@@ -220,17 +224,12 @@ static void test_data_free(void *test_data)
 		user->hciemu_type = HCIEMU_TYPE_BREDRLE; \
 		user->io_id = 0; \
 		user->test_data = data; \
-		user->disable_esco = _disable_esco; \
+		user->features = feat; \
 		tester_add_full(name, data, \
 				test_pre_setup, setup, func, NULL, \
 				test_post_teardown, 2, user, test_data_free); \
 	} while (0)
 
-#define test_sco(name, data, setup, func) \
-	test_sco_full(name, data, setup, func, false)
-
-#define test_sco_11(name, data, setup, func) \
-	test_sco_full(name, data, setup, func, true)
 
 static const struct sco_client_data connect_success = {
 	.expect_err = 0
@@ -575,31 +574,37 @@ end:
 
 int main(int argc, char *argv[])
 {
+	static struct adapter_features features;
+
 	tester_init(&argc, &argv);
 
+	features.disable_esco = 0;
+
 	test_sco("Basic Framework - Success", NULL, setup_powered,
-							test_framework);
+						test_framework, features);
 
 	test_sco("Basic SCO Socket - Success", NULL, setup_powered,
-							test_socket);
+						test_socket, features);
 
 	test_sco("Basic SCO Get Socket Option - Success", NULL, setup_powered,
-							test_getsockopt);
+						test_getsockopt, features);
 
 	test_sco("Basic SCO Set Socket Option - Success", NULL, setup_powered,
-							test_setsockopt);
+						test_setsockopt, features);
 
 	test_sco("eSCO CVSD - Success", &connect_success, setup_powered,
-							test_connect);
+						test_connect, features);
 
 	test_sco("eSCO MSBC - Success", &connect_success, setup_powered,
-							test_connect_transp);
+						test_connect_transp, features);
+
+	features.disable_esco = 1;
 
-	test_sco_11("SCO CVSD 1.1 - Success", &connect_success, setup_powered,
-							test_connect);
+	test_sco("SCO CVSD 1.1 - Success", &connect_success, setup_powered,
+						test_connect, features);
 
-	test_sco_11("SCO MSBC 1.1 - Failure", &connect_failure, setup_powered,
-							test_connect_transp);
+	test_sco("SCO MSBC 1.1 - Failure", &connect_failure, setup_powered,
+						test_connect_transp, features);
 
 	return tester_run();
 }
-- 
1.7.9.5

[PATCH 3/3] sco-tester: Test transparent data feature bit

[Frédéric Dalleau]

---
 tools/sco-tester.c |   20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/tools/sco-tester.c b/tools/sco-tester.c
index e826d9c..e45956a 100644
--- a/tools/sco-tester.c
+++ b/tools/sco-tester.c
@@ -45,6 +45,7 @@
 
 struct adapter_features {
 	bool disable_esco;
+	bool enable_transp;
 };
 
 struct test_data {
@@ -145,6 +146,7 @@ static void read_index_list_callback(uint8_t status, uint16_t length,
 					const void *param, void *user_data)
 {
 	struct test_data *data = tester_get_data();
+	uint8_t *features;
 
 	tester_print("Read Index List callback");
 	tester_print("  Status: 0x%02x", status);
@@ -168,14 +170,16 @@ static void read_index_list_callback(uint8_t status, uint16_t length,
 
 	tester_print("New hciemu instance created");
 
-	if (data->features.disable_esco) {
-		uint8_t *features;
+	features = hciemu_get_features(data->hciemu);
 
+	if (data->features.disable_esco) {
 		tester_print("Disabling eSCO packet type support");
+		features[3] &= ~0x80;
+	}
 
-		features = hciemu_get_features(data->hciemu);
-		if (features)
-			features[3] &= ~0x80;
+	if (data->features.enable_transp) {
+		tester_print("Enabling transparent data support");
+		features[2] |= 0x08;
 	}
 }
 
@@ -579,6 +583,7 @@ int main(int argc, char *argv[])
 	tester_init(&argc, &argv);
 
 	features.disable_esco = 0;
+	features.enable_transp = 1;
 
 	test_sco("Basic Framework - Success", NULL, setup_powered,
 						test_framework, features);
@@ -598,6 +603,11 @@ int main(int argc, char *argv[])
 	test_sco("eSCO MSBC - Success", &connect_success, setup_powered,
 						test_connect_transp, features);
 
+	features.enable_transp = 0;
+
+	test_sco("eSCO MSBC - Failure", &connect_failure, setup_powered,
+						test_connect_transp, features);
+
 	features.disable_esco = 1;
 
 	test_sco("SCO CVSD 1.1 - Success", &connect_success, setup_powered,
-- 
1.7.9.5

Re: [PATCH 1/3] sco-tester: Update ECONNABORTED to EOPNOTSUPP

[Marcel Holtmann]

Hi Fred,

> Kernel interface has evolved in between.
> ---
> tools/sco-tester.c |    2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/sco-tester.c b/tools/sco-tester.c
> index 70b8a5d..1e8351f 100644
> --- a/tools/sco-tester.c
> +++ b/tools/sco-tester.c
> @@ -237,7 +237,7 @@ static const struct sco_client_data connect_success = {
> };
> 
> static const struct sco_client_data connect_failure = {
> -	.expect_err = ECONNABORTED
> +	.expect_err = EOPNOTSUPP
> };

can we please have a proper thread on what error is appropriate to send from a socket interface when this situation happens. I see that we are flipping errors, but my request to discuss this gets ignored.

Please look for a good reference of the error code usage in other socket protocols or manpages

Regards

Marcel

Re: [PATCH 1/3] sco-tester: Update ECONNABORTED to EOPNOTSUPP

[Frédéric DALLEAU]

Hi Marcel,

> can we please have a proper thread on what error is appropriate to send  > from a socket interface when this situation happens. I see that
 > we are flipping errors, but my request to discuss this gets ignored.
>

I already gave my opinion previously. Let's recall it :"About the error 
ECONNABORTED, it is what the patch does : abort a connection. It is not 
used for other purposes. If you really want to change, I'm ok with 
EOPNOTSUPP, otherwise please suggest."

Regarding EOPNOTSUPP. Most use case are when a pointer in an interface 
is not implemented. We could consider that SetupSyncConn is not 
implemented in 2.0 adapter interfaces.


 > Please look for a good reference of the error code usage in other 
socket protocols or manpages

It is not obvious to find one good reference. But for example there is : 
Documentation/ioctl/hdio.txt:     EOPNOTSUPP    Hardware interface does 
not support bus power

Another option could be:
[ENODEV]
     No such device An attempt was made to apply an inappropriate 
function to a device; for example, trying to read a write-only device 
such as a printer.

BR,
Fred

Re: [PATCH 1/3] sco-tester: Update ECONNABORTED to EOPNOTSUPP

[Marcel Holtmann]

Hi Fred,

>> can we please have a proper thread on what error is appropriate to send  > from a socket interface when this situation happens. I see that
> > we are flipping errors, but my request to discuss this gets ignored.
>> 
> 
> I already gave my opinion previously. Let's recall it :"About the error ECONNABORTED, it is what the patch does : abort a connection. It is not used for other purposes. If you really want to change, I'm ok with EOPNOTSUPP, otherwise please suggest."

the last patchset that I reviewed had a different error code in the commit message than it had in the implementation. For me it is then impossible to understand what you intent was. Also an explanation why which error code is picked is important.

I really want commit messages be explanatory on why things a done this way. Use it as introduction to a patch and its justification.

> Regarding EOPNOTSUPP. Most use case are when a pointer in an interface is not implemented. We could consider that SetupSyncConn is not implemented in 2.0 adapter interfaces.

I spent some extra time now to dig into EOPNOTSUPP and how it is used in the current network subsystems the Linux kernel has. I normally refers to a local operation that is not supported. It is rarely used within the connect() system call, but it is indeed used there.

When EOPNOTSUPP is used, then it is in the context of the connect() system call implementation. It is used before any connection attempt is made. An example is when calling connect() on a DGRAM socket, but it only supports connect() in the STREAM socket.

For ECONNABORTED it is normally used in the context when the connection attempt is already in progress and it is then normally delivered via sk->sk_err to the user of the socket.

> > Please look for a good reference of the error code usage in other socket protocols or manpages
> 
> It is not obvious to find one good reference. But for example there is : Documentation/ioctl/hdio.txt:     EOPNOTSUPP    Hardware interface does not support bus power
> 
> Another option could be:
> [ENODEV]
>    No such device An attempt was made to apply an inappropriate function to a device; for example, trying to read a write-only device such as a printer.

ENODEV is not meant for sockets. We use it inside the Bluetooth subsystem for errors that relate to HCI devices, HID/input devices or TTY devices.

In summary so far we have the closest match with EOPNOTSUPP for one reason. The connect() system call is actually failing without any connection attempt. It tries to find the local controller assigned to this socket and if it does not support eSCO, it bails out. That makes it fall in line with how it should be used if we look at other systems calls or socket families.

However I only looked at the error codes you listed in this email. Maybe there is even a better one, maybe there is not. I am pretty much open to discuss alternatives here.

Regards

Marcel

Re: [PATCH 1/3] sco-tester: Update ECONNABORTED to EOPNOTSUPP

[Frédéric DALLEAU]

Hi Marcel,

Le 13/08/2013 01:22, Marcel Holtmann a écrit :

> However I only looked at the error codes you listed in this email. Maybe there is even a better one, maybe there is not. I am pretty much open to discuss alternatives here.

Thanks for detailed feedback. For the vast majority of other errors
code, there was no need to go through code to eliminate them: Their 
purpose if usually very restrictive.

Let's list some potential candidate just in case:
#define ENOSYS          38      /* Function not implemented */
#define ENOSR           63      /* Out of streams resources */
#define ENOMEDIUM       123     /* No medium found */

Regards,
Frederic

Re: [PATCH 1/3] sco-tester: Update ECONNABORTED to EOPNOTSUPP

[Frédéric DALLEAU]

Le 09/08/2013 17:42, Frédéric Dalleau a écrit :
> Kernel interface has evolved in between.
> ---
>   tools/sco-tester.c |    2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/sco-tester.c b/tools/sco-tester.c
> index 70b8a5d..1e8351f 100644
> --- a/tools/sco-tester.c
> +++ b/tools/sco-tester.c
> @@ -237,7 +237,7 @@ static const struct sco_client_data connect_success = {
>   };
>
>   static const struct sco_client_data connect_failure = {
> -	.expect_err = ECONNABORTED
> +	.expect_err = EOPNOTSUPP
>   };
>
>   static void client_connectable_complete(uint16_t opcode, uint8_t status,
>

Hello,

SCO sockets are now using EOPNOTSUPP.
What about these patches?

Thanks,
Frédéric

Re: [PATCH 1/3] sco-tester: Update ECONNABORTED to EOPNOTSUPP

[Johan Hedberg]

Hi Frederic,

On Fri, Aug 09, 2013, Frédéric Dalleau wrote:
> Kernel interface has evolved in between.
> ---
>  tools/sco-tester.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

I've applied and pushed this patch, but there are a couple of details in
the other ones that I'd like to get fixed before pushing.

Johan

Re: [PATCH 2/3] sco-tester: Introduce adapter features

[Johan Hedberg]

Hi Frederic,

On Fri, Aug 09, 2013, Frédéric Dalleau wrote:
> +struct adapter_features {
> +	bool disable_esco;
> +};
> +
>  struct test_data {
>  	const void *test_data;
>  	struct mgmt *mgmt;
> @@ -50,7 +54,7 @@ struct test_data {
>  	struct hciemu *hciemu;
>  	enum hciemu_type hciemu_type;
>  	unsigned int io_id;
> -	bool disable_esco;
> +	struct adapter_features features;
>  };
>  
>  struct sco_client_data {
> @@ -164,7 +168,7 @@ static void read_index_list_callback(uint8_t status, uint16_t length,
>  
>  	tester_print("New hciemu instance created");
>  
> -	if (data->disable_esco) {
> +	if (data->features.disable_esco) {
>  		uint8_t *features;
>  
>  		tester_print("Disabling eSCO packet type support");
> @@ -211,7 +215,7 @@ static void test_data_free(void *test_data)
>  	free(data);
>  }
>  
> -#define test_sco_full(name, data, setup, func, _disable_esco) \
> +#define test_sco(name, data, setup, func, feat) \
>  	do { \
>  		struct test_data *user; \
>  		user = malloc(sizeof(struct test_data)); \
> @@ -220,17 +224,12 @@ static void test_data_free(void *test_data)
>  		user->hciemu_type = HCIEMU_TYPE_BREDRLE; \
>  		user->io_id = 0; \
>  		user->test_data = data; \
> -		user->disable_esco = _disable_esco; \
> +		user->features = feat; \
>  		tester_add_full(name, data, \
>  				test_pre_setup, setup, func, NULL, \
>  				test_post_teardown, 2, user, test_data_free); \
>  	} while (0)
>  
> -#define test_sco(name, data, setup, func) \
> -	test_sco_full(name, data, setup, func, false)
> -
> -#define test_sco_11(name, data, setup, func) \
> -	test_sco_full(name, data, setup, func, true)
>  
>  static const struct sco_client_data connect_success = {
>  	.expect_err = 0
> @@ -575,31 +574,37 @@ end:
>  
>  int main(int argc, char *argv[])
>  {
> +	static struct adapter_features features;
> +
>  	tester_init(&argc, &argv);
>  
> +	features.disable_esco = 0;

First of all, you should use "false" here instead of "0" (and same for
true vs 1 later).

Secondly, whenever you want to manipulate some parameter of a test case
it should not be done in the main function like that but instead you
should have it as part of the test case data (struct sco_client_data)
and create a separate instance for each test case.

I do realize that the initial sco-tester didn't get this right (having
disable_sco in struct test_data instead of struct sco_client_data), but
it'd be good to get this fixed before adding any new test cases.

Johan

Re: [PATCH 2/3] sco-tester: Introduce adapter features

[Frédéric DALLEAU]

Hi Johan,

> First of all, you should use "false" here instead of "0" (and same for
> true vs 1 later).
>
> Secondly, whenever you want to manipulate some parameter of a test case
> it should not be done in the main function like that but instead you
> should have it as part of the test case data (struct sco_client_data)
> and create a separate instance for each test case.
 >
> I do realize that the initial sco-tester didn't get this right (having
> disable_sco in struct test_data instead of struct sco_client_data), but
> it'd be good to get this fixed before adding any new test cases.

The problem is that the adapter feature has to be defined far before the 
test is started.

Another problem is that sco_client_data is test dependant, eg. each
test can implement sco_client_data differently. In this case, 
adapter_features could simply be undefined! This is already the case in 
the first tests which do not use struct sco_client_data at all 
(test_socket).

IMHO We do not want that. adapter_features must be defined explictly 
before each test. This is mandatory.

In future I'm also thinking to implement some adapter "behavior". For 
example : one test could be : try connect, emulator must accept, and 
expected result is success. then try connect, emulator must refuse, and 
result is expected to be connection refused.

My suggestion would be really different from yours :
I would write a big table with the elements of test_sco.

struct test {
	char *name;
	struct adapter_features *features;
	struct adapter_behavior *behavior; /* If needed */
	void (*test_cb)(struct test *test, void *data);
         void *user_data; /* sco_client_data*/
	int expected_result;
} tests [] = {...};

while (test[i].name) {
     test_sco(test[i].name, test[i].test_cb, ...);
     i++;
}

Let me know what you think.

Regards,
Frédéric

Re: [PATCH 2/3] sco-tester: Introduce adapter features

[Johan Hedberg]

Hi Frederic,

On Mon, Sep 09, 2013, Frédéric DALLEAU wrote:
> >First of all, you should use "false" here instead of "0" (and same for
> >true vs 1 later).
> >
> >Secondly, whenever you want to manipulate some parameter of a test case
> >it should not be done in the main function like that but instead you
> >should have it as part of the test case data (struct sco_client_data)
> >and create a separate instance for each test case.
> >
> >I do realize that the initial sco-tester didn't get this right (having
> >disable_sco in struct test_data instead of struct sco_client_data), but
> >it'd be good to get this fixed before adding any new test cases.
> 
> The problem is that the adapter feature has to be defined far before
> the test is started.

The sco_client structs are static ones and therefore available when the
tester starts, so I'm not sure what the issue is.

> Another problem is that sco_client_data is test dependant, eg. each
> test can implement sco_client_data differently. In this case,
> adapter_features could simply be undefined! This is already the case
> in the first tests which do not use struct sco_client_data at all
> (test_socket).
> 
> IMHO We do not want that. adapter_features must be defined explictly
> before each test. This is mandatory.
> 
> In future I'm also thinking to implement some adapter "behavior".
> For example : one test could be : try connect, emulator must accept,
> and expected result is success. then try connect, emulator must
> refuse, and result is expected to be connection refused.
> 
> My suggestion would be really different from yours :
> I would write a big table with the elements of test_sco.
> 
> struct test {
> 	char *name;
> 	struct adapter_features *features;
> 	struct adapter_behavior *behavior; /* If needed */
> 	void (*test_cb)(struct test *test, void *data);
>         void *user_data; /* sco_client_data*/
> 	int expected_result;
> } tests [] = {...};
> 
> while (test[i].name) {
>     test_sco(test[i].name, test[i].test_cb, ...);
>     i++;
> }
> 
> Let me know what you think.

I wasn't trying to suggest something new but just describe what the
other end-to-end testers under tools/ do. Take a look at e.g.
l2cap-tester or mgmt-tester (which has the most extensive list of test
cases). Neither one of those has needed anything similar to what you're
proposing and if we go ahead and adopt a new model for sco-tester then
all the testers should follow it.

If you feel like the adapter properties have to be somewhere "higher up"
than sco_client_data (which they really don't have to be) one option is
to move it to the test_data and simply have separate macros for defining
test cases for different types of controllers, just like mgmt-tester
does with test_bredrle(), test_bredr() and test_le().

Johan