Re: [CRIU] [PATCH 1/4] signalfd: add ability to return siginfo in a raw format

[Oleg Nesterov <oleg@redhat.com>]

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.