fcntl error

Hi Andrew, Linus,

The attached patch fixes a minor problem with fcntl.

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.

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.

This is also wrong on 2.4 kernels.

David

@@ -293,11 +293,11 @@
                        err = dupfd(filp, arg);
                        break;
                case F_GETFD:
-                       err = get_close_on_exec(fd);
+                       err = get_close_on_exec(fd) ? FD_CLOEXEC : 0;
                        break;
                case F_SETFD:
                        err = 0;
-                       set_close_on_exec(fd, arg&1);
+                       set_close_on_exec(fd, arg & FD_CLOEXEC);
                        break;
                case F_GETFL:
                        err = filp->f_flags;

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

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.

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

POSIX clearly says that _any_ non-zero value is ok, similar to the ctype.h functions. Of course, the kernel can set different standards internally.

Andreas.

Andreas Schwab, SuSE Labs, [email protected]
SuSE Linux AG, Maxfeldstraße 5, 90409 Nürnberg, Germany
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Or maybe:

#define __FD_ISSET(d, set) (!!((set)->fds_bits[__FDELT(d)] & __FDMASK(d)))

Which is probably better than a shift and bit-and. That then leaves it up to the compiler as to whether the generation of 0 or 1 is actually necessary (which if isn't if it's just the condition in an if-statement).

Or even:

#define FD_ISSET(fd,fdsetp) (!!__FD_ISSET(fd,fdsetp))

Which would then apply to all arch's regardless.

David