syscalls/bind03: Add version compare according to behaviour difference.

PATCH New Junchi Chen 4 participants 9 messages Aug 31, 2018 – Sep 7, 2018

Share & tools

Junchi Chen

author

2018-08-31 13:24 UTC #0 lore raw

PATCH 1/1 syscalls/bind03: Add version compare according to behaviour difference.

ISSUE:
The case is designed to test the behaviour diverse caused by kernel commit 0fb44559ffd6. And it failed for the new behaviour.

ACTION:
Add a version compare to split part of the test.

Signed-off-by Junchi Chen <[email protected]>

---
 testcases/kernel/syscalls/bind/bind03.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

testcases/kernel/syscalls/bind/bind03.c +17 -4

index 2fe342a54..955a69dd2 100644
@@ -13,6 +13,7 @@
 #include <stdlib.h>
 #include <string.h>
 
+#include "tst_kvercmp.h"
 #include "tst_test.h"
 #include "tst_safe_net.h"

@@ -37,16 +38,28 @@ void run(void)
 
 	/*
 	 * Once a STREAM UNIX domain socket has been bound, it can't be
-	 * rebound. Expected error is EINVAL.
+	 * rebound.
 	 */
 	if (bind(sock1, (struct sockaddr *)&sun, sizeof(sun)) == 0) {
 		tst_res(TFAIL, "re-binding of socket succeeded");
 		return;
 	}
 
-	if (errno != EINVAL) {
-		tst_res(TFAIL | TERRNO, "expected EINVAL");
-		return;
+	/*
+	 * The behavious diverse according to kernel version
+	 * for v4.10 or later, the expected error is EADDRINUSE,
+	 * otherwise EINVAL.
+	 */
+	if (tst_kvercmp(4, 10, 0) < 0) {
+		if (errno != EINVAL) {
+			tst_res(TFAIL | TERRNO, "expected EINVAL");
+			return;
+		}
+	} else {
+		if (errno != EADDRINUSE) {
+			tst_res(TFAIL | TERRNO, "expected EADDRINUSE");
+			return;
+		}
 	}
 
 	sock2 = SAFE_SOCKET(PF_UNIX, SOCK_STREAM, 0);

Signature · 1 line

2.17.1

Petr Vorel

Applied

2018-08-31 08:37 UTC #1 lore raw

PATCH 1/1 syscalls/bind03: Add version compare according to behaviour difference.

Hi Junchi,

Quoted 8 lines

ISSUE:
The case is designed to test the behaviour diverse caused by kernel commit 0fb44559ffd6. And it failed for the new behaviour.
ACTION:
Add a version compare to split part of the test.
Signed-off-by Junchi Chen <[email protected]>

Merged, thanks!

Kind regards,
Petr

Cyril Hrubis

2018-08-31 08:42 UTC #2 lore raw

PATCH 1/1 syscalls/bind03: Add version compare according to behaviour difference.

Hi!

Quoted 10 lines

Quoted 8 lines
ISSUE:
The case is designed to test the behaviour diverse caused by kernel commit 0fb44559ffd6. And it failed for the new behaviour.
ACTION:
Add a version compare to split part of the test.
Signed-off-by Junchi Chen <[email protected]>
Merged, thanks!

I think that this should be reverted, until at least we got a clear reply from kernel developers...

Signature · 2 lines

Cyril Hrubis
[email protected]

Petr Vorel

2018-09-07 10:12 UTC #3 lore raw

PATCH 1/1 syscalls/bind03: Add version compare according to behaviour difference.

Hi,

Quoted 7 lines

Quoted 6 lines
Quoted 6 lines
ISSUE:
The case is designed to test the behaviour diverse caused by kernel commit 0fb44559ffd6. And it failed for the new behaviour.
ACTION:
Add a version compare to split part of the test.

Quoted 6 lines

Quoted 3 lines
Quoted 1 line
Signed-off-by Junchi Chen <[email protected]>
Merged, thanks!
I think that this should be reverted, until at least we got a clear reply from kernel developers...

Michal Kubecek doesn't plan to force his patch, so we can keep the fix.

Although we should improve the test of EINVAL (not to pass two invalid parameters to the syscall at once).

Kind regards,
Petr

Xiao Yang

2018-08-31 08:48 UTC #4 lore raw

PATCH 1/1 syscalls/bind03: Add version compare according to behaviour difference.

In reply to Petr Vorel

Quoted 9 lines · Petr Vorel

Hi Junchi,
Quoted 6 lines
ISSUE:
The case is designed to test the behaviour diverse caused by kernel commit 0fb44559ffd6. And it failed for the new behaviour.
ACTION:
Add a version compare to split part of the test.
Signed-off-by Junchi Chen<[email protected]>
Merged, thanks!

Hi Petr,

According to bind(2) manpge, bind() should return EINVAL when the socket is already
bound to an address. Commit 0fb4455 actually change the errno to EADDRINUSE, but
i am not sure if this change is a expeceted behavior.

Thanks,
Xiao Yang

Quoted 4 lines

Kind regards,
Petr

Cyril Hrubis

2018-08-31 09:34 UTC #5 lore raw

PATCH 1/1 syscalls/bind03: Add version compare according to behaviour difference.

Hi!

Quoted 5 lines

According to bind(2) manpge, bind() should return EINVAL when the socket is already
bound to an address. Commit 0fb4455 actually change the errno to EADDRINUSE, but
i am not sure if this change is a expeceted behavior.

As Michal Kubecek writes in the commit message of the patch I've linked, the kernel has changed the order of checks which returns different errno than it used to for no good reason. I would like to be conservative in this case and have the previous behavior restored, but I suppose that's up to kernel guys to decide.

Signature · 2 lines

Cyril Hrubis
[email protected]

Cyril Hrubis

2018-08-31 08:41 UTC #6 lore raw

PATCH 1/1 syscalls/bind03: Add version compare according to behaviour difference.

In reply to Junchi Chen

Hi!

Quoted 6 lines

ISSUE:
The case is designed to test the behaviour diverse caused by kernel commit 0fb44559ffd6. And it failed for the new behaviour.
ACTION:
Add a version compare to split part of the test.

This is actually a not-yet fixed kernel bug as the correct errno to be reported in this case is EADDRINUSE at least that is what I understand after reading POSIX.

See:

https://patchwork.ozlabs.org/patch/782692/

Signature · 2 lines

Cyril Hrubis
[email protected]

Cyril Hrubis

2018-08-31 09:37 UTC #7 lore raw

PATCH 1/1 syscalls/bind03: Add version compare according to behaviour difference.

Hi!

Quoted 10 lines

Quoted 6 lines
ISSUE:
The case is designed to test the behaviour diverse caused by kernel commit 0fb44559ffd6. And it failed for the new behaviour.
ACTION:
Add a version compare to split part of the test.
This is actually a not-yet fixed kernel bug as the correct errno to be reported in this case is EADDRINUSE at least that is what I understand after reading POSIX.

Sigh, not enough coffee I would say, the rationale is correctly written in the patch from Michal.

"in general, we do not want to change return code for given testcase and
old error (-EINVAL) is consistent with AF_INET(6)"

Sorry for the confusion.

Quoted 3 lines

See:
https://patchwork.ozlabs.org/patch/782692/

Signature · 2 lines

Cyril Hrubis
[email protected]

Petr Vorel

2018-08-31 10:39 UTC #8 lore raw

PATCH 1/1 syscalls/bind03: Add version compare according to behaviour difference.

Hi,

Quoted 11 lines

Quoted 10 lines
Quoted 6 lines
ISSUE:
The case is designed to test the behaviour diverse caused by kernel commit 0fb44559ffd6. And it failed for the new behaviour.
ACTION:
Add a version compare to split part of the test.
This is actually a not-yet fixed kernel bug as the correct errno to be reported in this case is EADDRINUSE at least that is what I understand after reading POSIX.

Quoted 11 lines

Sigh, not enough coffee I would say, the rationale is correctly written in the patch from Michal.
"in general, we do not want to change return code for given testcase and
old error (-EINVAL) is consistent with AF_INET(6)"
Sorry for the confusion.
Quoted 3 lines
See:
https://patchwork.ozlabs.org/patch/782692/

I've already merged this fix, but as Cyril pointed out it might not be what is wanted. I'll ask David Miller and Michal Kubecek at netdev ML.

Kind regards,
Petr