On Sat, 17 Mar 2007, Arnd Bergmann wrote:
> > +asmlinkage long sys_signalfd(int ufd, sigset_t __user *user_mask, size_t
> > sizemask) +{
> > + int error;
> > + unsigned long flags;
> > + sigset_t sigmask;
> > + struct signalfd_ctx *ctx;
> > + struct sighand_struct *sighand;
> > + struct file *file;
> > + struct inode *inode;
> > +
> > + error = -EINVAL;
> > + if (sizemask != sizeof(sigset_t) ||
> > + copy_from_user(&sigmask, user_mask, sizeof(sigmask)))
> > + goto err_exit;
>
> sizeof(sigset_t) may be different for native and 32-bit compat code.
> It would be good if you could handle sizemask==4 && sizeof(sigset_t)==8
> in this code, so that there is no need for an extra compat_sys_signalfd
> function.
As Stephen reported, we do need the compat in any case. Better keep all
the compat adjustments under CONFIG_COMPAT, so archs that don't need it
don't need to link to it.
> > + if ((sighand = signalfd_get_sighand(ctx, &flags)) != NULL) {
> > + if (next_signal(&ctx->tsk->pending, &ctx->sigmask) > 0 ||
> > + next_signal(&ctx->tsk->signal->shared_pending,
> > + &ctx->sigmask) > 0)
> > + events |= POLLIN;
> > + signalfd_put_sighand(ctx, sighand, &flags);
> > + } else
> > + events |= POLLIN;
> > +
> > + return events;
> > +}
>
> I never really understood the events mask, but other subsystems often
> use (POLLIN | POLLRDNORM) instead of just POLLIN. Is there a reason
> for not returning POLLRDNORM here?
I don't think those fds will have to deal with the concept of bands and
priorities. I believe POLLIN is fine here.
> > +static int signalfd_copyinfo(struct signalfd_siginfo __user *uinfo,
> > + siginfo_t const *kinfo)
> > +{
> > + long err;
> > +
> > + err = __clear_user(uinfo, sizeof(*uinfo));
> > +
> > + /*
> > + * If you change siginfo_t structure, please be sure
> > + * this code is fixed accordingly.
> > + */
> > + err |= __put_user(kinfo->si_signo, &uinfo->signo);
> > + err |= __put_user(kinfo->si_errno, &uinfo->err);
> > + err |= __put_user((short)kinfo->si_code, &uinfo->code);
> > + switch (kinfo->si_code & __SI_MASK) {
> > + case __SI_KILL:
> > + err |= __put_user(kinfo->si_pid, &uinfo->pid);
> > + err |= __put_user(kinfo->si_uid, &uinfo->uid);
> > + break;
> > + case __SI_TIMER:
> > + err |= __put_user(kinfo->si_tid, &uinfo->tid);
> > + err |= __put_user(kinfo->si_overrun, &uinfo->overrun);
> > + err |= __put_user(kinfo->si_ptr, &uinfo->svptr);
> > + break;
> > + case __SI_POLL:
> > + err |= __put_user(kinfo->si_band, &uinfo->band);
> > + err |= __put_user(kinfo->si_fd, &uinfo->fd);
> > + break;
> > + case __SI_FAULT:
> > + err |= __put_user(kinfo->si_addr, &uinfo->addr);
> > +#ifdef __ARCH_SI_TRAPNO
> > + err |= __put_user(kinfo->si_trapno, &uinfo->trapno);
> > +#endif
> > + break;
> > + case __SI_CHLD:
> > + err |= __put_user(kinfo->si_pid, &uinfo->pid);
> > + err |= __put_user(kinfo->si_uid, &uinfo->uid);
> > + err |= __put_user(kinfo->si_status, &uinfo->status);
> > + err |= __put_user(kinfo->si_utime, &uinfo->utime);
> > + err |= __put_user(kinfo->si_stime, &uinfo->stime);
> > + break;
> > + case __SI_RT: /* This is not generated by the kernel as of now. */
> > + case __SI_MESGQ: /* But this is */
> > + err |= __put_user(kinfo->si_pid, &uinfo->pid);
> > + err |= __put_user(kinfo->si_uid, &uinfo->uid);
> > + err |= __put_user(kinfo->si_ptr, &uinfo->svptr);
> > + break;
> > + default: /* this is just in case for now ... */
> > + err |= __put_user(kinfo->si_pid, &uinfo->pid);
> > + err |= __put_user(kinfo->si_uid, &uinfo->uid);
> > + break;
> > + }
> > +
> > + return err ? -EFAULT: sizeof(*uinfo);
> > +}
>
> Doing it this way looks rather inefficient to me. I think it's
> better to just prepare the signalfd_siginfo on the stack and
> do a single copy_to_user.
bah, __put_user is basically a move, so I don't think that efficency would
be that different (assuming that it'd matter in this case). The only thing
many __put_user do, is increase the exception table sizes.
> Also, what's the reasoning behind defining a new structure
> instead of just returning siginfo_t? Sure siginfo_t is ugly
> but it is a well-defined structure and users already deal
> with the problems it causes.
Compat on sys_read() would be insane ;)
>
> > +static void __exit signalfd_exit(void)
> > +{
> > + kmem_cache_destroy(signalfd_ctx_cachep);
> > +}
> > +
> > +module_init(signalfd_init);
> > +module_exit(signalfd_exit);
> > +
> > +MODULE_LICENSE("GPL");
>
> Since this file defines a syscall, it can't be a module, so why bother
> with this?
Agreed, remove exit function and using fs_initcall.
>
> > +
> > +struct signalfd_siginfo {
> > + __u32 signo;
> > + __s32 err;
> > + __s32 code;
> > + __u32 pid;
> > + __u32 uid;
> > + __s32 fd;
> > + __u32 tid;
> > + __u32 band;
> > + __u32 overrun;
> > + __u32 trapno;
> > + __s32 status;
> > + __s32 svint;
> > + __u64 svptr;
> > + __u64 utime;
> > + __u64 stime;
> > + __u64 addr;
> > +};
> > +
>
> Since you define the structure using __u32 etc types, I assume
> you mean it to be included from libc or other user space, right?
> In this case it needs to be listed in include/linux/Kbuild for
> make headers_install to work.
Ack!
>
> > +void signalfd_deliver(struct task_struct *tsk, int sig);
> > +
> > +/*
> > + * No need to fall inside signalfd_deliver() if no signal listeners are
> > available. + */
> > +static inline void signalfd_notify(struct task_struct *tsk, int sig)
> > +{
> > + if (unlikely(!list_empty(&tsk->sighand->sfdlist)))
> > + signalfd_deliver(tsk, sig);
> > +}
> > +
> > +static inline void signalfd_detach_locked(struct task_struct *tsk)
> > +{
> > + if (unlikely(!list_empty(&tsk->sighand->sfdlist)))
> > + signalfd_deliver(tsk, -1);
> > +}
> > +
> > +static inline void signalfd_detach(struct task_struct *tsk)
> > +{
> > + struct sighand_struct *sighand = tsk->sighand;
> > +
> > + if (unlikely(!list_empty(&sighand->sfdlist))) {
> > + spin_lock_irq(&sighand->siglock);
> > + signalfd_deliver(tsk, -1);
> > + spin_unlock_irq(&sighand->siglock);
> > + }
> > +}
> > +
>
> And all of these need to be surrounded by #ifdef __KERNEL__ so
> they don't bleed out to the user space visible parts.
Ack!
- Davide
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
