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/
This archive was generated by hypermail 2b29 : Thu Aug 15 2002 - 22:00:40 EST