Re: [DTrace-devel] [PATCH v2 05/14] probe: improve dt_probe_lookup2()

[Nick Alcock <nick.alcock@oracle.com>]

On 29 Oct 2024, Kris Van Hees outgrape:

> On Mon, Oct 28, 2024 at 09:17:54PM +0000, Nick Alcock via DTrace-devel wrote:
>> diff --git a/libdtrace/dt_impl.h b/libdtrace/dt_impl.h
>> index 340dc1960c5e..d11ad2839f8e 100644
>> --- a/libdtrace/dt_impl.h
>> +++ b/libdtrace/dt_impl.h
>> @@ -759,6 +759,8 @@ extern dtrace_difo_t *dt_difo_copy(dtrace_hdl_t *dtp, const dtrace_difo_t *odp);
>>  extern int dt_consume_init(dtrace_hdl_t *);
>>  extern void dt_consume_fini(dtrace_hdl_t *);
>>  
>> +extern void dt_desc_destroy(dtrace_hdl_t *dtp, dtrace_probedesc_t *);
>
> I think it would be better to let dt_desc_destroy() take a third arg, to
> indicate whether the desc itself should be free'd as well or whether it
> should be left alone.
>
> That avoids calling it and then following it up with a dt_free() on the desc.

... er, OK? Is that really clearer than the alternative?

Implemented, anyway (and made it return straight away if called with a
NULL pdp, for consistency with free(), which lets us drop a conditional
as well).

>> diff --git a/libdtrace/dt_subr.c b/libdtrace/dt_subr.c
>> index d6aad7637fb9..72631b33a0ad 100644
>> --- a/libdtrace/dt_subr.c
>> +++ b/libdtrace/dt_subr.c
>> @@ -175,6 +175,16 @@ dtrace_desc2str(const dtrace_probedesc_t *pdp, char *buf, size_t len)
>>  	return buf;
>>  }
>>  
>> +/* Only use on probedescs derived from dtrace_xstr2desc above. */
>
> Why can't it used on other dtrace_probedesc_t that contain references to
> memory that needs to be free'd?  I don't think that dtrace_xstr2desc() does
> anything to make the desc it populates be special.

I'm a bit worried that some of them might be using pointers to regions
within storage owned by someone else rather than strdup()s (I mean they
*are* const, and that's what that generally means). If that's not true,
why are the pointers const?

-- 
NULL && (void)