[DTrace-devel] [PATCH 06/13] Add provider-dependent struct in dt_probe_t
Eugene Loh
eugene.loh at oracle.com
Tue Jul 7 22:11:44 PDT 2020
I'm trying to incorporate all your suggestions, but have a few questions
here:
On 07/06/2020 02:50 PM, Kris Van Hees wrote:
> On Wed, Jul 01, 2020 at 10:41:11PM -0400, eugene.loh at oracle.com wrote:
>> From: Eugene Loh <eugene.loh at oracle.com>
>>
>> The dt_probe_t struct had members like event_id and event_fd.
>> This made sense for the providers we were supporting, since they
>> were all tracepoint-like. For future providers, however, those
>> members might not make sense and others will be needed. Therefore,
>> replace event_id and event_fd with an opaque pointer to a
>> provider-dependent struct.
>>
>> Signed-off-by: Eugene Loh <eugene.loh at oracle.com>
>> ---
>> libdtrace/dt_probe.c | 19 +++-------
>> libdtrace/dt_probe.h | 4 +--
>> libdtrace/dt_prov_dtrace.c | 42 +++++++++++++++-------
>> libdtrace/dt_prov_fbt.c | 41 +++++++++++++++------
>> libdtrace/dt_prov_sdt.c | 72 +++++++++++++++++++++++--------------
>> libdtrace/dt_prov_syscall.c | 34 +++++++++++++-----
>> libdtrace/dt_provider.h | 6 ++--
>> 7 files changed, 143 insertions(+), 75 deletions(-)
>>
>> diff --git a/libdtrace/dt_probe.c b/libdtrace/dt_probe.c
>> index be407dd2..496ba11e 100644
>> --- a/libdtrace/dt_probe.c
>> +++ b/libdtrace/dt_probe.c
>> @@ -505,6 +505,7 @@ dt_probe_destroy(dt_probe_t *prp)
>>
>> if (prp->prov && prp->prov->impl && prp->prov->impl->probe_fini)
>> prp->prov->impl->probe_fini(dtp, prp);
>> + dt_free(dtp, prp->prv_data);
> OK
>
>>
>> dt_node_list_free(&prp->nargs);
>> dt_node_list_free(&prp->xargs);
>> @@ -665,7 +666,8 @@ dt_probe_tag(dt_probe_t *prp, uint_t argn, dt_node_t *dnp)
>> }
>>
>> dt_probe_t *
>> -dt_probe_insert(dtrace_hdl_t *dtp, dt_provider_t *prov, const char *prv,
>> +dt_probe_insert(dtrace_hdl_t *dtp, dt_provider_t *prov, const void *prv_data,
>> + const char *prv,
>> const char *mod, const char *fun, const char *prb)
> Please move the prv_data to the end of the argument list, which is more
> consistent with other code that supports opaque pointers. It also shows its
> status as auxilliary information rather than being mixed with the elements
> that are part of the generic probe. And perhaps name it datap since that is
> the name you used for the provider-specific data variable in other places.
>
> I would also modify dt_probe_insert() to free datap (if not NULL) if the probe
> cannot be created. That avoids needing to have each caller check if the probe
> got created, and if not, free datap.
I don't think this really works. The datap points to provider-specific
stuff. It might even point to memory that contains pointers to other
things. So the provider needs to free datap.
Another option is that the provider only allocates datap but doesn't set
up the memory to which datap points, doing that only once the probe has
been inserted. But then if there is a problem with initializing the
memory to which datap points, would we want to delete the probe we just
inserted? We do not yet have a good function for that (deleting one
probe, vs cleaning them all up at the end... e.g., never do we decrement
dt_probe_id).
Anyhow, one can shoehorn the change you're talking about, but it strikes
me as pretty ugly.
> You already do a freeing of the datap in
> dt_probr_destroy() so doing it in dt_probe_insert() as well (when needed) is
> OK.
Well, no. I mean, yes, there is a free(datap) in dt_probe_destroy(),
but it comes right after calling the provider-specific fini() function.
So, do we want dt_probe_insert(), called by a provider, to then call a
provider-specific callback we need to define? My head is spinning just
thinking of the back-and-forth.
> The caller can count on the fact that if the probe could not be created,
> datap is no longer valid.
>
>> {
>> dt_probe_t *prp;
>> @@ -711,8 +713,7 @@ dt_probe_insert(dtrace_hdl_t *dtp, dt_provider_t *prov, const char *prv,
>>
>> prp->desc = desc;
>> prp->prov = prov;
>> - prp->event_id = -1;
>> - prp->event_fd = -1;
>> + prp->prv_data = prv_data;
>>
>> dt_htab_insert(dtp->dt_byprv, prp);
>> dt_htab_insert(dtp->dt_bymod, prp);
>> @@ -872,26 +873,16 @@ dt_probe_delete(dtrace_hdl_t *dtp, dt_probe_t *prp)
>> static void
>> dt_probe_args_info(dtrace_hdl_t *dtp, dt_probe_t *prp)
>> {
>> - int id = DTRACE_IDNONE;
>> int argc = 0;
>> dt_argdesc_t *argv = NULL;
>> int i, nc, xc;
>> dtrace_typeinfo_t dtt;
>>
>> - /*
>> - * If we already have an event ID information for this probe, there is
>> - * no need to retrieve it again.
>> - */
>> - if (prp->event_id != -1)
>> - return;
>> -
>> if (!prp->prov->impl->probe_info)
>> return;
>> - if (prp->prov->impl->probe_info(dtp, prp, &id, &argc, &argv) < 0)
>> + if (prp->prov->impl->probe_info(dtp, prp, &argc, &argv) < 0)
>> return;
>>
>> - if (id > 0)
>> - prp->event_id = id;
>> if (!argc || !argv)
>> return;
>>
>> diff --git a/libdtrace/dt_probe.h b/libdtrace/dt_probe.h
>> index 7feeeff2..92768f9b 100644
>> --- a/libdtrace/dt_probe.h
>> +++ b/libdtrace/dt_probe.h
>> @@ -38,8 +38,7 @@ typedef struct dt_probe {
>> struct dt_hentry he_fun; /* function name htab links */
>> struct dt_hentry he_prb; /* probe name htab link */
>> struct dt_hentry he_fqn; /* fully qualified name htab link */
>> - int event_id; /* tracing event id */
>> - int event_fd; /* perf event file descriptor */
>> + void *prv_data; /* provider-specific data */
> OK.
>
>> dt_ident_t *pr_ident; /* pointer to probe identifier */
>> const char *pr_name; /* pointer to name component */
>> dt_node_t *nargs; /* native argument list */
>> @@ -70,6 +69,7 @@ extern int dt_probe_define(dt_provider_t *, dt_probe_t *,
>> extern dt_node_t *dt_probe_tag(dt_probe_t *, uint_t, dt_node_t *);
>>
>> extern dt_probe_t *dt_probe_insert(dtrace_hdl_t *dtp, dt_provider_t *prov,
>> + const void *prv_data,
>> const char *prv, const char *mod,
>> const char *fun, const char *prb);
> Move to end of parameter list.
>
>> extern dt_probe_t *dt_probe_lookup(dtrace_hdl_t *dtp,
>> diff --git a/libdtrace/dt_prov_dtrace.c b/libdtrace/dt_prov_dtrace.c
>> index 34c99388..c5c2a84d 100644
>> --- a/libdtrace/dt_prov_dtrace.c
>> +++ b/libdtrace/dt_prov_dtrace.c
>> @@ -15,6 +15,11 @@
>> #include "dt_provider.h"
>> #include "dt_probe.h"
>>
>> +typedef struct prv_data {
>> + int event_id;
>> + int event_fd;
>> +} prv_data_t;
> Since multiple providers use the tracepoint-based mechanism, the provider
> specific data is identical for these providers and can therefore be declared
> in a more central location. I would put it in dt_provider.h (where the
> prototypes for tp_event_info() and tp_attach() sre defined..
>
> I would not use the same name for the struct, arg name and member name.
> Since this is common for all tracepoint-based providers, how about using
> tp_probe (and tp_probe_t)? Other names can be profile_probe_t, and so on.
> That is similar to the naming in DTrace v1 (at the kernel level) where e.g.
> FBT specific information was in fbt_probe_t.
>
>> +
>> static const char prvname[] = "dtrace";
>> static const char modname[] = "";
>> static const char funname[] = "";
>> @@ -34,18 +39,26 @@ static const dtrace_pattr_t pattr = {
>> static int populate(dtrace_hdl_t *dtp)
>> {
>> dt_provider_t *prv;
>> - int n = 0;
>> + int i, n = 0;
>> + char *prbnames[] = { "BEGIN", "END", "ERROR" };
> This (along with the code below) is a bit of an issue because the ERROR probe
> is not the same as the BEGIN and END probes. BEGIN/END are implemented as
> uprobes, whereas ERROR is not implemented as a probe that is attached to
> anything at the kernel level. All three belong in the dtrace provider, but
> the behaviour is definitely different.
>
>> prv = dt_provider_create(dtp, prvname, &dt_dtrace, &pattr);
>> if (prv == NULL)
>> return 0;
>>
>> - if (dt_probe_insert(dtp, prv, prvname, modname, funname, "BEGIN"))
>> - n++;
>> - if (dt_probe_insert(dtp, prv, prvname, modname, funname, "END"))
>> - n++;
>> - if (dt_probe_insert(dtp, prv, prvname, modname, funname, "ERROR"))
>> - n++;
>> + for (i = 0; i < sizeof (prbnames) / sizeof (*prbnames); i++) {
>> + prv_data_t *datap;
>> +
>> + datap = dt_zalloc(dtp, sizeof (prv_data_t));
>> + if (datap == NULL)
>> + continue;
>> + datap->event_id = -1;
>> + datap->event_fd = -1;
> I think that the above should be done in a tp_probe_alloc() function in the
> SDT provider code (where the other generic tp_* functions are) since we should
> use the same struct for all tracepoint-based providers anyway.
>
> So the next statement becomes:
>
> if (dt_probe_insert(dtp, prv, datap, prvname, modname, funname,
> prbnames[i], tp_probe_event()))
>
> It is also possible to add a tp_probe_insert() function that wraps the call
> to dt_probe_insert along with allocation of the tp_probe_t. Either solution
> seems reasonable to me.
>
> If you prefer the tp_probe_insert() wrapper function, adjust your reading of
> my further comments in that context (I am commenting as if there is a
> tp_probe_alloc() function that will be used in dt_probe_insert() calls).
If we want to share "tp" code, I think tp_probe_insert() is the way to
go. It'll be in the revised patch set. No sense in having a common
function that simply allocs when there is more common functionality that
can be swept in.
>> + if (dt_probe_insert(dtp, prv, datap, prvname, modname, funname, prbnames[i]))
> The datap argument would move to the end per my comments above.
>
>> + n++;
>> + else
>> + dt_free(dtp, datap);
> Already done in dt_probe_insert() if the probe cannot be created (see above).
>> + }
>>
>> return n;
>> }
>> @@ -230,7 +243,7 @@ out:
>> }
>>
>> static int probe_info(dtrace_hdl_t *dtp, const dt_probe_t *prp,
>> - int *idp, int *argcp, dt_argdesc_t **argvp)
>> + int *argcp, dt_argdesc_t **argvp)
>> {
>> char *spec;
>> char *fn = NULL;
>> @@ -238,8 +251,12 @@ static int probe_info(dtrace_hdl_t *dtp, const dt_probe_t *prp,
>> FILE *f;
>> int rc = -ENOENT;
>> size_t len;
>> + int *idp = &((prv_data_t *)(prp->prv_data))->event_id;
>> +
>> + /* if we have an event ID, no need to retrieve it again */
>> + if (*idp != -1)
>> + return -1;
> I think it would be more clear if you did this as:
>
> struct tp_probe_t *datap = prp->prv_data;
>
> if (datap->event_id != -1)
> return -1;
>
>> - *idp = -1;
>> *argcp = 0; /* no arguments */
>> *argvp = NULL;
> and then later on you can do:
>
> rc = tp_event_info(dtp, f, 0, datap, NULL, NULL);
>
> and tp_event_info() can take a tp_probe_t * argument and do with it whatever
> it needs to (in this case, assign event_id).
>
>> @@ -295,10 +312,11 @@ out:
>> static int probe_fini(dtrace_hdl_t *dtp, dt_probe_t *prp)
>> {
>> int fd;
>> + prv_data_t *datap = prp->prv_data;
>>
>> - if (prp->event_fd != -1) {
>> - close(prp->event_fd);
>> - prp->event_fd = -1;
>> + if (datap->event_fd != -1) {
>> + close(datap->event_fd);
>> + datap->event_fd = -1;
>> }
> Make this into a standard tp_probe_finit and use it in providers that are
> tracepoint-based and need no further cleanup? Or you could introduce a new
> provider function (detach) to handle this, and leave probe_fini in cases
> where we need to perform further cleanup, Perhaps that is nice (even though
> it means more functions) because it provides a balanced set of provider
> operations attach vs detach, populate/provide vs probe_fini). What do you
> think?
Since you have question marks, I'm assuming it's okay not to do this.
First, having separate detach and fini functions makes no sense to me.
I understand the balance, but we intentionally separate populate/provide
from attach since we want to list probes but not necessarily create more
stuff than we have to. In contrast, I am unaware that we currently are
interested in separating detach from fini.
As far as a standard tp fini function goes, the code is only a few lines
and is used only by dtrace and fbt, not by sdt or syscall. So the value
proposition of a common function is different from the other tp stuff
we're doing.
>
>> fd = open(UPROBE_EVENTS, O_WRONLY | O_APPEND);
>> diff --git a/libdtrace/dt_prov_fbt.c b/libdtrace/dt_prov_fbt.c
>> index cb80082e..a58584c0 100644
>> --- a/libdtrace/dt_prov_fbt.c
>> +++ b/libdtrace/dt_prov_fbt.c
>> @@ -37,6 +37,11 @@
>> #include "dt_probe.h"
>> #include "dt_pt_regs.h"
>>
>> +typedef struct prv_data {
>> + int event_id;
>> + int event_fd;
>> +} prv_data_t;
> See above.
>
>> +
>> static const char prvname[] = "fbt";
>> static const char modname[] = "vmlinux";
>>
>> @@ -65,9 +70,10 @@ static int populate(dtrace_hdl_t *dtp)
>> char buf[256];
>> char *p;
>> const char *mod = modname;
>> - int n = 0;
>> + int i, n = 0;
>> dtrace_syminfo_t sip;
>> dtrace_probedesc_t pd;
>> + char *prbnames[] = { "entry", "return" };
> Not necessary (see below).
>
>> prv = dt_provider_create(dtp, prvname, &dt_fbt, &pattr);
>> if (prv == NULL)
>> @@ -136,10 +142,20 @@ static int populate(dtrace_hdl_t *dtp)
>> if (dt_probe_lookup(dtp, &pd) != NULL)
>> continue;
>>
>> - if (dt_probe_insert(dtp, prv, prvname, mod, buf, "entry"))
>> - n++;
>> - if (dt_probe_insert(dtp, prv, prvname, mod, buf, "return"))
>> - n++;
>> + for (i = 0; i < sizeof (prbnames) / sizeof (*prbnames); i++) {
>> + prv_data_t *datap;
>> +
>> + datap = dt_zalloc(dtp, sizeof (prv_data_t));
>> + if (datap == NULL)
>> + continue;
>> + datap->event_id = -1;
>> + datap->event_fd = -1;
>> + if (dt_probe_insert(dtp, prv, datap, prvname, mod,
>> + buf, prbnames[i]))
>> + n++;
>> + else
>> + dt_free(dtp, datap);
>> + }
> This could become (based on earlier comments):
>
> if (dt_probe_insert(dtp, prv, prvname, mod, buf, "entry",
> tp_probe_alloc()))
> n++
> if (dt_probe_insert(dtp, prv, prvname, mod, buf, "return",
> tp_probe_alloc()))
> n++
>> }
>>
>> fclose(f);
>> @@ -273,15 +289,19 @@ static void trampoline(dt_pcb_t *pcb)
>> }
>>
>> static int probe_info(dtrace_hdl_t *dtp, const dt_probe_t *prp,
>> - int *idp, int *argcp, dt_argdesc_t **argvp)
>> + int *argcp, dt_argdesc_t **argvp)
>> {
>> int fd;
>> char *fn;
>> size_t len;
>> FILE *f;
>> int rc = -1;
>> + int *idp = &((prv_data_t *)(prp->prv_data))->event_id;
>> +
>> + /* if we have an event ID, no need to retrieve it again */
>> + if (*idp != -1)
>> + return -1;
> See above (for dt_prov_dtrace).
>
>> - *idp = -1;
>> *argcp = 0; /* no arguments by default */
>> *argvp = NULL;
>>
>> @@ -335,10 +355,11 @@ out:
>> static int probe_fini(dtrace_hdl_t *dtp, dt_probe_t *prp)
>> {
>> int fd;
>> + prv_data_t *datap = prp->prv_data;
>>
>> - if (prp->event_fd != -1) {
>> - close(prp->event_fd);
>> - prp->event_fd = -1;
>> + if (datap->event_fd != -1) {
>> + close(datap->event_fd);
>> + datap->event_fd = -1;
>> }
> See above (for dt_prov_dtrace). If you introduce a detach(dtp, prp) then you
> can have a custom detahc here that calls tp_detach(dtp, prp) to do the actual
> detach and then do the rest of the cleanup. Or if you prefer to stick with
> probe_fini (no detach) then you can have this on call a tp_probe_fini(dtp, prp)
> to handle the common detach-cleanup and then continue with the provider specific
> cleanup.
>
>> fd = open(KPROBE_EVENTS, O_WRONLY | O_APPEND);
>> diff --git a/libdtrace/dt_prov_sdt.c b/libdtrace/dt_prov_sdt.c
>> index 1a445c65..9a76f029 100644
>> --- a/libdtrace/dt_prov_sdt.c
>> +++ b/libdtrace/dt_prov_sdt.c
>> @@ -34,6 +34,11 @@
>> #include "dt_probe.h"
>> #include "dt_pt_regs.h"
>>
>> +typedef struct prv_data {
>> + int event_id;
>> + int event_fd;
>> +} prv_data_t;
> See above. Put this in dt_provider.h, with names tp_probe / tp_probe_t.
>
>> +
>> static const char prvname[] = "sdt";
>> static const char modname[] = "vmlinux";
>>
>> @@ -226,29 +231,31 @@ done:
>> return 0;
>> }
>>
>> -int tp_event_attach(dtrace_hdl_t *dtp, dt_probe_t *prp, int bpf_fd)
>> +int tp_event_attach(dtrace_hdl_t *dtp, const dt_probe_t *prp, int bpf_fd)
> tp_attach (per comments to an earlier patch)
Sorry, I don't understand what this means. I guess I do not know which
"comments to an earlier patch" you mean. Are you saying
s/tp_event_attach/tp_attach/ ?
>> {
>> - if (prp->event_fd == -1) {
>> - int fd;
>> - struct perf_event_attr attr = { 0, };
>> + prv_data_t *datap = prp->prv_data;
> tp_probe_t *
>
> Add:
> if (datap->event_id == -1)
> return 0;
>
> because if there is no event id, there is nothing to attach to, and we are
> done. But it might still be a valid attach request, so we return 0.
I made the change, but I confess I do not understand what scenario
you're talking about.
>> + if (datap->event_fd == -1) {
>> + int fd;
>> + struct perf_event_attr attr = { 0, };
>>
>> - attr.type = PERF_TYPE_TRACEPOINT;
>> - attr.sample_type = PERF_SAMPLE_RAW;
>> - attr.sample_period = 1;
>> - attr.wakeup_events = 1;
>> - attr.config = prp->event_id;
>> + attr.type = PERF_TYPE_TRACEPOINT;
>> + attr.sample_type = PERF_SAMPLE_RAW;
>> + attr.sample_period = 1;
>> + attr.wakeup_events = 1;
>> + attr.config = datap->event_id;
>>
>> - fd = perf_event_open(&attr, -1, 0, -1, 0);
>> - if (fd < 0)
>> - return dt_set_errno(dtp, errno);
>> + fd = perf_event_open(&attr, -1, 0, -1, 0);
>> + if (fd < 0)
>> + return dt_set_errno(dtp, errno);
>>
>> - prp->event_fd = fd;
>> - }
>> + datap->event_fd = fd;
>> + }
>>
>> - if (ioctl(prp->event_fd, PERF_EVENT_IOC_SET_BPF, bpf_fd) < 0)
>> - return dt_set_errno(dtp, errno);
>> + if (ioctl(datap->event_fd, PERF_EVENT_IOC_SET_BPF, bpf_fd) < 0)
>> + return dt_set_errno(dtp, errno);
>>
>> - return 0;
>> + return 0;
>> }
>>
>> /*
>> @@ -275,8 +282,10 @@ static int populate(dtrace_hdl_t *dtp)
>> return 0;
>>
>> while (fgets(buf, sizeof(buf), f)) {
>> - int dummy;
>> - char str[sizeof(buf)];
>> + int dummy;
>> + char str[sizeof(buf)];
>> + const char *mod, *prb;
>> + prv_data_t *datap;
>>
>> p = strchr(buf, '\n');
>> if (p)
>> @@ -301,13 +310,21 @@ static int populate(dtrace_hdl_t *dtp)
>> strcmp(buf, UPROBES) == 0)
>> continue;
>>
>> - if (dt_probe_insert(dtp, prv, prvname, buf, "", p))
>> - n++;
>> + mod = buf;
>> + prb = p;
>> } else {
>> - if (dt_probe_insert(dtp, prv, prvname, modname, "",
>> - buf))
>> - n++;
>> + mod = modname;
>> + prb = buf;
>> }
>> + datap = dt_zalloc(dtp, sizeof (prv_data_t));
>> + if (datap == NULL)
>> + continue;
>> + datap->event_id = -1;
>> + datap->event_fd = -1;
>> + if (dt_probe_insert(dtp, prv, datap, prvname, mod, "", prb))
>> + n++;
>> + else
>> + dt_free(dtp, datap);
> This can become:
>
> if (dt_probe_insert(dtp, prv, prvname, buf, "", p,
> tp_probe_alloc()))
> n++;
> } else {
> if (dt_probe_insert(dtp, prv, prvname, modname, "", buf,
> tp_probe_alloc()))
> n++;
> }
>
>> }
>>
>> fclose(f);
>> @@ -408,13 +425,16 @@ static void trampoline(dt_pcb_t *pcb)
>> }
>>
>> static int probe_info(dtrace_hdl_t *dtp, const dt_probe_t *prp,
>> - int *idp, int *argcp, dt_argdesc_t **argvp)
>> + int *argcp, dt_argdesc_t **argvp)
>> {
>> FILE *f;
>> char fn[256];
>> int rc;
>> + int *idp = &((prv_data_t *)(prp->prv_data))->event_id;
>>
>> - *idp = -1;
>> + /* if we have an event ID, no need to retrieve it again */
>> + if (*idp != -1)
>> + return -1;
> See above.
>
>> strcpy(fn, EVENTSFS);
>> strcat(fn, prp->desc->mod);
>> diff --git a/libdtrace/dt_prov_syscall.c b/libdtrace/dt_prov_syscall.c
>> index cb54ce5a..ecab4545 100644
>> --- a/libdtrace/dt_prov_syscall.c
>> +++ b/libdtrace/dt_prov_syscall.c
>> @@ -35,6 +35,11 @@
>> #include "dt_probe.h"
>> #include "dt_pt_regs.h"
>>
>> +typedef struct prv_data {
>> + int event_id;
>> + int event_fd;
>> +} prv_data_t;
> See above.
>
>> static const char prvname[] = "syscall";
>> static const char modname[] = "vmlinux";
>>
>> @@ -85,6 +90,7 @@ static int populate(dtrace_hdl_t *dtp)
>>
>> while (fgets(buf, sizeof(buf), f)) {
>> char *p;
>> + char *prb;
>>
>> /* Here buf is "group:event". */
>> p = strchr(buf, '\n');
>> @@ -107,22 +113,31 @@ static int populate(dtrace_hdl_t *dtp)
>> p = buf;
>> if (memcmp(p, PROV_PREFIX, sizeof(PROV_PREFIX) - 1) != 0)
>> continue;
>> -
> I think the empty line here adds clarity, in the sense that the conditional
> is important to highlight as a condition to not continue execution in this
> function. The next statement is part of the actual work done in this function.
>
>> p += sizeof(PROV_PREFIX) - 1;
>> +
>> /*
>> * Now p will be just "event", and we are only interested in
>> * events that match "sys_enter_*" or "sys_exit_*".
>> */
>> + prb = NULL;
>> if (!memcmp(p, ENTRY_PREFIX, sizeof(ENTRY_PREFIX) - 1)) {
>> p += sizeof(ENTRY_PREFIX) - 1;
>> - if (dt_probe_insert(dtp, prv, prvname, modname, p,
>> - "entry"))
>> - n++;
>> + prb = "entry";
>> } else if (!memcmp(p, EXIT_PREFIX, sizeof(EXIT_PREFIX) - 1)) {
>> p += sizeof(EXIT_PREFIX) - 1;
>> - if (dt_probe_insert(dtp, prv, prvname, modname, p,
>> - "return"))
>> + prb = "return";
>> + }
>> + if (prb != NULL) {
>> + prv_data_t *datap = dt_zalloc(dtp, sizeof (prv_data_t));
>> +
>> + if (datap == NULL)
>> + continue;
>> + datap->event_id = -1;
>> + datap->event_fd = -1;
>> + if (dt_probe_insert(dtp, prv, datap, prvname, modname, p, prb))
>> n++;
>> + else
>> + dt_free(dtp, datap);
> Becomes:
> if (!memcmp(p, ENTRY_PREFIX, sizeof(ENTRY_PREFIX) - 1)) {
> p += sizeof(ENTRY_PREFIX) - 1;
> if (dt_probe_insert(dtp, prv, prvname, modname, p,
> "entry", tp_probe_alloc()))
> n++;
> } else if (!memcmp(p, EXIT_PREFIX, sizeof(EXIT_PREFIX) - 1)) {
> p += sizeof(EXIT_PREFIX) - 1;
> if (dt_probe_insert(dtp, prv, prvname, modname, p,
> "return", tp_probe_alloc()))
> n++;
>
>> }
>> }
>>
>> @@ -235,13 +250,16 @@ static void trampoline(dt_pcb_t *pcb)
>> }
>>
>> static int probe_info(dtrace_hdl_t *dtp, const dt_probe_t *prp,
>> - int *idp, int *argcp, dt_argdesc_t **argvp)
>> + int *argcp, dt_argdesc_t **argvp)
>> {
>> FILE *f;
>> char fn[256];
>> int rc;
>> + int *idp = &((prv_data_t *)(prp->prv_data))->event_id;
>>
>> - *idp = -1;
>> + /* if we have an event ID, no need to retrieve it again */
>> + if (*idp != -1)
>> + return -1;
> See above.
>
>> /*
>> * We know that the probe name is either "entry" or "return", so we can
>> diff --git a/libdtrace/dt_provider.h b/libdtrace/dt_provider.h
>> index acd01983..9ea89898 100644
>> --- a/libdtrace/dt_provider.h
>> +++ b/libdtrace/dt_provider.h
>> @@ -61,19 +61,19 @@ typedef struct dt_provimpl {
>> int (*populate)(dtrace_hdl_t *dtp); /* register probes */
>> int (*probe_info)(dtrace_hdl_t *dtp, /* get probe info */
>> const struct dt_probe *prp,
>> - int *idp, int *argcp, dt_argdesc_t **argvp);
>> + int *argcp, dt_argdesc_t **argvp);
>> int (*provide)(dtrace_hdl_t *dtp, /* provide probes */
>> const dtrace_probedesc_t *pdp);
>> void (*trampoline)(dt_pcb_t *pcb); /* generate BPF trampoline */
>> int (*attach)(dtrace_hdl_t *dtp, /* attach BPF program to probe */
>> - struct dt_probe *prp, int bpf_fd);
>> + const struct dt_probe *prp, int bpf_fd);
>> int (*probe_fini)(dtrace_hdl_t *dtp, /* probe cleanup */
>> struct dt_probe *prb);
> const dt_probe *prp
>
>> } dt_provimpl_t;
>>
>> extern int tp_event_info(dtrace_hdl_t *dtp, FILE *f, int skip, int *idp,
>> int *argcp, dt_argdesc_t **argvp);
>> -extern int tp_event_attach(dtrace_hdl_t *dtp, struct dt_probe *prp, int bpf_fd);
>> +extern int tp_event_attach(dtrace_hdl_t *dtp, const struct dt_probe *prp, int bpf_fd);
> tp_attach (per comments to an earlier patch)
>
>> extern dt_provimpl_t dt_dtrace;
>> extern dt_provimpl_t dt_fbt;
>> --
>> 2.18.2
>>
>>
>> _______________________________________________
>> DTrace-devel mailing list
>> DTrace-devel at oss.oracle.com
>> https://oss.oracle.com/mailman/listinfo/dtrace-devel
More information about the DTrace-devel
mailing list