[DTrace-devel] [PATCH 1/3] Rename "path" helper to "tstring1" helper

[Eugene Loh]

I made many of the changes but am curious about one thing:

On 3/17/22 7:15 PM, Eugene Loh wrote:
> On 3/17/22 6:45 PM, Kris Van Hees wrote:
>> I am not sure about this patch...  First of all, passing the isptr 
>> argument
>> seems unnecessary because the type of dnp should give us that info 
>> already.
> Okay.  I'll try to eliminate isptr.
>> But more importantly, you are now using this helper function to make BPF
>> calls to functions with differnet (incompatible) prototypes (once the 
>> other
>> patches are applied also):
>>
>>     void dt_basename(char *src, char *dst)
>>     void dt_dirname(char *src, char *dst)
>>     int dt_lltostr(uint64_t VAL, char *STR)
>>     uint64_t dt_inet_ntoa(uint8_t *src, char *dst)
> Ugh.  Both dt_lltostr() and dt_inet_ntoa() are actually void. I'll fix 
> those comments (which is all they are) in their respective patches.
>> And the comment block before the helper states:
>>
>>      /*
>>       * This function is a helper function for subroutines that take one
>>       * argument and return the tstring resulting from applying a 
>> given BPF
>>       * function (passed by name) on it.
>>       */
>>
>> So, there is quite some inconsistency here.  None of the functions 
>> you use
>> this helper for take just one argument.  None of them return a tstring.
>> And both their argument lists vary (well, the first argument), and the
>> return type is different.
>>
>> While it does "work", I do not thiink it is appropriate to do things 
>> this
>> way.  At a minimum, the first argument should probably be uintptr_t 
>> so that
>> you properly capture that it is a value that can be a pointer or an 
>> integer.
Actually, I don't really get this.  There is no type checking.  And the 
code generation to call the precompiled BPF function is the same 
regardless of whether that first arg is a pointer or integer or 
whatever.  (Well, modulo that isptr thing you pointed out.)  The 
"prototypes" we're talking about here are just comments in the various 
bpf/*.S files.  Changing the types of the first args from their current 
types to uintptr_t -- again, we're only talking about changes in 
comments -- would make those comments less clear.

So would you be okay with leaving the types of the first args, as 
described in the comments, as varying?

I'm only talking about the types of those first args.  I agree with you 
on the return values:  they should all be void and I've updated the 
comments accordingly.
>> And ensure that they all have the same return type.
> Got it.
>> Then, with the comment describing the function updated, this seems to be
>> both useful and consistent.
> Okay.  I'll clean up the patch series and repost.  Thanks for the 
> comments.
>> On Thu, Mar 17, 2022 at 04:37:49PM -0400, eugene.loh--- via 
>> DTrace-devel wrote:
>>> From: Eugene Loh <eugene.loh at oracle.com>
>>>
>>> When the code generator sets up a call to a precompiled BPF function,
>>> it emits a lot of boilerplate code.  The implementations of DTrace
>>> subroutines typically make such calls.  So there is potential for
>>> much code reuse.
>>>
>>> The "path" helper is one such attempt.  Since its usefulness is
>>> broader than just for a "path name" argument, rename this helper to
>>> suggest its broader usage more clearly.