linux-kernel - Re: [PATCH 1/4] 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] [day] [month] [year] [list]

Message-Id: <20080506014235.3905626FA20@magilla.localdomain>
Date:	Mon,  5 May 2008 18:42:35 -0700 (PDT)
From:	Roland McGrath <roland@...hat.com>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Oleg Nesterov <oleg@...sign.ru>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/4] do_wait reorganization

> I absolutely detest your propensity for multiple return values. You have 
> "int ret" for the return value, and then you *also* have a "int *retval" 
> for the return value.
> 
> Which is what, and why?

The "int ret" (actual return value of wait_consider_task and wait_task_*)
is the nonzero return value if this task produced the final result.  The
"int *retval" is the return value of last resort if there turns out to be
no task with a final result.

> I'm pretty sure it should be possible to return a positive value for 
> "eligible but not available" and make do with just one return value, but 
> if that is just not possible or too complicated, 

It's not possible and would be too complicated.  (A positive value
already means the pid for a successful result, and the task_struct has been
freed by then so the pid for the syscall return value can't be gotten later.)

> at least don't call it "retval" and have totally different semantics from
> the return value we return?

A better name would be "notask_error".  It is the error to return when there
is no task with status to return.  The error is zero if there were matches,
since we'll return zero for WNHOHANG or we will block.

> So for example, maybe it could just be count of eligible children, and we 
> call could it "int *eligible", and then rather than initialize to -ECHILD, 

There is no use for a count, but there is a use for the error value.  In
patch 3/3, we restore the old behavior of keeping track of the error value
other than -ECHILD if there was one (like -EACCES due to bad selinux policy).

I agree the name of the parameter and variable should not be "retval".
A change beyond that just for the sake of making it not be something
like looks like it might the syscall's return value seems gratuitous and
obfuscating, and will complicate the code (since it really is a value
we're keeping around that might be the return value for the syscall).

Thanks,
Roland
--
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/