Message-ID: <20130210100724.GB15978@paralelels.com>
Date:	Sun, 10 Feb 2013 14:07:24 +0400
From:	Andrew Vagin <avagin@...allels.com>
To:	Oleg Nesterov <oleg@...hat.com>
CC:	"Michael Kerrisk (man-pages)" <mtk.manpages@...il.com>,
	David Howells <dhowells@...hat.com>,
	Pavel Emelyanov <xemul@...allels.com>,
	<linux-api@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<criu@...nvz.org>, Cyrill Gorcunov <gorcunov@...nvz.org>,
	Andrey Wagin <avagin@...il.com>,
	Alexander Viro <viro@...iv.linux.org.uk>,
	<linux-fsdevel@...r.kernel.org>, Dave Jones <davej@...hat.com>,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
Subject: Re: [CRIU] [PATCH 3/3] signalfd: add ability to read siginfo-s
 without	dequeuing signals (v2)

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 <oleg@...hat.com> 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@...nvz.org
> https://lists.openvz.org/mailman/listinfo/criu
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists