Re: [LTP] [PATCH v5 8/8] libswap: Refactor is_swap_supported function to return status

[Petr Vorel <pvorel@suse.cz>]

> 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.

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