[LTP] [PATCH] syscalls/sendto02.c: add new testcase

[LTP] [PATCH] syscalls/sendto02.c: add new testcase

[Xiao Yang]

When sctp protocol is selected in socket(2) and buffer is invalid,
sendto(2) should fail and set errno to EFAULT, but it sets errno
to ENOMEM.

This is a regression test and has been fixed by kernel patch:
'commit 6e51fe757259 ("sctp: fix -ENOMEM result with invalid
user space pointer in sendto() syscall")'

Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
---
 runtest/syscalls                            |   1 +
 testcases/kernel/syscalls/.gitignore        |   1 +
 testcases/kernel/syscalls/sendto/sendto02.c | 126 ++++++++++++++++++++++++++++
 3 files changed, 128 insertions(+)
 create mode 100644 testcases/kernel/syscalls/sendto/sendto02.c

diff --git a/runtest/syscalls b/runtest/syscalls
index b781241..6918795 100644
--- a/runtest/syscalls
+++ b/runtest/syscalls
@@ -1005,6 +1005,7 @@ sendmsg01 sendmsg01
 sendmsg02 sendmsg02
 
 sendto01 sendto01
+sendto02 sendto02
 
 setdomainname01	setdomainname01
 setdomainname02	setdomainname02
diff --git a/testcases/kernel/syscalls/.gitignore b/testcases/kernel/syscalls/.gitignore
index f53cc05..eb9bf5c 100644
--- a/testcases/kernel/syscalls/.gitignore
+++ b/testcases/kernel/syscalls/.gitignore
@@ -814,6 +814,7 @@
 /sendmsg/sendmsg01
 /sendmsg/sendmsg02
 /sendto/sendto01
+/sendto/sendto02
 /set_robust_list/set_robust_list01
 /set_thread_area/set_thread_area01
 /set_thread_area/set_thread_area02
diff --git a/testcases/kernel/syscalls/sendto/sendto02.c b/testcases/kernel/syscalls/sendto/sendto02.c
new file mode 100644
index 0000000..67cc38d
--- /dev/null
+++ b/testcases/kernel/syscalls/sendto/sendto02.c
@@ -0,0 +1,126 @@
+/*
+ * Copyright(c) 2016 Fujitsu Ltd.
+ * Author: Xiao Yang <yangx.jy@cn.fujitsu.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of version 2 of the GNU General Public License as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it would be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
+ *
+ * You should have received a copy of the GNU General Public License
+ * alone with this program.
+ */
+
+/*
+ * Test Name: sendto02
+ *
+ * Description:
+ * When sctp protocol is selected in socket(2) and buffer is invalid,
+ * sendto(2) should fail and set errno to EFAULT, but it sets errno
+ * to ENOMEM.
+ *
+ * This is a regression test and has been fixed by kernel commit:
+ * 6e51fe7572590d8d86e93b547fab6693d305fd0d (sctp: fix -ENOMEM result
+ * with invalid user space pointer in sendto() syscall)
+ */
+
+#include <errno.h>
+#include <sys/types.h>
+#include <sys/socket.h>
+
+#include "tst_test.h"
+
+static int sockfd;
+static int rds_flag;
+static struct sockaddr_in sa;
+
+static void setup(void)
+{
+	int acc_res, load_res;
+	const char *cmd[] = {"modprobe", "sctp", NULL};
+
+	acc_res = access("/proc/sys/net/sctp", F_OK);
+	if (acc_res == -1 && errno != ENOENT)
+		tst_brk(TFAIL | TERRNO, "failed to check stcp module");
+
+	if (acc_res == -1 && errno == ENOENT) {
+		load_res = tst_run_cmd(cmd, NULL, NULL, 1);
+		if (load_res) {
+			tst_brk(TCONF, "failed to loaded sctp module, "
+				"so sctp modeule was not support by system");
+		}
+
+		tst_res(TINFO, "succeeded to load sctp module");
+		rds_flag = 1;
+	}
+
+	tst_res(TINFO, "sctp module was supported by system");
+
+	sockfd = SAFE_SOCKET(AF_INET, SOCK_STREAM, 132 /*IPPROTO_SCTP*/);
+
+	memset(&sa, 0, sizeof(sa));
+	sa.sin_family = AF_INET;
+	sa.sin_addr.s_addr = inet_addr("127.0.0.1");
+	sa.sin_port = htons(11111);
+}
+
+static void cleanup(void)
+{
+	int unload_res;
+	const char *cmd[] = {"modprobe", "-r", "sctp", NULL};
+
+	if (rds_flag == 1) {
+		unload_res = tst_run_cmd(cmd, NULL, NULL, 1);
+		if (unload_res) {
+			/* We failed to unload sctp module(modprobe -r sctp)
+			 * and the operation failed with "FATAL: Module sctp is
+			 * in use." lsmod shows a reference count of 2 for sctp.
+			 * If we rebuild kernel with CONFIG_MODULE_FORCE_UNLOAD
+			 * enabled, we can succeed to unload sctp module
+			 * (rmmod -f sctp).
+			 */
+			tst_res(TINFO, "failed to unload sctp module, you can"
+				" reboot system to unload sctp after testing");
+		} else {
+			tst_res(TINFO, "succeeded to unload sctp modules");
+		}
+	}
+
+	if (sockfd > 0 && close(sockfd))
+		tst_res(TWARN | TERRNO, "failed to close file");
+}
+
+static void verify_sendto(void)
+{
+	TEST(sendto(sockfd, NULL, 1, 0, (struct sockaddr *) &sa, sizeof(sa)));
+	if (TEST_RETURN != -1) {
+		tst_res(TFAIL, "sendto() succeeded unexpectedly");
+		return;
+	}
+
+	if (TEST_ERRNO == ENOMEM) {
+		tst_res(TFAIL | TTERRNO, "sendto() got unrepaired errno with"
+			" invalid buffer when sctp is selected in socket()");
+		return;
+	}
+
+	if (TEST_ERRNO == EFAULT) {
+		tst_res(TPASS | TTERRNO, "sendto() got repaired errno with"
+			" invalid buffer when sctp is selected in socket()");
+		return;
+	}
+
+	tst_res(TFAIL | TTERRNO, "sendto() got unexpected errno with invalid"
+		" buffer when sctp is selected in socket(), expected ENOMEM "
+		"or EFAULT");
+}
+
+static struct tst_test test = {
+	.tid = "sendto02",
+	.setup = setup,
+	.cleanup = cleanup,
+	.test_all = verify_sendto,
+};
-- 
1.8.3.1

[LTP] [PATCH] syscalls/sendto02.c: add new testcase

[Cyril Hrubis]

Hi!
> +#include <errno.h>
> +#include <sys/types.h>
> +#include <sys/socket.h>
> +
> +#include "tst_test.h"
> +
> +static int sockfd;
> +static int rds_flag;
> +static struct sockaddr_in sa;
> +
> +static void setup(void)
> +{
> +	int acc_res, load_res;
> +	const char *cmd[] = {"modprobe", "sctp", NULL};
> +
> +	acc_res = access("/proc/sys/net/sctp", F_OK);
> +	if (acc_res == -1 && errno != ENOENT)
> +		tst_brk(TFAIL | TERRNO, "failed to check stcp module");
                                         ^
I would make the error message as specific as possible here, so I would
print what exactly has failed:

tst_brk(TBROK | TERRNO, "access(/proc/sys/net/sctp, F_OK)");

> +	if (acc_res == -1 && errno == ENOENT) {
> +		load_res = tst_run_cmd(cmd, NULL, NULL, 1);
> +		if (load_res) {
> +			tst_brk(TCONF, "failed to loaded sctp module, "
                                                    ^
						    load
> +				"so sctp modeule was not support by system");
                                           ^
					  module

Also I would say that all that needs to be printed here is the first
part of the string, i.e. "failed to load sctp module".

> +		}
> +
> +		tst_res(TINFO, "succeeded to load sctp module");
> +		rds_flag = 1;
                 ^
		 It would be a bit better to name it
		 rds_module_loaded or just module_loaded
> +	}
> +
> +	tst_res(TINFO, "sctp module was supported by system");

Just omit this message, it's misleading anyway.

> +	sockfd = SAFE_SOCKET(AF_INET, SOCK_STREAM, 132 /*IPPROTO_SCTP*/);
                                                    ^
						    This is way too ugly.

If IPPROTO_SCTP is not supported on you system you should add a fallback
definition such as:

#ifndef IPPROTO_SCTP
# define IPPROTO_SCTP 132
#endif

Also you didn't even try to include the <netinet/in.h> header that
contains this definition, so it's quite likely that all you need to do
is to include this header as well.

> +	memset(&sa, 0, sizeof(sa));
> +	sa.sin_family = AF_INET;
> +	sa.sin_addr.s_addr = inet_addr("127.0.0.1");
> +	sa.sin_port = htons(11111);
> +}
> +
> +static void cleanup(void)
> +{
> +	int unload_res;
> +	const char *cmd[] = {"modprobe", "-r", "sctp", NULL};
> +
> +	if (rds_flag == 1) {
> +		unload_res = tst_run_cmd(cmd, NULL, NULL, 1);
> +		if (unload_res) {
> +			/* We failed to unload sctp module(modprobe -r sctp)
> +			 * and the operation failed with "FATAL: Module sctp is
> +			 * in use." lsmod shows a reference count of 2 for sctp.
> +			 * If we rebuild kernel with CONFIG_MODULE_FORCE_UNLOAD
> +			 * enabled, we can succeed to unload sctp module
> +			 * (rmmod -f sctp).
> +			 */
> +			tst_res(TINFO, "failed to unload sctp module, you can"
> +				" reboot system to unload sctp after testing");

Isn't the problem here that you have to close the sctp socket *berofe*
you try to remove the module?

It's pretty clear that the module would be in use at least until the
socked is closed.

> +		} else {
> +			tst_res(TINFO, "succeeded to unload sctp modules");
> +		}
> +	}
> +
> +	if (sockfd > 0 && close(sockfd))
> +		tst_res(TWARN | TERRNO, "failed to close file");
> +}
> +
> +static void verify_sendto(void)
> +{
> +	TEST(sendto(sockfd, NULL, 1, 0, (struct sockaddr *) &sa, sizeof(sa)));
> +	if (TEST_RETURN != -1) {
> +		tst_res(TFAIL, "sendto() succeeded unexpectedly");
> +		return;
> +	}
> +
> +	if (TEST_ERRNO == ENOMEM) {
> +		tst_res(TFAIL | TTERRNO, "sendto() got unrepaired errno with"
> +			" invalid buffer when sctp is selected in socket()");

This message is absurdly long as well. Be short and to the point.

Something as:

	tst_res(TFAIL | TTERRNO, "sendto(fd, NULL, ...) failed unexpectedly");

> +		return;
> +	}
> +
> +	if (TEST_ERRNO == EFAULT) {
> +		tst_res(TPASS | TTERRNO, "sendto() got repaired errno with"
> +			" invalid buffer when sctp is selected in socket()");

Here as well. Shorten the message to something more reasonable.

> +		return;
> +	}
> +
> +	tst_res(TFAIL | TTERRNO, "sendto() got unexpected errno with invalid"
> +		" buffer when sctp is selected in socket(), expected ENOMEM "
> +		"or EFAULT");

Why do we have special case for errno != ENOMEM && errno != EFAULT. We
fail the testcase if the errno is not EFAULT anyway. There is no reason
to complicate the code like this.

> +}
> +
> +static struct tst_test test = {
> +	.tid = "sendto02",
> +	.setup = setup,
> +	.cleanup = cleanup,
> +	.test_all = verify_sendto,
> +};
> -- 
> 1.8.3.1
> 
> 
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] syscalls/sendto02.c: add new testcase

[Xiao Yang]

Hi cyril

Thanks for your review.
I will rewrite this pacth as you said, but i still have some doubts.
Please see the following comment.

On 2016/11/02 0:20, Cyril Hrubis wrote:
> Hi!
>> +#include<errno.h>
>> +#include<sys/types.h>
>> +#include<sys/socket.h>
>> +
>> +#include "tst_test.h"
>> +
>> +static int sockfd;
>> +static int rds_flag;
>> +static struct sockaddr_in sa;
>> +
>> +static void setup(void)
>> +{
>> +	int acc_res, load_res;
>> +	const char *cmd[] = {"modprobe", "sctp", NULL};
>> +
>> +	acc_res = access("/proc/sys/net/sctp", F_OK);
>> +	if (acc_res == -1&&  errno != ENOENT)
>> +		tst_brk(TFAIL | TERRNO, "failed to check stcp module");
>                                           ^
> I would make the error message as specific as possible here, so I would
> print what exactly has failed:
>
> tst_brk(TBROK | TERRNO, "access(/proc/sys/net/sctp, F_OK)");
>
>> +	if (acc_res == -1&&  errno == ENOENT) {
>> +		load_res = tst_run_cmd(cmd, NULL, NULL, 1);
>> +		if (load_res) {
>> +			tst_brk(TCONF, "failed to loaded sctp module, "
>                                                      ^
> 						    load
>> +				"so sctp modeule was not support by system");
>                                             ^
> 					  module
>
> Also I would say that all that needs to be printed here is the first
> part of the string, i.e. "failed to load sctp module".
>
>> +		}
>> +
>> +		tst_res(TINFO, "succeeded to load sctp module");
>> +		rds_flag = 1;
>                   ^
> 		 It would be a bit better to name it
> 		 rds_module_loaded or just module_loaded
>> +	}
>> +
>> +	tst_res(TINFO, "sctp module was supported by system");
> Just omit this message, it's misleading anyway.
>
>> +	sockfd = SAFE_SOCKET(AF_INET, SOCK_STREAM, 132 /*IPPROTO_SCTP*/);
>                                                      ^
> 						    This is way too ugly.
>
> If IPPROTO_SCTP is not supported on you system you should add a fallback
> definition such as:
>
> #ifndef IPPROTO_SCTP
> # define IPPROTO_SCTP 132
> #endif
>
> Also you didn't even try to include the<netinet/in.h>  header that
> contains this definition, so it's quite likely that all you need to do
> is to include this header as well.
>
>> +	memset(&sa, 0, sizeof(sa));
>> +	sa.sin_family = AF_INET;
>> +	sa.sin_addr.s_addr = inet_addr("127.0.0.1");
>> +	sa.sin_port = htons(11111);
>> +}
>> +
>> +static void cleanup(void)
>> +{
>> +	int unload_res;
>> +	const char *cmd[] = {"modprobe", "-r", "sctp", NULL};
>> +
>> +	if (rds_flag == 1) {
>> +		unload_res = tst_run_cmd(cmd, NULL, NULL, 1);
>> +		if (unload_res) {
>> +			/* We failed to unload sctp module(modprobe -r sctp)
>> +			 * and the operation failed with "FATAL: Module sctp is
>> +			 * in use." lsmod shows a reference count of 2 for sctp.
>> +			 * If we rebuild kernel with CONFIG_MODULE_FORCE_UNLOAD
>> +			 * enabled, we can succeed to unload sctp module
>> +			 * (rmmod -f sctp).
>> +			 */
>> +			tst_res(TINFO, "failed to unload sctp module, you can"
>> +				" reboot system to unload sctp after testing");
> Isn't the problem here that you have to close the sctp socket *berofe*
> you try to remove the module?
>
> It's pretty clear that the module would be in use at least until the
> socked is closed.
>
this is not the problem here.  the module was in use even we have closed 
sctp socket before removing the module.
the sctp module was in use because of some references.
Please see the following url for detailed information:
http://www.spinics.net/lists/linux-sctp/msg02137.html

Thanks,
Xiao Yang
>> +		} else {
>> +			tst_res(TINFO, "succeeded to unload sctp modules");
>> +		}
>> +	}
>> +
>> +	if (sockfd>  0&&  close(sockfd))
>> +		tst_res(TWARN | TERRNO, "failed to close file");
>> +}
>> +
>> +static void verify_sendto(void)
>> +{
>> +	TEST(sendto(sockfd, NULL, 1, 0, (struct sockaddr *)&sa, sizeof(sa)));
>> +	if (TEST_RETURN != -1) {
>> +		tst_res(TFAIL, "sendto() succeeded unexpectedly");
>> +		return;
>> +	}
>> +
>> +	if (TEST_ERRNO == ENOMEM) {
>> +		tst_res(TFAIL | TTERRNO, "sendto() got unrepaired errno with"
>> +			" invalid buffer when sctp is selected in socket()");
> This message is absurdly long as well. Be short and to the point.
>
> Something as:
>
> 	tst_res(TFAIL | TTERRNO, "sendto(fd, NULL, ...) failed unexpectedly");
>
>> +		return;
>> +	}
>> +
>> +	if (TEST_ERRNO == EFAULT) {
>> +		tst_res(TPASS | TTERRNO, "sendto() got repaired errno with"
>> +			" invalid buffer when sctp is selected in socket()");
> Here as well. Shorten the message to something more reasonable.
>
>> +		return;
>> +	}
>> +
>> +	tst_res(TFAIL | TTERRNO, "sendto() got unexpected errno with invalid"
>> +		" buffer when sctp is selected in socket(), expected ENOMEM "
>> +		"or EFAULT");
> Why do we have special case for errno != ENOMEM&&  errno != EFAULT. We
> fail the testcase if the errno is not EFAULT anyway. There is no reason
> to complicate the code like this.
>> +}
>> +
>> +static struct tst_test test = {
>> +	.tid = "sendto02",
>> +	.setup = setup,
>> +	.cleanup = cleanup,
>> +	.test_all = verify_sendto,
>> +};
>> -- 
>> 1.8.3.1
>>
>>
>>
>>
>> -- 
>> Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH v2] syscalls/sendto02.c: add new testcase

[Xiao Yang]

When sctp protocol is selected in socket(2) and buffer is invalid,
sendto(2) should fail and set errno to EFAULT, but it sets errno
to ENOMEM.

This is a regression test and has been fixed by kernel patch:
'commit 6e51fe757259 ("sctp: fix -ENOMEM result with invalid
user space pointer in sendto() syscall")'

Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
---
 runtest/syscalls                            |  1 +
 testcases/kernel/syscalls/.gitignore        |  1 +
 testcases/kernel/syscalls/sendto/sendto02.c | 91 +++++++++++++++++++++++++++++
 3 files changed, 93 insertions(+)
 create mode 100644 testcases/kernel/syscalls/sendto/sendto02.c

diff --git a/runtest/syscalls b/runtest/syscalls
index 7c84296..a9da8ca 100644
--- a/runtest/syscalls
+++ b/runtest/syscalls
@@ -1006,6 +1006,7 @@ sendmsg01 sendmsg01
 sendmsg02 sendmsg02
 
 sendto01 sendto01
+sendto02 sendto02
 
 setdomainname01	setdomainname01
 setdomainname02	setdomainname02
diff --git a/testcases/kernel/syscalls/.gitignore b/testcases/kernel/syscalls/.gitignore
index f53cc05..eb9bf5c 100644
--- a/testcases/kernel/syscalls/.gitignore
+++ b/testcases/kernel/syscalls/.gitignore
@@ -814,6 +814,7 @@
 /sendmsg/sendmsg01
 /sendmsg/sendmsg02
 /sendto/sendto01
+/sendto/sendto02
 /set_robust_list/set_robust_list01
 /set_thread_area/set_thread_area01
 /set_thread_area/set_thread_area02
diff --git a/testcases/kernel/syscalls/sendto/sendto02.c b/testcases/kernel/syscalls/sendto/sendto02.c
new file mode 100644
index 0000000..2d5887d
--- /dev/null
+++ b/testcases/kernel/syscalls/sendto/sendto02.c
@@ -0,0 +1,91 @@
+/*
+ * Copyright(c) 2016 Fujitsu Ltd.
+ * Author: Xiao Yang <yangx.jy@cn.fujitsu.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of version 2 of the GNU General Public License as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it would be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
+ *
+ * You should have received a copy of the GNU General Public License
+ * alone with this program.
+ */
+
+/*
+ * Test Name: sendto02
+ *
+ * Description:
+ * When sctp protocol is selected in socket(2) and buffer is invalid,
+ * sendto(2) should fail and set errno to EFAULT, but it sets errno
+ * to ENOMEM.
+ *
+ * This is a regression test and has been fixed by kernel commit:
+ * 6e51fe7572590d8d86e93b547fab6693d305fd0d (sctp: fix -ENOMEM result
+ * with invalid user space pointer in sendto() syscall)
+ */
+
+#include <errno.h>
+#include <string.h>
+#include <unistd.h>
+#include <sys/types.h>
+#include <sys/socket.h>
+#include <netinet/in.h>
+
+#include "tst_test.h"
+
+#ifndef IPPROTO_SCTP
+# define IPPROTO_SCTP	132
+#endif
+
+static int sockfd;
+static struct sockaddr_in sa;
+
+static void setup(void)
+{
+	sockfd = socket(AF_INET, SOCK_STREAM, IPPROTO_SCTP);
+	if (sockfd == -1) {
+		if (errno == EPROTONOSUPPORT)
+			tst_brk(TCONF, "sctp protocol was not supported");
+		else
+			tst_brk(TBROK | TERRNO, "socket() failed with sctp");
+	}
+
+	memset(&sa, 0, sizeof(sa));
+	sa.sin_family = AF_INET;
+	sa.sin_addr.s_addr = inet_addr("127.0.0.1");
+	sa.sin_port = htons(11111);
+}
+
+static void cleanup(void)
+{
+	if (sockfd > 0 && close(sockfd))
+		tst_res(TWARN | TERRNO, "failed to close file");
+}
+
+static void verify_sendto(void)
+{
+	TEST(sendto(sockfd, NULL, 1, 0, (struct sockaddr *) &sa, sizeof(sa)));
+	if (TEST_RETURN != -1) {
+		tst_res(TFAIL, "sendto(fd, NULL, ...) succeeded unexpectedly");
+		return;
+	}
+
+	if (TEST_ERRNO == EFAULT) {
+		tst_res(TPASS | TTERRNO,
+			"sendto(fd, NULL, ...) failed expectedly");
+		return;
+	}
+
+	tst_res(TFAIL | TTERRNO,
+		"sendto(fd, NULL, ...) failed unexpectedly, expected EFAULT");
+}
+
+static struct tst_test test = {
+	.tid = "sendto02",
+	.setup = setup,
+	.cleanup = cleanup,
+	.test_all = verify_sendto,
+};
-- 
1.8.3.1

[LTP] [PATCH] syscalls/sendto02.c: add new testcase

[Xiao Yang]

Hi cyril

I find that system would load sctp module when calling scoket(..., 
IPPROTO_SCTP).
If sctp module is not supported by system, it returns EPROTONOSUPPORT.
I will remove loading/unloading code.

Thanks,
Xiao Yang

On 2016/11/02 0:20, Cyril Hrubis wrote:
> Hi!
>> +#include<errno.h>
>> +#include<sys/types.h>
>> +#include<sys/socket.h>
>> +
>> +#include "tst_test.h"
>> +
>> +static int sockfd;
>> +static int rds_flag;
>> +static struct sockaddr_in sa;
>> +
>> +static void setup(void)
>> +{
>> +	int acc_res, load_res;
>> +	const char *cmd[] = {"modprobe", "sctp", NULL};
>> +
>> +	acc_res = access("/proc/sys/net/sctp", F_OK);
>> +	if (acc_res == -1&&  errno != ENOENT)
>> +		tst_brk(TFAIL | TERRNO, "failed to check stcp module");
>                                           ^
> I would make the error message as specific as possible here, so I would
> print what exactly has failed:
>
> tst_brk(TBROK | TERRNO, "access(/proc/sys/net/sctp, F_OK)");
>
>> +	if (acc_res == -1&&  errno == ENOENT) {
>> +		load_res = tst_run_cmd(cmd, NULL, NULL, 1);
>> +		if (load_res) {
>> +			tst_brk(TCONF, "failed to loaded sctp module, "
>                                                      ^
> 						    load
>> +				"so sctp modeule was not support by system");
>                                             ^
> 					  module
>
> Also I would say that all that needs to be printed here is the first
> part of the string, i.e. "failed to load sctp module".
>
>> +		}
>> +
>> +		tst_res(TINFO, "succeeded to load sctp module");
>> +		rds_flag = 1;
>                   ^
> 		 It would be a bit better to name it
> 		 rds_module_loaded or just module_loaded
>> +	}
>> +
>> +	tst_res(TINFO, "sctp module was supported by system");
> Just omit this message, it's misleading anyway.
>
>> +	sockfd = SAFE_SOCKET(AF_INET, SOCK_STREAM, 132 /*IPPROTO_SCTP*/);
>                                                      ^
> 						    This is way too ugly.
>
> If IPPROTO_SCTP is not supported on you system you should add a fallback
> definition such as:
>
> #ifndef IPPROTO_SCTP
> # define IPPROTO_SCTP 132
> #endif
>
> Also you didn't even try to include the<netinet/in.h>  header that
> contains this definition, so it's quite likely that all you need to do
> is to include this header as well.
>
>> +	memset(&sa, 0, sizeof(sa));
>> +	sa.sin_family = AF_INET;
>> +	sa.sin_addr.s_addr = inet_addr("127.0.0.1");
>> +	sa.sin_port = htons(11111);
>> +}
>> +
>> +static void cleanup(void)
>> +{
>> +	int unload_res;
>> +	const char *cmd[] = {"modprobe", "-r", "sctp", NULL};
>> +
>> +	if (rds_flag == 1) {
>> +		unload_res = tst_run_cmd(cmd, NULL, NULL, 1);
>> +		if (unload_res) {
>> +			/* We failed to unload sctp module(modprobe -r sctp)
>> +			 * and the operation failed with "FATAL: Module sctp is
>> +			 * in use." lsmod shows a reference count of 2 for sctp.
>> +			 * If we rebuild kernel with CONFIG_MODULE_FORCE_UNLOAD
>> +			 * enabled, we can succeed to unload sctp module
>> +			 * (rmmod -f sctp).
>> +			 */
>> +			tst_res(TINFO, "failed to unload sctp module, you can"
>> +				" reboot system to unload sctp after testing");
> Isn't the problem here that you have to close the sctp socket *berofe*
> you try to remove the module?
>
> It's pretty clear that the module would be in use at least until the
> socked is closed.
>
>> +		} else {
>> +			tst_res(TINFO, "succeeded to unload sctp modules");
>> +		}
>> +	}
>> +
>> +	if (sockfd>  0&&  close(sockfd))
>> +		tst_res(TWARN | TERRNO, "failed to close file");
>> +}
>> +
>> +static void verify_sendto(void)
>> +{
>> +	TEST(sendto(sockfd, NULL, 1, 0, (struct sockaddr *)&sa, sizeof(sa)));
>> +	if (TEST_RETURN != -1) {
>> +		tst_res(TFAIL, "sendto() succeeded unexpectedly");
>> +		return;
>> +	}
>> +
>> +	if (TEST_ERRNO == ENOMEM) {
>> +		tst_res(TFAIL | TTERRNO, "sendto() got unrepaired errno with"
>> +			" invalid buffer when sctp is selected in socket()");
> This message is absurdly long as well. Be short and to the point.
>
> Something as:
>
> 	tst_res(TFAIL | TTERRNO, "sendto(fd, NULL, ...) failed unexpectedly");
>
>> +		return;
>> +	}
>> +
>> +	if (TEST_ERRNO == EFAULT) {
>> +		tst_res(TPASS | TTERRNO, "sendto() got repaired errno with"
>> +			" invalid buffer when sctp is selected in socket()");
> Here as well. Shorten the message to something more reasonable.
>
>> +		return;
>> +	}
>> +
>> +	tst_res(TFAIL | TTERRNO, "sendto() got unexpected errno with invalid"
>> +		" buffer when sctp is selected in socket(), expected ENOMEM "
>> +		"or EFAULT");
> Why do we have special case for errno != ENOMEM&&  errno != EFAULT. We
> fail the testcase if the errno is not EFAULT anyway. There is no reason
> to complicate the code like this.
>
>> +}
>> +
>> +static struct tst_test test = {
>> +	.tid = "sendto02",
>> +	.setup = setup,
>> +	.cleanup = cleanup,
>> +	.test_all = verify_sendto,
>> +};
>> -- 
>> 1.8.3.1
>>
>>
>>
>>
>> -- 
>> Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH] syscalls/sendto02.c: add new testcase

[Cyril Hrubis]

Hi!
> I find that system would load sctp module when calling scoket(..., 
> IPPROTO_SCTP).
> If sctp module is not supported by system, it returns EPROTONOSUPPORT.
> I will remove loading/unloading code.

I started to wonder about that as I looked at the v2 patch. Sounds
better than loading/unloading the module explicitly.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v2] syscalls/sendto02.c: add new testcase

[Cyril Hrubis]

Hi!
Pushed, thanks.

-- 
Cyril Hrubis
chrubis@suse.cz