linux-kernel - Re: [PATCH 8/8] Char: vt_ioctl, use wait_event

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: <20070529170806.c4791255.akpm@linux-foundation.org>
Date:	Tue, 29 May 2007 17:08:06 -0700
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	Jiri Slaby <jirislaby@...il.com>
Cc:	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 8/8] Char: vt_ioctl, use wait_event_interruptible

On Mon, 28 May 2007 15:31:12 +0200 (CEST)
Jiri Slaby <jirislaby@...il.com> wrote:

> vt_ioctl, use wait_event_interruptible
> 
> Signed-off-by: Jiri Slaby <jirislaby@...il.com>
> 
> ---
> commit fbe1931e02f11b2fef771ff1698f1598b3567520
> tree fcfcd72a5619f6e26598ac2ee3132aed4b070987
> parent c025c4b3eca99f50b05bc24c445b861e91226539
> author Jiri Slaby <jirislaby@...il.com> Sat, 26 May 2007 22:58:17 +0200
> committer Jiri Slaby <jirislaby@...il.com> Sat, 26 May 2007 22:58:17 +0200
> 
>  drivers/char/vt_ioctl.c |   22 ++++------------------
>  1 files changed, 4 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/char/vt_ioctl.c b/drivers/char/vt_ioctl.c
> index c6f6f42..2056367 100644
> --- a/drivers/char/vt_ioctl.c
> +++ b/drivers/char/vt_ioctl.c
> @@ -1035,12 +1035,8 @@ static DECLARE_WAIT_QUEUE_HEAD(vt_activate_queue);
>  int vt_waitactive(int vt)
>  {
>  	int retval;
> -	DECLARE_WAITQUEUE(wait, current);
> -
> -	add_wait_queue(&vt_activate_queue, &wait);
> -	for (;;) {
> -		retval = 0;
>  
> +	return wait_event_interruptible(vt_activate_queue, ({
>  		/*
>  		 * Synchronize with redraw_screen(). By acquiring the console
>  		 * semaphore we make sure that the console switch is completed
> @@ -1049,20 +1045,10 @@ int vt_waitactive(int vt)
>  		 * updated, but the console switch hasn't been completed.
>  		 */
>  		acquire_console_sem();
> -		set_current_state(TASK_INTERRUPTIBLE);
> -		if (vt == fg_console) {
> -			release_console_sem();
> -			break;
> -		}
> +		retval = vt == fg_console;
>  		release_console_sem();
> -		retval = -EINTR;
> -		if (signal_pending(current))
> -			break;
> -		schedule();
> -	}
> -	remove_wait_queue(&vt_activate_queue, &wait);
> -	__set_current_state(TASK_RUNNING);
> -	return retval;
> +		retval;
> +	}));
>  }
>  

So we end up with

int vt_waitactive(int vt)
{
	int retval;

	return wait_event_interruptible(vt_activate_queue, ({
		/*
		 * Synchronize with redraw_screen(). By acquiring the console
		 * semaphore we make sure that the console switch is completed
		 * before we return. If we didn't wait for the semaphore, we
		 * could return at a point where fg_console has already been
		 * updated, but the console switch hasn't been completed.
		 */
		acquire_console_sem();
		retval = vt == fg_console;
		release_console_sem();
		retval;
	}));
}

Again, I do think this needs a helper function.  Or something.  The handling
of `retval' in there is pretty perverse.  We're modifying a local variable
within the macro and then returning it by value?  Perhaps a bit cleaner
would be to move `retval' inside the macro body.

But a helper function would be better.  Again, remember that the macro
evaluates that expression twice - you may find that the helper function
even generates less code.

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