[PATCH 2.4] sys_select() return error on bad file

[PATCH 2.4] sys_select() return error on bad file

[Armin Schindler]

Hi,

the following patch now returns -EBADF in sys_select()/do_select()
if all specified file-descriptors are bad (e.g. were closed by another
thread).

Without this patch, the for-loop in do_select() won't stop if there are no
valid files any more. It stops only if a specified timeout occured or a
signal arrived.

If there are no objections on this patch, I would also create a patch for
kernel 2.6 and the poll() code. I didn't have a closer look at this yet.

Armin

--- linux/fs/select.c_orig	2004-03-02 19:06:44.000000000 +0100
+++ linux/fs/select.c	2004-03-03 09:25:24.000000000 +0100
@@ -183,6 +183,8 @@
 		wait = NULL;
 	retval = 0;
 	for (;;) {
+		int file_err = 1;
+
 		set_current_state(TASK_INTERRUPTIBLE);
 		for (i = 0 ; i < n; i++) {
 			unsigned long bit = BIT(i);
@@ -199,6 +201,7 @@
 						  i /*  The fd*/,
 						  __timeout,
 						  NULL);
+				file_err = 0;
 				mask = DEFAULT_POLLMASK;
 				if (file->f_op && file->f_op->poll)
 					mask = file->f_op->poll(file, wait);
@@ -227,6 +230,10 @@
 			retval = table.error;
 			break;
 		}
+		if (file_err) {
+			retval = -EBADF;
+			break;
+		}
 		__timeout = schedule_timeout(__timeout);
 	}
 	current->state = TASK_RUNNING;

Re: [PATCH 2.4] sys_select() return error on bad file

[Willy Tarreau]

Hi,

On Wed, Mar 03, 2004 at 09:46:54AM +0100, Armin Schindler wrote:
> --- linux/fs/select.c_orig	2004-03-02 19:06:44.000000000 +0100
> +++ linux/fs/select.c	2004-03-03 09:25:24.000000000 +0100
> @@ -183,6 +183,8 @@
>  		wait = NULL;
>  	retval = 0;
>  	for (;;) {
> +		int file_err = 1;
> +

Just a thought, select() is often performance-critical, and adding one more
variable inside the loop can slow it down a bit. Wouldn't it be cheaper to
set retval to -EBADF above and avoid using file_err ?

>  		set_current_state(TASK_INTERRUPTIBLE);
>  		for (i = 0 ; i < n; i++) {
>  			unsigned long bit = BIT(i);
> @@ -199,6 +201,7 @@
>  						  i /*  The fd*/,
>  						  __timeout,
>  						  NULL);
> +				file_err = 0;

There you would put retval=0

>  				mask = DEFAULT_POLLMASK;
>  				if (file->f_op && file->f_op->poll)
>  					mask = file->f_op->poll(file, wait);
> @@ -227,6 +230,10 @@
>  			retval = table.error;
>  			break;
>  		}
> +		if (file_err) {
> +			retval = -EBADF;
> +			break;
> +		}

And there : if (retval == -EBADF) break;

>  		__timeout = schedule_timeout(__timeout);
>  	}
>  	current->state = TASK_RUNNING;

Just a thought anyway, I've not read the complete function.

Cheers,
Willy

Re: [PATCH 2.4] sys_select() return error on bad file

[Armin Schindler]

On Thu, 4 Mar 2004, Willy Tarreau wrote:
> Hi,
>
> On Wed, Mar 03, 2004 at 09:46:54AM +0100, Armin Schindler wrote:
> > --- linux/fs/select.c_orig	2004-03-02 19:06:44.000000000 +0100
> > +++ linux/fs/select.c	2004-03-03 09:25:24.000000000 +0100
> > @@ -183,6 +183,8 @@
> >  		wait = NULL;
> >  	retval = 0;
> >  	for (;;) {
> > +		int file_err = 1;
> > +
>
> Just a thought, select() is often performance-critical, and adding one more
> variable inside the loop can slow it down a bit. Wouldn't it be cheaper to
> set retval to -EBADF above and avoid using file_err ?

retval cannot be used for that, it may get changed in the loop.

Anyway, I don't see how your proposal would do better performance?
My patch just adds a new variable on the stack, which should not make any
difference in performance. And later, it is the same if the new or another
variable gets changed or checked.

Armin

Re: [PATCH 2.4] sys_select() return error on bad file

[Willy Tarreau]

On Thu, Mar 04, 2004 at 10:20:41AM +0100, Armin Schindler wrote:
 
> retval cannot be used for that, it may get changed in the loop.

OK

> Anyway, I don't see how your proposal would do better performance?
> My patch just adds a new variable on the stack, which should not make any
> difference in performance. And later, it is the same if the new or another
> variable gets changed or checked.

It is possible that in current code the, compiler is able to allocate a
register for each variable used inside the loop, while it may not be able
anymore with an additionnal variable. But that's just speculation anyway.

Cheers,
Willy

Re: [PATCH 2.4] sys_select() return error on bad file

[Marcelo Tosatti]

On Thu, 4 Mar 2004, Armin Schindler wrote:

> On Thu, 4 Mar 2004, Willy Tarreau wrote:
> > Hi,
> >
> > On Wed, Mar 03, 2004 at 09:46:54AM +0100, Armin Schindler wrote:
> > > --- linux/fs/select.c_orig	2004-03-02 19:06:44.000000000 +0100
> > > +++ linux/fs/select.c	2004-03-03 09:25:24.000000000 +0100
> > > @@ -183,6 +183,8 @@
> > >  		wait = NULL;
> > >  	retval = 0;
> > >  	for (;;) {
> > > +		int file_err = 1;
> > > +
> >
> > Just a thought, select() is often performance-critical, and adding one more
> > variable inside the loop can slow it down a bit. Wouldn't it be cheaper to
> > set retval to -EBADF above and avoid using file_err ?
> 
> retval cannot be used for that, it may get changed in the loop.
> 
> Anyway, I don't see how your proposal would do better performance?
> My patch just adds a new variable on the stack, which should not make any
> difference in performance. And later, it is the same if the new or another
> variable gets changed or checked.

Curiosity: Does SuS/POSIX define behaviour for "all fds are closed" ? 

Re: [PATCH 2.4] sys_select() return error on bad file

[Manfred Spraul]

[-- Attachment #1: Type: text/plain, Size: 1482 bytes --]

Marcelo wrote:

>> 
>> Anyway, I don't see how your proposal would do better performance?
>> My patch just adds a new variable on the stack, which should not make any
>> difference in performance. And later, it is the same if the new or another
>> variable gets changed or checked.
>
>Curiosity: Does SuS/POSIX define behaviour for "all fds are closed" ? 
>  
>
I'd interpret SuS that a closed fd is ready for reading and writing:
 From the select page:
<<<
A descriptor shall be considered ready for reading when a call to an 
input function with O_NONBLOCK clear would not block, whether or not the 
function would transfer data successfully. (The function might return 
data, an end-of-file indication, or an error other than one indicating 
that it is blocked, and in each of these cases the descriptor shall be 
considered ready for reading.)
<<<
read(fd,,) will return immediately with EBADF, thus the fd is ready.

But that's a grey area, especially if you close the fd during the select 
call. For example HP UX just kills the current process if an fd that is 
polled is closed by overwriting it with dup2. I didn't test select, but 
I'd expect a similar behavior.

Armin: did you compare the Linux behavior with other unices? Are there 
other unices that return EBADF for select() if all fds are closed?

Attached is an untested proposal, against 2.6, but I'm not sure if it's 
really a good idea to change the current code - it might break existing 
apps.

--
    Manfred

[-- Attachment #2: patch-select-2.6 --]
[-- Type: text/plain, Size: 1013 bytes --]

--- 2.6/fs/select.c	2004-03-14 14:28:28.000000000 +0100
+++ build-2.6/fs/select.c	2004-03-14 19:08:57.000000000 +0100
@@ -223,25 +223,25 @@
 					break;
 				if (!(bit & all_bits))
 					continue;
+				mask = DEFAULT_POLLMASK;
 				file = fget(i);
 				if (file) {
 					f_op = file->f_op;
-					mask = DEFAULT_POLLMASK;
 					if (f_op && f_op->poll)
 						mask = (*f_op->poll)(file, retval ? NULL : wait);
 					fput(file);
-					if ((mask & POLLIN_SET) && (in & bit)) {
-						res_in |= bit;
-						retval++;
-					}
-					if ((mask & POLLOUT_SET) && (out & bit)) {
-						res_out |= bit;
-						retval++;
-					}
-					if ((mask & POLLEX_SET) && (ex & bit)) {
-						res_ex |= bit;
-						retval++;
-					}
+				}
+				if ((mask & POLLIN_SET) && (in & bit)) {
+					res_in |= bit;
+					retval++;
+				}
+				if ((mask & POLLOUT_SET) && (out & bit)) {
+					res_out |= bit;
+					retval++;
+				}
+				if ((mask & POLLEX_SET) && (ex & bit)) {
+					res_ex |= bit;
+					retval++;
 				}
 			}
 			if (res_in)

Re: [PATCH 2.4] sys_select() return error on bad file

[Armin Schindler]

On Sun, 14 Mar 2004, Manfred Spraul wrote:
> Marcelo wrote:
>
> >>
> >> Anyway, I don't see how your proposal would do better performance?
> >> My patch just adds a new variable on the stack, which should not make any
> >> difference in performance. And later, it is the same if the new or another
> >> variable gets changed or checked.
> >
> >Curiosity: Does SuS/POSIX define behaviour for "all fds are closed" ?
> >
> >
> I'd interpret SuS that a closed fd is ready for reading and writing:
>  From the select page:
> <<<
> A descriptor shall be considered ready for reading when a call to an
> input function with O_NONBLOCK clear would not block, whether or not the
> function would transfer data successfully. (The function might return
> data, an end-of-file indication, or an error other than one indicating
> that it is blocked, and in each of these cases the descriptor shall be
> considered ready for reading.)
> <<<
> read(fd,,) will return immediately with EBADF, thus the fd is ready.
>
> But that's a grey area, especially if you close the fd during the select
> call. For example HP UX just kills the current process if an fd that is
> polled is closed by overwriting it with dup2. I didn't test select, but
> I'd expect a similar behavior.
>
> Armin: did you compare the Linux behavior with other unices? Are there
> other unices that return EBADF for select() if all fds are closed?

No, I didn't compare yet, but I could not find any definition on that. It
really seems to be a "grey area".

> Attached is an untested proposal, against 2.6, but I'm not sure if it's
> really a good idea to change the current code - it might break existing
> apps.

This patch should also work on 2.4 and looks good to me, if "ready" should
be returned instead of EBADF. I don't think this would break existing
apps. Without such a patch, the app would sleep forever unless a signal
arrives. If any app depends on that behavior, I think it is bad coded.

Armin