<div dir="ltr">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.</div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Mar 27, 2018 at 3:52 AM, Dmitry V. Levin <span dir="ltr"><<a href="mailto:ldv@altlinux.org" target="_blank">ldv@altlinux.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On Tue, Mar 27, 2018 at 01:20:14AM +0800, Zhibin Li wrote:<br>
> Signed-off-by: Zhibin Li <<a href="mailto:08826794brmt@gmail.com">08826794brmt@gmail.com</a>><br>
> ---<br>
>  tests/fcntl.c | 63 ++++++++++++++++++++++++++++++<wbr>+++++++++++++++++++++++++++++<br>
>  1 file changed, 63 insertions(+)<br>
><br>
> diff --git a/tests/fcntl.c b/tests/fcntl.c<br>
> index 4f62ca2a..8c74ed22 100644<br>
> --- a/tests/fcntl.c<br>
> +++ b/tests/fcntl.c<br>
> @@ -36,6 +36,9 @@<br>
>  # include "struct_flock.c"<br>
><br>
>  # define TEST_FLOCK64_EINVAL(cmd) test_flock64_einval(cmd, #cmd)<br>
> +# define TEST_F_OWNER_EX_ESRCH(cmd) test_f_owner_ex_esrch(cmd, #cmd)<br>
> +<br>
> +typedef struct f_owner_ex struct_kernel_f_owner_ex;<br>
<br>
</span>The new typedef is used only once, do you really need it now?<br>
<span class=""><br>
>  static void<br>
>  test_flock64_einval(const int cmd, const char *name)<br>
> @@ -69,12 +72,72 @@ test_flock64(void)<br>
>  #endif<br>
>  }<br>
><br>
> +static int<br>
> +test_f_owner_ex_type_pid(<wbr>const int cmd, const char *const cmd_name,<br>
> +                             const int type, const char *const type_name,<br>
> +                             const pid_t pid)<br>
> +{<br>
> +     struct_kernel_f_owner_ex fo = { .type = type, .pid = pid };<br>
> +<br>
> +     long rc = invoke_test_syscall(cmd, &fo);<br>
> +     printf("%s(0, %s, {type=%s, pid=%d}) = %s\n",<br>
> +             TEST_SYSCALL_STR, cmd_name, type_name, pid, sprintrc(rc));<br>
> +     if (rc)<br>
> +             return 1;<br>
> +     return 0;<br>
<br>
</span>There is a simple way to write this:<br>
        return !!rc;<br>
<br>
Alternatively, you can change the return type from "int" to "long"<br>
and just return rc.<br>
<span class=""><br>
> +}<br>
> +<br>
> +static void<br>
> +test_f_owner_ex_esrch(const int cmd, const char *const cmd_name)<br>
> +{<br>
> +     test_f_owner_ex_type_pid(cmd, cmd_name,<br>
> +                                     ARG_STR(F_OWNER_TID), 1234567890);<br>
> +     test_f_owner_ex_type_pid(cmd, cmd_name,<br>
> +                                     ARG_STR(F_OWNER_PID), 1234567890);<br>
> +     test_f_owner_ex_type_pid(cmd, cmd_name,<br>
> +                                     ARG_STR(F_OWNER_PGRP), -1234567890);<br>
<br>
</span>Something went wrong with the indentation in this function.<br>
<span class=""><br>
> +}<br>
> +<br>
> +static void<br>
> +test_f_owner_ex(void)<br>
> +{<br>
> +     TEST_F_OWNER_EX_ESRCH(F_<wbr>SETOWN_EX);<br>
<br>
</span>Looks like this could be written without TEST_F_OWNER_EX_ESRCH macro:<br>
<br>
        test_f_owner_ex_esrch(ARG_STR(<wbr>F_SETOWN_EX));<br>
<div><div class="h5"><br>
> +<br>
> +     long rc;<br>
> +<br>
> +     rc = test_f_owner_ex_type_pid(ARG_<wbr>STR(F_SETOWN_EX),<br>
> +                                     ARG_STR(F_OWNER_TID), 20);<br>
> +     if (rc)<br>
> +             return;<br>
> +<br>
> +     test_f_owner_ex_type_pid(ARG_<wbr>STR(F_GETOWN_EX),<br>
> +                                     ARG_STR(F_OWNER_TID), 20);<br>
> +<br>
> +     rc = test_f_owner_ex_type_pid(ARG_<wbr>STR(F_SETOWN_EX),<br>
> +                                     ARG_STR(F_OWNER_PID), 30);<br>
> +     if (rc)<br>
> +             return;<br>
> +<br>
> +     test_f_owner_ex_type_pid(ARG_<wbr>STR(F_GETOWN_EX),<br>
> +                                     ARG_STR(F_OWNER_PID), 30);<br>
> +<br>
> +     rc = test_f_owner_ex_type_pid(ARG_<wbr>STR(F_SETOWN_EX),<br>
> +                                     ARG_STR(F_OWNER_PGRP), 40);<br>
> +     if (rc)<br>
> +             return;<br>
> +<br>
> +     test_f_owner_ex_type_pid(ARG_<wbr>STR(F_GETOWN_EX),<br>
> +                                     ARG_STR(F_OWNER_PGRP), 40);<br>
<br>
</div></div>The same F_SETOWN_EX/check/F_GETOWN_EX pattern is repeated thrice here,<br>
wouldn't it be better if it was moved to a separate function?<br>
<span class="HOEnZb"><font color="#888888"><br>
<br>
--<br>
ldv<br>
</font></span></blockquote></div><br></div>