On Wed, 2023-12-13 at 11:18 -0800, Andrii Nakryiko wrote:
[...]
> > > @@ -6846,7 +6846,35 @@ int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog)
[...]
> > > + if (tag) {
> > > + /* disallow arg tags in static subprogs */
> > > + if (!is_global) {
> > > + bpf_log(log, "arg#%d type tag is not supported in static functions\n", i);
> > > + return -EOPNOTSUPP;
> > > + }
> > > + if (strcmp(tag, "ctx") == 0) {
> > > + sub->args[i].arg_type = ARG_PTR_TO_CTX;
> > > + continue;
> >
> > Nit: personally, I'd keep tags parsing and processing logically separate:
> > - at this point set a flag 'is_ctx'
> > - and modify the check below as follows:
> >
> > if (is_ctx || (btf_type_is_ptr(t) && btf_get_prog_ctx_type(log, btf, t, prog_type, i))) {
> > sub->args[i].arg_type = ARG_PTR_TO_CTX;
> > continue;
> > }
> >
> > So that there is only one place where ARG_PTR_TO_CTX is assigned.
> > Feel free to ignore.
>
> I think it's actually more convoluted and error-prone with the is_ctx
> flag. Tag is overriding whatever BTF type information we have. But if
> we delay ARG_PTR_TO_CTX setting to later, we need to make sure that
> post-tag BTF processing is aware of is_ctx (and potentially other)
> flags and doesn't freak out. We might add more BTF processing before
> ARG_PTR_TO_CTX, etc. Having to worry about not overriding tag-based
> decisions seems very error-prone.
>
> Also, btf_prepare_func_args() really simplifies the "definition" of an
> argument to one enum (and in some cases maybe an extra argument like
> memory size). It should be fine to just set this enum in two places
> separately, doesn't seem like a hindrance for maintainability.
Well, I disagree a bit, but that does not matter.
You are right that after extracting btf_prepare_func_args()
the code is easy enough to follow.
