libbpf-rs: optionally return full vecs from query #269

alexgartrell · 2022-09-09T04:11:10Z

NOTE: this is totally API-breaking, but the initial API wasn't really useful.

This avoids allocation overhead and guessing by reading the values and re-invoking the system call. Each array is blocked by a feature flag from the iterator.

Here's an example program using iced-x86 to disassembly the jited code:

use iced_x86::Formatter;

fn main() {
    for prog in libbpf_rs::query::ProgInfoIter::default().include_jited_prog_insns(true) {
        let mut d = iced_x86::Decoder::new(32, &prog.jited_prog_insns, 0);
        let mut f = iced_x86::GasFormatter::new();
        while d.can_decode() {
            let insn = d.decode();
            let mut f_insn = String::new();
            f.format(&insn, &mut f_insn);
            println!("{}", f_insn);
        }
    }
}

alexgartrell · 2022-09-09T04:12:41Z

I don't think that this is a straightforward change so I'd consider this pull request more of an RFC. The tl;dr; is that it's really not valuable to just get the number of instructions if you actually want to introspect. The btf stuff is extra not-valuable for the same reason.

This probably ultimately requires actually writing out the code for the iterators instead of a big chunky macro to generate them, but I don't think that's the end of the world.

anakryiko

I like these changes, as you said makes for much more usable API. I don't think breaking API change is a big deal, so I'd go ahead with this.

I left few comments on details (e.g., two explicit steps vs a loop), but those are minor. Let's wait for @danielocfb to return to properly review this and land.

Thanks for improving libbpf-rs! :)

anakryiko · 2022-09-22T18:41:19Z

libbpf-rs/src/query.rs

+        let item_ptr: *mut libbpf_sys::bpf_prog_info = &mut item;
+        let mut len = size_of::<libbpf_sys::bpf_prog_info>() as u32;
+
+        loop {


the actual loop looks unnecessary. You shouldn't need more than two calls for bpf_obj_get_info_by_fd: first to get lengths of necessary buffers, second to fill those buffers out by kernel. And then skip the second step if none of options were set. It should also simplify update_vec_field!() macro, probably, as you know that you are resizing vec.

anakryiko · 2022-09-22T18:43:05Z

libbpf-rs/src/query.rs

    pub id: u32,
-    pub jited_prog_len: u32,


a bit conflicted on removing len fields. They seem useful for cases when user doesn't use ProgInfoQueryOptions and wants to do something smarted and custom on their own after fetching lengths. But this might be a bit hypothethical problem... Having vec fields and separate lens doesn't seem like a big deal?

In this model, the _len is just available by calling .len() on the resulting vector. So I do think it's worth it to just do it the clean way in this instance

I have no strong feelings either way. Unless we have a good use case for the additional length fields this model of just the Vec carrying the length is fine with me. We label ourselves "opinionated eBPF tooling" after all :-)

FWIW, upon reflection, the reason to do this would be to write an application that only takes the jited instructions if there are <= N of them (for example). This is likely niche enough to ignore but I'll let people argue with me in V2

danielocfb

Sorry for the delay. Left some comments. I'd really want for us to try and limit the number of macros; it is borderline unmaintainable. I don't think it's obvious what can be passed in to said macros and there is basically no contract that each argument has to fulfill.

libbpf-rs/src/query.rs

danielocfb · 2023-08-01T17:16:20Z

NOTE: this is totally API-breaking

I think that is fine.

danielocfb

The updated changes look great! Thanks (and thanks for illustrating API usage in the example)! I left a few more minor comments.
Other than that I'd just hope we could add one or more unit tests for the changed functionality. Do you see a way to go about that? At least something like query a program and check non-zero instruction count or similar. Could also include parts of the example. It's mostly that we have something runnable in an automated fashion.

danielocfb · 2023-08-02T17:45:14Z

libbpf-rs/src/query.rs

+        if ret != 0 {
+            return None;
+        }


One suggestion: Would it make sense to make the constructor return a Result and use util::parse_ret here? We are implicitly and unnecessarily throwing away the error information here, when we should ideally leave that up to callers (with something like Result::ok) to give them the option of providing more context.

examples/bpf_query/Cargo.toml

danielocfb · 2023-08-02T17:47:43Z

libbpf-rs/src/query.rs

+#[derive(Clone, Debug)]
+/// BTF Line information


Nit: Here and below, can we keep documentation comments above all attributes to keep style consistent with rest of the code base.

Suggested change

#[derive(Clone, Debug)]

/// BTF Line information

/// BTF Line information

#[derive(Clone, Debug)]

danielocfb · 2023-08-02T18:03:25Z

libbpf-rs/src/query.rs

+        if ret != 0 {
+            return None;
+        }


Same comment as earlier about propagating errors up.

danielocfb · 2023-08-02T18:04:29Z

libbpf-rs/src/query.rs

+        let mut name: Vec<u8> = Vec::new();
+
+        let item_ptr: *mut libbpf_sys::bpf_btf_info = &mut item;
+        let mut len = size_of::<libbpf_sys::bpf_btf_info>() as u32;


Suggested change

let mut len = size_of::<libbpf_sys::bpf_btf_info>() as u32;

let mut len = size_of_val(&item) as u32;

Fixing this instance, but there are others in this file for a subsequent diff

danielocfb · 2023-08-02T18:08:31Z

libbpf-rs/src/query.rs

+    pub name: String,
+    pub btf: Vec<u8>,


Would you mind terribly documenting these changed members? We try to document everything that is new at the very least in the hopes that eventually we can remove all #[allow(missing_docs)].

danielocfb · 2023-08-02T18:11:04Z

libbpf-rs/src/query.rs

-            id: s.id,
-        })
+    fn load_from_fd(fd: i32) -> Option<Self> {
+        let mut item: libbpf_sys::bpf_btf_info = unsafe { std::mem::zeroed() };


Nit: let's import std items we use at the top for the sake of consistency with existing code:

Suggested change

let mut item: libbpf_sys::bpf_btf_info = unsafe { std::mem::zeroed() };

let mut item: libbpf_sys::bpf_btf_info = unsafe { mem::zeroed() };

danielocfb · 2023-08-02T18:16:26Z

libbpf-rs/src/query.rs

+            unsafe { libbpf_sys::bpf_obj_get_info_by_fd(fd, item_ptr as *mut c_void, &mut len) };
+        if ret == 0 {
+            Some(BtfInfo {
+                name: String::from_utf8(name).unwrap_or_else(|_| "(?)".to_string()),


One more suggestion: how about we make BtfInfo::name a CString. This way we can forego this opinionated fallible conversion and let users choose how to handle errors and it's barely more than a to_string_lossy call on their end if they opt not to.
It's what we do in the existing BTF functionality: https://docs.rs/libbpf-rs/0.21.2/libbpf_rs/btf/struct.BtfType.html#method.name

For what it's worth, ProgramInfo::name is a String as well. Should that be changed?

Ah, I missed that. Yes, that would be great. Though I won't insist you do it as you didn't really touch that member. So I'll leave it up to you. I added it to my list of things to do as well.

NOTE: this is totally API-breaking, but the initial API wasn't really useful. This avoids allocation overhead and guessing by reading the values and re-invoking the system call. Each array is blocked by a feature flag from the iterator. The bpf_query example now adds the ability to dump disassembled code on x86-64 via iced-x86.

Fetch the actual data from the kernel. In this case, partial fetches of either name or btf data don't make sense, so we haven't implemented the Options pattern as we did for the BpfProgIter

alexgartrell · 2023-08-03T00:08:07Z

I agree the lack of testing is a problem, but I propose we provide it in another diff. The real path here is to write cfg(test) stubs for the libbpf calls and mock it up completely, but that will be a monster change.

Absent that, you end up with tests that need sudo, which isn't super necessary/desirable here IMHO

danielocfb

Seems mostly fine now. I don't think you addressed all the points or perhaps I dreamed about pointing them out or GitHub swallowed them. Will create a fixup commit and add a CHANGELOG entry, but merging this change. Thanks!

danielocfb · 2023-08-03T16:05:24Z

libbpf-rs/src/query.rs

+
+            match info {
+                Ok(i) => return Some(i),
+                Err(e) => eprintln!("Failed to load btf information: {}", e),


Let's please not print stuff in a library. It will end up in callers without any context in places where it is unlikely to be expected.

danielocfb · 2023-08-03T16:11:53Z

libbpf-rs/src/query.rs

-    pub btf: u64,
-    pub btf_size: u32,
+    /// The name associated with this btf information in the kernel
+    pub name: String,


Hm, I thought we agreed on making this CString. But now not even the comment shows up anymore...

danielocfb · 2023-08-03T16:17:09Z

Absent that, you end up with tests that need sudo, which isn't super necessary/desirable here IMHO

Half our tests already require sudo.

This change addresses a few remaining issues of #269 and adds a CHANGELOG entry. Signed-off-by: Daniel Müller <[email protected]>

This change adds a CHANGELOG entry for #541. Signed-off-by: Daniel Müller <[email protected]>

anakryiko reviewed Sep 22, 2022

View reviewed changes

alexgartrell marked this pull request as draft August 1, 2023 16:33

danielocfb requested changes Aug 1, 2023

View reviewed changes

alexgartrell marked this pull request as ready for review August 2, 2023 04:50

alexgartrell requested a review from danielocfb August 2, 2023 05:21

danielocfb reviewed Aug 2, 2023

View reviewed changes

alexgartrell added 2 commits August 2, 2023 19:59

libbpf-rs: Implement more complete BpfInfoIter

167db3f

Fetch the actual data from the kernel. In this case, partial fetches of either name or btf data don't make sense, so we haven't implemented the Options pattern as we did for the BpfProgIter

alexgartrell requested a review from danielocfb August 3, 2023 00:08

danielocfb approved these changes Aug 3, 2023

View reviewed changes

danielocfb merged commit 2575135 into libbpf:master Aug 3, 2023

danielocfb mentioned this pull request Aug 3, 2023

Add CHANGELOG entry for #269 #539

Merged

alexgartrell deleted the include-arrays branch August 3, 2023 21:42

danielocfb pushed a commit that referenced this pull request Aug 4, 2023

Add CHANGELOG entry for #269

e483371

This change addresses a few remaining issues of #269 and adds a CHANGELOG entry. Signed-off-by: Daniel Müller <[email protected]>

danielocfb pushed a commit that referenced this pull request Aug 7, 2023

Add CHANGELOG entry for #269

874d0f5

This change adds a CHANGELOG entry for #541. Signed-off-by: Daniel Müller <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

libbpf-rs: optionally return full vecs from query #269

libbpf-rs: optionally return full vecs from query #269

alexgartrell commented Sep 9, 2022

alexgartrell commented Sep 9, 2022

anakryiko left a comment

anakryiko Sep 22, 2022

anakryiko Sep 22, 2022

alexgartrell Aug 1, 2023

danielocfb Aug 1, 2023

alexgartrell Aug 1, 2023

danielocfb left a comment

danielocfb commented Aug 1, 2023

danielocfb left a comment

danielocfb Aug 2, 2023 •

edited

Loading

danielocfb Aug 2, 2023

danielocfb Aug 2, 2023

danielocfb Aug 2, 2023

alexgartrell Aug 2, 2023

danielocfb Aug 2, 2023

danielocfb Aug 2, 2023

danielocfb Aug 2, 2023

alexgartrell Aug 2, 2023

danielocfb Aug 2, 2023

alexgartrell commented Aug 3, 2023

danielocfb left a comment

danielocfb Aug 3, 2023

danielocfb Aug 3, 2023

danielocfb commented Aug 3, 2023

	let mut len = size_of::<libbpf_sys::bpf_btf_info>() as u32;
	let mut len = size_of_val(&item) as u32;

	let mut item: libbpf_sys::bpf_btf_info = unsafe { std::mem::zeroed() };
	let mut item: libbpf_sys::bpf_btf_info = unsafe { mem::zeroed() };

libbpf-rs: optionally return full vecs from query #269

libbpf-rs: optionally return full vecs from query #269

Conversation

alexgartrell commented Sep 9, 2022

alexgartrell commented Sep 9, 2022

anakryiko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielocfb left a comment

Choose a reason for hiding this comment

danielocfb commented Aug 1, 2023

danielocfb left a comment

Choose a reason for hiding this comment

danielocfb Aug 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexgartrell commented Aug 3, 2023

danielocfb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielocfb commented Aug 3, 2023

danielocfb Aug 2, 2023 •

edited

Loading