| Message ID | 20240128024838.2699248-9-liwang@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | improvement work on libswap library | expand |
> This updates the is_swap_supported function in the libltpswap > to return an integer status instead of void, allowing the function > to communicate success or failure to the caller. It introduces > checks and returns 0 on various failure conditions while logging > the failure without aborting the test case. > Signed-off-by: Li Wang <liwang@redhat.com> > --- > include/libswap.h | 2 +- > libs/libltpswap/libswap.c | 28 ++++++++++++++++++---------- > 2 files changed, 19 insertions(+), 11 deletions(-) > diff --git a/include/libswap.h b/include/libswap.h > index e67d65756..1e09db031 100644 > --- a/include/libswap.h > +++ b/include/libswap.h > @@ -20,5 +20,5 @@ int make_swapfile(const char *swapfile, int blocks, int safe); > * Check swapon/swapoff support status of filesystems or files > * we are testing on. > */ Reviewed-by: Petr Vorel <pvorel@suse.cz> Thanks for further improving it. Few comments to fix old issues, when we are at it (and plan to merge after the release). But feel free to ignore. BTW do you plan to use it somewhere? testcases/kernel/syscalls/swapoff/swapoff01.c nit: I would comment here the return code. Also maybe use bool (from stdbool.h)? The advantage is that we don't think what the return code is (sometimes we use syscalls approach when 0 is success, otherwise - like here - 0 is failure). Slowly converting to bool would be the best. > -void is_swap_supported(const char *filename); > +int is_swap_supported(const char *filename); > #endif /* __LIBSWAP_H__ */ > diff --git a/libs/libltpswap/libswap.c b/libs/libltpswap/libswap.c > index 8aecad48d..14b7d9e3a 100644 > --- a/libs/libltpswap/libswap.c > +++ b/libs/libltpswap/libswap.c > @@ -168,7 +168,7 @@ int make_swapfile(const char *swapfile, int blocks, int safe) > * Check swapon/swapoff support status of filesystems or files > * we are testing on. > */ nit: Although I did not like doc being just at the header, I now prefer it now to have it just at the header because we don't have to sync the same text on two places. Kind regards, Petr > -void is_swap_supported(const char *filename) > +int is_swap_supported(const char *filename) > { > int i, sw_support = 0; > int ret = make_swapfile(filename, 10, 1); ...
Hi Petr, On Mon, Jan 29, 2024 at 4:03 PM Petr Vorel <pvorel@suse.cz> wrote: > > This updates the is_swap_supported function in the libltpswap > > to return an integer status instead of void, allowing the function > > to communicate success or failure to the caller. It introduces > > checks and returns 0 on various failure conditions while logging > > the failure without aborting the test case. > > > Signed-off-by: Li Wang <liwang@redhat.com> > > --- > > include/libswap.h | 2 +- > > libs/libltpswap/libswap.c | 28 ++++++++++++++++++---------- > > 2 files changed, 19 insertions(+), 11 deletions(-) > > > diff --git a/include/libswap.h b/include/libswap.h > > index e67d65756..1e09db031 100644 > > --- a/include/libswap.h > > +++ b/include/libswap.h > > @@ -20,5 +20,5 @@ int make_swapfile(const char *swapfile, int blocks, > int safe); > > * Check swapon/swapoff support status of filesystems or files > > * we are testing on. > > */ > Reviewed-by: Petr Vorel <pvorel@suse.cz> > > Thanks for further improving it. Few comments to fix old issues, when we > are at > it (and plan to merge after the release). But feel free to ignore. > > BTW do you plan to use it somewhere? > > testcases/kernel/syscalls/swapoff/swapoff01.c > > nit: I would comment here the return code. > Also maybe use bool (from stdbool.h)? The advantage is that we don't think > what > the return code is (sometimes we use syscalls approach when 0 is success, > otherwise - like here - 0 is failure). Slowly converting to bool would be > the > best. > Good point, actually I am tired of figuring out the return 0 means 'FAIL' or 'SUCCESS' in the lib, it messes a lot in some functions. And as you suggest, bool value should be a good way to resolve this. But the bool type is not built into the language prior to the C99 standard. The C99 standard introduced a _Bool type and the header <stdbool.h>, which defines bool as a macro for _Bool. I am not sure if LTP nowadays only supports C99 and later. or should we make use of bool safely (or use syscalls approach 0 == success) in our patch? @Cyril Hrubis <chrubis@suse.cz> what do you think? > > @@ -168,7 +168,7 @@ int make_swapfile(const char *swapfile, int blocks, > int safe) > > * Check swapon/swapoff support status of filesystems or files > > * we are testing on. > > */ > nit: Although I did not like doc being just at the header, I now prefer it > now > to have it just at the header because we don't have to sync the same text > on two > places. > Agree.
> Hi Petr, > On Mon, Jan 29, 2024 at 4:03 PM Petr Vorel <pvorel@suse.cz> wrote: > > > This updates the is_swap_supported function in the libltpswap > > > to return an integer status instead of void, allowing the function > > > to communicate success or failure to the caller. It introduces > > > checks and returns 0 on various failure conditions while logging > > > the failure without aborting the test case. > > > Signed-off-by: Li Wang <liwang@redhat.com> > > > --- > > > include/libswap.h | 2 +- > > > libs/libltpswap/libswap.c | 28 ++++++++++++++++++---------- > > > 2 files changed, 19 insertions(+), 11 deletions(-) > > > diff --git a/include/libswap.h b/include/libswap.h > > > index e67d65756..1e09db031 100644 > > > --- a/include/libswap.h > > > +++ b/include/libswap.h > > > @@ -20,5 +20,5 @@ int make_swapfile(const char *swapfile, int blocks, > > int safe); > > > * Check swapon/swapoff support status of filesystems or files > > > * we are testing on. > > > */ > > Reviewed-by: Petr Vorel <pvorel@suse.cz> > > Thanks for further improving it. Few comments to fix old issues, when we > > are at > > it (and plan to merge after the release). But feel free to ignore. > > BTW do you plan to use it somewhere? > > testcases/kernel/syscalls/swapoff/swapoff01.c > > nit: I would comment here the return code. > > Also maybe use bool (from stdbool.h)? The advantage is that we don't think > > what > > the return code is (sometimes we use syscalls approach when 0 is success, > > otherwise - like here - 0 is failure). Slowly converting to bool would be > > the > > best. > Good point, actually I am tired of figuring out the return 0 means 'FAIL' or > 'SUCCESS' in the lib, it messes a lot in some functions. And as you suggest, > bool value should be a good way to resolve this. > But the bool type is not built into the language prior to the C99 standard. > The C99 standard introduced a _Bool type and the header <stdbool.h>, > which defines bool as a macro for _Bool. > I am not sure if LTP nowadays only supports C99 and later. > or should we make use of bool safely (or use syscalls approach > 0 == success) in our patch? FYI now compile with -std=gnu99, see include/mk/config.mk.in dc7be30e2 ("config: Explicitly set gnu99") So I would say LTP supports C99 with GNU extensions. > @Cyril Hrubis <chrubis@suse.cz> what do you think? FYI we already use stdbool.h and bool on some places in the library. Kind regards, Petr > > > @@ -168,7 +168,7 @@ int make_swapfile(const char *swapfile, int blocks, > > int safe) > > > * Check swapon/swapoff support status of filesystems or files > > > * we are testing on. > > > */ > > nit: Although I did not like doc being just at the header, I now prefer it > > now > > to have it just at the header because we don't have to sync the same text > > on two > > places. > Agree.
diff --git a/include/libswap.h b/include/libswap.h index e67d65756..1e09db031 100644 --- a/include/libswap.h +++ b/include/libswap.h @@ -20,5 +20,5 @@ int make_swapfile(const char *swapfile, int blocks, int safe); * Check swapon/swapoff support status of filesystems or files * we are testing on. */ -void is_swap_supported(const char *filename); +int is_swap_supported(const char *filename); #endif /* __LIBSWAP_H__ */ diff --git a/libs/libltpswap/libswap.c b/libs/libltpswap/libswap.c index 8aecad48d..14b7d9e3a 100644 --- a/libs/libltpswap/libswap.c +++ b/libs/libltpswap/libswap.c @@ -168,7 +168,7 @@ int make_swapfile(const char *swapfile, int blocks, int safe) * Check swapon/swapoff support status of filesystems or files * we are testing on. */ -void is_swap_supported(const char *filename) +int is_swap_supported(const char *filename) { int i, sw_support = 0; int ret = make_swapfile(filename, 10, 1); @@ -188,23 +188,31 @@ void is_swap_supported(const char *filename) } if (ret != 0) { - if (fi_contiguous == 0 && sw_support == 0) + if (fi_contiguous == 0 && sw_support == 0) { tst_brk(TCONF, "mkswap on %s not supported", fstype); - else - tst_brk(TFAIL, "mkswap on %s failed", fstype); + } else { + tst_res(TFAIL, "mkswap on %s failed", fstype); + return 0; + } } TEST(tst_syscall(__NR_swapon, filename, 0)); if (TST_RET == -1) { - if (errno == EPERM) + if (errno == EPERM) { tst_brk(TCONF, "Permission denied for swapon()"); - else if (errno == EINVAL && fi_contiguous == 0 && sw_support == 0) + } else if (errno == EINVAL && fi_contiguous == 0 && sw_support == 0) { tst_brk(TCONF, "Swapfile on %s not implemented", fstype); - else - tst_brk(TFAIL | TTERRNO, "swapon() on %s failed", fstype); + } else { + tst_res(TFAIL | TTERRNO, "swapon() on %s failed", fstype); + return 0; + } } TEST(tst_syscall(__NR_swapoff, filename, 0)); - if (TST_RET == -1) - tst_brk(TFAIL | TTERRNO, "swapoff on %s failed", fstype); + if (TST_RET == -1) { + tst_res(TFAIL | TTERRNO, "swapoff on %s failed", fstype); + return 0; + } + + return (TST_RET == 0); }
This updates the is_swap_supported function in the libltpswap to return an integer status instead of void, allowing the function to communicate success or failure to the caller. It introduces checks and returns 0 on various failure conditions while logging the failure without aborting the test case. Signed-off-by: Li Wang <liwang@redhat.com> --- include/libswap.h | 2 +- libs/libltpswap/libswap.c | 28 ++++++++++++++++++---------- 2 files changed, 19 insertions(+), 11 deletions(-)