Re: [LTP] [PATCH] fcntl30.c: Convert the test to new LTP API

[Avinesh Kumar <akumar@suse.de>]

On Friday, June 2, 2023 1:02:49 PM IST Li Wang wrote:
> On Fri, Jun 2, 2023 at 3:14 PM Avinesh Kumar <akumar@suse.de> wrote:
> > Hi Li,
> > 
> > On Friday, June 2, 2023 8:10:11 AM IST Li Wang wrote:
> > > On Thu, Jun 1, 2023 at 7:22 PM Avinesh Kumar <akumar@suse.de> wrote:
> > > > Signed-off-by: Avinesh Kumar <akumar@suse.de>
> > > > ---
> > > > 
> > > >  testcases/kernel/syscalls/fcntl/fcntl30.c | 112
> > > >  +++++++---------------
> > > >  1 file changed, 36 insertions(+), 76 deletions(-)
> > > > 
> > > > diff --git a/testcases/kernel/syscalls/fcntl/fcntl30.c
> > > > b/testcases/kernel/syscalls/fcntl/fcntl30.c
> > > > index c4c3f81f1..62cc39e73 100644
> > > > --- a/testcases/kernel/syscalls/fcntl/fcntl30.c
> > > > +++ b/testcases/kernel/syscalls/fcntl/fcntl30.c
> > > > @@ -1,103 +1,63 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > 
> > > >  /*
> > > >  
> > > >   * Copyright (c) 2014 Fujitsu Ltd.
> > > >   * Author: Xiaoguang Wang <wangxg.fnst@cn.fujitsu.com>
> > > > 
> > > > - *
> > > > - * This program is free software; you can redistribute it and/or
> > 
> > modify
> > 
> > > > it
> > > > - * under the terms of version 2 of the GNU General Public License as
> > > > - * published by the Free Software Foundation.
> > > > - *
> > > > - * This program is distributed in the hope that it would be useful,
> > 
> > but
> > 
> > > > - * WITHOUT ANY WARRANTY; without even the implied warranty of
> > > > - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
> > > > - *
> > > > - * You should have received a copy of the GNU General Public License
> > > > along
> > > > - * with this program; if not, write the Free Software Foundation,
> > 
> > Inc.,
> > 
> > > > - * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
> > > > + * Copyright (c) 2023 SUSE LLC Avinesh Kumar <avinesh.kumar@suse.com>
> > > > 
> > > >   */
> > > > 
> > > > -/*
> > > > - * Description:
> > > > - * Verify that,
> > > > - *     Basic test for fcntl(2) using F_SETPIPE_SZ, F_GETPIPE_SZ
> > 
> > argument.
> > 
> > > > +/*\
> > > > + * [Description]
> > > > + *
> > > > + * Verify that, fetching and changing the capacity of a pipe works as
> > > > + * expected with fcntl(2) syscall using F_GETPIPE_SZ, F_SETPIPE_SZ
> > > > arguments.
> > > > 
> > > >   */
> > > > 
> > > > -#include <stdio.h>
> > > > -#include <errno.h>
> > > > -#include <unistd.h>
> > > > -#include <string.h>
> > > > -#include <signal.h>
> > > > -#include <sys/types.h>
> > > > -#include <pwd.h>
> > > > -
> > > > -#include "test.h"
> > > > -#include "safe_macros.h"
> > > > +#include "tst_test.h"
> > > > 
> > > >  #include "lapi/fcntl.h"
> > > > 
> > > > -char *TCID = "fcntl30";
> > > > -int TST_TOTAL = 1;
> > > > +static int fds[2];
> > > > +static int max_size_unpriv;
> > > > 
> > > > -static void setup(void);
> > > > -static void cleanup(void);
> > > > -
> > > > -int main(int ac, char **av)
> > > > +static void run(void)
> > > > 
> > > >  {
> > > > 
> > > > -       int lc;
> > > > -       int pipe_fds[2], test_fd;
> > > > -       int orig_pipe_size, new_pipe_size;
> > > > -
> > > > -
> > > > -       tst_parse_opts(ac, av, NULL, NULL);
> > > > +       int orig_size, new_size;
> > > > 
> > > > -       setup();
> > > > +       SAFE_PIPE(fds);
> > > > 
> > > > -       for (lc = 0; TEST_LOOPING(lc); lc++) {
> > > > -               tst_count = 0;
> > > > +       TST_EXP_POSITIVE(fcntl(fds[1], F_GETPIPE_SZ));
> > > > 
> > > > -               SAFE_PIPE(cleanup, pipe_fds);
> > > > -               test_fd = pipe_fds[1];
> > > > +       orig_size = TST_RET;
> > > > +       new_size = orig_size * 2;
> > > > 
> > > > -               TEST(fcntl(test_fd, F_GETPIPE_SZ));
> > > > -               if (TEST_RETURN < 0) {
> > > > -                       tst_brkm(TFAIL | TTERRNO, cleanup,
> > > > -                                "fcntl get pipe size failed");
> > > > -               }
> > > > +       if (new_size > max_size_unpriv)
> > > > +               tst_brk(TBROK, "Requested pipe size above the allowed
> > > > limit %d", max_size_unpriv);
> > > 
> > > It is possible that the pipe-max-size value was set to a smaller size
> > > so that the test gets failure here. But it would not be a problem.
> > 
> > Do you mean the EPERM scenario when unprivileged process trying to set the
> > size larger than the value in /proc/sys/fs/pipe-max-size? if yes, this is
> > being covered in fcntl37.
> 
> No,  as this file allows us to configure the maximum size of the
> buffer used for data transfer between processes through pipes.
> 
> Imagine that, if a test machine set it with a smaller size, this test
> will get a failure like:
okay, so instead we can just do ?

-       new_size = orig_size * 2;
+       new_size = max_size_unpriv;

this should always test the F_SETPIPE_SZ positive case.

> 
> # echo 8192 > /proc/sys/fs/pipe-max-size
> 
> # cat /proc/sys/fs/pipe-max-size
> 8192
> 
> # ./fcntl30
> tst_test.c:1560: TINFO: Timeout per run is 0h 00m 30s
> fcntl30.c:28: TPASS: fcntl(fds[1], F_GETPIPE_SZ) returned 65536
> fcntl30.c:34: TBROK: Requested pipe size above the allowed limit 8192
> 
> Summary:
> passed   1
> failed   0
> broken   1
> skipped  0
> warnings 0
> 
> > > I guess TCONF should be more proper.
> > 
> > I'm not sure if TCONF is more apt.
> > 
> > > Otherwise, LGTM.
> > > Reviewed-by: Li Wang <liwang@redhat.com>
> > 
> > Thank you for the review.
> > 
> > > > -               orig_pipe_size = TEST_RETURN;
> > > > -               new_pipe_size = orig_pipe_size * 2;
> > > > -               TEST(fcntl(test_fd, F_SETPIPE_SZ, new_pipe_size));
> > > > -               if (TEST_RETURN < 0) {
> > > > -                       tst_brkm(TFAIL | TTERRNO, cleanup,
> > > > -                                "fcntl test F_SETPIPE_SZ failed");
> > > > -               }
> > > > +       TST_EXP_POSITIVE(fcntl(fds[1], F_SETPIPE_SZ, new_size));
> > > > +       TST_EXP_POSITIVE(fcntl(fds[1], F_GETPIPE_SZ));
> > > > +       TST_EXP_EXPR(TST_RET >= new_size,
> > > > +                               "new pipe size (%ld) >= requested size
> > > > (%d)",
> > > > +                               TST_RET, new_size);
> > > > 
> > > > -               TEST(fcntl(test_fd, F_GETPIPE_SZ));
> > > > -               if (TEST_RETURN < 0) {
> > > > -                       tst_brkm(TFAIL | TTERRNO, cleanup,
> > > > -                                "fcntl test F_GETPIPE_SZ failed");
> > > > -               }
> > > > -               tst_resm(TINFO, "orig_pipe_size: %d new_pipe_size:
> > > > %d",
> > > > -                        orig_pipe_size, new_pipe_size);
> > > > -               if (TEST_RETURN >= new_pipe_size) {
> > > > -                       tst_resm(TPASS, "fcntl test F_GETPIPE_SZ and
> > > > F_SETPIPE_SZ passed");
> > > > -               } else {
> > > > -                       tst_resm(TFAIL, "fcntl test F_GETPIPE_SZ and
> > > > F_SETPIPE_SZ failed");
> > > > -               }
> > > > -               SAFE_CLOSE(cleanup, pipe_fds[0]);
> > > > -               SAFE_CLOSE(cleanup, pipe_fds[1]);
> > > > -       }
> > > > -
> > > > -       cleanup();
> > > > -       tst_exit();
> > > > +       SAFE_CLOSE(fds[0]);
> > > > +       SAFE_CLOSE(fds[1]);
> > > > 
> > > >  }
> > > >  
> > > >  static void setup(void)
> > > >  {
> > > > 
> > > > -       tst_sig(NOFORK, DEF_HANDLER, cleanup);
> > > > -
> > > > -       TEST_PAUSE;
> > > > +       SAFE_FILE_SCANF("/proc/sys/fs/pipe-max-size", "%d",
> > > > 
> > > >  &max_size_unpriv);
> > > >  }
> > > >  
> > > >  static void cleanup(void)
> > > >  {
> > > > 
> > > > +       if (fds[0] > 0)
> > > > +               SAFE_CLOSE(fds[0]);
> > > > +       if (fds[1] > 0)
> > > > +               SAFE_CLOSE(fds[1]);
> > > > 
> > > >  }
> > > > 
> > > > +
> > > > +static struct tst_test test = {
> > > > +       .test_all = run,
> > > > +       .setup = setup,
> > > > +       .cleanup = cleanup
> > > > +};
> > > > --
> > > > 2.40.1
> > > > 
> > > > 
> > > > --
> > > > Mailing list info: https://lists.linux.it/listinfo/ltp
> > 
> > --
> > Regards,
> > Avinesh





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