Code review for my first libc wrapper?

1

Hi, I've been working on wrapping libc::getifaddrs and I've gotten pretty far. However this is my first time writing unsafe Rust: please take a look at it and tell me how badly I've screwed up?

https://github.com/LeoTindall/getaddrs-rs

The primary entrypoint function is get_if_addrs which returns a Vec of Interface, which has a name and a Vec of InterfaceProto which contains addresses and flags.

2

Make this code compiling first, then silence compilers warnings (unused variables), then get rid of transmute.

I would wrap libc::ifaddrs some RAII type which would call libc::freeifaddrs(_) when it go out of scope. The wrapper would provide iteration via interface like this: fn iter<'a>(&'a self) -> impl Iterator<Item = &'a libc::ifaddrs>. This would let me rewrite all this nasty while loop into nice for.

Lines 123..129: reinvented wheel, you could let mut index = interfaces.position(|iface| name == iface.name). Also lines 123..139 and 170 is no Rusty at all... Why is this vector instead of HashMap<String, Vec<InterfaceProtocol>>? Using HashMap would definetly be more convinient here and simplify code much. If you thing in "cache effitiency" terms, stop - here you have heap allocations both in String and Vec, so this vector isn't cache friendly at all.

Lines 156..160: check match statement, would me much more elegant.

In general: this code looks more like C in Rust syntax, than real Rust code. Try to think more functional, and check interfaces you are using (in particular when you are trying to do something in iterator, check if Iterator trait hasn't helper for it).

3

pub fn sockaddr_to_ipaddr (sockaddr_input: Option<&libc::sockaddr>)

The Option in arguments doesn't seem to have semantic purpose.

That might be subjective, but I'd return Option only for cases where the result is valid, and the correct result means there is nothing to return. For error cases, such as unsupported type, I'd return Result.

Instead of while !ptr_to_ifaddrs_current.is_null() you could have while let Some(p) = ptr_to_ifaddrs_current.as_ref(), which converts to a "safe" reference.

4

@hashedone Thank you for the suggestions. I went ahead and reimplemented it as an Iterator-based API.

@kornel Thanks for the suggestions! I realized, in this situation a null pointer does have semantic meaning, so I've documented that distinction.

1 Like
5

In Rust (almost) everything is an expression, so you can change:

let name;
unsafe {
    name = foo();
}

to

let name = unsafe {
   foo()
};

And that works even for if, even if one branch returns:

let p = if let Some(p) = self.current.as_ref() {p} else {return None;};
6

Ah, thanks. I'll change those as well.
What do you think of the new API design? Should I also provide a function that returns HashMap<String, InterfaceAddr> as well as the Iterator-based interface?

7

I don't think you need a function to return a HashMap<String, InterfaceAddr>. HashMap implements FromIterator so someone can use your iterator to create a key/value tuple and then collect them into a HashMap if they want.

8

One thing you should consider is propagating errors from libc to the user via Result return types. Right now it looks like you'll return None (i.e. an Option return type), but that will mask problems that a user may want to know about.

9

sockaddr:7..44 - there is function for this, Option::and_then:

unsafe { sockaddr_input.as_ref() }.and_then(|sa| {
    match sa.sa_famili as i32 {
        // ...
    }
});

sockaddr:21..24 - as kornel said, unsafe blocks are also values, betterr would be:

let data_v4: &libc::sockaddr_in = unsafe { transmute(as) };

Same in 28..29, 35..38.

In lib:46 you ain't need this check, if its created in proper way, thats no way this would be null. Same in line 100. In general I would rather change InterfaceAddrs to this:

pub struct InterfaceAddrs {
    inner: &'static libc::ifaddrs,
    current: &'static libc::ifaddrs,
}

Then lines 35..40 to:

p.as_ref().and_then(|p| InterfaceAddrs { inner: p, current: p });

You can change ref back to pointer, so Drop is also easy to implement, but this way you don't need to care about special null value everywhere.

10

Thanks for the tips! I implemented a lot of them, though the actual struct I ended up using is

pub struct InterfaceAddrs {
    inner: *mut libc::ifaddrs,
    current: Option<&'static libc::ifaddrs>,
}

I'll do docs this weekend and hopefully release on crates.io by Monday!

11

I'm not sure I'd personally hack the 'static lifetime there but it'll work so long as you never leak that reference out anywhere.

The usecase is really more of the NonZero<*mut ifaddrs> type but that's not stable (yet).

12

There's no way for it to leak, but I see why it could be problematic.

Do you know of a timeline for NonZero to be stabilized?

13

Yeah, with your current code as-is it won't leak so it's "safe". This is just paranoi in case you refactor it later and this "slips through" :slight_smile:.

It's a long-winded issue, and to be honest I'm not sure where it stands right now.

14

Thank you!

I have another problem I'd appreciate some advice on: I'm working on tests and documentation, and I'm not really sure how to test this library's behavior. It's totally reliant on external, unknowable state, by design.

15

Yeah that's going to be tricky. I suppose you can create fake ifaddrs and write some unit tests against them. For integration tests, provide a simple example main that someone can run on their machine and see the output. Setting up any sort of virtual environment/interfaces seems too heavyweight to me.

1 Like
16

Are you planning on developing a standalone crate for this? If not, this would be welcome within the nix crate (I'm a maintainer).

17

I didn't know about nix. I've published it as getaddrs but I'd be happy to move it into nix.