On Mon, 2022-08-01 at 10:33 +0000, Roberto Sassu wrote: > > From: Andrii Nakryiko [mailto:andrii.nakryiko@xxxxxxxxx] > > Sent: Friday, July 29, 2022 8:49 PM > > On Mon, Jul 25, 2022 at 12:10 AM Roberto Sassu < > > roberto.sassu@xxxxxxxxxx> > > wrote: > > > > From: Alexei Starovoitov [mailto:alexei.starovoitov@xxxxxxxxx] > > > > Sent: Friday, July 22, 2022 7:55 PM > > > > On Fri, Jul 22, 2022 at 07:18:23PM +0200, Roberto Sassu wrote: > > > > > The bpf() system call validates the bpf_attr structure > > > > > received as > > > > > argument, and considers data until the last field, defined > > > > > for each > > > > > operation. The remaing space must be filled with zeros. > > > > > > > > > > Currently, for bpf_*_get_fd_by_id() functions except > > bpf_map_get_fd_by_id() > > > > > the last field is *_id. Setting open_flags to BPF_F_RDONLY > > > > > from user space > > > > > will result in bpf() rejecting the argument. > > > > > > > > The kernel is doing the right thing. It should not ignore > > > > fields. > > > > > > Exactly. As Andrii requested to add opts to all > > > bpf_*_get_fd_by_id() > > > functions, the last field in the kernel needs to be updated > > > accordingly. > > > > > > > It's been a while ago so details are hazy. But the idea was that if > > we > > add _opts variant for bpf_map_get_fd_by_id() for interface > > consistency > > all the other bpf_*_get_fd_by_id() probably should get _opts > > variant > > and use the same opts struct. Right now kernel doesn't support > > specifying flags for non-maps and that's fine. I agree with Alexei > > that kernel shouldn't just ignore unrecognized field silently. > > > > I think we still can add _opts() for all APIs, but user will need > > to > > know that non-map variants expect 0 as flags. For now. If we > > eventually add ability to specify flags for, say, links, then > > existing > > API will just work. One can see how this get_fd_by_id() can use > > read-only flags to return FDs that only support read-only > > operations > > on objects (e.g., fetching link info for links, dumping prog > > instructions for programs), but not modification operations (e.g., > > updating prog for links, or whatever write operation could be for > > programs). > > > > So I don't think there is contradiction here. We might choose to > > add > > bpf_map_get_fd_by_id_opts() only, but we probably still should use > > common struct name as if all bpf_*_get_fd_by_id_opts() exist. > > Ok, understood. Hi Andrii I'm about to send v4 with the suggestions you made. Since now libbpf v1 has been released, how it works for new patches? It seems there is not a new section in libbpf.map (kernel) new API functions should be added to. Also, I'm using a custom step in the CI: https://github.com/robertosassu/libbpf-ci/commit/7743eb92f81f95355571c07e5b7082a9a2b0bfe0 Do you want me to create a new PR before sending the patch set? Thanks Roberto
