Re: [PATCH bpf-next v2 4/5] bpftool: add support for subskeletons

[Delyan Kratunov <delyank@fb.com>]

On Mon, 2022-03-14 at 16:13 -0700, Delyan Kratunov wrote:
> Thanks, Andrii! The nits/refactors are straightforward but have some questions
> below:
> 
> On Fri, 2022-03-11 at 15:27 -0800, Andrii Nakryiko wrote:
> > > +
> > > +                       /* sanitize variable name, e.g., for static vars inside
> > > +                        * a function, it's name is '<function name>.<variable name>',
> > > +                        * which we'll turn into a '<function name>_<variable name>'.
> > > +                        */
> > > +                       sanitize_identifier(var_ident + 1);
> > 
> > btw, I think we don't need sanitization anymore. We needed it for
> > static variables (they would be of the form <func_name>.<var_name> for
> > static variables inside the functions), but now it's just unnecessary
> > complication
> 
> How would we handle static variables inside functions in libraries then?

Ah, just realized 31332ccb7562 (bpftool: Stop emitting static variables in BPF
skeleton) stopped that. I'll remove the sanitization then.

[...]


> > we don't know the name of the final object, why would we allow to set
> > any object name at all?
> 
> We don't really care about the final object name but we do need an object name
> for the subskeleton. The subskeleton type name, header guard etc all use it.
> We can say that it's always taken from the file name, but giving the user the
> option to override it feels right, given the parallel with skeletons (and what
> would we do if the file name is a pipe from a subshell invocation?).

In particular, with the current test setup, if we don't use the name param, we'd
end up inferring names like test_subskeleton_lib_linked3 from the filename. The
tests case is a bit contrived and we can work around it but there may be other
similar situations out there.


-- Delyan