Re: fcntl error

[Linus Torvalds]

On Thu, 18 Mar 2004, David Howells wrote:
> 
> The attached patch fixes a minor problem with fcntl.

I agree that it is a cleanup, but I disagree on the "problem" part.

> get_close_on_exec() uses FD_ISSET() to determine the fd state. However,
> FD_ISSET() does not return 0 or 1 on all archs. On some it returns 0 or non-0,
> which is fine by POSIX.

FD_ISSET() is broken if it returns anything but 0/1, in my not-so-humble 
opinion.

Looking at the implementations, you are right that some architectures 
don't do this right, but that is a bug, and it's a bug in FD_ISSET(), not 
in fcntl.

The fact is, FD_ISSET() isn't always used in just as a conditional, and
you're supposed to be able to do

	int was_set = FD_ISSET(..);
	...

and in fact I'd suggest very _strongly_ that it also should work with

	bool is_set = FD_ISSET(..);

where some people use "char" for booleans for space reasons.

That implies that while non-zero for "set" is ok, that non-zero had better
have the _low_ bits set. Which is not true on architectures that use just
a logical and with the bits in the word.

Which implies that FD_ISSET() really must NOT be of that "logical and" 
approach, which in turn implies that it should be either a inequality 
expression, or it should be a "shift down and then and with 1".

And in both of those cases, the result ends up being 0/1. So we might as 
well just make it so.

In short, the real bug is elsewhere.

> Also, the argument of set_close_on_exec() is being AND'ed with literal 1. This
> is incorrect - there's no requirement for FD_CLOEXEC to be 1.

Not in theory, no. In practice, it always is.

I'd suggest architecture maintainers fix their __FD_ISSET() 
implementations to conform to the proper return value.

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