[PATCH 0/2] Fix SCTP tests on systems with disabled IPv6

[PATCH 0/2] Fix SCTP tests on systems with disabled IPv6

[Petr Vorel]

Hi,

this is needed for systems which boot with ipv6.disable=1.
Although we have some discussion what should be the primary repository [1],
I'd like to get this fixed in LTP sooner than later.

I could not stop myself to fix little bit of broken formatting (first
commit), but I can drop it, if that's easier for backporting to the
other repository.

Kind regards,
Petr

[1] https://lore.kernel.org/linux-sctp/Yfu4ucYOvXbOqXYt@yuki/T/#md06d317a4200592ed5c3a47ff30884a224c503c9

Petr Vorel (2):
  sctputil.h: Fix some formatting
  sctputil.h: TCONF on EAFNOSUPPORT

 utils/sctp/testlib/sctputil.h | 105 ++++++++++++++++++++++++----------
 1 file changed, 74 insertions(+), 31 deletions(-)

-- 
2.35.1

[PATCH 1/2] sctputil.h: Fix some formatting

[Petr Vorel]

* use tabs (code indent should use tabs where possible,
  please, no spaces at the start of a line)
* constant on second place in if()
* missing a blank line after declarations

Just few problems related to fix in the next commit, there are more if
you run:
$ ./scripts/checkpatch.pl -f --no-tree --terse --no-summary --ignore CONST_STRUCT,VOLATILE,SPLIT_STRING utils/sctp/testlib/sctputil.h

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 utils/sctp/testlib/sctputil.h | 100 +++++++++++++++++++++++-----------
 1 file changed, 69 insertions(+), 31 deletions(-)

diff --git a/utils/sctp/testlib/sctputil.h b/utils/sctp/testlib/sctputil.h
index 6c3371c960..1e21760bec 100644
--- a/utils/sctp/testlib/sctputil.h
+++ b/utils/sctp/testlib/sctputil.h
@@ -133,64 +133,80 @@ extern int TST_CNT;
 static inline int test_socket(int domain, int type, int protocol)
 {
 	int sk = socket(domain, type, protocol);
-        if (-1 == sk)
-                tst_brkm(TBROK, tst_exit, "socket: %s", strerror(errno));
+
+	if (sk == -1)
+		tst_brkm(TBROK, tst_exit, "socket: %s", strerror(errno));
+
 	return sk;
 }
 
 static inline int test_bind(int sk, struct sockaddr *addr, socklen_t addrlen)
 {
 	int error = bind(sk, addr, addrlen);
-        if (-1 == error)
-                tst_brkm(TBROK, tst_exit, "bind: %s", strerror(errno));
+
+	if (error == -1)
+		tst_brkm(TBROK, tst_exit, "bind: %s", strerror(errno));
+
 	return error;
 }
 
 static inline int test_bindx_add(int sk, struct sockaddr *addr, int count)
 {
 	int error = sctp_bindx(sk, addr, count, SCTP_BINDX_ADD_ADDR);
-        if (-1 == error)
-                tst_brkm(TBROK, tst_exit, "bindx (add): %s", strerror(errno));
+
+	if (error == -1)
+		tst_brkm(TBROK, tst_exit, "bindx (add): %s", strerror(errno));
+
 	return error;
 }
 
 static inline int test_listen(int sk, int backlog)
 {
 	int error = listen(sk, backlog);
-        if (-1 == error)
-                tst_brkm(TBROK, tst_exit, "listen: %s", strerror(errno));
+
+	if (error == -1)
+		tst_brkm(TBROK, tst_exit, "listen: %s", strerror(errno));
+
 	return error;
 }
 
 static inline int test_connect(int sk, struct sockaddr *addr, socklen_t addrlen)
 {
 	int error = connect(sk, addr, addrlen);
-        if (-1 == error)
-                tst_brkm(TBROK, tst_exit, "connect: %s", strerror(errno));
+
+	if (error == -1)
+		tst_brkm(TBROK, tst_exit, "connect: %s", strerror(errno));
+
 	return error;
 }
 
 static inline int test_connectx(int sk, struct sockaddr *addr, int count)
 {
 	int error = sctp_connectx(sk, addr, count, NULL);
-        if (-1 == error)
-                tst_brkm(TBROK, tst_exit, "connectx: %s", strerror(errno));
+
+	if (error == -1)
+		tst_brkm(TBROK, tst_exit, "connectx: %s", strerror(errno));
+
 	return error;
 }
 
 static inline int test_accept(int sk, struct sockaddr *addr, socklen_t *addrlen)
 {
 	int error = accept(sk, addr, addrlen);
-        if (-1 == error)
-                tst_brkm(TBROK, tst_exit, "accept: %s", strerror(errno));
+
+	if (error == -1)
+		tst_brkm(TBROK, tst_exit, "accept: %s", strerror(errno));
+
 	return error;
 }
 
 static inline int test_send(int sk, const void *msg, size_t len, int flags)
 {
 	int error = send(sk, msg, len, flags);
+
 	if ((long)len != error)
 		tst_brkm(TBROK, tst_exit, "send: error:%d errno:%d", error, errno);
+
 	return error;
 }
 
@@ -198,8 +214,10 @@ static inline int test_sendto(int sk, const void *msg, size_t len, int flags,
 			      const struct sockaddr *to, socklen_t tolen)
 {
 	int error = sendto(sk, msg, len, flags, to, tolen);
+
 	if ((long)len != error)
 		tst_brkm(TBROK, tst_exit, "sendto: error:%d errno:%d", error, errno);
+
 	return error;
 }
 
@@ -207,33 +225,40 @@ static inline int test_sendmsg(int sk, const struct msghdr *msg, int flags,
 			       int msglen)
 {
 	int error = sendmsg(sk, msg, flags);
-        if (msglen != error)
-                tst_brkm(TBROK, tst_exit, "sendmsg: error:%d errno:%d",
-			 error, errno);
+
+	if (msglen != error)
+		tst_brkm(TBROK, tst_exit, "sendmsg: error:%d errno:%d", error, errno);
+
 	return error;
 }
 
 static inline int test_recv(int sk, void *buf, size_t len, int flags)
 {
 	int error = recv(sk, buf, len, flags);
-        if (-1 == error)
-                tst_brkm(TBROK, tst_exit, "recv: %s", strerror(errno));
+
+	if (error == -1)
+		tst_brkm(TBROK, tst_exit, "recv: %s", strerror(errno));
+
 	return error;
 }
 
 static inline int test_recvmsg(int sk, struct msghdr *msg, int flags)
 {
 	int error = recvmsg(sk, msg, flags);
-        if (-1 == error)
-                tst_brkm(TBROK, tst_exit, "recvmsg: %s", strerror(errno));
+
+	if (error == -1)
+		tst_brkm(TBROK, tst_exit, "recvmsg: %s", strerror(errno));
+
 	return error;
 }
 
 static inline int test_shutdown(int sk, int how)
 {
 	int error = shutdown(sk, how);
-        if (-1 == error)
-                tst_brkm(TBROK, tst_exit, "shutdown: %s", strerror(errno));
+
+	if (error == -1)
+		tst_brkm(TBROK, tst_exit, "shutdown: %s", strerror(errno));
+
 	return error;
 }
 
@@ -241,6 +266,7 @@ static inline int test_getsockopt(int sk, int optname, void *optval,
 				  socklen_t *optlen)
 {
 	int error = getsockopt(sk, SOL_SCTP, optname, optval, optlen);
+
 	if (error)
 		tst_brkm(TBROK, tst_exit, "getsockopt(%d): %s", optname,
 			 strerror(errno));
@@ -251,17 +277,21 @@ static inline int test_setsockopt(int sk, int optname, const void *optval,
 				  socklen_t optlen)
 {
 	int error = setsockopt(sk, SOL_SCTP, optname, optval, optlen);
+
 	if (error)
 		tst_brkm(TBROK, tst_exit, "setsockopt(%d): %s", optname,
 			 strerror(errno));
+
 	return error;
 }
 
 static inline int test_sctp_peeloff(int sk, sctp_assoc_t assoc_id)
 {
 	int error = sctp_peeloff(sk, assoc_id);
-        if (-1 == error)
-                tst_brkm(TBROK, tst_exit, "sctp_peeloff: %s", strerror(errno));
+
+	if (error == -1)
+		tst_brkm(TBROK, tst_exit, "sctp_peeloff: %s", strerror(errno));
+
 	return error;
 }
 
@@ -272,10 +302,12 @@ static inline int test_sctp_sendmsg(int s, const void *msg, size_t len,
 				    uint32_t context)
 {
 	int error = sctp_sendmsg(s, msg, len, to, tolen, ppid, flags, stream_no,
-	  		         timetolive, context);
-	if ((long)len != error)
+				 timetolive, context);
+
+	if (error != (long)len)
 		tst_brkm(TBROK, tst_exit, "sctp_sendmsg: error:%d errno:%d",
 			 error, errno);
+
 	return error;
 }
 
@@ -284,9 +316,11 @@ static inline int test_sctp_send(int s, const void *msg, size_t len,
 				 int flags)
 {
 	int error = sctp_send(s, msg, len, sinfo, flags);
-	if ((long)len != error)
+
+	if (error != (long)len)
 		tst_brkm(TBROK, tst_exit, "sctp_send: error:%d errno:%d",
 			 error, errno);
+
 	return error;
 }
 
@@ -296,16 +330,20 @@ static inline int test_sctp_recvmsg(int sk, void *msg, size_t len,
 				    int *msg_flags)
 {
 	int error = sctp_recvmsg(sk, msg, len, from, fromlen, sinfo, msg_flags);
-	if (-1 == error)
+
+	if (error == -1)
 		tst_brkm(TBROK, tst_exit, "sctp_recvmsg: %s", strerror(errno));
+
 	return error;
 }
 
 static inline void *test_malloc(size_t size)
 {
 	void *buf = malloc(size);
-        if (NULL == buf)
-                tst_brkm(TBROK, tst_exit, "malloc failed");
+
+	if (NULL == buf)
+		tst_brkm(TBROK, tst_exit, "malloc failed");
+
 	return buf;
 }
 
-- 
2.35.1

[PATCH 2/2] sctputil.h: TCONF on EAFNOSUPPORT

[Petr Vorel]

That's returned on systems without IPv6 support
(e.g. boot with ipv6.disable=1).

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 utils/sctp/testlib/sctputil.h | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/utils/sctp/testlib/sctputil.h b/utils/sctp/testlib/sctputil.h
index 1e21760bec..c4bedb47cf 100644
--- a/utils/sctp/testlib/sctputil.h
+++ b/utils/sctp/testlib/sctputil.h
@@ -133,9 +133,14 @@ extern int TST_CNT;
 static inline int test_socket(int domain, int type, int protocol)
 {
 	int sk = socket(domain, type, protocol);
+	int res = TBROK;
 
-	if (sk == -1)
-		tst_brkm(TBROK, tst_exit, "socket: %s", strerror(errno));
+	if (sk == -1) {
+		if (errno == EAFNOSUPPORT)
+			res = TCONF;
+
+		tst_brkm(res, tst_exit, "socket: %s", strerror(errno));
+	}
 
 	return sk;
 }
-- 
2.35.1

Re: [PATCH 1/2] sctputil.h: Fix some formatting

[Cyril Hrubis]

Hi!
>  static inline void *test_malloc(size_t size)
>  {
>  	void *buf = malloc(size);
> -        if (NULL == buf)
> -                tst_brkm(TBROK, tst_exit, "malloc failed");
> +
> +	if (NULL == buf)
> +		tst_brkm(TBROK, tst_exit, "malloc failed");

This one has still constant on the right and I would rather change this
to the more common if (!buf) variant as well.

Other than that it's pretty obviously fine:

Reviewed-by: Cyril Hrubis <chrubis@suse.cz>

>  	return buf;
>  }
>  
> -- 
> 2.35.1
> 

-- 
Cyril Hrubis
chrubis@suse.cz

Re: [PATCH 1/2] sctputil.h: Fix some formatting

[Cyril Hrubis]

Hi!
> +	if (sk == -1)
> +		tst_brkm(TBROK, tst_exit, "socket: %s", strerror(errno));
> +
>  	return sk;
>  }

Also one more thing, we can just do TBROK | TERRNO instead of the
strerror() as well.

-- 
Cyril Hrubis
chrubis@suse.cz

Re: [PATCH 2/2] sctputil.h: TCONF on EAFNOSUPPORT

[Cyril Hrubis]

Hi!
> diff --git a/utils/sctp/testlib/sctputil.h b/utils/sctp/testlib/sctputil.h
> index 1e21760bec..c4bedb47cf 100644
> --- a/utils/sctp/testlib/sctputil.h
> +++ b/utils/sctp/testlib/sctputil.h
> @@ -133,9 +133,14 @@ extern int TST_CNT;
>  static inline int test_socket(int domain, int type, int protocol)
>  {
>  	int sk = socket(domain, type, protocol);
> +	int res = TBROK;
>  
> -	if (sk == -1)
> -		tst_brkm(TBROK, tst_exit, "socket: %s", strerror(errno));
> +	if (sk == -1) {
> +		if (errno == EAFNOSUPPORT)
> +			res = TCONF;
> +
> +		tst_brkm(res, tst_exit, "socket: %s", strerror(errno));
> +	}

I would keep the messages separated here, i.e. do something as:

	if (errno == EAFNOSUPPORT)
		tst_brkm(TBROK | TERRNO, "socket(%i, %i, %i) not supported",
			 domain, type, protocol);

	tst_brkm(TBROK | TERRNO, "socket()");


Btw this code actually duplicates the safe_socket() function we do have
already, so it may as well be easier to just replace the test_socket()
with SAFE_SOCKET() in the tests...

-- 
Cyril Hrubis
chrubis@suse.cz

Re: [PATCH 2/2] sctputil.h: TCONF on EAFNOSUPPORT

[Petr Vorel]

Hi Cyril, all,

> Hi!
> > diff --git a/utils/sctp/testlib/sctputil.h b/utils/sctp/testlib/sctputil.h
> > index 1e21760bec..c4bedb47cf 100644
> > --- a/utils/sctp/testlib/sctputil.h
> > +++ b/utils/sctp/testlib/sctputil.h
> > @@ -133,9 +133,14 @@ extern int TST_CNT;
> >  static inline int test_socket(int domain, int type, int protocol)
> >  {
> >  	int sk = socket(domain, type, protocol);
> > +	int res = TBROK;

> > -	if (sk == -1)
> > -		tst_brkm(TBROK, tst_exit, "socket: %s", strerror(errno));
> > +	if (sk == -1) {
> > +		if (errno == EAFNOSUPPORT)
> > +			res = TCONF;
> > +
> > +		tst_brkm(res, tst_exit, "socket: %s", strerror(errno));
> > +	}

> I would keep the messages separated here, i.e. do something as:

> 	if (errno == EAFNOSUPPORT)
> 		tst_brkm(TBROK | TERRNO, "socket(%i, %i, %i) not supported",
> 			 domain, type, protocol);

> 	tst_brkm(TBROK | TERRNO, "socket()");
+1


> Btw this code actually duplicates the safe_socket() function we do have
> already, so it may as well be easier to just replace the test_socket()
> with SAFE_SOCKET() in the tests...
I originally wanted to use safe_macros.h in sctputil.h to replace these
test_{bind,connect,listen,socket} with their SAFE_*() variants.

But it leads into dependency many redefinition problems due mixing
<netinet/in.h> and <linux/in.h>, e.g.:

/usr/include/netinet/in.h:68:5: error: redeclaration of enumerator ‘IPPROTO_GRE’
   68 |     IPPROTO_GRE = 47,      /* General Routing Encapsulation.  */
/usr/include/linux/in.h:55:3: note: previous definition of ‘IPPROTO_GRE’ with type ‘enum <anonymous>’
   55 |   IPPROTO_GRE = 47,             /* Cisco GRE tunnels (rfc 1701,1702)    */
      |   ^~~~~~~~~~~
in utils/sctp/func_tests/test_1_to_1_rtoinfo.c, which requires <linux/in.h> for
at least IPPROTO_SCTP which is not in <netinet/in.h>.

IPPROTO_SCTP is also in <linux/sctp.h>, but it also requires <netinet/sctp.h>
for sctp_recvmsg() and it creates another redefinition conflict due using
<netinet/sctp.h> with <linux/sctp.h> => dependency hell :).

FYI test_1_to_1_rtoinfo.c uses test_socket() and sctputil.h.

Sure this is solvable via either using lapi headers which would load only one of
them and with adding extra definitions or simple just adding the missing
definitions into sctputil.h.

But IMHO a cleaner solution is to rewrite test one by one (which would take
time), but we're waiting reply from SCTP maintainers where (and who) is going to
maintain these tests which deserve a massive cleanup...

Thus for now, I'll follow your other suggestions and merge so that we have IPv6
fixes in.

Kind regards,
Petr

Re: [PATCH 2/2] sctputil.h: TCONF on EAFNOSUPPORT

[Petr Vorel]

Hi Cyril, all,

patchset merged (with suggested changes + few more TERRNO replaced).

Thanks!

Kind regards,
Petr