On 3/9/21 11:15, Oleg Nesterov wrote:
> Jim,
>
> Thanks, the patch looks good to me. Yet I think you need to send V3 even
> if I personally do not care ;) Please consider ./scripts/checkpatch.pl,
> it reports all the coding-style problems I was going to mention.
Thanks! I'd thought clang-format with the included configuration would
be sufficient, but apparently not :)
> Both if's use "int retval", to me it would be better to declare this variable
> at the start of do_wait_pid(). But again, I won't insist this is up to you.
My usual inclination is to avoid uninitialized variables and prefer
putting them in tighter scopes. I don't think it's really much of an
issue in this relatively short function though; happy to go with the
prevailing style.
> I am wondering if something like
>
> static inline bool is_parent(struct task_struct *tsk, struct
> task_struct *p, int flags)
> {
> return tsk == p || !(flags & __WNOTHREAD)) &&
> same_thread_group(tsk, p);
> }
>
> makes any sense to make do_wait_pid() more clear... probably not.
Yeah, I lean slightly towards the extra level of indirection not being
worth the deduplication.
I made a couple other small changes as well:
* No need for do_wait_pid to take the parameter `tsk` since it's only
ever called with `current`
* With that change, the declaration of `tsk` in `do_wait` can be moved
into a tighter scope of where it's used in the loop.
v3: https://lkml.org/lkml/2021/3/9/1134
