Re: [PATCH bpf-next v2 02/15] bpf: Make btf_find_field more generic

[Alexei Starovoitov <alexei.starovoitov@gmail.com>]

On Sun, Mar 20, 2022 at 01:36:41AM +0530, Kumar Kartikeya Dwivedi wrote:
> > > >  			return -EINVAL;
> > > > +
> > > > +		switch (field_type) {
> > > > +		case BTF_FIELD_SPIN_LOCK:
> > > > +		case BTF_FIELD_TIMER:
> > >
> > > Since spin_lock vs timer is passed into btf_find_struct_field() as field_type
> > > argument there is no need to pass name, sz, align from the caller.
> > > Pls make btf_find_spin_lock() to pass BTF_FIELD_SPIN_LOCK only
> > > and in the above code do something like:
> > >  switch (field_type) {
> > >  case BTF_FIELD_SPIN_LOCK:
> > >      name = "bpf_spin_lock";
> > >      sz = ...
> > >      break;
> > >  case BTF_FIELD_TIMER:
> > >      name = "bpf_timer";
> > >      sz = ...
> > >      break;
> > >  }
> >
> > Would doing this in btf_find_field be better? Then we set these once instead of
> > doing it twice in btf_find_struct_field, and btf_find_datasec_var.

yeah. probably.

> >
> > >  switch (field_type) {
> > >  case BTF_FIELD_SPIN_LOCK:
> > >  case BTF_FIELD_TIMER:
> > > 	if (!__btf_type_is_struct(member_type))
> > > 		continue;
> > > 	if (strcmp(__btf_name_by_offset(btf, member_type->name_off), name))
> > >         ...
> > >         btf_find_field_struct(btf, member_type, off, sz, info);
> > >  }
> > >
> > > It will cleanup the later patch which passes NULL, sizeof(u64), alignof(u64)
> > > only to pass something into the function.
> > > With above suggestion it wouldn't need to pass dummy args. BTF_FIELD_KPTR will be enough.
> > >
> 
> Just to be clear, for the kptr case we still use size and align, only name is
> optional. size is used for datasec_var call, align is used in both struct_field
> and datasec_var. So I'm not sure whether moving it around has much effect,
> instead of the caller it will now be set based on field_type inside
> btf_find_field.

There is no use case to do BTF_FIELD_KPTR, sizeof(u64) and BTF_FIELD_KPTR, sizeof(u32), right?
So best to avoid such mistakes.
In other words consider every function to be a uapi.
Not in a way that it can never change, but from pov that you wouldn't want the user space
to specify all details for the kernel when BTF_FIELD_KPTR is enough to figure out the rest.