Re: [PATCH 2/3] Defer stripping out "__data_loc" until we know that is useful

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

On 9/14/25 09:17, Nick Alcock wrote:

> On 13 Sep 2025, eugene loh uttered the following:
>
>> From: Eugene Loh <eugene.loh@oracle.com>
>>
>> Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
> Reviewed-by: Nick Alcock <nick.alcock@oracle.com>

Great, thanks.

> with the tiny caveat below.
>
>> ---
>>   libdtrace/dt_provider_tp.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/libdtrace/dt_provider_tp.c b/libdtrace/dt_provider_tp.c
>> index 33821f2c7..a6f978e3f 100644
>> --- a/libdtrace/dt_provider_tp.c
>> +++ b/libdtrace/dt_provider_tp.c
>> @@ -208,12 +208,13 @@ dt_tp_event_info(dtrace_hdl_t *dtp, FILE *f, int skip, tp_probe_t *tpp,
>>   		p = buf;
>>   		if (sscanf(buf, " field:%[^;]", p) <= 0)
>>   			continue;
>> -		sscanf(p, "__data_loc %[^;]", p);
>>   
>>   		/* We found a field: description - see if we should skip it. */
>>   		if (argc < 0)
>>   			goto skip;
>>   
>> +		sscanf(p, "__data_loc %[^;]", p);
>> +
> ... why not defer the sscanf above it too?

Because it's watching for "field:" lines.  We're not counting lines;  
we're counting lines with "field:" in them.

> The manipulation of argc at
> this point looks like this:
>
>          argc = -skip;
>
> (in getline loop)
>
>                  /* We found a field: description - see if we should skip it. */
>                  if (argc < 0)
>                          goto skip;
> (at loop end)
> 	skip:
>                  argc++;
>          }
>
>
> So if argc is < 0, it's going to *stay* like that until a given number
> of getlines have happened. Doing more work than just skipping and
> getlining is pointless.

Again, we're counting lines that have "field:" in them.

Now, I suppose we could count lines, period, whether they have "field:" 
in them or not (e.g., the ID: line), but I'm okay with forgoing that 
change until some future patch.  I get that this "__data_loc" change is 
no more relevant than what you propose.  But, if folks are okay with it, 
I wouldn't mind just pushing ahead with the patch as it is.

But, yeah, you make a reasonable suggestion.  Thanks.

> (This affects the next patch as much as this
> one. More generally, it might be clearer to move skipping to a separate
> getline loop entirely, above this one that also does all the other work.)