Re: [DTrace-devel] [PATCH 10/22] Simplify dtrace_stmt_create() attr init

Eugene Loh
2024-09-19T17:38:56Z

On 9/14/24 12:32, Kris Van Hees wrote:

> I adjusted the patch, and changed the commit message to be:
>
> Simplify dt_stmt_create() attr init
>
> Even though dt_stmt_create() initializes dtsd_descattr and dtsd_stmtattr,
> there is no point to doing so. It calls dtrace_stmt_create(), which also
> sets these members.
>
> and added my R-b.

That doesn't seem to work.  In dtrace_stmt_create(), we have:
sdp->dtsd_descattr = _dtrace_defattr;
sdp->dtsd_stmtattr = _dtrace_defattr;

In dt_stmt_create(), we have:
dtrace_stmtdesc_t *sdp = dtrace_stmt_create(dtp, edp);
sdp->dtsd_descattr = descattr;
sdp->dtsd_stmtattr = stmtattr;
which means that whatever dtrace_stmt_create() does, we overwrite it
with the attributes passed in by dt_stmt_create()'s caller.  So, what
dtrace_stmt_create() does is irrelevant and what dt_stmt_create()'s
caller asks for matters.  So removing the assignments from
dtrace_stmt_create() makes sense (except for the API issue) and removing
the assignments from dt_stmt_create() is wrong.

Specifically, removing the attr assignments in dt_stmt_create() causes
test/unittest/dtrace-util/tst.VerboseStabilityReport.d to fail.

Either we should go with the original version of the patch or, more
likely, to preserve the API, the patch should be withdrawn.

> On Sat, Sep 14, 2024 at 12:25:57PM -0400, Kris Van Hees via DTrace-devel wrote:
>> On Thu, Aug 29, 2024 at 01:22:07AM -0400, eugene.loh@oracle.com wrote:
>>> From: Eugene Loh <eugene.loh@oracle.com>
>>>
>>> Even though dtrace_stmt_create() initializes dtsd_descattr and
>>> dtsd_stmtattr, there is no point to doing so. Its only caller
>>> is dt_stmt_create(), which itself sets these members.
>> No, it is the other way around... It should be done in dtrace_stmt_create()
>> and thus it is no longer needed in dt_stmt_create(). As dtrace_stmt_create()
>> is a libdtrace API function, it can be called from other code, and since it
>> is the function that actual creates the statement and initializes some of its
>> members, it is the logical place to retain setting the attr fields.
>>
>> So, instead remove the assignments from dt_stmt_create().
>>
>>> Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
>>> ---
>>> libdtrace/dt_program.c | 2 --
>>> 1 file changed, 2 deletions(-)
>>>
>>> diff --git a/libdtrace/dt_program.c b/libdtrace/dt_program.c
>>> index a4b052fc..bdb434e0 100644
>>> --- a/libdtrace/dt_program.c
>>> +++ b/libdtrace/dt_program.c
>>> @@ -240,8 +240,6 @@ dtrace_stmt_create(dtrace_hdl_t *dtp, dtrace_ecbdesc_t *edp)
>>>
>>> dt_ecbdesc_hold(edp);
>>> sdp->dtsd_ecbdesc = edp;
>>> - sdp->dtsd_descattr = _dtrace_defattr;
>>> - sdp->dtsd_stmtattr = _dtrace_defattr;
>>>
>>> return sdp;
>>> }
>>> --
>>> 2.43.5
>>>
>> _______________________________________________
>> DTrace-devel mailing list
>> DTrace-devel@oss.oracle.com
>> https://oss.oracle.com/mailman/listinfo/dtrace-devel