On 1/20/20 2:52 PM, Andrii Nakryiko wrote:
>> + }
>> + if (tr->extension_prog) {
>> + /* cannot attach fentry/fexit if extension prog is attached */
>> + err = -EBUSY;
>> + goto out;
>> + }
> move this check before BPF_TRAMP_REPLACE check and check additonally
> for fentry+fexit for BPF_TRAMP_REPLACE? Nothing can replace
> extension_prog, right?
makes sense. fixed.
>> + if (tgt_prog->type == BPF_PROG_TYPE_TRACING &&
>> + tgt_prog->expected_attach_type != BPF_TRACE_RAW_TP &&
> if the intent is to prevent extending FENTRY/FEXIT, why not checking
> explicitly for those two instead of making assumption that
> expected_attach_type can be only one of RAW_TP/FENTRY/FEXIT, this can
> easily change in the future. Besides, direct FENTRY/FEXIT comparison
> is more self-documenting as well.
sure. fixed as well.
>> }
>> + if (prog_extension &&
>> + btf_check_type_match(env, prog, btf, t))
> this reads so weird... btf_check_type_match (and
> btf_check_func_type_match as well) are boolean functions (i.e., either
> matches or not, or some error), why not using a conventional
> boolean+error return convention: 0 - false, 1 - true, <0 - error
> (bug)?
I cannot agree here. Such return convention will be very odd.
The one I picked is consistent with other places.
