Re: [PATCH][RFC] sigurg/sigio cleanup for 2.5.31

[Matthew Wilcox <willy@debian.org>]

On Fri, Aug 16, 2002 at 03:16:57AM +1000, James Morris wrote:
> This patch is a clean up of the sigurg and sigio related code, for 
> the 2.5.31 kernel.

In general, this is good... I think it could be better:

> +	lock_kernel();
> +	error = f_setown(filp, current->pid);
> +	unlock_kernel();

There are a lot of these, and you even batch it up as sock_setown()
later.  May I suggest renaming f_setown to __setown and sock_setown
to f_setown?

> @@ -306,19 +334,11 @@
>  			break;
>  		case F_SETOWN:
>  			lock_kernel();
> -
> -			err = security_ops->file_set_fowner(filp);
> +			err = f_setown(filp, arg);
>  			if (err) {
>  				unlock_kernel();
>  				break;
>  			}
> -
> -			filp->f_owner.pid = arg;
> -			filp->f_owner.uid = current->uid;
> -			filp->f_owner.euid = current->euid;
> -			err = 0;
> -			if (S_ISSOCK (filp->f_dentry->d_inode->i_mode))
> -				err = sock_fcntl (filp, F_SETOWN, arg);
>  			unlock_kernel();
>  			break;
>  		case F_GETSIG:

this one's particularly silly -- now you've done the good job of wrapping
the security_ops up inside f_setown this can simply be:

			lock_kernel();
			err = f_setown(filp, arg);
			unlock_kernel();
			break;

though if you accept my suggestion above, it's even easier.

> +int sk_send_sigurg(struct sock *sk)
> +{
> +	if (sk->socket && sk->socket->file)
> +		return send_sigurg(&sk->socket->file->f_owner);
> +	return 0;
> +}
> +

I notice that both the callers of this do:

>  	/* Tell the world about our new urgent pointer. */
> +	if (sk_send_sigurg(sk))
>  		sk_wake_async(sk, 3, POLL_PRI);

Might make more sense to refactor as:

void sk_send_sigurg(struct sock *sk)
{
	if (!sk->socket || !sk->socket->file)
		return;
	if (send_sigurg(&sk->socket->file->f_owner))
		sk_wake_async(sk, 3, POLL_PRI);
}

-- 
Revolutions do not require corporate support.