[1/2] signalfd01: Refactor old case with new API

signalfd01 is written in old API and some code seems unreachable. It needs
to be refactored with new API to simplify the logic.

Signed-off-by: Ma Xinjian <maxj.fnst@fujitsu.com>
---
 .../kernel/syscalls/signalfd/signalfd01.c     | 350 +++++-------------
 1 file changed, 96 insertions(+), 254 deletions(-)

Hi!

On 5/28/24 12:32, Ma Xinjian via ltp wrote:
> signalfd01 is written in old API and some code seems unreachable. It needs
> to be refactored with new API to simplify the logic.
>
> Signed-off-by: Ma Xinjian <maxj.fnst@fujitsu.com>
> ---
>   .../kernel/syscalls/signalfd/signalfd01.c     | 350 +++++-------------
>   1 file changed, 96 insertions(+), 254 deletions(-)
>
> diff --git a/testcases/kernel/syscalls/signalfd/signalfd01.c b/testcases/kernel/syscalls/signalfd/signalfd01.c
> index 8fb16800f..f3c305194 100644
> --- a/testcases/kernel/syscalls/signalfd/signalfd01.c
> +++ b/testcases/kernel/syscalls/signalfd/signalfd01.c
> @@ -1,302 +1,144 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
>   /*
> - *
> - *   Copyright (c) Red Hat Inc., 2008
> - *
> - *   This program is free software;  you can redistribute it and/or modify
> - *   it under the terms of the GNU General Public License as published by
> - *   the Free Software Foundation; either version 2 of the License, or
> - *   (at your option) any later version.
> - *
> - *   This program is distributed in the hope that it will be useful,
> - *   but WITHOUT ANY WARRANTY;  without even the implied warranty of
> - *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See
> - *   the GNU General Public License for more details.
> - *
> - *   You should have received a copy of the GNU General Public License
> - *   along with this program;  if not, write to the Free Software
> - *   Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> + * Copyright (c) Red Hat Inc., 2008
> + * Copyright (c) Linux Test Project, 2006-2024
>    */
>   
> -/*
> - * NAME
> - *	signalfd01.c
> +/*\
> + * [Description]
>    *
> - * DESCRIPTION
> - *	Check signalfd can receive signals
> + * Verify that signalfd() can receive signals
This description is a bit too simple and we can describe what test does 
in particular.
>    *
> - * USAGE
> - * 	signalfd01
> - *
> - * HISTORY
> - *	9/2008 Initial version by Masatake YAMATO <yamato@redhat.com>
> - *
> - * RESTRICTIONS
> - * 	None
>    */
> -#define _GNU_SOURCE
> -
> -#include "config.h"
>   
> -#include "test.h"
> +#define _GNU_SOURCE
_GNU_SOURCE is not needed, since signalfd() is a standard feature and no 
other parts of the test require nonstandard features.
>   
> -#include <errno.h>
>   #include <signal.h>
> -#include <unistd.h>
> -#include <fcntl.h>
> -#include <inttypes.h>
> -#include "ltp_signal.h"
> -
> -TCID_DEFINE(signalfd01);
> -int TST_TOTAL = 1;
> +#include "tst_test.h"
> +#include "config.h"
>   
>   #ifndef HAVE_SIGNALFD
> -#define  USE_STUB
> +#define USE_STUB
signalfd() is present since Linux 2.6.22 and glibc 2.8, which are below 
our minimum supported versions.
For this reason, we don't need any stub function definition.

Please take a look at 
https://linux-test-project.readthedocs.io/en/latest/users/supported_systems.html
>   #endif
>   
> -#if defined HAVE_SYS_SIGNALFD_H
> +#ifdef HAVE_SYS_SIGNALFD_H
No need for this check, for the same reason above. Maybe it can be 
removed from autotools configuration as well.
>   #include <sys/signalfd.h>
>   #elif defined HAVE_LINUX_SIGNALFD_H

not needed for the same reason above.
>   #include <linux/signalfd.h>
>   #define USE_OWNIMPL
not needed for the same reason above.
>   #else
> -#define  USE_STUB
> -#endif
> +#define USE_STUB
> +#endif /* HAVE_SYS_SIGNALFD_H */
>   
>   #ifndef HAVE_STRUCT_SIGNALFD_SIGINFO_SSI_SIGNO

not needed for the same reason above.
>   #define USE_STUB
> -#endif
> +#endif /* HAVE_STRUCT_SIGNALFD_SIGINFO_SSI_SIGNO */
>   
>   #ifdef USE_STUB
> -int main(void)
> -{
> -	tst_brkm(TCONF, NULL, "System doesn't support execution of the test");
> -}
> -
> -#else
> +TST_TEST_TCONF("This system does not support signalfd()");

not needed for the same reason above.
> +#else /* USE_STUB */
>   #if defined USE_OWNIMPL
>   #include "lapi/syscalls.h"
> -int signalfd(int fd, const sigset_t * mask, int flags)
> +int signalfd(int fd, const sigset_t *mask, int flags)
>   {
> -	/* Taken from GLIBC. */
> -	return tst_syscall(__NR_signalfd, fd, mask, SIGSETSIZE);
> +	return tst_syscall(__NR_signalfd, fd, mask, flags);
>   }
> -#endif
> +#endif /* USE_OWNIMPL */
>   
> -void cleanup(void);
> -void setup(void);
> +static int fd_signal = -1;
> +static sigset_t mask1;
> +static sigset_t mask2;
>   
> -int do_test1(uint32_t sig)
> +static int check_signal(int fd, uint32_t signal)
>   {
> -	int sfd_for_next;
> -	int sfd;
> -	sigset_t mask;
> -	pid_t pid;
> -	struct signalfd_siginfo fdsi;
> -	ssize_t s;
> -
> -	sigemptyset(&mask);
> -	sigaddset(&mask, sig);
> -	if (sigprocmask(SIG_BLOCK, &mask, NULL) < 0) {
> -		tst_brkm(TBROK, cleanup,
> -			 "sigprocmask() Failed: errno=%d : %s",
> -			 errno, strerror(errno));
> -	}
> -
> -	TEST(signalfd(-1, &mask, 0));
> -
> -	if ((sfd = TEST_RETURN) == -1) {
> -		tst_resm(TFAIL,
> -			 "signalfd() Failed, errno=%d : %s",
> -			 TEST_ERRNO, strerror(TEST_ERRNO));
> -		sfd_for_next = -1;
> -		return sfd_for_next;
> -
> -	} else {
> -		tst_resm(TPASS, "signalfd is created successfully");
> -		sfd_for_next = sfd;
> -		goto out;
> -	}
> -
> -	if (fcntl(sfd, F_SETFL, O_NONBLOCK) == -1) {
> -		close(sfd);
> -		tst_brkm(TBROK, cleanup,
> -			 "setting signalfd nonblocking mode failed: errno=%d : %s",
> -			 errno, strerror(errno));
> -	}
> -
> -	pid = getpid();
> -	if (kill(pid, sig) == -1) {
> -		close(sfd);
> -		tst_brkm(TBROK, cleanup,
> -			 "kill(self, %s) failed: errno=%d : %s",
> -			 strsignal(sig), errno, strerror(errno));
> -	}
> -
> -	s = read(sfd, &fdsi, sizeof(struct signalfd_siginfo));
> -	if ((s > 0) && (s != sizeof(struct signalfd_siginfo))) {
> -		tst_resm(TFAIL,
> -			 "getting incomplete signalfd_siginfo data: "
> -			 "actual-size=%zd, expected-size=%zu",
> -			 s, sizeof(struct signalfd_siginfo));
> -		sfd_for_next = -1;
> -		close(sfd);
> -		goto out;
> -	} else if (s < 0) {
> -		if (errno == EAGAIN) {
> -			tst_resm(TFAIL,
> -				 "signalfd_siginfo data is not delivered yet");
> -			sfd_for_next = -1;
> -			close(sfd);
> -			goto out;
> -		} else {
> -			close(sfd);
> -			tst_brkm(TBROK, cleanup,
> -				 "read signalfd_siginfo data failed: errno=%d : %s",
> -				 errno, strerror(errno));
> -		}
> -	} else if (s == 0) {
> -		tst_resm(TFAIL, "got EOF unexpectedly");
> -		sfd_for_next = -1;
> -		close(sfd);
> -		goto out;
> -	}
> -
> -	if (fdsi.ssi_signo == sig) {
> -		tst_resm(TPASS, "got expected signal");
> -		sfd_for_next = sfd;
> -		goto out;
> +	pid_t pid = getpid();
> +	ssize_t bytes;
> +	struct signalfd_siginfo siginfo;
> +	int ret = -1;
> +
> +	SAFE_KILL(pid, signal);
> +	bytes = read(fd, &siginfo, sizeof(siginfo));
We can just use SAFE_READ()
> +	if (bytes < 0) {
> +		if (errno == EAGAIN)
> +			tst_res(TFAIL | TERRNO, "%s",
> +				"signal has not been received yet");
> +		else
> +			tst_res(TFAIL | TERRNO, "%s",
> +				"error occured during reading signal");
>   	} else {
> -		tst_resm(TFAIL, "got unexpected signal: signal=%d : %s",
> -			 fdsi.ssi_signo, strsignal(fdsi.ssi_signo));
> -		sfd_for_next = -1;
> -		close(sfd);
> -		goto out;
> +		if (bytes != sizeof(siginfo))
Better to use TST_EXP_EQ_LI()
> +			tst_res(TFAIL | TERRNO, "%s",
> +				"signal received is not completed");
> +		else
> +			if (siginfo.ssi_signo != signal)
Better to use TST_EXP_EQ_LI()
> +				tst_res(TFAIL | TERRNO,
> +					"unexpected signal <%s>(%d) received",
> +					strsignal(siginfo.ssi_signo),
> +					siginfo.ssi_signo);
> +			else
> +				ret = 0;
>   	}

The whole function can be simplified as following:

     bytes = SAFE_READ(0, fd, &siginfo, sizeof(siginfo));
     TST_EXP_EQ_LI(bytes, sizeof(siginfo));
     TST_EXP_EQ_LI(siginfo.ssi_signo, signal);

> -
> -out:
> -	return sfd_for_next;
> +	return ret;
There's no need to return any value here, since we can directly handle 
TPASS.
>   }
>   
> -void do_test2(int fd, uint32_t sig)
> +static void setup(void)
>   {
> -	int sfd;
> -	sigset_t mask;
> -	pid_t pid;
> -	struct signalfd_siginfo fdsi;
> -	ssize_t s;
> -
> -	sigemptyset(&mask);
> -	sigaddset(&mask, sig);
> -	if (sigprocmask(SIG_BLOCK, &mask, NULL) < 0) {
> -		close(fd);
> -		tst_brkm(TBROK, cleanup,
> -			 "sigprocmask() Failed: errno=%d : %s",
> -			 errno, strerror(errno));
> -	}
> -
> -	TEST(signalfd(fd, &mask, 0));
> -
> -	if ((sfd = TEST_RETURN) == -1) {
> -		tst_resm(TFAIL,
> -			 "reassignment the file descriptor by signalfd() failed, errno=%d : %s",
> -			 TEST_ERRNO, strerror(TEST_ERRNO));
> -		return;
> -	} else if (sfd != fd) {
> -		tst_resm(TFAIL,
> -			 "different fd is returned in reassignment: expected-fd=%d, actual-fd=%d",
> -			 fd, sfd);
> -		close(sfd);
> -		return;
> -
> -	} else {
> -		tst_resm(TPASS, "signalfd is successfully reassigned");
> -		goto out;
> -	}
> -
> -	pid = getpid();
> -	if (kill(pid, sig) == -1) {
> -		close(sfd);
> -		tst_brkm(TBROK, cleanup,
> -			 "kill(self, %s) failed: errno=%d : %s",
> -			 strsignal(sig), errno, strerror(errno));
> -	}
> -
> -	s = read(sfd, &fdsi, sizeof(struct signalfd_siginfo));
> -	if ((s > 0) && (s != sizeof(struct signalfd_siginfo))) {
> -		tst_resm(TFAIL,
> -			 "getting incomplete signalfd_siginfo data: "
> -			 "actual-size=%zd, expected-size= %zu",
> -			 s, sizeof(struct signalfd_siginfo));
> -		goto out;
> -	} else if (s < 0) {
> -		if (errno == EAGAIN) {
> -			tst_resm(TFAIL,
> -				 "signalfd_siginfo data is not delivered yet");
> -			goto out;
> -		} else {
> -			close(sfd);
> -			tst_brkm(TBROK, cleanup,
> -				 "read signalfd_siginfo data failed: errno=%d : %s",
> -				 errno, strerror(errno));
> -		}
> -	} else if (s == 0) {
> -		tst_resm(TFAIL, "got EOF unexpectedly");
> -		goto out;
> -	}
> -
> -	if (fdsi.ssi_signo == sig) {
> -		tst_resm(TPASS, "got expected signal");
> -		goto out;
> -	} else {
> -		tst_resm(TFAIL, "got unexpected signal: signal=%d : %s",
> -			 fdsi.ssi_signo, strsignal(fdsi.ssi_signo));
> -		goto out;
> -	}
> -
> -out:
> -	return;
> +	SAFE_SIGEMPTYSET(&mask1);
> +	SAFE_SIGADDSET(&mask1, SIGUSR1);
> +	SAFE_SIGPROCMASK(SIG_BLOCK, &mask1, NULL);
> +	SAFE_SIGEMPTYSET(&mask2);
> +	SAFE_SIGADDSET(&mask2, SIGUSR2);
> +	SAFE_SIGPROCMASK(SIG_BLOCK, &mask2, NULL);
>   }
>   
> -int main(int argc, char **argv)
> +static void cleanup(void)
>   {
> -	int lc;
> -	int sfd;
> -
> -	tst_parse_opts(argc, argv, NULL, NULL);
> -
> -	setup();
> -	for (lc = 0; TEST_LOOPING(lc); lc++) {
> -		tst_count = 0;
> -
> -		sfd = do_test1(SIGUSR1);
> -		if (sfd < 0)
> -			continue;
> +	if (fd_signal > 0)
> +		SAFE_CLOSE(fd_signal);
> +}
>   
> -		do_test2(sfd, SIGUSR2);
> -		close(sfd);
> +static void do_create(void)
> +{
> +	TST_EXP_FD_SILENT(signalfd(fd_signal, &mask1, 0),
> +		"%s", "signalfd() create fd");
We want to test signalfd(), so we want to use TST_EXP_FD() instead.
> +	if (TST_RET > 0) {
> +		fd_signal = TST_RET;
> +		if (!check_signal(fd_signal, SIGUSR1))
> +			tst_res(TPASS, "%s",
> +				"signalfd() created fd successfully");

Better to handle TPASS inside check_signal()
In general, this whole test part would look better like this:

     TST_EXP_FD(signalfd(fd_signal, &mask1, 0));
     if (TST_RET == -1)
         return;

     fd_signal = TST_RET;
     check_signal(fd_signal, SIGUSR1);

>   	}
> -
> -	cleanup();
> -
> -	tst_exit();
>   }
>   
> -/*
> - * setup() - performs all the ONE TIME setup for this test.
> - */
> -void setup(void)
> +static void do_reassign(void)
>   {
> -
> -	TEST_PAUSE;
> +	if (fd_signal < 0)
> +		tst_brk(TBROK, "%s", "Invalid fd. Skip reassign");
> +
> +	TST_EXP_FD_SILENT(signalfd(fd_signal, &mask2, 0), "%s",
> +		"signalfd() reassign fd");
We want to test signalfd(), so we want to use TST_EXP_FD() instead.
> +	if (TST_RET > 0) {
> +		if (TST_RET == fd_signal) {
> +			if (!check_signal(fd_signal, SIGUSR2))
> +				tst_res(TPASS, "%s",
> +					"signalfd() reassigned fd "
> +					"successfully");
Better to handle TPASS inside check_signal()
> +		} else {
> +			tst_res(TFAIL | TERRNO, "%s",
> +				"fd changed unexpectedly during signalfd()");

No need to handle errors if we use TST_EXP_FD()

Same as before, it would look much better like this:


     TST_EXP_FD(signalfd(fd_signal, &mask2, 0), "%s", "signalfd() 
reassign fd");
     if (TST_RET == -1)
         return;

     TST_EXP_EQ_LI(TST_RET, fd_signal);
     check_signal(fd_signal, SIGUSR2);

> +		}
> +	}
>   }
>   
> -/*
> - * cleanup() - performs all the ONE TIME cleanup for this test at completion
> - * 	       or premature exit.
> - */
> -void cleanup(void)
> +static void verify_signalfd(void)
>   {
> -
> +	do_create();
> +	do_reassign();
No need to use 2 functions for this. They can be merged together
>   }
>   
> -#endif
> +static struct tst_test test = {
> +	.test_all = verify_signalfd,
> +	.setup = setup,
> +	.cleanup = cleanup,
> +};
> +
> +#endif /* USE_STUB */

Andrea