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

[Eugene Loh <eugene.loh@oracle.com>]

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