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

[Andrey Wagin]

2012/12/28 Oleg Nesterov <oleg@xxxxxxxxxx>:
> On 12/27, Andrey Wagin wrote:
>>
>> 2012/12/27 Oleg Nesterov <oleg@xxxxxxxxxx>:
>> > 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.
>> >>
>> ...
>> > 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.
>>
>> Yes. But ppos is updated, because pread() reads all siginfo-s from a queue,
>> which fit in a buffer starting with the initial ppos.
>
> Can't understand. And I guess you misunderstood.
>
> I meant, we can update *ppos in signalfd_peek() _and_ we can do this
> unconditionally even if copy_to_user() failed.

I see. Any way I fixed that in a new version of this patch. Thanks.

The new patch is a part of
[PATCH 0/3] signalfd: a kernel interface for dumping/restoring pending
signals (v2)

>
>> >> @@ -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() ???
>>
>> It doesn't need for sys_read(), but this patch is about pread() and
>> sys_pread() checks it.
>>
>> SYSCALL_DEFINE(pread64)(unsigned int fd, char __user *buf,
>>                         size_t count, loff_t pos)
>> {
>> ....
>>         if (f.file) {
>>                 ret = -ESPIPE;
>>                 if (f.file->f_mode & FMODE_PREAD)
>>                         ret = vfs_read(f.file, buf, count, &pos);
>
> And sys_read() checks it too, that was my point.

sys_read() doesn't check this flag. I tryed to remove this code from
this patch and pread returned ESPIPE as expected.

>
> So either this code was already broken before this patch, or FMODE_PREAD
> is already set and you do not need this chunk.

>
> Oleg.
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/