[v4,2/5] drm: Add private data field to trace control block

Submitted by Patrik Jakobsson on Aug. 24, 2015, 12:42 p.m.

Details

Message ID 1440420170-13337-3-git-send-email-patrik.jakobsson@linux.intel.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Patrik Jakobsson Aug. 24, 2015, 12:42 p.m.
We need to be able to store private data in the tcb across it's
lifetime. To ensure proper destruction of the data a free_priv_data
callback must be provided if an allocation is stored in priv_data. The
callback is executed automatically when the life of the tcb ends.

* defs.h: Add extern declaration of free_tcb_priv_data.
 (struct tcb): Add priv_data and free_priv_data.
* strace.c (free_tcb_priv_data): New function
(drop_tcb): Execute free_tcb_priv_data callback
* syscall.c (trace_syscall_exiting): Execute free_tcb_priv_data callback

Signed-off-by: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
---
 defs.h    |  6 ++++++
 strace.c  | 14 ++++++++++++++
 syscall.c |  1 +
 3 files changed, 21 insertions(+)

Patch hide | download patch | download mbox

diff --git a/defs.h b/defs.h
index 9059026..bc3bd83 100644
--- a/defs.h
+++ b/defs.h
@@ -266,6 +266,10 @@  struct tcb {
 	int u_error;		/* Error code */
 	long scno;		/* System call number */
 	long u_arg[MAX_ARGS];	/* System call arguments */
+
+	void *priv_data;	/* Private data for syscall decoding functions */
+	void (*free_priv_data)(void *); /* Callback for freeing priv_data */
+
 #if defined(LINUX_MIPSN32) || defined(X32)
 	long long ext_arg[MAX_ARGS];
 	long long u_lrval;	/* long long return value */
@@ -470,6 +474,8 @@  extern void get_regs(pid_t pid);
 extern int get_scno(struct tcb *tcp);
 extern const char *syscall_name(long scno);
 
+extern void free_tcb_priv_data(struct tcb *tcp);
+
 extern int umoven(struct tcb *, long, unsigned int, void *);
 #define umove(pid, addr, objp)	\
 	umoven((pid), (addr), sizeof(*(objp)), (void *) (objp))
diff --git a/strace.c b/strace.c
index 9b93e79..7a71152 100644
--- a/strace.c
+++ b/strace.c
@@ -711,12 +711,26 @@  alloctcb(int pid)
 	error_msg_and_die("bug in alloctcb");
 }
 
+void
+free_tcb_priv_data(struct tcb *tcp)
+{
+	if (tcp->priv_data) {
+		if (tcp->free_priv_data) {
+			tcp->free_priv_data(tcp->priv_data);
+			tcp->free_priv_data = NULL;
+		}
+		tcp->priv_data = NULL;
+	}
+}
+
 static void
 droptcb(struct tcb *tcp)
 {
 	if (tcp->pid == 0)
 		return;
 
+	free_tcb_priv_data(tcp);
+
 #ifdef USE_LIBUNWIND
 	if (stack_trace_enabled) {
 		unwind_tcb_fin(tcp);
diff --git a/syscall.c b/syscall.c
index 396a7dd..39b973b 100644
--- a/syscall.c
+++ b/syscall.c
@@ -1099,6 +1099,7 @@  trace_syscall_exiting(struct tcb *tcp)
 #endif
 
  ret:
+	free_tcb_priv_data(tcp);
 	tcp->flags &= ~TCB_INSYSCALL;
 	tcp->sys_func_rval = 0;
 	return 0;

Comments

On 24 Aug 2015 14:42, Patrik Jakobsson wrote:
> We need to be able to store private data in the tcb across it's
> lifetime. To ensure proper destruction of the data a free_priv_data
> callback must be provided if an allocation is stored in priv_data. The
> callback is executed automatically when the life of the tcb ends.
> 
> * defs.h: Add extern declaration of free_tcb_priv_data.
>  (struct tcb): Add priv_data and free_priv_data.
> * strace.c (free_tcb_priv_data): New function
> (drop_tcb): Execute free_tcb_priv_data callback
> * syscall.c (trace_syscall_exiting): Execute free_tcb_priv_data callback
> 
> Signed-off-by: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
> ---
>  defs.h    |  6 ++++++
>  strace.c  | 14 ++++++++++++++
>  syscall.c |  1 +
>  3 files changed, 21 insertions(+)
> 
> diff --git a/defs.h b/defs.h
> index 9059026..bc3bd83 100644
> --- a/defs.h
> +++ b/defs.h
> @@ -266,6 +266,10 @@ struct tcb {
>  	int u_error;		/* Error code */
>  	long scno;		/* System call number */
>  	long u_arg[MAX_ARGS];	/* System call arguments */
> +
> +	void *priv_data;	/* Private data for syscall decoding functions */
> +	void (*free_priv_data)(void *); /* Callback for freeing priv_data */

should we name these _priv_data and _free_priv_data and provides accessor
functions ?  i worry that code paths might stomp on each other by accident
and we don't end up noticing.

static void set_tcb_priv_data(struct tcb *tcp, void *data, void (*free_data)(void *))
{
	assert(tcp->_priv_data == NULL && tcp->_free_priv_data == NULL);
	...
}
-mike
On Tue, Aug 25, 2015 at 11:12 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> On 24 Aug 2015 14:42, Patrik Jakobsson wrote:
>> We need to be able to store private data in the tcb across it's
>> lifetime. To ensure proper destruction of the data a free_priv_data
>> callback must be provided if an allocation is stored in priv_data. The
>> callback is executed automatically when the life of the tcb ends.
>>
>> * defs.h: Add extern declaration of free_tcb_priv_data.
>>  (struct tcb): Add priv_data and free_priv_data.
>> * strace.c (free_tcb_priv_data): New function
>> (drop_tcb): Execute free_tcb_priv_data callback
>> * syscall.c (trace_syscall_exiting): Execute free_tcb_priv_data callback
>>
>> Signed-off-by: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
>> ---
>>  defs.h    |  6 ++++++
>>  strace.c  | 14 ++++++++++++++
>>  syscall.c |  1 +
>>  3 files changed, 21 insertions(+)
>>
>> diff --git a/defs.h b/defs.h
>> index 9059026..bc3bd83 100644
>> --- a/defs.h
>> +++ b/defs.h
>> @@ -266,6 +266,10 @@ struct tcb {
>>       int u_error;            /* Error code */
>>       long scno;              /* System call number */
>>       long u_arg[MAX_ARGS];   /* System call arguments */
>> +
>> +     void *priv_data;        /* Private data for syscall decoding functions */
>> +     void (*free_priv_data)(void *); /* Callback for freeing priv_data */
>
> should we name these _priv_data and _free_priv_data and provides accessor
> functions ?  i worry that code paths might stomp on each other by accident
> and we don't end up noticing.
>
> static void set_tcb_priv_data(struct tcb *tcp, void *data, void (*free_data)(void *))
> {
>         assert(tcp->_priv_data == NULL && tcp->_free_priv_data == NULL);
>         ...
> }
> -mike

Yes, that's a good idea. My use case is pretty simple but usage can easliy grow.

I'll resend this patch and take it out of the drm/i915 series.

-Patrik
On Wed, Aug 26, 2015 at 03:26:08PM +0200, Patrik Jakobsson wrote:
> On Tue, Aug 25, 2015 at 11:12 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> > On 24 Aug 2015 14:42, Patrik Jakobsson wrote:
> >> We need to be able to store private data in the tcb across it's
> >> lifetime. To ensure proper destruction of the data a free_priv_data
> >> callback must be provided if an allocation is stored in priv_data. The
> >> callback is executed automatically when the life of the tcb ends.
> >>
> >> * defs.h: Add extern declaration of free_tcb_priv_data.
> >>  (struct tcb): Add priv_data and free_priv_data.
> >> * strace.c (free_tcb_priv_data): New function
> >> (drop_tcb): Execute free_tcb_priv_data callback
> >> * syscall.c (trace_syscall_exiting): Execute free_tcb_priv_data callback
> >>
> >> Signed-off-by: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
> >> ---
> >>  defs.h    |  6 ++++++
> >>  strace.c  | 14 ++++++++++++++
> >>  syscall.c |  1 +
> >>  3 files changed, 21 insertions(+)
> >>
> >> diff --git a/defs.h b/defs.h
> >> index 9059026..bc3bd83 100644
> >> --- a/defs.h
> >> +++ b/defs.h
> >> @@ -266,6 +266,10 @@ struct tcb {
> >>       int u_error;            /* Error code */
> >>       long scno;              /* System call number */
> >>       long u_arg[MAX_ARGS];   /* System call arguments */
> >> +
> >> +     void *priv_data;        /* Private data for syscall decoding functions */
> >> +     void (*free_priv_data)(void *); /* Callback for freeing priv_data */
> >
> > should we name these _priv_data and _free_priv_data and provides accessor
> > functions ?  i worry that code paths might stomp on each other by accident
> > and we don't end up noticing.
> >
> > static void set_tcb_priv_data(struct tcb *tcp, void *data, void (*free_data)(void *))
> > {
> >         assert(tcp->_priv_data == NULL && tcp->_free_priv_data == NULL);
> >         ...
> > }
> > -mike
> 
> Yes, that's a good idea. My use case is pretty simple but usage can easliy grow.
> 
> I'll resend this patch and take it out of the drm/i915 series.
> 
> -Patrik

Here's my take on it (I assume it needs some discussion):

int
set_tcb_priv_data(struct tcb *tcp, void *priv_data)
{
	/* A free callback is required before setting private data and private
	 * data must be set back to NULL before being set again.
	 */
	if (tcp->_free_priv_data == NULL ||
	    (tcp->_priv_data && priv_data != NULL))
		return -1;

	tcp->_priv_data = priv_data;
	return 0;
}

void *
get_tcb_priv_data(struct tcb *tcp)
{
	return tcp->_priv_data;
}

int
set_tcb_free_priv_data(struct tcb *tcp, void (*free_priv_data)(void *))
{
 	/* _free_priv_data must be set back to NULL before being set again. */
	if (tcp->_free_priv_data && free_priv_data != NULL)
		return -1;

	tcp->_free_priv_data = free_priv_data;
	return 0;
}

void
free_tcb_priv_data(struct tcb *tcp)
{
	if (tcp->_priv_data) {
		if (tcp->_free_priv_data) {
			tcp->_free_priv_data(tcp->_priv_data);
			tcp->_free_priv_data = NULL;
		}
		tcp->_priv_data = NULL;
	}
}

Cheers
Patrik
On Mon, Aug 31, 2015 at 02:37:07PM +0200, Patrik Jakobsson wrote:
[...]
> Here's my take on it (I assume it needs some discussion):
> 
> int
> set_tcb_priv_data(struct tcb *tcp, void *priv_data)
> {
> 	/* A free callback is required before setting private data and private
> 	 * data must be set back to NULL before being set again.
> 	 */

I think a single function initializing both _priv_data and _free_priv_data
would suffice:

int
set_tcb_priv_data(struct tcb *tcp, void *priv_data,
		  void (*free_priv_data)(void *))
{
	if (tcp->_priv_data)
		return -1;

	tcp->_free_priv_data = free_priv_data;
	tcp->_priv_data = priv_data;

	return 0;
}
On Mon, Sep 7, 2015 at 6:51 PM, Dmitry V. Levin <ldv@altlinux.org> wrote:
> On Mon, Aug 31, 2015 at 02:37:07PM +0200, Patrik Jakobsson wrote:
> [...]
>> Here's my take on it (I assume it needs some discussion):
>>
>> int
>> set_tcb_priv_data(struct tcb *tcp, void *priv_data)
>> {
>>       /* A free callback is required before setting private data and private
>>        * data must be set back to NULL before being set again.
>>        */
>
> I think a single function initializing both _priv_data and _free_priv_data
> would suffice:
>
> int
> set_tcb_priv_data(struct tcb *tcp, void *priv_data,
>                   void (*free_priv_data)(void *))
> {
>         if (tcp->_priv_data)
>                 return -1;
>
>         tcp->_free_priv_data = free_priv_data;
>         tcp->_priv_data = priv_data;
>
>         return 0;
> }

Sure, and since they always come in a pairs it might be even better. If it turns
out we need it split up it is easily done later.

Thanks
Patrik
On Mon, Sep 07, 2015 at 08:23:57PM +0200, Patrik Jakobsson wrote:
> On Mon, Sep 7, 2015 at 6:51 PM, Dmitry V. Levin wrote:
> > On Mon, Aug 31, 2015 at 02:37:07PM +0200, Patrik Jakobsson wrote:
> > [...]
> >> Here's my take on it (I assume it needs some discussion):
> >>
> >> int
> >> set_tcb_priv_data(struct tcb *tcp, void *priv_data)
> >> {
> >>       /* A free callback is required before setting private data and private
> >>        * data must be set back to NULL before being set again.
> >>        */
> >
> > I think a single function initializing both _priv_data and _free_priv_data
> > would suffice:
> >
> > int
> > set_tcb_priv_data(struct tcb *tcp, void *priv_data,
> >                   void (*free_priv_data)(void *))
> > {
> >         if (tcp->_priv_data)
> >                 return -1;
> >
> >         tcp->_free_priv_data = free_priv_data;
> >         tcp->_priv_data = priv_data;
> >
> >         return 0;
> > }
> 
> Sure, and since they always come in a pairs it might be even better. If it turns
> out we need it split up it is easily done later.

The discussion seems to be stalled.
Patrik, would you like to prepare a patch?
On Tue, Nov 24, 2015 at 6:46 AM, Dmitry V. Levin <ldv@altlinux.org> wrote:
> On Mon, Sep 07, 2015 at 08:23:57PM +0200, Patrik Jakobsson wrote:
>> On Mon, Sep 7, 2015 at 6:51 PM, Dmitry V. Levin wrote:
>> > On Mon, Aug 31, 2015 at 02:37:07PM +0200, Patrik Jakobsson wrote:
>> > [...]
>> >> Here's my take on it (I assume it needs some discussion):
>> >>
>> >> int
>> >> set_tcb_priv_data(struct tcb *tcp, void *priv_data)
>> >> {
>> >>       /* A free callback is required before setting private data and private
>> >>        * data must be set back to NULL before being set again.
>> >>        */
>> >
>> > I think a single function initializing both _priv_data and _free_priv_data
>> > would suffice:
>> >
>> > int
>> > set_tcb_priv_data(struct tcb *tcp, void *priv_data,
>> >                   void (*free_priv_data)(void *))
>> > {
>> >         if (tcp->_priv_data)
>> >                 return -1;
>> >
>> >         tcp->_free_priv_data = free_priv_data;
>> >         tcp->_priv_data = priv_data;
>> >
>> >         return 0;
>> > }
>>
>> Sure, and since they always come in a pairs it might be even better. If it turns
>> out we need it split up it is easily done later.
>
> The discussion seems to be stalled.
> Patrik, would you like to prepare a patch?

Hi Dmitry

I'll send you new patches this weekend. Thanks for reminding me.

-Patrik

>
>
> --
> ldv
On Mon, Sep 07, 2015 at 08:23:57PM +0200, Patrik Jakobsson wrote:
> On Mon, Sep 7, 2015 at 6:51 PM, Dmitry V. Levin wrote:
> > On Mon, Aug 31, 2015 at 02:37:07PM +0200, Patrik Jakobsson wrote:
> > [...]
> >> Here's my take on it (I assume it needs some discussion):
> >>
> >> int
> >> set_tcb_priv_data(struct tcb *tcp, void *priv_data)
> >> {
> >>       /* A free callback is required before setting private data and private
> >>        * data must be set back to NULL before being set again.
> >>        */
> >
> > I think a single function initializing both _priv_data and _free_priv_data
> > would suffice:
> >
> > int
> > set_tcb_priv_data(struct tcb *tcp, void *priv_data,
> >                   void (*free_priv_data)(void *))
> > {
> >         if (tcp->_priv_data)
> >                 return -1;
> >
> >         tcp->_free_priv_data = free_priv_data;
> >         tcp->_priv_data = priv_data;
> >
> >         return 0;
> > }
> 
> Sure, and since they always come in a pairs it might be even better. If it turns
> out we need it split up it is easily done later.

JFYI, I've finalized and merged this set_tcb_priv_data interface.
On Jul 20, 2016 4:50 PM, "Dmitry V. Levin" <ldv@altlinux.org> wrote:
>
> On Mon, Sep 07, 2015 at 08:23:57PM +0200, Patrik Jakobsson wrote:
> > On Mon, Sep 7, 2015 at 6:51 PM, Dmitry V. Levin wrote:
> > > On Mon, Aug 31, 2015 at 02:37:07PM +0200, Patrik Jakobsson wrote:
> > > [...]
> > >> Here's my take on it (I assume it needs some discussion):
> > >>
> > >> int
> > >> set_tcb_priv_data(struct tcb *tcp, void *priv_data)
> > >> {
> > >>       /* A free callback is required before setting private data and
private
> > >>        * data must be set back to NULL before being set again.
> > >>        */
> > >
> > > I think a single function initializing both _priv_data and
_free_priv_data
> > > would suffice:
> > >
> > > int
> > > set_tcb_priv_data(struct tcb *tcp, void *priv_data,
> > >                   void (*free_priv_data)(void *))
> > > {
> > >         if (tcp->_priv_data)
> > >                 return -1;
> > >
> > >         tcp->_free_priv_data = free_priv_data;
> > >         tcp->_priv_data = priv_data;
> > >
> > >         return 0;
> > > }
> >
> > Sure, and since they always come in a pairs it might be even better. If
it turns
> > out we need it split up it is easily done later.
>
> JFYI, I've finalized and merged this set_tcb_priv_data interface.

Thanks Dmitry

-Patrik

>
>
> --
> ldv