Add fd to ProgramInfo

[mdaverde]

A bpf program's fd can be useful to obtain relevant data such as memlock (like how it's done in bpftool). This change allows users to obtain the fd of a bpf program without making them resort to an unsafe call in libbpf-sys.

Signed-off-by: Milan [email protected]

Alternatives

Add fd to Iterator::Item. Instead of the iterator producing ProgramInfo, a tuple can be generated instead: (fd, ProgramInfo).

Add ability to safely convert from ProgramInfo to Program (which has necessary fd())

Leave the user to call unsafe libbpf_sys::bpf_prog_get_fd_by_id()

Other considerations

We can also return fds for other bpf object types (MapInfo, BtfInfo, LinkInfo)

[anakryiko]

Did you test this code? Did it work? From what I see, by the time ProgramInfo is returned to user that fd is already closed.

I think the right way is to let user do id to fd conversion with bpf_prog_get_fd_by_id(). If libbpf-rs doesn't have a safe wrapper, we should probably add that instead.

[mdaverde]

What are your thoughts on moving the close to the Drop handler for the Info types? At least then (from what I understand), we keep bpf object alive for the Info type lifetime.

[anakryiko]

What are your thoughts on moving the close to the Drop handler for the Info types? At least then (from what I understand), we keep bpf object alive for the Info type lifetime.

That would work (thanks Rust!), but I still feel like that it's not very appropriate to keep so many FDs open, especially that in almost all the cases no one really needs that FD. So I think a safe wrapper around bpf_prog_get_fd_by_id() is a better approach (and more general as well).

[mdaverde]

Ya I think there's value in adding a more general safe wrapper to go from id to fd. We could also consider adding safe methods to each relevant type (e.g. ProgramInfo::fd() -> Result), but I still feel like the default iterator can lead to unnecessary complexity for the user?

We already pay the cost of opening all the fds but we close them before passing the Info type to the user. The bpf object might not be alive for the operation involving the Info type so we've forced the user to deal with that possible inconsistency.

So even if the user might not need the fd per se, they might have expectations on the lifetime of the bpf object represented in the iterator? An option would be to create a new iterator ProgInfoIter::with_fd() instead of changing the default.

[anakryiko]

We already pay the cost of opening all the fds but we close them before passing the Info type to the user. The bpf object might not be alive for the operation involving the Info type so we've forced the user to deal with that possible inconsistency.

So even if the user might not need the fd per se, they might have expectations on the lifetime of the bpf object represented in the iterator? An option would be to create a new iterator ProgInfoIter::with_fd() instead of changing the default.

Yeah, I definitely see a value of keeping FD open, but it feels wrong to add fd field to ProgramInfo as ProgramInfo is supposed to match kernel's UAPI bpf_prog_info struct. So I think ProgInfoIter::with_fd() seems like the best compromise, giving users an option to either keep or not FDs. With explicit ::with_fd() we might not even need to do Drop implementation to force FD closure, it will be up to users to take care of that (though I'm not sure how file descriptors are handled by other libraries in Rust?). Having an iterator implementation that returns a pair of (fd, ProgramInfo) seems fine to me.

@insearchoflosttime, what do you think?

[github-actions[bot]]

This pull request is considered stale because it has been open 30 days with no activity. Remove stale label or comment or it will be closed in 5 days.

[github-actions[bot]]

Closing pull request as it is stale.