[LTP] [PATCH 1/2] signalfd01: Refactor old case with new API
Andrea Cervesato
andrea.cervesato@suse.com
Tue May 28 13:29:59 CEST 2024
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
More information about the ltp
mailing list