Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lxc::Container::get_ips() design improvement #12

Open
koutheir opened this issue Sep 24, 2021 · 6 comments
Open

lxc::Container::get_ips() design improvement #12

koutheir opened this issue Sep 24, 2021 · 6 comments

Comments

@koutheir
Copy link

koutheir commented Sep 24, 2021

The lxc::Container::get_ips() API encapsulates the get_ips() of lxc_sys::lxc_container.

There are multiples issues with the design of lxc::Container::get_ips():

  • It returns Vec<String> instead of Vec<std::net::IpAddr>. IP addresses are rarely used simply as strings. They're often parsed before becoming useful.
  • The first argument is interfaces: &str:
    • The raw get_ips() gets an interface: *const c_char. Using the plural form "interfaces" is misleading.
    • The raw get_ips() can accept NULL as the value for this parameter, which has its own semantics in the implementation of get_ips() in liblxc.
  • The second argument is family: &str: the raw get_ips() can accept NULL as the value for this parameter, which has its own semantics in the implementation of get_ips() in liblxc.
  • The third argument is scope: i32, but the raw get_ips() gets a scope: c_int. Assuming that a C int maps always to i32 is a bad idea, and is not always true.

Given all these points, I suggest changing the prototype of lxc::Container::get_ips() to the following:

pub fn get_ips(
    &self,
    interface: Option<&str>,
    family: Option<&str>,
    scope: std::os::raw::c_int)
    -> Result<Vec<std::net::IpAddr>>
{
    /*... */
}
@sanpii
Copy link
Owner

sanpii commented Sep 26, 2021

I made the PR but I didn’t try it: #14

@sanpii
Copy link
Owner

sanpii commented Sep 26, 2021

It’s work fine:

Container state: RUNNING
Container PID: 32254
Interfaces: ["eth0", "lo"]
IPs: [10.0.3.206]

@koutheir
Copy link
Author

The only issue remaining is the unwrap() calls that panic if liblxc generates an error. This is not reasonable for a library.

@sanpii
Copy link
Owner

sanpii commented Sep 28, 2021

The only issue remaining is the unwrap() calls that panic if liblxc generates an error. This is not reasonable for a library.

There is many unwrap in my code, I work on a redesign of errors handling.

@koutheir
Copy link
Author

Until panics are reduced to a minimum, the only crate I can currently use is lxc-sys, basically reimplementing the ergonomic structures while reporting liblxc errors as normal Results.

@sanpii
Copy link
Owner

sanpii commented Sep 28, 2021

@koutheir The PR is here: #16

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants