[GSOC][PATCH v2] tests/fcntl.c: add test for struct f_owner_ex

[Zhibin Li]

Thanks for pointing out the mistakes I've made and alternative ways to
simplify things. Actually I've thought less about avoiding duplication as
well as simplifying the code style. I'm getting used to it and I'll see
what I can do to avoid sending a bunch of duplicated patches(which is
annoying). The pace might be slow now but I'll keep learning.

On Tue, Mar 27, 2018 at 3:52 AM, Dmitry V. Levin <ldv at altlinux.org> wrote:

> On Tue, Mar 27, 2018 at 01:20:14AM +0800, Zhibin Li wrote:
> > Signed-off-by: Zhibin Li <08826794brmt at gmail.com>
> > ---
> >  tests/fcntl.c | 63 ++++++++++++++++++++++++++++++
> +++++++++++++++++++++++++++++
> >  1 file changed, 63 insertions(+)
> >
> > diff --git a/tests/fcntl.c b/tests/fcntl.c
> > index 4f62ca2a..8c74ed22 100644
> > --- a/tests/fcntl.c
> > +++ b/tests/fcntl.c
> > @@ -36,6 +36,9 @@
> >  # include "struct_flock.c"
> >
> >  # define TEST_FLOCK64_EINVAL(cmd) test_flock64_einval(cmd, #cmd)
> > +# define TEST_F_OWNER_EX_ESRCH(cmd) test_f_owner_ex_esrch(cmd, #cmd)
> > +
> > +typedef struct f_owner_ex struct_kernel_f_owner_ex;
>
> The new typedef is used only once, do you really need it now?
>
> >  static void
> >  test_flock64_einval(const int cmd, const char *name)
> > @@ -69,12 +72,72 @@ test_flock64(void)
> >  #endif
> >  }
> >
> > +static int
> > +test_f_owner_ex_type_pid(const int cmd, const char *const cmd_name,
> > +                             const int type, const char *const
> type_name,
> > +                             const pid_t pid)
> > +{
> > +     struct_kernel_f_owner_ex fo = { .type = type, .pid = pid };
> > +
> > +     long rc = invoke_test_syscall(cmd, &fo);
> > +     printf("%s(0, %s, {type=%s, pid=%d}) = %s\n",
> > +             TEST_SYSCALL_STR, cmd_name, type_name, pid, sprintrc(rc));
> > +     if (rc)
> > +             return 1;
> > +     return 0;
>
> There is a simple way to write this:
>         return !!rc;
>
> Alternatively, you can change the return type from "int" to "long"
> and just return rc.
>
> > +}
> > +
> > +static void
> > +test_f_owner_ex_esrch(const int cmd, const char *const cmd_name)
> > +{
> > +     test_f_owner_ex_type_pid(cmd, cmd_name,
> > +                                     ARG_STR(F_OWNER_TID), 1234567890);
> > +     test_f_owner_ex_type_pid(cmd, cmd_name,
> > +                                     ARG_STR(F_OWNER_PID), 1234567890);
> > +     test_f_owner_ex_type_pid(cmd, cmd_name,
> > +                                     ARG_STR(F_OWNER_PGRP),
> -1234567890);
>
> Something went wrong with the indentation in this function.
>
> > +}
> > +
> > +static void
> > +test_f_owner_ex(void)
> > +{
> > +     TEST_F_OWNER_EX_ESRCH(F_SETOWN_EX);
>
> Looks like this could be written without TEST_F_OWNER_EX_ESRCH macro:
>
>         test_f_owner_ex_esrch(ARG_STR(F_SETOWN_EX));
>
> > +
> > +     long rc;
> > +
> > +     rc = test_f_owner_ex_type_pid(ARG_STR(F_SETOWN_EX),
> > +                                     ARG_STR(F_OWNER_TID), 20);
> > +     if (rc)
> > +             return;
> > +
> > +     test_f_owner_ex_type_pid(ARG_STR(F_GETOWN_EX),
> > +                                     ARG_STR(F_OWNER_TID), 20);
> > +
> > +     rc = test_f_owner_ex_type_pid(ARG_STR(F_SETOWN_EX),
> > +                                     ARG_STR(F_OWNER_PID), 30);
> > +     if (rc)
> > +             return;
> > +
> > +     test_f_owner_ex_type_pid(ARG_STR(F_GETOWN_EX),
> > +                                     ARG_STR(F_OWNER_PID), 30);
> > +
> > +     rc = test_f_owner_ex_type_pid(ARG_STR(F_SETOWN_EX),
> > +                                     ARG_STR(F_OWNER_PGRP), 40);
> > +     if (rc)
> > +             return;
> > +
> > +     test_f_owner_ex_type_pid(ARG_STR(F_GETOWN_EX),
> > +                                     ARG_STR(F_OWNER_PGRP), 40);
>
> The same F_SETOWN_EX/check/F_GETOWN_EX pattern is repeated thrice here,
> wouldn't it be better if it was moved to a separate function?
>
>
> --
> ldv
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.strace.io/pipermail/strace-devel/attachments/20180328/547cbc8e/attachment.html>