lkml.org 
[lkml]   [2012]   [Dec]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [CRIU] [PATCH 1/4] signalfd: add ability to return siginfo in a raw format
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.


\
 
 \ /
  Last update: 2012-12-27 17:01    [W:0.065 / U:0.404 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site