Skip to content

libbpf-cargo: Getters for struct fields #1161

Closed
@Waujito

Description

@Waujito

Hi!
Thank you for libbpf-rs, it's amazing!

Now, the access to struct fields is performed directly:
Consider we have the iphdr struct:

#[derive(Debug, Default, Copy, Clone)]
#[repr(C)]
pub struct iphdr {
    pub __pad_0: [u8; 1],
    pub tos: u8,
    pub tot_len: u16,
    pub id: u16,
    pub frag_off: u16,
    pub ttl: u8,
    pub protocol: u8,
    pub check: u16,
    pub __anon_6: __anon_6,
}

If we want to access any field, we will do

let tos = iph.tos;

The main problem I see here is with anonymous structures. In my case, to access saddr field of my struct, I will do iph.__anon_6.addrs.saddr. But what if we change our BPF program a little bit, like add a new anonymous union before iphdr? Now, we will access saddr with iph.__anon_7.addrs.saddr. This also involves problems with readability.

I think the solution may be to simulate access to these fields and implement fields as getters in Rust:

impl iphdr {
    pub fn check(&self) -> u16 { self.check }
    pub fn protocol(&self) -> u8 { self.protocol }
    pub fn tot_len(&self) -> u16 { self.tot_len }
    pub fn id(&self) -> u16 { self.id }
    pub fn frag_off(&self) -> u16 { self.frag_off }
    pub fn tos(&self) -> u8 { self.tos }
    pub fn ttl(&self) -> u8 { self.ttl }
    pub unsafe fn daddr(&self) -> u32 { unsafe { self.__anon_7.addrs.daddr } }
    pub unsafe fn saddr(&self) -> u32 { unsafe { self.__anon_7.addrs.saddr } }
}

Also, the getter semantics will allow us to implement bitfield access in the future.

I have written some reference recursive implementation for generator:

diff --git a/libbpf-cargo/src/gen/btf.rs b/libbpf-cargo/src/gen/btf.rs
index 3be3cba..0ea22a4 100644
--- a/libbpf-cargo/src/gen/btf.rs
+++ b/libbpf-cargo/src/gen/btf.rs
@@ -681,6 +681,74 @@ impl<'s> GenBtf<'s> {
         self.type_definition_for_composites_with_opts(def, dependent_types, t, &opts)
     }
 
+    fn generate_explicit_field_tree(
+        &self,
+        t: types::Composite<'_>,
+        vc: &mut HashMap<String, String>,
+        opts: &TypeDeclOpts,
+        precontext: &String,
+        mut field_unsafe: bool,
+    ) -> Result<()> {
+        if !t.is_struct {
+            field_unsafe = true;
+        }
+
+        for member in t.iter() {
+            let member_offset = match member.attr {
+                MemberAttr::Normal { offset } => offset,
+                // Bitfields are tricky to get correct, if at all possible. For
+                // now we just skip them, which results in them being covered by
+                // padding bytes.
+                MemberAttr::BitField { .. } => continue,
+            };
+
+            let field_ty = self
+                .type_by_id::<BtfType<'_>>(member.ty)
+                .unwrap()
+                .skip_mods_and_typedefs();
+
+            let field_name = if let Some(name) = member.name {
+                escape_reserved_keyword(name.to_string_lossy())
+            } else {
+                // Only anonymous unnamed unions should ever have no name set.
+                // We just name them the same as their anonymous type. As there
+                // can only be one member of this very type, there can't be a
+                // conflict.
+                self.type_map.type_name_or_anon(&field_ty)
+            };
+            let field_ty_str = type_declaration_impl(field_ty, &self.type_map, opts)?;
+            let field_ty_str = if is_unsafe(field_ty) {
+                Cow::Owned(format!("std::mem::MaybeUninit<{field_ty_str}>"))
+            } else {
+                Cow::Borrowed(field_ty_str.as_str())
+            };
+
+            if let Ok(composite) = TryInto::<types::Composite<'_>>::try_into(field_ty) {
+                if composite.is_empty_union() {
+                    // Skip empty union field; we do not generate a type for them.
+                    continue;
+                }
+                if let None = field_ty.name() {
+                    // Anonymous struct or union
+                    self.generate_explicit_field_tree(composite, vc, opts, &format!("{precontext}.{field_name}"), field_unsafe)?;
+                    continue;
+                }
+            }
+ 
+            let mut st = String::new();
+
+            if field_unsafe {
+                st.push_str(format!(r#"    pub unsafe fn {field_name}(&self) -> {field_ty_str} {{ unsafe {{ self{precontext}.{field_name} }} }}"#).as_str()); 
+            } else {
+                st.push_str(format!(r#"    pub fn {field_name}(&self) -> {field_ty_str} {{ self{precontext}.{field_name} }}"#).as_str()); 
+            }
+
+            vc.insert(String::from(field_name), st);
+        }
+
+        Ok(())
+    }
+
     fn type_definition_for_composites_with_opts<'a>(
         &'a self,
         def: &mut String,
@@ -697,6 +765,7 @@ impl<'s> GenBtf<'s> {
 
         // fields in the aggregate
         let mut agg_content: Vec<String> = Vec::new();
+        let mut getter_content: HashMap<String, String> = HashMap::new();
 
         // structs with arrays > 32 length need to impl Default
         // rather than #[derive(Default)]
@@ -812,6 +881,8 @@ impl<'s> GenBtf<'s> {
             agg_content.push(format!(r#"    pub {field_name}: {field_ty_str},"#));
         }
 
+        self.generate_explicit_field_tree(t, &mut getter_content, opts, &String::from(""), false)?;
+
         if t.is_struct {
             let struct_size = t.size();
             let padding = required_padding(offset, struct_size, &t, packed)?;
@@ -850,6 +921,16 @@ impl<'s> GenBtf<'s> {
         }
         writeln!(def, "}}")?;
 
+        writeln!(
+            def, 
+            r#"impl {name} {{"#,
+            name = self.type_map.type_name_or_anon(&t)
+        )?;
+        for (field_name, st) in getter_content {
+            writeln!(def, "{st}")?;
+        }
+        writeln!(def, "}}")?;
+
         // if required write a Default implementation for this struct
         if gen_impl_default {
             writeln!(

Few opinionable problems I noticed here so far:

  • Mutability: We may want to duplicate getters with a _mut suffix to allow mutable members
  • Return type: Should our getter return a reference to the field or just copy it?
  • Name collisions: What if C structure has field default, like ::default() method?
  • Unsafe functions: To access union fields, we use unsafe in rust. And naturally the getter also will have it. So, the question is should we mark all the getters with unsafe or do this for only union ones.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions