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

[Oleg Nesterov <oleg@redhat.com>]

On 12/27, Andrey Wagin wrote:
>
> On Wed, Dec 26, 2012 at 05:31:12PM +0100, Oleg Nesterov wrote:
> >
> > 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.
>
> Yes. It is a good idea. A new patch is attached to this email. I
> implemented pread for signalfd and fixed all your previous comments.
>
> +static ssize_t signalfd_peek(struct signalfd_ctx *ctx,
> +				siginfo_t *info, unsigned long ppos)
> +{
> +	struct sigpending *pending;
> +	struct sigqueue *q;
> +	bool group = 0;
> +	loff_t seq, i;
> +	int ret = 0;
> +
> +	if (ppos < SIGNALFD_PRIVATE_OFFSET)
> +		return -EINVAL;
> +
> +	if (ppos >= SIGNALFD_SHARED_OFFSET) {
> +		seq = ppos - SIGNALFD_SHARED_OFFSET;
> +		group = 1;
> +	} else
> +		seq = ppos - SIGNALFD_PRIVATE_OFFSET;
> +
> +	spin_lock_irq(&current->sighand->siglock);
> +
> +	if (group)
> +		pending = &current->signal->shared_pending;
> +	else
> +		pending = &current->pending;

Oh, this looks overcomplicated. Why do you need "bool group" ? Just do

	if (ppos > SHARED) {
		seq = ppos - SHARED;
		pending = &current->signal->shared_pending;
	} else if (ppos > PRIVATE) {
		seq = ppos - PRIVATE;
		pending = &current->pending;
	} else {
		return -EINVAL;
	}

	spin_lock_irq(&current->sighand->siglock);
	...

note also I made the PRIVATE/SHARED checks more symmetric, but this is minor.

In fact I think "loff_t i" is not needed as well. You can check --seq == 0
instead in the loop below, but this is up to you.

> @@ -230,7 +274,11 @@ static ssize_t signalfd_read(struct file *file, char __user *buf, size_t count,
>
>  	siginfo = (struct signalfd_siginfo __user *) buf;
>  	do {
> -		ret = signalfd_dequeue(ctx, &info, nonblock);
> +		if (*ppos == 0)
> +			ret = signalfd_dequeue(ctx, &info, nonblock);
> +		else
> +			ret = signalfd_peek(ctx, &info, *ppos);

I think it would be better to pass ppos, not *ppos, because ...

> +		if (*ppos)
> +			(*ppos)++;

in this case we can update *ppos in signalfd_peek() and simplify the
code a bit.

Compared to the previous version it is "safe" to change *ppos even if
copy_to_user() fails, *ppos will be "lost" anyway after we return.

> @@ -321,6 +372,7 @@ SYSCALL_DEFINE4(signalfd4, int, ufd, sigset_t __user *, user_mask,
>  		}
>
>  		file->f_flags |= flags & SFD_RAW;
> +		file->f_mode |= FMODE_PREAD;
>
>  		fd_install(ufd, file);

Hmm. Looks like it is based on other patches I didnt see...

But I don't understand how FMODE_PREAD connects to this patch, we need
this flag set even for regular sys_read() ???

> +#define SIGNALFD_SHARED_OFFSET (1LL << 62)

OK... this assumes we are not going to add more SIGNAL_*_OFFSET's, and
probably this is true...

Oleg.