linux-kernel - Re: [FOR REVIEW, PATCH 2/2] introduce "struct wait_opts" to simplify do

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20090506072756.GA17457@elte.hu>
Date:	Wed, 6 May 2009 09:27:56 +0200
From:	Ingo Molnar <mingo@...e.hu>
To:	Oleg Nesterov <oleg@...hat.com>
Cc:	Roland McGrath <roland@...hat.com>, linux-kernel@...r.kernel.org
Subject: Re: [FOR REVIEW, PATCH 2/2] introduce "struct wait_opts" to
	simplify do_wait() pathes


* Oleg Nesterov <oleg@...hat.com> wrote:

> Introduce "struct wait_opts" which holds the parameters for misc 
> helpers in do_wait() pathes.
> 
> This adds 17 lines to kernel/exit.c, but saves 256 bytes from .o 
> and imho makes the code much more readable.
> 
> (untested, not signed)
> 
>  kernel/exit.c |  211 +++++++++++++++++++++++++++++++---------------------------
>  1 file changed, 114 insertions(+), 97 deletions(-)

> +struct wait_opts {
> +	enum pid_type		wtype;
> +	struct pid		*wpid;
> +	int			wflags;
> +
> +	struct siginfo __user	*winfo;
> +	int __user		*wstat;
> +	struct rusage __user	*wrusage;
> +
> +	int			notask_error;
> +};

Nice idea!

One small nit with the definition above: when using vertical spacing 
(which really looks nice) we tend to put the asterix to the type 
itself, not to the variable. I.e.:

	enum pid_type		wtype;
	struct pid *		wpid;
	int			wflags;

( This is done to separate the field name from the type - the 
  pointer nature of the field is part of the type, not part of the
  name. )

It's impressive how well the function prototypes get compressed and 
cleaned up by this helper structure:

> -static int eligible_child(enum pid_type type, struct pid *pid, int options,
> -			  struct task_struct *p)
> +static int eligible_child(struct wait_opts *wopts, struct task_struct *p)

> -static int wait_noreap_copyout(struct task_struct *p, pid_t pid, uid_t uid,
> -			       int why, int status,
> -			       struct siginfo __user *infop,
> -			       struct rusage __user *rusagep)
> +static int wait_noreap_copyout(struct wait_opts *wopts, struct task_struct *p,
> +				pid_t pid, uid_t uid, int why, int status)

> -static int wait_task_zombie(struct task_struct *p, int options,
> -			    struct siginfo __user *infop,
> -			    int __user *stat_addr, struct rusage __user *ru)
> +static int wait_task_zombie(struct wait_opts *wopts, struct task_struct *p)

> -static int wait_task_stopped(int ptrace, struct task_struct *p,
> -			     int options, struct siginfo __user *infop,
> -			     int __user *stat_addr, struct rusage __user *ru)
> +static int wait_task_stopped(struct wait_opts *wopts,
> +				int ptrace, struct task_struct *p)

> -static int wait_task_continued(struct task_struct *p, int options,
> -			       struct siginfo __user *infop,
> -			       int __user *stat_addr, struct rusage __user *ru)
> +static int wait_task_continued(struct wait_opts *wopts, struct task_struct *p)

> -static int wait_consider_task(struct task_struct *parent, int ptrace,
> -			      struct task_struct *p, int *notask_error,
> -			      enum pid_type type, struct pid *pid, int options,
> -			      struct siginfo __user *infop,
> -			      int __user *stat_addr, struct rusage __user *ru)
> +static int wait_consider_task(struct wait_opts *wopts, struct task_struct *parent,
> +				int ptrace, struct task_struct *p)

> -static int do_wait_thread(struct task_struct *tsk, int *notask_error,
> -			  enum pid_type type, struct pid *pid, int options,
> -			  struct siginfo __user *infop, int __user *stat_addr,
> -			  struct rusage __user *ru)
> +static int do_wait_thread(struct wait_opts *wopts, struct task_struct *tsk)

> -static int ptrace_do_wait(struct task_struct *tsk, int *notask_error,
> -			  enum pid_type type, struct pid *pid, int options,
> -			  struct siginfo __user *infop, int __user *stat_addr,
> -			  struct rusage __user *ru)
> +static int ptrace_do_wait(struct wait_opts *wopts, struct task_struct *tsk)

> -static long do_wait(enum pid_type type, struct pid *pid, int options,
> -		    struct siginfo __user *infop, int __user *stat_addr,
> -		    struct rusage __user *ru)
> +static long do_wait(struct wait_opts *wopts)

One small (style) detail here as well:

> -	ret = do_wait(type, pid, options, infop, NULL, ru);
> +
> +	wopts.wtype = type;
> +	wopts.wpid = pid;
> +	wopts.wflags = options;
> +
> +	wopts.winfo = infop;
> +	wopts.wstat = NULL;
> +	wopts.wrusage = ru;
> +
> +	ret = do_wait(&wopts);

it makes sense to write this as:

> +	wopts.wtype	= type;
> +	wopts.wpid	= pid;
> +	wopts.wflags	= options;
> +
> +	wopts.winfo	= infop;
> +	wopts.wstat	= NULL;
> +	wopts.wrusage	= ru;
> +
> +	ret = do_wait(&wopts);

(and in other places as well). Vertical spacing for assignments 
looks messy if done for 1-3 assignment lines, but in the case above 
we've got 6 of them so it has a nice vertical structure already that 
helps readability.

Regarding the patch itself: i guess we could do it as-is - but if 
you think there's regression risks, a safer approach would be to 
create 5-6 patches to build up all the structure parameters one by 
one. Such a series is a lot easier to check (and a lot easier to 
bisect to).

Anyway ... provided you give it some testing:

Reviewed-by: Ingo Molnar <mingo@...e.hu>

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/