Fix ebpf_program_batch_end_invoke_function_t definition and clean up NPI #2329
Conversation
mtfriesen
requested review from
dthaler,
poornagmsft,
Alan-Jowett,
saxena-anurag,
shankarseal,
dv-msft,
delaramamiri and
gtrevi
as code owners
| @@ -31,7 +24,7 @@ typedef struct _net_ebpf_extension_hook_client | |||
| GUID client_module_id; ///< NMR module Id. | |||
| const void* client_binding_context; ///< Client supplied context to be passed when invoking eBPF program. | |||
| const ebpf_extension_data_t* client_data; ///< Client supplied attach parameters. | |||
| ebpf_invoke_program_function_t invoke_program; ///< Pointer to function to invoke eBPF program. | |||
| ebpf_program_invoke_function_t invoke_program; ///< Pointer to function to invoke eBPF program. | |||
There was a problem hiding this comment.
Why the type rename? Previously "invoke_program" was an aptly named member because it was of type "invoke_program_function". But with this PR it no longer matches, and would be renamed "invoke_function". In short, I think the old type name is better and renaming it doesn't seem to have anything to do with the bug being fixed.
There was a problem hiding this comment.
The PR description outlines the reasoning for why the change is being made, which I'll copy here for posterity:
- To let the compiler catch this error, both in the eBPF project and in extension projects, explicitly define the dispatch table used to invoke programs.
- Remove redundant (and erroneous) definitions throughout the eBPF test and sample codebase.
If the naming in a test file is preferred over the pre-existing, unmodified naming in the published eBPF extension header, I can go ahead and fix that, too.
There was a problem hiding this comment.
FWIW, I agree invoke_program is a better name, too.
Description
The
_ebpf_link_instance_invoke_batch_enddefinition in the eBPF implementation diverged from theebpf_program_batch_end_invoke_function_tdefinition in the extension header: astateparameter was added to the implementation, but not the header.Resolves #2310
Testing
Builds locally. CI/CD should cover the rest.
Note: the batch begin/invoke/end routines do not appear to be tested via NPI dispatch tables in the eBPF project.
Documentation
Documentation updated.