[DTrace-devel] [PATCH] options, test: correct bufsize calculations and tests

[Eugene Loh]

R-b ("under protest" but in order to move forward).  But I still think 
updating the err*too-low tests makes sense.  These tests work in tandem, 
so if one test is going to be updated I think its partner should be as 
well, regardless of whether the partner was passing anyhow.  We 
routinely clean stuff up even when it was passing anyhow.

On 9/19/2024 1:57 PM, Kris Van Hees wrote:
> On Thu, Sep 19, 2024 at 04:39:26PM -0400, Eugene Loh wrote:
>> As we have discussed, I do not see the point of such a check.  We change the
>> user-supplied value anyhow;  so why should we check it?  How about we come
>> to a decision about what to do about this auto resizing stuff (which we've
>> changed since legacy DTrace) before fine-tuning what we're checking.  (FWIW,
>> I vote for no resizing.  The user typically has no need to specify a size.
>> If they do, we cannot guess if they want their size to be a min, max, or
>> what.)
> We have discussed this at great length, yes, and since there is no resolution
> yet (because it is a more involved problem that needs discussion) and yet we
> have failing tests, I think it is best to make sure the tests pass.
>
> We can work on a redesign , but in the meantime, the tests should really pass,
> and the current status quo maintained.
>
>> Also...
>>
>> On 9/19/24 08:54, Kris Van Hees wrote:
>>> Reported-by: Eugene Loh <eugene.loh at oracle.com>
>>> Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
>>> ---
>>>    libdtrace/dt_work.c                 |  9 +++------
>>>    test/unittest/options/tst.b.d       | 19 ++++++++++---------
>>>    test/unittest/options/tst.bufsize.d | 19 ++++++++++---------
>>>    3 files changed, 23 insertions(+), 24 deletions(-)
>> What about the "too low" tests?
> Those pass as expected so no change was needed.
>
>>> diff --git a/libdtrace/dt_work.c b/libdtrace/dt_work.c
>>> @@ -268,15 +268,12 @@ dtrace_go(dtrace_hdl_t *dtp, uint_t cflags)
>>>    	dtrace_getopt(dtp, "bufsize", &size);
>>>    	if (size == 0 ||
>>> -	    size < sizeof(struct perf_event_header) + sizeof(uint32_t) +
>>> -		   dtp->dt_maxreclen)
>>> +	    size < sizeof(struct perf_event_header) + dtp->dt_maxreclen)
>>>    		return dt_set_errno(dtp, EDT_BUFTOOSMALL);
>>>    	if (dt_pebs_init(dtp, size) == -1)
>>>    		return dt_set_errno(dtp, EDT_NOMEM);
>> Why does the code special-case "size == 0"?
> I honestly cannot remember why that was there.  I bet it was a mistake I made
> when I originally wrote that code.  I'll remove it since that is (of course)
> meaningless if t is followed by || size < ...