| Message ID | 1440420170-13337-3-git-send-email-patrik.jakobsson@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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;
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(+)