SQUASH: Address review comments

This includes adding test cases for the successful use of
expected_revision.

Signed-off-by: Andre Fredette <afredette@redhat.com>

aya/src/programs/links.rs

@@ -397,13 +397,14 @@ pub enum LinkError {
     SyscallError(#[from] SyscallError),
 }
 
-/// A [`Link`] identifier, which may or may not be owned by aya.
+/// A [`Link`] identifier.
 pub struct LinkId(u32);
 
 impl LinkId {
-    /// A wrapper for an arbitrary loaded link specified by its kernel id,
-    /// unsafe because there is no guarantee provided by aya that the link is
-    /// still loaded or even exists.
+    /// Create a new [`LinkId`] from its kernel id.
+    ///
+    /// This method is unsafe since it doesn't check that the given `id` is a an
+    /// existing link id.
     pub unsafe fn new(id: u32) -> Self {
         Self(id)
     }
@@ -427,8 +428,7 @@ bitflags::bitflags! {
     }
 }
 
-/// Struct defining the arguments required for interacting with the kernel's
-/// multi-prog API.
+/// Arguments required for interacting with the kernel's multi-prog API.
 ///
 /// # Minimum kernel version
 ///
@@ -466,7 +466,7 @@ impl Default for LinkOrder {
 }
 
 impl LinkOrder {
-    /// Ensure the link is created before all other links at a given attachment point.
+    /// Attach before all other links.
     pub fn first() -> Self {
         Self {
             link_ref: LinkRef::Id(0),
@@ -475,7 +475,7 @@ impl LinkOrder {
         }
     }
 
-    /// Ensure the link is created after all other links for a given attachment point.
+    /// Attach after all other links.
     pub fn last() -> Self {
         Self {
             link_ref: LinkRef::Id(0),
@@ -484,7 +484,7 @@ impl LinkOrder {
         }
     }
 
-    /// Ensure the link is created before the specified aya-owned link for a given attachment point.
+    /// Attach before the given link.
     pub fn before_link<L: MultiProgLink>(link: &L) -> Result<Self, LinkError> {
         Ok(Self {
             link_ref: LinkRef::Fd(link.fd()?.as_raw_fd()),
@@ -493,7 +493,7 @@ impl LinkOrder {
         })
     }
 
-    /// Ensure the link is created after the specified aya-owned link for a given attachment point.
+    /// Attach after the given link.
     pub fn after_link<L: MultiProgLink>(link: &L) -> Result<Self, LinkError> {
         Ok(Self {
             link_ref: LinkRef::Fd(link.fd()?.as_raw_fd()),
@@ -502,7 +502,7 @@ impl LinkOrder {
         })
     }
 
-    /// Ensure the link is created before a link specified by its kernel id for a given attachment point.
+    /// Attach before the link with the given id.
     pub fn before_link_id(id: LinkId) -> Result<Self, LinkError> {
         Ok(Self {
             link_ref: LinkRef::Id(id.0),
@@ -511,7 +511,7 @@ impl LinkOrder {
         })
     }
 
-    /// Ensure the link is created after a link specified by its kernel id for a given attachment point.
+    /// Attach after the link with the given id.
     pub fn after_link_id(id: LinkId) -> Result<Self, LinkError> {
         Ok(Self {
             link_ref: LinkRef::Id(id.0),
@@ -520,7 +520,7 @@ impl LinkOrder {
         })
     }
 
-    /// Ensure the link is created before the specified aya-owned program for a given attachment point.
+    /// Attach before the given program.
     pub fn before_program<P: MultiProgProgram>(program: &P) -> Result<Self, ProgramError> {
         Ok(Self {
             link_ref: LinkRef::Fd(program.fd()?.as_raw_fd()),
@@ -529,7 +529,7 @@ impl LinkOrder {
         })
     }
 
-    /// Ensure the link is created after the specified aya-owned program for a given attachment point.
+    /// Attach after the given program.
     pub fn after_program<P: MultiProgProgram>(program: &P) -> Result<Self, ProgramError> {
         Ok(Self {
             link_ref: LinkRef::Fd(program.fd()?.as_raw_fd()),
@@ -538,7 +538,7 @@ impl LinkOrder {
         })
     }
 
-    /// Ensure the link is created before a program specified by its kernel id for a given attachment point.
+    /// Attach before the program with the given id.
     pub fn before_program_id(id: ProgramId) -> Self {
         Self {
             link_ref: LinkRef::Id(id.0),
@@ -547,7 +547,7 @@ impl LinkOrder {
         }
     }
 
-    /// Ensure the link is created after a program specified by its kernel id for a given attachment point.
+    /// Attach after the program with the given id.
     pub fn after_program_id(id: ProgramId) -> Self {
         Self {
             link_ref: LinkRef::Id(id.0),

aya/src/programs/mod.rs

@@ -217,6 +217,15 @@ pub enum ProgramError {
     /// An error occurred while working with IO.
     #[error(transparent)]
     IOError(#[from] io::Error),
+
+    /// An error occurred when attmpting to convert a type.
+    #[error("conversion error from {from} to {to}")]
+    ConversionError {
+        /// The type being converted from.
+        from: String,
+        /// The type being converted to.
+        to: String,
+    },
 }
 
 /// A [`Program`] file descriptor.
@@ -240,13 +249,14 @@ impl AsFd for ProgramFd {
 }
 
 /// The various eBPF programs.
-/// A [`Program`] identifier, which may or may not be owned by aya.
+/// A [`Program`] identifier.
 pub struct ProgramId(u32);
 
 impl ProgramId {
-    /// A wrapper for an arbitrary loaded program specified by its kernel id,
-    /// unsafe because there is no guarantee provided by aya that the program is
-    /// still loaded or even exists.
+    /// Create a new program id.  
+    ///  
+    /// This method is unsafe since it doesn't check that the given `id` is a
+    /// valid program id.
     pub unsafe fn new(id: u32) -> Self {
         Self(id)
     }
@@ -811,15 +821,14 @@ impl_fd!(
     CgroupDevice,
 );
 
-/// Defines which [`Program`]s support the kernel's
-/// generic multi-prog API.
+/// Defines the [`Program`] types which support the kernel's
+/// [generic multi-prog API](https://github.com/torvalds/linux/commit/053c8e1f235dc3f69d13375b32f4209228e1cb96).
 ///
 /// # Minimum kernel version
 ///
 /// The minimum kernel version required to use this feature is 6.6.0.
 pub trait MultiProgProgram {
-    /// Returns a borrowed reference to the file descriptor of a given
-    /// [`Program`] which has support for the kernel's generic multi-prog API.
+    /// Borrows the file descriptor.
     fn fd(&self) -> Result<BorrowedFd<'_>, ProgramError>;
 }
 
@@ -827,7 +836,6 @@ macro_rules! impl_multiprog_fd {
     ($($struct_name:ident),+ $(,)?) => {
         $(
             impl MultiProgProgram for $struct_name {
-                /// Returns the a borrowed reference file descriptor of this Program.
                 fn fd(&self) -> Result<BorrowedFd<'_>, ProgramError> {
                     Ok(self.fd()?.as_fd())
                 }
@@ -838,14 +846,14 @@ macro_rules! impl_multiprog_fd {
 
 impl_multiprog_fd!(SchedClassifier);
 
-/// Defines the [Link] types which support the kernel's [generic multi-prog API](https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=053c8e1f235dc3f69d13375b32f4209228e1cb96).
+/// Defines the [`Link`] types which support the kernel's
+/// [generic multi-prog API](https://github.com/torvalds/linux/commit/053c8e1f235dc3f69d13375b32f4209228e1cb96).
 ///
 /// # Minimum kernel version
 ///
 /// The minimum kernel version required to use this feature is 6.6.0.
 pub trait MultiProgLink {
-    /// Returns a borrowed reference to the file descriptor of a given
-    /// [`Link`] which has support for the kernel's generic multi-prog API.
+    /// Borrows the file descriptor.
     fn fd(&self) -> Result<BorrowedFd<'_>, LinkError>;
 }
 
@@ -853,7 +861,6 @@ macro_rules! impl_multiproglink_fd {
     ($($struct_name:ident),+ $(,)?) => {
         $(
             impl MultiProgLink for $struct_name {
-                /// Returns the a borrowed reference file descriptor of this Program.
                 fn fd(&self) -> Result<BorrowedFd<'_>, LinkError> {
                     let link: &FdLink = self.try_into()?;
                     Ok(link.fd.as_fd())

aya/src/programs/sock_ops.rs

@@ -75,6 +75,8 @@ impl SockOps {
                 attach_type,
                 None,
                 mode.into(),
+                None,
+                None,
             )
             .map_err(|(_, io_error)| SyscallError {
                 call: "bpf_link_create",

aya/src/programs/tc.rs

@@ -6,7 +6,6 @@ use std::{
     path::Path,
 };
 
-use log::debug;
 use thiserror::Error;
 
 use super::FdLink;
@@ -97,12 +96,12 @@ pub enum TcError {
     /// the clsact qdisc is already attached
     #[error("the clsact qdisc is already attached")]
     AlreadyAttached,
-    /// tcx links can only be attached to ingress or egress
+    /// tcx links can only be attached to ingress or egress, custom attachment is not supported
     #[error("tcx links can only be attached to ingress or egress, custom attachment: {0} is not supported")]
     InvalidTcxAttach(u32),
-    /// program was loaded via tcx not netlink
-    #[error("program was loaded via tcx not netlink")]
-    InvalidLink,
+    /// operation not supported for programs loaded via tcx
+    #[error("operation not supported for programs loaded via tcx")]
+    InvalidLinkOperation,
 }
 
 impl TcAttachType {
@@ -114,11 +113,11 @@ impl TcAttachType {
         }
     }
 
-    pub(crate) fn tcx_attach(&self) -> Result<bpf_attach_type, TcError> {
+    pub(crate) fn tcx_attach_type(&self) -> Result<bpf_attach_type, TcError> {
         match self {
             Self::Ingress => Ok(BPF_TCX_INGRESS),
             Self::Egress => Ok(BPF_TCX_EGRESS),
-            Self::Custom(i) => Err(TcError::InvalidTcxAttach(i.to_owned())),
+            Self::Custom(tcx_attach_type) => Err(TcError::InvalidTcxAttach(*tcx_attach_type)),
         }
     }
 }
@@ -127,8 +126,8 @@ impl TcAttachType {
 ///
 /// The options vary based on what is supported by the current kernel. Kernels
 /// older than 6.6.0 must utilize netlink for attachments, while newer kernels
-/// can utilize the modern TCX eBPF link type which support's the kernel's
-/// multi-prog api.
+/// can utilize the modern TCX eBPF link type which supports the kernel's
+/// multi-prog API.
 #[derive(Debug)]
 pub enum TcAttachOptions {
     /// Netlink attach options.
@@ -137,7 +136,7 @@ pub enum TcAttachOptions {
     TcxOrder(LinkOrder),
 }
 
-/// Options for SchedClassifier attach via netlink
+/// Options for SchedClassifier attach via netlink.
 #[derive(Debug, Default, Hash, Eq, PartialEq)]
 pub struct NlOptions {
     /// Priority assigned to tc program with lower number = higher priority.
@@ -157,7 +156,7 @@ impl SchedClassifier {
     /// Attaches the program to the given `interface`.
     ///
     /// On kernels >= 6.6.0, it will attempt to use the TCX interface and attach as
-    /// the first program. On older kernels, it will fallback to using the
+    /// the last TCX program. On older kernels, it will fallback to using the
     /// legacy netlink interface.
     ///
     /// For finer grained control over link ordering use [`SchedClassifier::attach_with_options`].
@@ -167,7 +166,7 @@ impl SchedClassifier {
     /// # Errors
     ///
     /// When attaching fails, [`ProgramError::SyscallError`] is returned for
-    /// kernels `>= 6.6.0`, and instead [`TcError::NetlinkError`] is returned for
+    /// kernels `>= 6.6.0`, and [`TcError::NetlinkError`] is returned for
     /// older kernels. A common cause of netlink attachment failure is not having added
     /// the `clsact` qdisc to the given interface, see [`qdisc_add_clsact`]
     ///
@@ -177,14 +176,12 @@ impl SchedClassifier {
         attach_type: TcAttachType,
     ) -> Result<SchedClassifierLinkId, ProgramError> {
         if KernelVersion::current().unwrap() >= KernelVersion::new(6, 6, 0) {
-            debug!("attaching schedClassifier program via tcx link API");
             self.attach_with_options(
                 interface,
                 attach_type,
                 TcAttachOptions::TcxOrder(LinkOrder::default()),
             )
         } else {
-            debug!("attaching SchedClassifier program via netlink API");
             self.attach_with_options(
                 interface,
                 attach_type,
@@ -272,7 +269,12 @@ impl SchedClassifier {
                 let name = CString::new(name).unwrap();
                 let (priority, handle) = unsafe {
                     netlink_qdisc_attach(
-                        if_index as i32,
+                        if_index
+                            .try_into()
+                            .map_err(|_| ProgramError::ConversionError {
+                                from: "u32".to_string(),
+                                to: "i32".to_string(),
+                            })?,
                         &attach_type,
                         prog_fd,
                         &name,
@@ -296,7 +298,7 @@ impl SchedClassifier {
                 let link_fd = bpf_link_create(
                     prog_fd,
                     LinkTarget::IfIndex(if_index),
-                    attach_type.tcx_attach()?,
+                    attach_type.tcx_attach_type()?,
                     None,
                     options.flags.bits(),
                     Some(&options.link_ref),
@@ -365,14 +367,14 @@ impl Link for NlLink {
     }
 
     fn detach(self) -> Result<(), ProgramError> {
-        unsafe {
-            netlink_qdisc_detach(
-                self.if_index as i32,
-                &self.attach_type,
-                self.priority,
-                self.handle,
-            )
-        }
+        let if_index: i32 =
+            self.if_index
+                .try_into()
+                .map_err(|_| ProgramError::ConversionError {
+                    from: "u32".to_string(),
+                    to: "i32".to_string(),
+                })?;
+        unsafe { netlink_qdisc_detach(if_index, &self.attach_type, self.priority, self.handle) }
             .map_err(|io_error| TcError::NetlinkError { io_error })?;
         Ok(())
     }
@@ -436,7 +438,6 @@ impl TryFrom<FdLink> for SchedClassifierLink {
     type Error = LinkError;
 
     fn try_from(fd_link: FdLink) -> Result<Self, Self::Error> {
-        // unwrap of fd_link.fd will not panic since it's only None when being dropped.
         let info = bpf_link_get_info_by_fd(fd_link.fd.as_fd())?;
         if info.type_ == (bpf_link_type::BPF_LINK_TYPE_TCX as u32) {
             return Ok(Self::new(TcLinkInner::FdLink(fd_link)));
@@ -506,7 +507,7 @@ impl SchedClassifierLink {
         if let TcLinkInner::NlLink(n) = self.inner() {
             Ok(n.attach_type)
         } else {
-            Err(TcError::InvalidLink.into())
+            Err(TcError::InvalidLinkOperation.into())
         }
     }
 
@@ -515,7 +516,7 @@ impl SchedClassifierLink {
         if let TcLinkInner::NlLink(n) = self.inner() {
             Ok(n.priority)
         } else {
-            Err(TcError::InvalidLink.into())
+            Err(TcError::InvalidLinkOperation.into())
         }
     }
 
@@ -524,7 +525,7 @@ impl SchedClassifierLink {
         if let TcLinkInner::NlLink(n) = self.inner() {
             Ok(n.handle)
         } else {
-            Err(TcError::InvalidLink.into())
+            Err(TcError::InvalidLinkOperation.into())
         }
     }
 }

aya/src/sys/bpf.rs

@@ -429,7 +429,7 @@ pub(crate) fn bpf_link_create(
                 .__bindgen_anon_1
                 .relative_id = id.to_owned();
         }
-        _ => {}
+        None => {}
     };
 
     // SAFETY: BPF_LINK_CREATE returns a new file descriptor.

test/integration-test/Cargo.toml

@@ -27,6 +27,7 @@ test-case = { workspace = true }
 test-log = { workspace = true, features = ["log"] }
 tokio = { workspace = true, features = ["macros", "rt-multi-thread", "time"] }
 xdpilone = { workspace = true }
+
 [build-dependencies]
 cargo_metadata = { workspace = true }
 # TODO(https://github.com/rust-lang/cargo/issues/12375): this should be an artifact dependency, but

test/integration-test/src/tests/tcx.rs

@@ -104,22 +104,17 @@ async fn tcx_ordering() {
         .unwrap();
     prog3.load().unwrap();
 
-    // Test LinkOrder::first() and LinkOrder::set_expected_revision()
+    // Test LinkOrder::last() with correct expected_revision
     let mut order: LinkOrder = LinkOrder::last();
-    order.set_expected_revision(u64::MAX);
-    let options = TcAttachOptions::TcxOrder(order);
-    let result = prog0.attach_with_options("lo", TcAttachType::Ingress, options);
-    assert!(result.is_err());
-
-    let mut order: LinkOrder = LinkOrder::last();
-    order.set_expected_revision(0);
+    order.set_expected_revision(1);
     let options = TcAttachOptions::TcxOrder(order);
     prog0
         .attach_with_options("lo", TcAttachType::Ingress, options)
         .unwrap();
 
-    // Test LinkOrder::after_program()
-    let order = LinkOrder::after_program(prog0).unwrap();
+    // Test LinkOrder::after_program() with correct expected_revision
+    let mut order = LinkOrder::after_program(prog0).unwrap();
+    order.set_expected_revision(2);
     let options = TcAttachOptions::TcxOrder(order);
     let prog1_link_id = prog1
         .attach_with_options("lo", TcAttachType::Ingress, options)
@@ -127,19 +122,31 @@ async fn tcx_ordering() {
 
     let prog1_link = prog1.take_link(prog1_link_id).unwrap();
 
-    // Test LinkOrder::after_link()
-    let order = LinkOrder::after_link(&prog1_link).unwrap();
+    // Test incorrect expected_revision and expect an error
+    let mut order = LinkOrder::after_link(&prog1_link).unwrap();
+    order.set_expected_revision(7);
+    let options = TcAttachOptions::TcxOrder(order);
+    let result = prog2.attach_with_options("lo", TcAttachType::Ingress, options);
+    assert!(result.is_err());
+
+    // Test LinkOrder::after_link() again with expected_revision == 0 which
+    // means the expected_revision should be ignored.
+    let mut order = LinkOrder::after_link(&prog1_link).unwrap();
+    order.set_expected_revision(0);
     let options = TcAttachOptions::TcxOrder(order);
     prog2
         .attach_with_options("lo", TcAttachType::Ingress, options)
         .unwrap();
 
-    // Test LinkOrder::last()
+    // Test LinkOrder::last() with no expected_revision
     let options = TcAttachOptions::TcxOrder(LinkOrder::last());
     prog3
         .attach_with_options("lo", TcAttachType::Ingress, options)
         .unwrap();
 
+    // Wait for system to stabilize
+    tokio::time::sleep(std::time::Duration::from_millis(100)).await;
+
     const PAYLOAD: &str = "hello tcx";
 
     let sock = UdpSocket::bind("127.0.0.1:1778").unwrap();