test: replace loopback with veth pairs in integration tests

[cong-or]

The integration tests used to run on loopback, but that only supports generic (SKB-mode) XDP, which doesn’t really reflect a real production setup. Now, they use a veth pair instead, which supports native XDP and behaves much more like an actual NIC.

NetNsGuard sets up the veth pair. For the traffic-heavy tests like af_xdp, PeerNsGuard moves the peer into its own namespace, assigns IPs on both ends, and installs static ARP entries to prevent random ARP traffic from interfering with the XDP redirect programs.

The cpumap_chain test stays on loopback for now, because cpumap chaining doesn’t reliably work through veth on arm64. As a result, the old 5.15 kernel version gate for generic XDP mode remains in place.

Additionally, there’s a small veth_connectivity test that sends a plain UDP packet across the pair with no BPF attached. If anything’s broken in the basic setup, it fails quickly and clearly, avoiding confusing issues down the line.

---

This change is 

[netlify]

✅ Deploy Preview for aya-rs-docs ready!

Built without sensitive environment variables

Name	Link
🔨 Latest commit	4acde42
🔍 Latest deploy log	https://app.netlify.com/projects/aya-rs-docs/deploys/69f75bfd14dfcb0008de1418
😎 Deploy Preview	https://deploy-preview-1484--aya-rs-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
🤖 Make changes	Run an agent on this branch

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[cong-or]

Closes #422

[tamird]

Seems like you tried to solve this, but then the LLM ran into some problems, gave up, and reverted most of it?

@tamird reviewed 7 files and all commit messages, and made 22 comments.
Reviewable status: all files reviewed, 21 unresolved discussions (waiting on cong-or).

---

-- commits line 35 at r8:
so the LLM just gave up in the end?

---

test/integration-test/src/netlink.rs line 4 at r4 (raw file):

//!
//! These live here (rather than in the `aya` crate) to avoid adding test-only
//! public API surface to the production library.

on the other hand...there's a fair bit of duplication here

Code quote:

//! Minimal netlink helpers for integration-test network setup.
//!
//! These live here (rather than in the `aya` crate) to avoid adding test-only
//! public API surface to the production library.

---

test/integration-test/src/netlink.rs line 32 at r4 (raw file):

#[derive(Copy, Clone)]
#[repr(C)]
struct Ifinfomsg {

can we use bindgen for this?

---

test/integration-test/src/utils.rs line 148 at r1 (raw file):

    const PEER_IFACE: &str = "veth1";
    pub(crate) const IFACE_ADDR: &str = "10.0.0.1";
    pub(crate) const PEER_ADDR: &str = "10.0.0.2";

while I understand that these are strings, does it make sense to use them unless you have a NetNsGuard in scope? if not, perhaps these should be methods that return a &str with a lifetime matching that of the NetNsGuard/Peer (even though they'd really be static strings)

Code quote:

    pub(crate) const IFACE_ADDR: &str = "10.0.0.1";
    pub(crate) const PEER_ADDR: &str = "10.0.0.2";

---

test/integration-test/src/utils.rs line 152 at r1 (raw file):

    pub(crate) fn name(&self) -> &str {
        &self.name
    }

do you need this? it's only used in this same file, where the field is accessible

Code quote:

    pub(crate) fn name(&self) -> &str {
        &self.name
    }

---

test/integration-test/src/utils.rs line 215 at r1 (raw file):

            "name",
            Self::PEER_IFACE,
        ]);

shouldn't this happen only on the creation of the peer?

Code quote:

        // Create a veth pair for tests that attach XDP/TC programs. Veth supports
        // native XDP (unlike loopback which only supports SKB/generic mode), so tests
        // exercise the same code path used in production.
        run_ip(&[
            "link",
            "add",
            Self::IFACE,
            "type",
            "veth",
            "peer",
            "name",
            Self::PEER_IFACE,
        ]);

---

test/integration-test/src/utils.rs line 219 at r1 (raw file):

        // Bring up both ends.
        for iface in [Self::IFACE, Self::PEER_IFACE] {
            let name = CString::new(iface).unwrap();

consider making the constants C-strings

EDIT: this is also duplicated above

---

test/integration-test/src/utils.rs line 223 at r1 (raw file):

                let idx = if_nametoindex(name.as_ptr());
                assert!(
                    idx != 0,

i suppose this can be assert_ne (but keep the custom message)

---

test/integration-test/src/utils.rs line 278 at r1 (raw file):

        .status()
        .unwrap_or_else(|e| panic!("ip {}: {e}", args.join(" ")));
    assert!(status.success(), "ip {} failed", args.join(" "));

you can just keep cmd in scope and use its debug output (here and below

---

test/integration-test/src/utils.rs line 302 at r1 (raw file):

    full_args.extend(args);
    run_ip_output(&full_args)
}

this is a lot of helpers and they don't make it easy to understand what's going on

we also have existing use of "ip", so either use these everywhere or nowhere please

Code quote:

fn run_ip_output(args: &[&str]) -> String {
    let output = Command::new("ip")
        .args(args)
        .output()
        .unwrap_or_else(|e| panic!("ip {}: {e}", args.join(" ")));
    assert!(output.status.success(), "ip {} failed", args.join(" "));
    String::from_utf8(output.stdout).unwrap()
}

/// Run `ip netns exec <ns> ip <args...>`.
fn run_ip_netns(ns: &str, args: &[&str]) {
    let mut full_args = vec!["netns", "exec", ns, "ip"];
    full_args.extend(args);
    run_ip(&full_args);
}

/// Run `ip netns exec <ns> ip <args...>` and return stdout.
fn run_ip_netns_output(ns: &str, args: &[&str]) -> String {
    let mut full_args = vec!["netns", "exec", ns, "ip"];
    full_args.extend(args);
    run_ip_output(&full_args)
}

---

test/integration-test/src/utils.rs line 405 at r1 (raw file):

            .unwrap_or_else(|e| panic!("open(\"{}\"): {e}", peer_ns_path.display()));

        std::thread::scope(|s| {

scope for one thread? why?

---

test/integration-test/src/tests/xdp.rs line 18 at r1 (raw file):

#[test_log::test]
fn veth_connectivity() {
    // peer must be declared after netns so it drops first (see PeerNsGuard docs).

useless comment; this is enforced by the compiler

---

test/integration-test/src/tests/xdp.rs line 22 at r1 (raw file):

    let peer = PeerNsGuard::new(&netns);

    let sock = UdpSocket::bind(format!("{}:0", NetNsGuard::IFACE_ADDR)).unwrap();

I think you can pass (NetNsGuard::IFACE_ADDR, 0)

---

test/integration-test/src/tests/xdp.rs line 28 at r1 (raw file):

    peer.run(|| {
        let sock = UdpSocket::bind(format!("{}:0", NetNsGuard::PEER_ADDR)).unwrap();

ditto, but here i think there's no need to bind at all?

---

test/integration-test/src/tests/xdp.rs line 34 at r1 (raw file):

    let mut buf = [0u8; 16];
    let n = sock.recv(&mut buf).unwrap();
    assert_eq!(&buf[..n], b"veth ok");

extract constant (and use its size for the buf above)

---

test/integration-test/src/tests/xdp.rs line 43 at r1 (raw file):

)]
fn af_xdp() {
    // peer must be declared after netns so it drops first (see PeerNsGuard docs).

ditto

---

test/integration-test/src/tests/xdp.rs line 108 at r1 (raw file):

    writer.commit();

    let dst = format!("{}:1777", NetNsGuard::IFACE_ADDR);

same here, can be a tuple

---

test/integration-test/src/tests/xdp.rs line 197 at r1 (raw file):

#[test_log::test]
fn cpumap_chain() {
    // peer must be declared after netns so it drops first (see PeerNsGuard docs).

ditto

---

test/integration-test/src/tests/xdp.rs line 238 at r1 (raw file):

    const PAYLOAD: &str = "hello cpumap";

    let sock = UdpSocket::bind(format!("{}:0", NetNsGuard::IFACE_ADDR)).unwrap();

same

---

test/integration-test/src/tests/xdp.rs line 243 at r1 (raw file):

        .unwrap();
    peer.run(|| {
        let sock = UdpSocket::bind(format!("{}:0", NetNsGuard::PEER_ADDR)).unwrap();

same

---

test/integration-test/src/tests/xdp.rs line 227 at r6 (raw file):

    // not reliably deliver packets through veth on older kernels (confirmed
    // failing on 5.10). Gate at 6.1 which is the oldest CI-tested kernel where
    // this works end-to-end.

a bit unsatisfying :(

Code quote:

    // While veth supports native XDP attachment from 5.9, cpumap chaining does
    // not reliably deliver packets through veth on older kernels (confirmed
    // failing on 5.10). Gate at 6.1 which is the oldest CI-tested kernel where
    // this works end-to-end.

[cong-or]

The veth work is actually all there — PeerNsGuard creates the pair via raw netlink (CI VM doesn't have iproute2), moves one end into a peer namespace, and wires up IP + static ARP. The XDP and load tests are all using veth now.

The only exception is cpumap_chain — spent a while trying to get CI to pass with that one on veth but cpumap chaining doesn't work with veth on arm64 since it needs generic/SKB-mode XDP, which only loopback supports. So that's a kernel limitation rather than something I gave up on — loopback is the correct interface for that test.

Commit history is a bit messy from all the CI iteration, I can clean it up?

@cong-or made 22 comments.
Reviewable status: 0 of 27 files reviewed, 21 unresolved discussions (waiting on tamird).

---

-- commits line 35 at r8:

Previously, tamird (Tamir Duberstein) wrote…

so the LLM just gave up in the end?

Done.

---

test/integration-test/src/netlink.rs line 4 at r4 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

on the other hand...there's a fair bit of duplication here

Yeah, it's a trade-off — didn't want to add test-only stuff to the aya crate API. Open to suggestions if you have a better idea though.

---

test/integration-test/src/netlink.rs line 32 at r4 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

can we use bindgen for this?

ifinfomsg is already coming from bindgen via aya_obj::generated. I could add Ifaddrmsg and Ndmsg to the codegen too — they're just not there yet. VETH_INFO_PEER and NLMSG_ALIGNTO are one-liner constants that libc doesn't export, didn't seem worth pulling them through bindgen but can do if you'd prefer.

---

test/integration-test/src/utils.rs line 148 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

while I understand that these are strings, does it make sense to use them unless you have a NetNsGuard in scope? if not, perhaps these should be methods that return a &str with a lifetime matching that of the NetNsGuard/Peer (even though they'd really be static strings)

Good point, changed them to methods on PeerNsGuard so you need a live instance to access them.

---

test/integration-test/src/utils.rs line 152 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

do you need this? it's only used in this same file, where the field is accessible

I removed it

---

test/integration-test/src/utils.rs line 215 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

shouldn't this happen only on the creation of the peer?

Yeah that's fixed now — moved it into PeerNsGuard::new() so it only runs when a test actually needs it

---

test/integration-test/src/utils.rs line 219 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

consider making the constants C-strings

EDIT: this is also duplicated above

Done.

---

test/integration-test/src/utils.rs line 223 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

i suppose this can be assert_ne (but keep the custom message)

Done.

---

test/integration-test/src/utils.rs line 278 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

you can just keep cmd in scope and use its debug output (here and below

Done.

---

test/integration-test/src/utils.rs line 302 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

this is a lot of helpers and they don't make it easy to understand what's going on

we also have existing use of "ip", so either use these everywhere or nowhere please

Done.

---

test/integration-test/src/utils.rs line 405 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

scope for one thread? why?

Done.

---

test/integration-test/src/tests/xdp.rs line 18 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

useless comment; this is enforced by the compiler

Done.

---

test/integration-test/src/tests/xdp.rs line 22 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

I think you can pass (NetNsGuard::IFACE_ADDR, 0)

Done.

---

test/integration-test/src/tests/xdp.rs line 28 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

ditto, but here i think there's no need to bind at all?

Done.

---

test/integration-test/src/tests/xdp.rs line 34 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

extract constant (and use its size for the buf above)

Done.

---

test/integration-test/src/tests/xdp.rs line 43 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

ditto

Done.

---

test/integration-test/src/tests/xdp.rs line 108 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

same here, can be a tuple

Done.

---

test/integration-test/src/tests/xdp.rs line 197 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

ditto

Done.

---

test/integration-test/src/tests/xdp.rs line 238 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

same

Done.

---

test/integration-test/src/tests/xdp.rs line 243 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

same

Done.

---

test/integration-test/src/tests/xdp.rs line 227 at r6 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

a bit unsatisfying :(

Yeah I'm not thrilled about it either. Couldn't figure out why cpumap chaining wouldn't work through veth on arm64 so I just kept it on loopback. Open to ideas

[tamird]

Yeah cleaning up would be good. Apologies for the delay getting back to this. Just a few comments, thanks for figuring out some of the kernel bugs.

@tamird reviewed 27 files and all commit messages, made 9 comments, and resolved 19 discussions.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on cong-or).

---

test/integration-test/src/netlink.rs line 4 at r4 (raw file):

Previously, cong-or wrote…

Yeah, it's a trade-off — didn't want to add test-only stuff to the aya crate API. Open to suggestions if you have a better idea though.

@alessandrod pinky promises he's going to rewrite a lot of the netlink machinery Real Soon Now so I guess we can leave it here.

Note that I did a pretty big rewrite of the netlink stuff since this PR opened, so if you followed the earlier style you could consider revising. Up to you.

---

test/integration-test/src/netlink.rs line 32 at r4 (raw file):

Previously, cong-or wrote…

ifinfomsg is already coming from bindgen via aya_obj::generated. I could add Ifaddrmsg and Ndmsg to the codegen too — they're just not there yet. VETH_INFO_PEER and NLMSG_ALIGNTO are one-liner constants that libc doesn't export, didn't seem worth pulling them through bindgen but can do if you'd prefer.

The more we can generate the better, I think.

---

test/integration-test/src/tests/xdp.rs line 28 at r1 (raw file):

Previously, cong-or wrote…

Done.

you can use either

https://doc.rust-lang.org/stable/std/net/struct.Ipv4Addr.html#associatedconstant.LOCALHOST

or

https://doc.rust-lang.org/stable/std/net/struct.Ipv4Addr.html#associatedconstant.UNSPECIFIED

but the choice just be justified, i guess you need UNSPECIFIED so you can send on non-localhost? the best way to document would be to extract a local constant or value and assign it UNSPECIFIED; the name you give it would document its intent.

---

test/integration-test/src/tests/xdp.rs line 33 at r14 (raw file):

    });

    let mut buf = [0u8; PAYLOAD.len()];

this should be oversized since you're asserting a precise read

---

test/integration-test/src/utils.rs line 239 at r14 (raw file):

/// Run a closure inside a network namespace identified by a file handle.
///
/// Uses a scoped thread because `setns` changes the network namespace of the

why does it need to be scoped? why can't it just be a scopeless std::thread::spawn?

---

test/integration-test/src/utils.rs line 279 at r14 (raw file):

    #[expect(clippy::unused_self, reason = "access requires a live PeerNsGuard")]
    pub(crate) fn iface(&self) -> &'static str {

i think the whole point is for this to be not static since you want the lifetime to be bound by the guard. ditto below

---

test/integration-test/src/utils.rs line 323 at r14 (raw file):

        }

        // Create peer netns: create a persist file, then use a scoped thread

same here, you don't need the scope or at least it's not clear why you do

---

test/integration-test/src/utils.rs line 467 at r14 (raw file):

    /// Run a closure inside the peer network namespace.
    ///
    /// Uses a scoped thread because `setns` changes the network namespace of the

would be good to reduce the number of copies of this

[cong-or]

@cong-or made 8 comments.
Reviewable status: 22 of 29 files reviewed, 7 unresolved discussions (waiting on tamird).

---

test/integration-test/src/netlink.rs line 4 at r4 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

@alessandrod pinky promises he's going to rewrite a lot of the netlink machinery Real Soon Now so I guess we can leave it here.

Note that I did a pretty big rewrite of the netlink stuff since this PR opened, so if you followed the earlier style you could consider revising. Up to you.

I'll leave it here for now, happy to adapt once the rewrite lands.

---

test/integration-test/src/netlink.rs line 32 at r4 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

The more we can generate the better, I think.

Pulled NLMSG_ALIGNTO from aya_obj::generated. For VETH_INFO_PEER I added linux/veth.h to the wrapper header and the allowlist in xtask, but I don't have cross sysroots locally so couldn't run codegen — left a TODO on the local constant for now

---

test/integration-test/src/utils.rs line 239 at r14 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

why does it need to be scoped? why can't it just be a scopeless std::thread::spawn?

Needs to be scoped because ns_file is borrowed, and f isn't 'static — once we tie the iface_addr() lifetime to the guard, callers end up capturing non-'static refs. Could dup the fd but we'd still need F: 'static which would break those call sites. Added a note in the doc.

---

test/integration-test/src/utils.rs line 279 at r14 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

i think the whole point is for this to be not static since you want the lifetime to be bound by the guard. ditto below

Fixed, returns &str now so the lifetime is bound by the guard.

---

test/integration-test/src/utils.rs line 323 at r14 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

same here, you don't need the scope or at least it's not clear why you do

You're right, switched to std::thread::spawn — just needed to clone the path.

---

test/integration-test/src/utils.rs line 467 at r14 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

would be good to reduce the number of copies of this

Consolidated on run_in_netns, PeerNsGuard::run just points there now.

---

test/integration-test/src/tests/xdp.rs line 28 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

you can use either

https://doc.rust-lang.org/stable/std/net/struct.Ipv4Addr.html#associatedconstant.LOCALHOST

or

https://doc.rust-lang.org/stable/std/net/struct.Ipv4Addr.html#associatedconstant.UNSPECIFIED

but the choice just be justified, i guess you need UNSPECIFIED so you can send on non-localhost? the best way to document would be to extract a local constant or value and assign it UNSPECIFIED; the name you give it would document its intent.

Switched to Ipv4Addr::UNSPECIFIED, added a comment — we need it so the kernel picks the right source (10.0.0.2) for the veth route.

---

test/integration-test/src/tests/xdp.rs line 33 at r14 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

this should be oversized since you're asserting a precise read

Good catch, oversized to 512

[tamird]

@tamird reviewed 7 files and all commit messages, made 8 comments, and resolved 2 discussions.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on cong-or).

---

test/integration-test/src/netlink.rs line 4 at r4 (raw file):

Previously, cong-or wrote…

I'll leave it here for now, happy to adapt once the rewrite lands.

It landed in #1483

---

test/integration-test/src/netlink.rs line 32 at r4 (raw file):

Previously, cong-or wrote…

Pulled NLMSG_ALIGNTO from aya_obj::generated. For VETH_INFO_PEER I added linux/veth.h to the wrapper header and the allowlist in xtask, but I don't have cross sysroots locally so couldn't run codegen — left a TODO on the local constant for now

Cool. Codegen should run automatically once this PR lands.

---

test/integration-test/src/utils.rs line 239 at r14 (raw file):

Previously, cong-or wrote…

Needs to be scoped because ns_file is borrowed, and f isn't 'static — once we tie the iface_addr() lifetime to the guard, callers end up capturing non-'static refs. Could dup the fd but we'd still need F: 'static which would break those call sites. Added a note in the doc.

Great; I think it's fine to keep the scoped thread to avoid cloning or whatever, I just don't think we should document that here.

As mentioned below, a better phrasing of this comment would focus on the Send bound and explain that the closure must run on another thread - without going into irrelevant details like the thread being scoped.

---

test/integration-test/src/utils.rs line 323 at r14 (raw file):

Previously, cong-or wrote…

You're right, switched to std::thread::spawn — just needed to clone the path.

I think it's fine to use a scoped thread to save an allocation, I just wanted us to be precise about the reasoning.

---

test/integration-test/src/utils.rs line 467 at r14 (raw file):

Previously, cong-or wrote…

Consolidated on run_in_netns, PeerNsGuard::run just points there now.

I think the comment should be here; run_in_netns is private. The comment should talk about the bounds rather than the underlying thread being scoped or not.

---

test/integration-test/src/tests/xdp.rs line 28 at r1 (raw file):

Previously, cong-or wrote…

Switched to Ipv4Addr::UNSPECIFIED, added a comment — we need it so the kernel picks the right source (10.0.0.2) for the veth route.

Again, the comment should cover all uses of this constant, not just one. That's why I suggested giving that constant a local alias with a descriptive name. The comment can then be anchored on that alias.

---

test/integration-test/src/tests/xdp.rs line 33 at r14 (raw file):

Previously, cong-or wrote…

Good catch, oversized to 512

I think it can just be PAYLOAD.len() + 1 to avoid magic numbers.

---

test/integration-test/src/tests/xdp.rs line 33 at r15 (raw file):

    // Send from the peer namespace. Bind UNSPECIFIED so the kernel picks the
    // right source address (10.0.0.2) for the veth route.

if you want the source address to be X, why not bind to X?

[tamird]

@tamird made 2 comments.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on cong-or).

---

test/integration-test/src/netlink.rs line 29 at r15 (raw file):

const VETH_INFO_PEER: u16 = 1;

/// `struct ifaddrmsg` from `linux/if_addr.h`.

can we codegen these structures too?

---

test/integration-test/src/netlink.rs line 55 at r15 (raw file):

#[derive(Copy, Clone)]
#[repr(C)]
struct LinkRequest {

these 4 don't have comments; should they?

[cong-or]

@cong-or made 8 comments.
Reviewable status: 24 of 29 files reviewed, 8 unresolved discussions (waiting on tamird).

---

test/integration-test/src/netlink.rs line 29 at r15 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

can we codegen these structures too?

Added linux/if_addr.h and linux/neighbour.h to the wrapper and ifaddrmsg/ndmsg to the types allowlist. Same as VETH_INFO_PEER — local definitions stay until codegen is re-run.

---

test/integration-test/src/netlink.rs line 55 at r15 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

these 4 don't have comments; should they?

They're internal request envelopes (header + message + attr buffer), don't think comments add much. Can add them if you'd prefer.

---

test/integration-test/src/utils.rs line 239 at r14 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

Great; I think it's fine to keep the scoped thread to avoid cloning or whatever, I just don't think we should document that here.

As mentioned below, a better phrasing of this comment would focus on the Send bound and explain that the closure must run on another thread - without going into irrelevant details like the thread being scoped.

Stripped the doc here down to a one-liner, moved the explanation to PeerNsGuard::run and focused on the bounds.

---

test/integration-test/src/utils.rs line 323 at r14 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

I think it's fine to use a scoped thread to save an allocation, I just wanted us to be precise about the reasoning.

Reverted back to scoped, agreed.

---

test/integration-test/src/utils.rs line 467 at r14 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

I think the comment should be here; run_in_netns is private. The comment should talk about the bounds rather than the underlying thread being scoped or not.

Done — doc is here now, talks about F: Send and not 'static rather than scoped threads

---

test/integration-test/src/tests/xdp.rs line 28 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

Again, the comment should cover all uses of this constant, not just one. That's why I suggested giving that constant a local alias with a descriptive name. The comment can then be anchored on that alias.

Ended up going a different direction — added a peer_addr() method and bind to it explicitly. let peer_ip = peer.peer_addr() is the local alias used everywhere now.

---

test/integration-test/src/tests/xdp.rs line 33 at r14 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

I think it can just be PAYLOAD.len() + 1 to avoid magic numbers.

done

---

test/integration-test/src/tests/xdp.rs line 33 at r15 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

if you want the source address to be X, why not bind to X?

Fair point, added a peer_addr() method and bind to it directly now.

[tamird]

@tamird reviewed 3 files and all commit messages, made 4 comments, and resolved 5 discussions.
Reviewable status: 26 of 29 files reviewed, 6 unresolved discussions (waiting on cong-or).

---

test/integration-test/src/netlink.rs line 55 at r15 (raw file):

Previously, cong-or wrote…

They're internal request envelopes (header + message + attr buffer), don't think comments add much. Can add them if you'd prefer.

Ah, I hadn't squinted at them properly. Now that I am, I don't quite understand the sizes of these attrs buffers. Are they significant? If so, can you document? if not, and the only requirement is that they are big enough so that you can use bytes_of to write them in a single libc::send call, then I wonder if we could do this with a vectorized write instead e.g. using writev.

EDIT: I ended up playing with this here (and that's why it took me this long to get back to this review). I think we can do much better with macros and get layouts that are entirely known at compile time, but for this PR I'd be happy with better commentary on magic numbers.

---

test/integration-test/src/utils.rs line 272 at r17 (raw file):

    const IFACE_IP: Ipv4Addr = Ipv4Addr::new(10, 0, 0, 1);
    const PEER_IP: Ipv4Addr = Ipv4Addr::new(10, 0, 0, 2);
    const IFACE_ADDR: &str = "10.0.0.1";

there's no reason for this to exist AFAICT because all places where it is used can just as well use an Ipv4Addr

---

test/integration-test/src/utils.rs line 285 at r17 (raw file):

    #[expect(clippy::unused_self, reason = "access requires a live PeerNsGuard")]
    pub(crate) fn peer_addr(&self) -> Ipv4Addr {

this is a bit silly because this returns a thing that is Copy

---

test/integration-test/src/utils.rs line 289 at r17 (raw file):

    }

    pub(crate) fn new(netns: &NetNsGuard) -> Self {

what would happen on a second call? seems like nothing good! This makes me wonder if instead of this shape we should have an alternative NetNsGuard function that returns the NetNsGuard and its peer so that it is clear that the relationship is importantly 1:1.

[cong-or]

@cong-or made 4 comments.
Reviewable status: 23 of 29 files reviewed, 6 unresolved discussions (waiting on tamird).

---

test/integration-test/src/netlink.rs line 55 at r15 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

Ah, I hadn't squinted at them properly. Now that I am, I don't quite understand the sizes of these attrs buffers. Are they significant? If so, can you document? if not, and the only requirement is that they are big enough so that you can use bytes_of to write them in a single libc::send call, then I wonder if we could do this with a vectorized write instead e.g. using writev.

EDIT: I ended up playing with this here (and that's why it took me this long to get back to this review). I think we can do much better with macros and get layouts that are entirely known at compile time, but for this PR I'd be happy with better commentary on magic numbers.

The sizes just need to be large enough for the attributes — I've added comments on each struct breaking down the payload math. Happy to move to a compile-time layout approach in a follow-up if you want to take that direction.

---

test/integration-test/src/utils.rs line 272 at r17 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

there's no reason for this to exist AFAICT because all places where it is used can just as well use an Ipv4Addr

Removed, callers use Ipv4Addr directly now.

---

test/integration-test/src/utils.rs line 285 at r17 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

this is a bit silly because this returns a thing that is Copy

Done, switched to pub constants.

---

test/integration-test/src/utils.rs line 289 at r17 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

what would happen on a second call? seems like nothing good! This makes me wonder if instead of this shape we should have an alternative NetNsGuard function that returns the NetNsGuard and its peer so that it is clear that the relationship is importantly 1:1.

Agreed — made PeerNsGuard::new() private, only way to get one is NetNsGuard::with_peer() which returns both together.