Re: [PATCH] dtprobed: handle multiple providers in a single piece of DOF

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

On 14 Nov 2024, Eugene Loh said:

> On 11/14/24 15:38, Nick Alcock wrote:
>
>> On 14 Nov 2024, Eugene Loh told this:
>>
>>> On 11/14/24 11:03, Nick Alcock wrote:
>>>> diff --git a/test/unittest/usdt/tst.multiprovider.r.p b/test/unittest/usdt/tst.multiprovider.r.p
>>>> new file mode 100755
>>>> index 0000000000000..ae8493e3d5be4
>>>> --- /dev/null
>>>> +++ b/test/unittest/usdt/tst.multiprovider.r.p
>>>> @@ -0,0 +1,2 @@
>>>> +#!/bin/sh
>>>> +grep -v '^ *ID' | sed 's,^[0-9]*,ID,; s,prov\(.\)[0-9]*,prov\1PID,; s,  *, ,g'
>>> Okay, though the "grep -v" would be clearer to me if it checked  '^ *ID *PROVIDER *MODULE *FUNCTION *NAME$'.
>> Why? We're not dependent on the format of the header, we just want to
>> get rid of it.
>
> It just seemed to me easier to figure out what was going on.  It looked like a recognizable header.

Oh I see!

> What do you think of ' *PROVIDER *'?  The 'ID' is short and strikes me as especially confusing if one looks at the post-processed
> output, which has 'ID' all over the place.

Yes, that's good :)

>> Having the test fail because the header we don't care
>> about changed layout seems wrong.
>>
>> I might just drop the first line with tail -n +1...
>>
>>> This whole thing would be clearer to me as a single awk, but maybe that's just me.
>> How? Piles of gmatches?
>
> gsub() or something.  Anyhow, your choice.

I always find gsub() sufficiently confusing that I have to check the
manual every time :(

>>> Comments might be nice.  I'm not sure how comfortably people read regex...  I need to concentrate hard to figure it out.
>> Honestly this is just doing some "replace changing numbers" stuff like a
>> hundred other .p's in the testsuite. Looking at the .r makes it obvious
>> what it's up to.
>
> No big deal either way (e.g., adding one-line comment).

Totally agree in re a comment :)

>>>> diff --git a/test/unittest/usdt/tst.multiprovider.sh b/test/unittest/usdt/tst.multiprovider.sh
>>>> new file mode 100755
>>>> index 0000000000000..d5b72c2be77ec
>>>> --- /dev/null
>>>> +++ b/test/unittest/usdt/tst.multiprovider.sh
>>>> @@ -0,0 +1,15 @@
>>>> +#!/bin/bash
>>>> +#
>>>> +# Oracle Linux DTrace.
>>>> +# Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved.
>>>> +# Licensed under the Universal Permissive License v 1.0 as shown at
>>>> +# http://oss.oracle.com/licenses/upl.
>>>> +#
>>>> +if [ $# != 1 ]; then
>>>> +	echo expected one argument: '<'dtrace-path'>'
>>>> +	exit 2
>>>> +fi
>>>> +
>>>> +dtrace=$1
>>>> +
>>>> +exec $dtrace $dt_flags -l -P 'prov*' -c `pwd`/test/triggers/usdt-tst-multiprovider
>>> Okay, but there should also be a test that actually fires probes.
>> ... why? What we care about is that DTrace can see the probes properly
>> (since we already know from other tests that once it can do that,
>> everything else works), and -l is an easy way to verify that (and in
>> general exercise the entire pathway up to USDT probe discovery in dtrace
>> itself) without having to get tangled up in even *more* possible bugs
>> around probe firing. I hit enough bugs in the parser writing this as it
>> was...
>>
>> I mean I can write one but I don't see what it's going to tell us.
>
> I'm less concerned about the bugs we can foresee.  I'm more concerned
> about the bugs we don't foresee.
>
> I'm even interested in the case where probes in different providers have the same name.

Seems reasonable! I'll add some tests...

> Different subject:  do we have a test for "dtrace -lv" on USDT typed args?

Yes: test/unittest/dtrace-util/tst.ListProbesArgsUSDT.sh

-- 
NULL && (void)