iter: add None/NULL variant for attach IterOpts#1352
Conversation
ver-nyan
force-pushed
the
iter_attach_opts-null
branch
from
2d7a794 to
eff21e4
Compare
|
I still have mixed feelings about Also since |
d-e-s-o
left a comment
d-e-s-o
left a comment
There was a problem hiding this comment.
I still have mixed feelings about
IterOpts::Nonevs changing signature toOption<IterOpts>, but ill defer to whatever you think is best.
I don't think a with_opts method should have optional opts. It makes no sense to me. That, and I don't want to break backwards compat for a minor change like this. If you can come up with a reasonable alternative design then we can go with that.
Also since
From<IterOpts>trait is a public API, should we still remove it? If keeping it, i can add a rustdoc note about theunreachableonIterOpts::Nonevariant in that case.
From my perspective it's an implementation detail that we needed such a conversion and added it via a public facing trait impl. Yes, removal is strictly speaking breaking, but I seriously doubt anybody will depend on the existence of this impl: no other public function even accepts a bpf_iter_link_info.
| let mut linkinfo = libbpf_sys::bpf_iter_link_info::from(opts); | ||
| let mut linkinfo = match opts { | ||
| IterOpts::None => None, | ||
| IterOpts::Map(map_opts) => Some(libbpf_sys::bpf_iter_link_info { |
There was a problem hiding this comment.
Can you please make sure to destruct the inner objects exhaustively, as was the case in the From impl? Same below.
I'd also prefer we don't name bindgen anonymous types ala xxx_bindgen_ty_1, but we can keep that if need be.
There was a problem hiding this comment.
They should be destructed now, and the bindgen_ty aren't directly referenced now. 👍
Add `IterOpts::None` variant for attaching with no `bpf_iter_link_info` to `bpf_program__attach_iter()`. Also removes the `From<IterOpts> for bpf_iter_link_info` trait since the `None` variant does not map to any of its union members. Closes libbpf#1348
ver-nyan
force-pushed
the
iter_attach_opts-null
branch
from
eff21e4 to
57003a4
Compare
d-e-s-o
left a comment
d-e-s-o
left a comment
There was a problem hiding this comment.
Seems fine to me now, thanks!
2 participants
Add
IterOpts::Nonevariant for attaching with nobpf_iter_link_infospecified tobpf_program__attach_iter.Also removes the
From<IterOpts> for bpf_iter_link_infotrait since theNonevariant does not map to any of its union members.Closes #1348
Currently set as draft since there's a compiler false positive
unused_assignments, same as rust-lang/rust#150656. I'll try to satisfy it after I get some confirmation in the Rust issue.