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

Matthew Wilcox (willy@debian.org)
Thu, 15 Aug 2002 20:04:36 +0100


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