[DTrace-devel] [PATCH 02/14] Clean up prp/uprp variable names
Eugene Loh
eugene.loh at oracle.com
Wed Jun 5 18:18:20 UTC 2024
On 6/4/24 14:44, Kris Van Hees wrote:
> On Tue, Jun 04, 2024 at 02:11:01PM -0400, eugene.loh--- via DTrace-devel wrote:
>> From: Eugene Loh <eugene.loh at oracle.com>
>>
>> Should prp be oprp? Or should we use opr and upr?
> This does not belong in the commit message. But yes, we should stick with
> prp for the overlying probe, which is consistent with the rest of the code
> base.
Yes, the comment was to solicit feedback like this. I'll go with "prp"
I guess, even though Nick (elsewhere) voted for "oprp". I liked "oprp"
because it's more explicit and the same number of letters as "uprp", but
sticking with "prp" is simpler and arguably clear enough given the
changes in this patch.
> Also, I think this patch should address a few other issues in this file, e.g.
> making sure comments are consistent with the code (e.g. if it mentions we are
> creating overlying and underlying probe lists, the code should also create
> tgem in that order - or the comment ought to be updated to match the code
> order). etc...
I didn't find much else in the file. I cleaned up some provide_probe()
stuff (I'm guessing it's what you're referring to), but if there is
something else egregious you're concerned about let me know.
>> Signed-off-by: Eugene Loh <eugene.loh at oracle.com>
>> ---
>> libdtrace/dt_prov_uprobe.c | 55 +++++++++++++++++++++++++++-----------
>> 1 file changed, 40 insertions(+), 15 deletions(-)
>>
>> diff --git a/libdtrace/dt_prov_uprobe.c b/libdtrace/dt_prov_uprobe.c
>> index afc1f628..cace406d 100644
>> --- a/libdtrace/dt_prov_uprobe.c
>> +++ b/libdtrace/dt_prov_uprobe.c
>> @@ -6,6 +6,31 @@
>> *
>> * The uprobe-based provider for DTrace (implementing pid and USDT providers).
>> */
>> +/*
>> + * This file uses both overlying probes (specified by the user) as well as
>> + * underlying probes (the uprobes recognized by the kernel). To minimize
>> + * confusion, this file should use consistent variable names:
>> + *
>> + * dt_probe_t *prp; // overlying probe
>> + * dt_probe_t *uprp; // underlying probe
>> + *
>> + * Either one might be returned by dt_probe_lookup() or
>> + * dt_probe_insert() or added to dt_enablings[] or dt_probes[].
>> + * Further, uprp might be returned by create_underlying().
>> + *
>> + * dt_uprobe_t *upp; // uprobe associated with an underlying probe
>> + *
>> + * list_probe_t *pop; // overlying probe list
>> + * list_probe_t *pup; // underlying probe list
>> + *
>> + * The provider-specific prv_data has these meanings:
>> + *
>> + * prp->prv_data // dt_list_t of associated underlying probes
>> + *
>> + * uprp->prv_data // upp (the associated uprobe)
>> + *
>> + * Finally, note that upp->probes is a dt_list_t of overlying probes.
>> + */
> This comment block is too verbose and I don't think it is really needed, if you
> are going to rename variables anyway to be consistent based on your proposal
> (as you do in this patch). So, the comment becomes unnecessary by the patch
> itself.
>
> Even if we were to retain a comment like this, I think it should be much more
> brief. But again, I think the patch itself accomplishes all that is needed,
> so no need to comment.
>
>> #include <sys/types.h>
>> #include <assert.h>
>> #include <errno.h>
>> @@ -118,7 +143,7 @@ static dt_probe_t *create_underlying(dtrace_hdl_t *dtp,
>> char mod[DTRACE_MODNAMELEN];
>> char prb[DTRACE_NAMELEN];
>> dtrace_probedesc_t pd;
>> - dt_probe_t *prp;
>> + dt_probe_t *uprp;
> I am OK with doing this renaming of variable name because you want some form
> of consustency throughout this file, but I don't believe it is really needed.
> This function only deals with one type of probes, identified both in the
> comment and the function name as underlying probes. So, the prp variable that
> is used in many places in DTrace source code to denote a probe pointer should
> not cause any confusion.
>
> But if you want to change it, no problem.
>
>> dt_uprobe_t *upp;
>> int is_enabled = 0;
>>
>> @@ -160,8 +185,8 @@ static dt_probe_t *create_underlying(dtrace_hdl_t *dtp,
>> pd.fun = psp->pps_fun;
>> pd.prb = prb;
>>
>> - prp = dt_probe_lookup(dtp, &pd);
>> - if (prp == NULL) {
>> + uprp = dt_probe_lookup(dtp, &pd);
>> + if (uprp == NULL) {
>> dt_provider_t *pvp;
>>
>> /* Get the provider for underlying probes. */
>> @@ -182,12 +207,12 @@ static dt_probe_t *create_underlying(dtrace_hdl_t *dtp,
>> if (upp->tp == NULL)
>> goto fail;
>>
>> - prp = dt_probe_insert(dtp, pvp, pd.prv, pd.mod, pd.fun, pd.prb,
>> + uprp = dt_probe_insert(dtp, pvp, pd.prv, pd.mod, pd.fun, pd.prb,
>> upp);
>> - if (prp == NULL)
>> + if (uprp == NULL)
>> goto fail;
>> } else
>> - upp = prp->prv_data;
>> + upp = uprp->prv_data;
>>
>> switch (psp->pps_type) {
>> case DTPPT_RETURN:
>> @@ -202,7 +227,7 @@ static dt_probe_t *create_underlying(dtrace_hdl_t *dtp,
>> */
>> }
>>
>> - return prp;
>> + return uprp;
>>
>> fail:
>> probe_destroy(dtp, upp);
>> @@ -394,8 +419,8 @@ static void enable_usdt(dtrace_hdl_t *dtp, dt_probe_t *prp)
>> static int trampoline(dt_pcb_t *pcb, uint_t exitlbl)
>> {
>> dt_irlist_t *dlp = &pcb->pcb_ir;
>> - const dt_probe_t *prp = pcb->pcb_probe;
>> - const dt_uprobe_t *upp = prp->prv_data;
>> + const dt_probe_t *uprp = pcb->pcb_probe;
>> + const dt_uprobe_t *upp = uprp->prv_data;
>> const list_probe_t *pop;
>> uint_t lbl_exit = pcb->pcb_exitlbl;
>>
>> @@ -508,8 +533,8 @@ copyout_val(dt_pcb_t *pcb, uint_t lbl, uint32_t val, int arg)
>> static int trampoline_is_enabled(dt_pcb_t *pcb, uint_t exitlbl)
>> {
>> dt_irlist_t *dlp = &pcb->pcb_ir;
>> - const dt_probe_t *prp = pcb->pcb_probe;
>> - const dt_uprobe_t *upp = prp->prv_data;
>> + const dt_probe_t *uprp = pcb->pcb_probe;
>> + const dt_uprobe_t *upp = uprp->prv_data;
>> const list_probe_t *pop;
>> uint_t lbl_assign = dt_irlist_label(dlp);
>> uint_t lbl_exit = pcb->pcb_exitlbl;
>> @@ -636,9 +661,9 @@ out:
>> return name;
>> }
>>
>> -static int attach(dtrace_hdl_t *dtp, const dt_probe_t *prp, int bpf_fd)
>> +static int attach(dtrace_hdl_t *dtp, const dt_probe_t *uprp, int bpf_fd)
>> {
>> - dt_uprobe_t *upp = prp->prv_data;
>> + dt_uprobe_t *upp = uprp->prv_data;
>> tp_probe_t *tpp = upp->tp;
>> FILE *f;
>> char *fn;
>> @@ -733,9 +758,9 @@ out:
>> * probes that didn't get created. If the removal fails for some reason we are
>> * out of luck - fortunately it is not harmful to the system as a whole.
>> */
>> -static void detach(dtrace_hdl_t *dtp, const dt_probe_t *prp)
>> +static void detach(dtrace_hdl_t *dtp, const dt_probe_t *uprp)
>> {
>> - dt_uprobe_t *upp = prp->prv_data;
>> + dt_uprobe_t *upp = uprp->prv_data;
>> tp_probe_t *tpp = upp->tp;
>>
>> if (!dt_tp_has_info(tpp))
>> --
>> 2.18.4
>>
>>
>> _______________________________________________
>> 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