On 12/26, Andrew Vagin wrote:
>
> On Tue, Dec 25, 2012 at 05:58:03PM +0100, Oleg Nesterov wrote:
> > On 12/25, Pavel Emelyanov wrote:
> > >
> > > On 12/25/2012 07:27 PM, Oleg Nesterov wrote:
> > > >
> > > > I guess that probably you actually need DUMP, not DEQUEUE. but the
> > > > latter is not trivial. However, perhaps we can do this assuming that
> > > > all other threads are sleeping and nobody can do dequeue_signal().
> > > > Say, we can play with ppos/llseek. If *ppos is not zero,
> > > > signalfd_dequeue() could dump the nth entry from list or return 0.
> > >
> > > This would be perfect, but isn't it better to preserve the pos
> > > semantics -- we do know size of entry we're about to copy, we can
> > > treat pos as offset in bytes, not in elements.
> >
> > nr-of-records looks better (more flexible) than nr-of-bytes to me. And
> > perhaps we can also encode private-or-shared into ppos. But I will not
> > argue in any case.
>
> Oleg and Pavel, could you look at these two patches. I implemented in them,
> what you described here.
cosmetics nits below, feel free to ignore...
Damn. But after I wrote this email I realized that llseek() probably can't
work. Because peek_offset/f_pos/whatever has to be shared with all processes
which have this file opened.
Suppose that the task forks after sys_signalfd(). Now if parent or child
do llseek this affects them both. This is insane because signalfd is
"strange" to say at least, fork/dup/etc inherits signalfd_ctx but not the
"source" of the data.
So I think we should not use llseek. But, probably we can rely on pread() ?
This way we can avoid the problem above, and this looks even simpler.
> +int peek_signal(struct task_struct *tsk, sigset_t *mask,
> + siginfo_t *info, int offset, bool group)
> +{
> + struct sigpending *pending;
> + struct sigqueue *q;
> + int i = 0, ret = 0;
> +
> + if (group)
> + pending = &tsk->signal->shared_pending;
> + else
> + pending = &tsk->pending;
> +
> + list_for_each_entry(q, &pending->list, list) {
> + if (sigismember(mask, q->info.si_signo))
> + continue;
> +
> + if (i == offset) {
> + copy_siginfo(info, &q->info);
> + ret = info->si_signo;
> + break;
> + }
> +
> + i++;
> + }
> +
> + return ret;
> +}
This helper is trivial. Any reason it should live in signal.c ? Just put
this code into signalfd_peek(). Besides, this helps if !CONFIG_SIGNALFD.
> If lseek sets a positive position, signals are taken from a shared queue.
> If lseek sets a negative position, signals are taken from a private queue.
Personally, I'd prefer, say,
#define SIGNALFD_SHARED_OFFSET big-enough-number
if offset >= SIGNALFD_SHARED_OFFSET we use ->shared_pending.
Again, I won't insist. And if we add SIGNALFD_SHARED_OFFSET
then we should probably define SIGNALFD_PRIVATE_OFFSET as
well for consistency.
> struct signalfd_ctx {
> sigset_t sigmask;
> + loff_t peek_offset;
Why we can't simply use ->f_pos ?
> @@ -242,6 +259,13 @@ static ssize_t signalfd_read(struct file *file, char __user *buf, size_t count,
> if (ret < 0)
> break;
>
> + if (ctx->peek_offset) {
> + if (ctx->peek_offset > 0)
> + ctx->peek_offset++;
> + else
> + ctx->peek_offset--;
...
> +loff_t signalfd_llseek(struct file *f, loff_t offset, int whence)
> +{
> + struct signalfd_ctx *ctx = f->private_data;
> +
> + switch (whence) {
> + case SEEK_SET:
> + ctx->peek_offset = offset;
> + break;
> + case SEEK_CUR:
> + ctx->peek_offset += offset;
probably you need some locking (say, f_lock) to ensure that these
peek_offset modifications can't race with each other. If you rely
on f_pos you only need to ensure thar signalfd_llseek() can't race
with itself.
Oleg.
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
