linux-kernel - Re: [PATCH 1/2] 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: <alpine.LFD.1.00.0803290906050.14670@woody.linux-foundation.org>
Date:	Sat, 29 Mar 2008 09:16:02 -0700 (PDT)
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Roland McGrath <roland@...hat.com>
cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Oleg Nesterov <oleg@...sign.ru>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] do_wait reorganization

On Fri, 28 Mar 2008, Roland McGrath wrote:
>
> The control flow is less nonobvious without so much goto.

How about a further non-obviousness?

> +static int wait_consider_task(struct task_struct *parent,
> +			      struct task_struct *p, int *retval,
> +			      enum pid_type type, struct pid *pid, int options,
> +			      struct siginfo __user *infop,
> +			      int __user *stat_addr, struct rusage __user *ru)
> +{
...
> +	if (task_is_stopped_or_traced(p)) {
> +		/*
> +		 * It's stopped now, so it might later
> +		 * continue, exit, or stop again.
> +		 */
> +		*retval = 0;
> +		if ((p->ptrace & PT_PTRACED) ||
> +		    (options & WUNTRACED)) {
> +			*retval = wait_task_stopped(p, (options & WNOWAIT),
> +						    infop, stat_addr, ru);
> +			if (*retval)
> +				return 1;
> +		}
> +	} else if (p->exit_state == EXIT_ZOMBIE && !delay_group_leader(p)) {
...
> +	return 0;

I think it would be even more obvious (or, to use your phrase, "less 
nonobvious") if this was written like so:

	if (task_is_stopped_or_traced(p)) {
		...
		....
			if (*retval}
				return 1;
		}
		return 0;
	}

	if (p->exit_state == EXIT_ZOMBIE && !delay_group_leader(p)) {
		...
		return 0;
	}

	if (...)

because then you can clearly see that smething like the
"task_is_stopped_or_traced(p)" case is complete in itself, and only has 
its own local stuff going on.

(And at some point I'd also almost make each case a trivial small inline 
function of its on, but in this case there are so many arguments to pass 
around that it probably becomes _less_ readable that way).

I also wonder if you really need both "int *retval" and the return value. 
Isn't "retval" always going to be zero or a negative errno? And the return 
value is going to be either true of false? Why not just fold them into one 
single thing:

 - negative: all done, with error
 - zero: this didn't trigger, continue with the next one in caller
 - positive: this thread triggered, all done, return 0 in the caller.

which is (I think) close to what we already do in eligible_child() (so 
this would not be a new calling convention for this particular code).

		Linus
--
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/