Re: [PATCH 3/3] signalfd: add ability to read siginfo-s without dequeuing signals (v2)

quoted

On Fri, Feb 8, 2013 at 8:10 PM, Oleg Nesterov [off-list ref] wrote:
On 02/08, Andrey Wagin wrote:
2013/2/7 Oleg Nesterov [off-list ref]:
Andrey, sorry for delay.

As for API, I leave this to you and Michael. Not that I like these
new flags, but I agree that pread() hack was not pretty too.

On 01/29, Andrey Vagin wrote:
+static ssize_t signalfd_peek(struct signalfd_ctx *ctx,
+                             siginfo_t *info, loff_t *ppos, int queue_mask)
+{
+     loff_t seq = *ppos / sizeof(struct signalfd_siginfo);
+     int signr = 0;
+
+     if (queue_mask & SIGQUEUE_PRIVATE)
+             signr = peek_signal(&current->pending,
+                                     &ctx->sigmask, info, &seq);
+     else if (queue_mask & SIGQUEUE_SHARED)
+             signr = peek_signal(&current->signal->shared_pending,
+                                      &ctx->sigmask, info, &seq);
+     (*ppos) += sizeof(struct signalfd_siginfo);
Now that this can work even with normal read(), we will actually change
f_pos. Then perhaps signalfd_fops->llseek() should work too. But this
is minor...
lseek works only if FMODE_LSEEK is set.

You have explained why read&lseek have strange semantics for SIGNALFD_PEEK.

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.
Yes. but I thought you decided to ignore this oddity ;)

So I want to suggest a way how to forbid read() for SIGNALFD_PEEK.
file->f_pos can be initialized to -1. read() returns EINVAL in this
case. In a man page we will write that signals can be dumped only with
help pread(). Is it overload or too ugly?
Well. I do not know. Up to you and Michael.

But honestly, I can't say this all looks really nice. And why do we
need SIGNALFD_PEEK then?
It surely is no beauty. The hope is at least to make it less ugly than it was.

Seriously, perhaps we should simply add signalfd_fops->ioctl() for PEEK.
Or add PTRACE_{PEEK,POKE}_SIGNAL which looks even logical and useful...
And much simpler/straightforward.

But I am not going to argue.
I suppose I had wondered along similar lines, but in a slightly
different direction: would the use of a /proc interface to get the
queued signals make some sense?

Cheers,

Michael


-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Author of "The Linux Programming Interface"; http://man7.org/tlpi/
On 02/08, Michael Kerrisk (man-pages) wrote:
On Fri, Feb 8, 2013 at 8:10 PM, Oleg Nesterov [off-list ref] wrote:
Well. I do not know. Up to you and Michael.

But honestly, I can't say this all looks really nice. And why do we
need SIGNALFD_PEEK then?
It surely is no beauty. The hope is at least to make it less ugly than it was.
This is subjective, but I am not sure about "less" ;) Yes, we avoid the
magic offsets, but we add SFD_SHARED/PER_THREAD which need to change
dequeue_signal plus other complications. And for what?

Seriously, perhaps we should simply add signalfd_fops->ioctl() for PEEK.
Or add PTRACE_{PEEK,POKE}_SIGNAL which looks even logical and useful...
And much simpler/straightforward.

But I am not going to argue.
I suppose I had wondered along similar lines, but in a slightly
different direction: would the use of a /proc interface to get the
queued signals make some sense?
(Can't resist sorry... yes we need /proc/pid/cr or /dev/cr or whatever
 which dumps almost everything c/r needs without need to add a lot of
 cr code everywhere).

Perhaps, but I am not sure about the textual representation.

And to me, the best solution is the simplest solution. Please look
at the patch below. It is trivial. And we can also drop the SFD_RAW
patch in -mm.

Oleg.

--- x/kernel/ptrace.c
+++ x/kernel/ptrace.c
@@ -618,6 +618,35 @@ static int ptrace_setsiginfo(struct task
 	return error;
 }
 
+static int ptrace_peek_signal(struct task_struct *child,
+				unsigned long addr, siginfo_t __user *uinfo)
+{
+	siginfo_t info;
+	struct sigpending *pending;
+	int ret = -ESOMETHING;
+
+	pending = &child->pending;
+	if (addr & PTRACE_PEEK_SHARED) {
+		addr &= ~PTRACE_PEEK_SHARED;
+		pending = &child->signal->shared_pending;
+	}
+
+	spin_lock_irq(&child->sighand->siglock);
+	list_for_each_entry(q, &pending->list, list) {
+		if (!addr--) {
+			copy_siginfo(info, &q->info);
+			ret = 0;
+			break;
+		}
+	}
+	spin_lock_irq(&child->sighand->siglock);
+
+	if (!ret)
+		ret = copy_siginfo_to_user(uinfo, info);
+	if (!ret)
+		ret = __put_user(info, si_code);
+	return ret;
+}
 
 #ifdef PTRACE_SINGLESTEP
 #define is_singlestep(request)		((request) == PTRACE_SINGLESTEP)
@@ -742,6 +771,10 @@ int ptrace_request(struct task_struct *c
 		ret = put_user(child->ptrace_message, datalp);
 		break;
 
+	case PTRACE_PEEKSIGNAL:
+		ret = ptrace_peek_signal(child, addr, datavp);
+		break;
+
 	case PTRACE_GETSIGINFO:
 		ret = ptrace_getsiginfo(child, &siginfo);
 		if (!ret)
On Sat, Feb 09, 2013 at 07:22:39PM +0100, Oleg Nesterov wrote:
On 02/08, Michael Kerrisk (man-pages) wrote:
On Fri, Feb 8, 2013 at 8:10 PM, Oleg Nesterov [off-list ref] wrote:
Well. I do not know. Up to you and Michael.

But honestly, I can't say this all looks really nice. And why do we
need SIGNALFD_PEEK then?
It surely is no beauty. The hope is at least to make it less ugly than it was.
This is subjective, but I am not sure about "less" ;) Yes, we avoid the
magic offsets, but we add SFD_SHARED/PER_THREAD which need to change
dequeue_signal plus other complications. And for what?

Seriously, perhaps we should simply add signalfd_fops->ioctl() for PEEK.
Or add PTRACE_{PEEK,POKE}_SIGNAL which looks even logical and useful...
And much simpler/straightforward.

But I am not going to argue.
I suppose I had wondered along similar lines, but in a slightly
different direction: would the use of a /proc interface to get the
queued signals make some sense?
(Can't resist sorry... yes we need /proc/pid/cr or /dev/cr or whatever
 which dumps almost everything c/r needs without need to add a lot of
 cr code everywhere).

Perhaps, but I am not sure about the textual representation.

And to me, the best solution is the simplest solution. Please look
at the patch below. It is trivial. And we can also drop the SFD_RAW
patch in -mm.
I'm agree with you and I will ask Andrew to drop the SFD_RAW patch.

Thanks.

Oleg.

--- x/kernel/ptrace.c
+++ x/kernel/ptrace.c
@@ -618,6 +618,35 @@ static int ptrace_setsiginfo(struct task
 	return error;
 }
 
+static int ptrace_peek_signal(struct task_struct *child,
+				unsigned long addr, siginfo_t __user *uinfo)
+{
+	siginfo_t info;
+	struct sigpending *pending;
+	int ret = -ESOMETHING;
+
+	pending = &child->pending;
+	if (addr & PTRACE_PEEK_SHARED) {
+		addr &= ~PTRACE_PEEK_SHARED;
+		pending = &child->signal->shared_pending;
+	}
+
+	spin_lock_irq(&child->sighand->siglock);
+	list_for_each_entry(q, &pending->list, list) {
+		if (!addr--) {
+			copy_siginfo(info, &q->info);
+			ret = 0;
+			break;
+		}
+	}
+	spin_lock_irq(&child->sighand->siglock);
+
+	if (!ret)
+		ret = copy_siginfo_to_user(uinfo, info);
+	if (!ret)
+		ret = __put_user(info, si_code);
+	return ret;
+}
 
 #ifdef PTRACE_SINGLESTEP
 #define is_singlestep(request)		((request) == PTRACE_SINGLESTEP)
@@ -742,6 +771,10 @@ int ptrace_request(struct task_struct *c
 		ret = put_user(child->ptrace_message, datalp);
 		break;
 
+	case PTRACE_PEEKSIGNAL:
+		ret = ptrace_peek_signal(child, addr, datavp);
+		break;
+
 	case PTRACE_GETSIGINFO:
 		ret = ptrace_getsiginfo(child, &siginfo);
 		if (!ret)

_______________________________________________
CRIU mailing list
CRIU@openvz.org
https://lists.openvz.org/mailman/listinfo/criu
On Friday 08 February 2013 21:15, Michael Kerrisk (man-pages) wrote:
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.
Yes. but I thought you decided to ignore this oddity ;)

So I want to suggest a way how to forbid read() for SIGNALFD_PEEK.
file->f_pos can be initialized to -1. read() returns EINVAL in this
case. In a man page we will write that signals can be dumped only with
help pread(). Is it overload or too ugly?
Well. I do not know. Up to you and Michael.

But honestly, I can't say this all looks really nice. And why do we
need SIGNALFD_PEEK then?
It surely is no beauty. The hope is at least to make it less ugly than it was.

Seriously, perhaps we should simply add signalfd_fops->ioctl() for PEEK.
Or add PTRACE_{PEEK,POKE}_SIGNAL which looks even logical and useful...
And much simpler/straightforward.

But I am not going to argue.
ptrace interface might find some use in debuggers.
Not that any of them expressed such desires so far,
but just maybe.

However, it needs coding in C to read it,
which brings me to:


I suppose I had wondered along similar lines, but in a slightly
different direction: would the use of a /proc interface to get the
queued signals make some sense?
I think that /proc interface beats adding magic flags and magic semantic
to [p]read.

It also has the benefit of being human-readable. You don't need
to write a special C program to "cat /proc/$$/foo".

Andrey, I know that it is hard to let go of the code you invested time
and efforts in creating. But this isn't the last patch, is it?
You will need to retrieve yet more data for process checkpointing.
When you start working on the next patch for it, consider trying
/proc approach.

-- 
vda
On Mon, Feb 11, 2013 at 10:29:50AM +0100, Denys Vlasenko wrote:
On Friday 08 February 2013 21:15, Michael Kerrisk (man-pages) wrote:
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.
Yes. but I thought you decided to ignore this oddity ;)

So I want to suggest a way how to forbid read() for SIGNALFD_PEEK.
file->f_pos can be initialized to -1. read() returns EINVAL in this
case. In a man page we will write that signals can be dumped only with
help pread(). Is it overload or too ugly?
Well. I do not know. Up to you and Michael.

But honestly, I can't say this all looks really nice. And why do we
need SIGNALFD_PEEK then?
It surely is no beauty. The hope is at least to make it less ugly than it was.

Seriously, perhaps we should simply add signalfd_fops->ioctl() for PEEK.
Or add PTRACE_{PEEK,POKE}_SIGNAL which looks even logical and useful...
And much simpler/straightforward.

But I am not going to argue.
ptrace interface might find some use in debuggers.
Not that any of them expressed such desires so far,
but just maybe.

However, it needs coding in C to read it,
which brings me to:


I suppose I had wondered along similar lines, but in a slightly
different direction: would the use of a /proc interface to get the
queued signals make some sense?
I think that /proc interface beats adding magic flags and magic semantic
to [p]read.

It also has the benefit of being human-readable. You don't need
to write a special C program to "cat /proc/$$/foo".

Andrey, I know that it is hard to let go of the code you invested time
and efforts in creating. But this isn't the last patch, is it?
You will need to retrieve yet more data for process checkpointing.
When you start working on the next patch for it, consider trying
/proc approach.
I don't think that we need to convert siginfo into a human readable format
in kernel. For example siginfo with a negative si_code contains binary data.
A kernel already has got signalfd_siginfo and we can see, that it's not
good.
I think would be better to print siginfo in a human readable format from
userspace (e.g. gdb) and implement a generic interface in kernel (e.g.
ptrace).

-- 
vda
_______________________________________________
CRIU mailing list
CRIU@openvz.org
https://lists.openvz.org/mailman/listinfo/criu

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help

Re: [PATCH 3/3] signalfd: add ability to read siginfo-s without dequeuing signals (v2)