Re: [LTP] [PATCH] inotify: clean up build and make check findings

[Andrea Cervesato via ltp <ltp@lists.linux.it>]

Hi!

> Fix various issues reported by `make` and `make check`:
> 
> - Drop ENOSYS-based logic and rely on TERRNO reporting
> - Silence known format-truncation warnings locally
> - Clean up string handling and permissions
> - Fix minor style and prototype issues

This is a lot of stuff for a single commit :-) we might need to separate
these changes into multiple commits.

> 
> Signed-off-by: Jinseok Kim <always.starving0@gmail.com>
> ---
>  testcases/kernel/syscalls/inotify/Makefile    |  1 +
>  testcases/kernel/syscalls/inotify/inotify.h   | 13 +++-------
>  testcases/kernel/syscalls/inotify/inotify01.c |  4 ++-
>  testcases/kernel/syscalls/inotify/inotify02.c | 26 ++++++++++---------
>  testcases/kernel/syscalls/inotify/inotify03.c |  5 ++--
>  testcases/kernel/syscalls/inotify/inotify04.c | 23 ++++++++--------
>  testcases/kernel/syscalls/inotify/inotify05.c |  2 +-
>  testcases/kernel/syscalls/inotify/inotify07.c | 16 ++++++------
>  testcases/kernel/syscalls/inotify/inotify08.c |  5 ++--
>  testcases/kernel/syscalls/inotify/inotify10.c | 18 +++++++------
>  10 files changed, 58 insertions(+), 55 deletions(-)
> 
> diff --git a/testcases/kernel/syscalls/inotify/Makefile b/testcases/kernel/syscalls/inotify/Makefile
> index 16d430864..57124c739 100644
> --- a/testcases/kernel/syscalls/inotify/Makefile
> +++ b/testcases/kernel/syscalls/inotify/Makefile
> @@ -4,6 +4,7 @@
>  top_srcdir		?= ../../../..
> 
>  include $(top_srcdir)/include/mk/testcases.mk
> +inotify02: CFLAGS+=-Wno-format-truncation

uhm? Is it really needed?

>  inotify09: CFLAGS+=-pthread
>  inotify09: LDLIBS+=-lrt
>  include $(top_srcdir)/include/mk/generic_leaf_target.mk
> diff --git a/testcases/kernel/syscalls/inotify/inotify.h b/testcases/kernel/syscalls/inotify/inotify.h
> index dbf814902..1bc36defa 100644
> --- a/testcases/kernel/syscalls/inotify/inotify.h
> +++ b/testcases/kernel/syscalls/inotify/inotify.h
> @@ -1,4 +1,4 @@
> -// SPDX-License-Identifier: GPL-2.0-or-later
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>  /*
>   * inotify testcase common definitions.
>   *
> @@ -33,14 +33,8 @@
>  static inline int safe_myinotify_init(const char *file, const int lineno, int fd)
>  {
>  	if (fd < 0) {
> -		if (errno == ENOSYS) {
> -			tst_brk(TCONF,
> -				"%s:%d: inotify is not configured in this kernel",

Here we are suppressing a TCONF and threating everything as TERRNO. This
needs to be reverted.

> +		tst_brk(TBROK | TERRNO, "%s:%d: inotify_init failed",
>  				file, lineno);
> -		} else {
> -			tst_brk(TBROK | TERRNO, "%s:%d: inotify_init failed",
> -				file, lineno);
> -		}
>  	}
>  	return fd;
>  }
> @@ -51,7 +45,8 @@ static inline int safe_myinotify_init(const char *file, const int lineno, int fd
>  #define SAFE_MYINOTIFY_INIT1(flags) \
>  	safe_myinotify_init(__FILE__, __LINE__, myinotify_init1(flags))
> 
> -static inline int safe_myinotify_watch(const char *file, const int lineno, int wd, int fd, const char* fname, const char* mask)
> +static inline int safe_myinotify_watch(const char *file, const int lineno, int wd,
> +				       int fd, const char *fname, const char *mask)
>  {
>  	if (wd < 0) {
>  		tst_brk(TBROK | TERRNO,
> diff --git a/testcases/kernel/syscalls/inotify/inotify01.c b/testcases/kernel/syscalls/inotify/inotify01.c
> index 972b1025e..12aadf862 100644
> --- a/testcases/kernel/syscalls/inotify/inotify01.c
> +++ b/testcases/kernel/syscalls/inotify/inotify01.c
> @@ -40,7 +40,7 @@ static unsigned int event_set[EVENT_MAX];
> 
>  static char event_buf[EVENT_BUF_LEN];
> 
> -void verify_inotify(void)
> +static void verify_inotify(void)
>  {
>  	int test_cnt = 0;
> 
> @@ -79,6 +79,7 @@ void verify_inotify(void)
>  	 * get list of events
>  	 */
>  	int len, i = 0, test_num = 0;
> +
>  	len = SAFE_READ(0, fd_notify, event_buf, EVENT_BUF_LEN);
> 
>  	/*
> @@ -86,6 +87,7 @@ void verify_inotify(void)
>  	 */
>  	while (i < len) {
>  		struct inotify_event *event;
> +
>  		event = (struct inotify_event *)&event_buf[i];
>  		if (test_num >= test_cnt) {
>  			tst_res(TFAIL,
> diff --git a/testcases/kernel/syscalls/inotify/inotify02.c b/testcases/kernel/syscalls/inotify/inotify02.c
> index 314c1bd49..5f288b270 100644
> --- a/testcases/kernel/syscalls/inotify/inotify02.c
> +++ b/testcases/kernel/syscalls/inotify/inotify02.c
> @@ -43,6 +43,7 @@ struct event_t {
>  	char name[BUF_SIZE];
>  	unsigned int mask;
>  };
> +
>  #define FILE_NAME1 "test_file1"
>  #define FILE_NAME2 "test_file2"
> 
> @@ -50,7 +51,7 @@ static struct event_t event_set[EVENT_MAX];
> 
>  static char event_buf[EVENT_BUF_LEN];
> 
> -void verify_inotify(void)
> +static void verify_inotify(void)
>  {
>  	unsigned int stored_cookie = UINT_MAX;
> 
> @@ -61,40 +62,40 @@ void verify_inotify(void)
>  	 */
>  	SAFE_CHMOD(".", 0755);
>  	event_set[test_cnt].mask = IN_ISDIR | IN_ATTRIB;
> -	strcpy(event_set[test_cnt].name, "");
> +	snprintf(event_set[test_cnt].name, sizeof(event_set[test_cnt].name), "%s", "");

This is quite an overkill. To reset a string in C you just need

  buff[0] = '\0';

And the same for the others.. also make sure that boundaries are
correct, because I have the feeling we might easily overbound memories
when using snprintf() here.

Kind regards,
--
Andrea Cervesato
SUSE QE Automation Engineer Linux
andrea.cervesato@suse.com

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp