Add High-level API

[LuoZijun]

trafficstars

File Changes:

system-configuration-sys/src/lib.rs
system-configuration-sys/src/network_configuration.rs
system-configuration-sys/src/preferences.rs

system-configuration/Cargo.toml
system-configuration/src/lib.rs
system-configuration/src/network_configuration.rs
system-configuration/examples/dns.rs

---

This change is 

[faern]

@LuoZijun Thanks for your contribution! I don't mean to run over your work and I'm sorry if I made you feel I did. But I prefer automatically generated bindings for a number of reasons. Mostly so we cover the complete library at once, instead of gradually adding functions and constants. Since I did not want to ask of you to do all of that work, to map the entire SCPreferences and SCNetworkConfiguration, I went ahead and generated both of them in this PR: #9.

Please check out that PR and check if your high level bindings work on top of those ffi bindings. They hopefully should. If they do we'll merge my complete bindings and then I can review your higher level ones!

Once again, thank you for contributing :)

[LuoZijun]

@faern

When PR https://github.com/mullvad/system-configuration-rs/pull/9 has merged, i will remove system-configuration-sys changes and rebase.

[faern]

@LuoZijun PR #9 is now merged. So you can rebase on latest master. Please also note that I merged two other PRs, adding Travis CI, you can check there if it accepts your contribution with regard to compilation and formatting.

I also added #![deny(missing_docs)] to the higher level crate, so make sure to have documentation for all public methods and structs in system-configuration. For most structs and methods the documentation can more or less be copied from the Apple documentation, to make it say the same thing as they do about their methods: https://developer.apple.com/documentation/systemconfiguration

[pronebird]

Review status: 0 of 5 files reviewed at latest revision, 16 unresolved discussions.

---

system-configuration/src/network_configuration.rs, line 260 at r4 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Nope, we have not implemented a safe abstraction for SetMultiple yet, no. But this method only sets one value, doesn't it?

As I understand it sets ServerAddresses and DomainName under Setup:/Network/Service/{}/DNS. See the branch above.

---

Comments from Reviewable

[faern]

@LuoZijun Sorry for being so slow reviewing. We want this merged and it's constantly getting better, so it's awesome. But I've been very busy lately. I'll continue reviewing whenever I have time!

[LuoZijun]

@faern

No problem, I didn't think too much when I was writing this code. thank you very much for doing a lot of code review work, which is very helpful to the improvement of code quality.

My future attention will be on the Windows ABI, and I want to end up with a project that will be able to configure the DNS configuration, routing table, and forwarding configuration for Windows, MacOS, and Linux.

[faern]

@LuoZijun Ah, the part with configuring DNS on Windows sounds very interesting. We actually need that for the same product (the VPN app) that uses this library for setting DNS on macOS. Do you have some link to your repo or some time frame for when it might work for Windows? It's highly interesting to us. We could likely help improve that library if needed, we have not started working on one yet.

[LuoZijun]

@faern

Windows iphlpapi ABI branch: https://github.com/LuoZijun/winapi-rs/tree/iphlpapi

then you can look this article to know how to setting dns on Windows.

https://www.codeproject.com/Articles/20639/Setting-DNS-using-iphelp-and-register-DhcpNotifyCo

[pronebird]

Reviewed 1 of 9 files at r2, 1 of 4 files at r10, 2 of 4 files at r11. Review status: 4 of 6 files reviewed at latest revision, 31 unresolved discussions.

---

Comments from Reviewable

[pronebird]

Reviewed 1 of 4 files at r11. Review status: 5 of 6 files reviewed at latest revision, 31 unresolved discussions.

---

Comments from Reviewable

[pronebird]

Reviewed 1 of 4 files at r10. Review status: all files reviewed at latest revision, 31 unresolved discussions.

---

Comments from Reviewable

[faern]

Reviewed 1 of 8 files at r2. Review status: all files reviewed at latest revision, 25 unresolved discussions.

---

system-configuration/src/lib.rs, line 19 at r11 (raw file):

//! [SystemConfiguration]: https://developer.apple.com/documentation/systemconfiguration?language=objc
//! [`system-configuration-sys`]: https://crates.io/crates/system-configuration-sys
#![feature(test)]

You can't activate feature gates. Then the library will only build on nightly. We need to support stable.

---

system-configuration/src/network_configuration.rs, line 130 at r9 (raw file):

Previously, LuoZijun (寧靜) wrote…

Current:

let array: CFArray<CFType> = unsafe {
            CFArray::wrap_under_get_rule(SCNetworkServiceCopyAll(prefs.as_concrete_TypeRef()))
        };

        array
            .iter()
            .map(|item| item.downcast::<SCNetworkService>().unwrap())
            .collect::<Vec<SCNetworkService>>()

New:

let array: CFArray<SCNetworkService> = unsafe {
            CFArray::wrap_under_get_rule(SCNetworkServiceCopyAll(prefs.as_concrete_TypeRef()))
        };
        
        array
            .iter()
            .map(|item| item.as_CFType().downcast::<SCNetworkService>().unwrap())
            .collect::<Vec<SCNetworkService>>()

Looks not so diffence.

You can remove the .map(..) completely. The iterator will already give you SCNetworkService so no need to upcast it to a CFType and then downcast it again.

---

system-configuration/src/preferences.rs, line 13 at r11 (raw file):

//! See the examples directory for examples how to use this module.
//!
//! [`SCPreferences`]: https://developer.apple.com/documentation/systemconfiguration/scpreferences

Please link to https://developer.apple.com/documentation/systemconfiguration/scpreferences-ft8 since that is where one can find all the funcions.

---

system-configuration/src/preferences.rs, line 37 at r11 (raw file):

    pub fn new(allocator: CFAllocatorRef, name: &str, prefs_id: Option<&str>) -> Self {
        let prefs_id = match prefs_id {
            Some(prefs_id) => CFString::new(prefs_id).as_concrete_TypeRef(),

This CFString will be dropped immediately since it's not living in any variable. Thus the concrete typeref might be an invalid pointer. Please assign the CFString::new(prefs_id) to a variable and then take out the raw pointer after that.

---

Comments from Reviewable

[faern]

Review status: all files reviewed at latest revision, 22 unresolved discussions.

---

system-configuration/src/network_configuration.rs, line 188 at r5 (raw file): From the docs:

The ordered list of service identifiers associated with the set, or NULL if no service order has been specified or if an error occurred.

So you must add a check if the returned pointer is null or not before wrapping it as a CFArray.

---

Comments from Reviewable

[faern]

Review status: all files reviewed at latest revision, 21 unresolved discussions.

---

system-configuration/src/network_configuration.rs, line 360 at r7 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Is it not possible to remove .get_all_values() here and call .iter() directly? Then your iterator should be correctly typed from the start, so there should be no need to then unsafely map the pointer to the correct type.

I still think you can define the array as CFArray<SCNetworkInterface> directly and not have to do the map. Or is there a risk that some item in it is something else? If so you can't have the current unwrap either.

---

Comments from Reviewable

[LuoZijun]

@faern

You can remove the .map(..) completely. The iterator will already give you SCNetworkService so no need to upcast it to a CFType and then downcast it again.

Is it not possible to remove .get_all_values() here and call .iter() directly? Then your iterator should be correctly typed from the start, so there should be no need to then unsafely map the pointer to the correct type.

I still think you can define the array as CFArray<SCNetworkInterface> directly and not have to do the map. Or is there a risk that some item in it is something else? If so you can't have the current unwrap either.

This is not possible, you will face an error directly:

   Compiling system-configuration v0.1.0 (file:///Users/system-configuration-rs/system-configuration)
error[E0277]: the trait bound `std::vec::Vec<network_configuration::SCNetworkInterface>: std::iter::FromIterator<core_foundation::array::ItemRef<'_, network_configuration::SCNetworkInterface>>` is not satisfied
   --> system-configuration/src/network_configuration.rs:341:14
    |
341 |             .collect::<Vec<SCNetworkInterface>>()
    |              ^^^^^^^ a collection of type `std::vec::Vec<network_configuration::SCNetworkInterface>` cannot be built from an iterator over elements of type `core_foundation::array::ItemRef<'_, network_configuration::SCNetworkInterface>`
    |
    = help: the trait `std::iter::FromIterator<core_foundation::array::ItemRef<'_, network_configuration::SCNetworkInterface>>` is not implemented for `std::vec::Vec<network_configuration::SCNetworkInterface>`

error: aborting due to previous error

error: Could not compile `system-configuration`.
warning: build failed, waiting for other jobs to finish...
error[E0277]: the trait bound `std::vec::Vec<network_configuration::SCNetworkInterface>: std::iter::FromIterator<core_foundation::array::ItemRef<'_, network_configuration::SCNetworkInterface>>` is not satisfied
   --> system-configuration/src/network_configuration.rs:341:14
    |
341 |             .collect::<Vec<SCNetworkInterface>>()
    |              ^^^^^^^ a collection of type `std::vec::Vec<network_configuration::SCNetworkInterface>` cannot be built from an iterator over elements of type `core_foundation::array::ItemRef<'_, network_configuration::SCNetworkInterface>`
    |
    = help: the trait `std::iter::FromIterator<core_foundation::array::ItemRef<'_, network_configuration::SCNetworkInterface>>` is not implemented for `std::vec::Vec<network_configuration::SCNetworkInterface>`

error: aborting due to previous error

error: Could not compile `system-configuration`.

To learn more, run the command again with --verbose.

[faern]

Reviewed 1 of 8 files at r2, 1 of 3 files at r11. Review status: all files reviewed at latest revision, 14 unresolved discussions.

---

a discussion (no related file): @luozijun I felt this PR was growing a bit too much. So I extracted the SCPreferences part and added my own modifications. Please see PR #9 and add feedback you might have there. When we get that part merged this PR can be rebased and simplified a little bit.

---

Comments from Reviewable

[faern]

Review status: 2 of 6 files reviewed at latest revision, 14 unresolved discussions, some commit checks failed.

---

a discussion (no related file):

Previously, faern (Linus Färnstrand) wrote…

@luozijun I felt this PR was growing a bit too much. So I extracted the SCPreferences part and added my own modifications. Please see PR #9 and add feedback you might have there. When we get that part merged this PR can be rebased and simplified a little bit.

I now merged the other preference PR. So you can rebase on top of that if you want to.

Also, I think we should try to stick to CF/SC types when inside the library. So could we maybe switch out all &str for &CFString in arguments etc. Also returning CFString instead of String. Makes it closer to the Apple APIs. From what I have seen this is what the other libraries wrapping these frameworks are doing as well.

---

Comments from Reviewable

[LuoZijun]

Also, I think we should try to stick to CF/SC types when inside the library. So could we maybe switch out all &str for &CFString in arguments etc. Also returning CFString instead of String. Makes it closer to the Apple APIs. From what I have seen this is what the other libraries wrapping these frameworks are doing as well.

I think this is a good idea, i will do that at tomorrow.

[faern]

Review status: 0 of 6 files reviewed at latest revision, 15 unresolved discussions.

---

system-configuration/src/lib.rs, line 26 at r13 (raw file):

extern crate system_configuration_sys;
#[cfg(all(feature = "nightly", test))]
extern crate test;

What is it that you need the test crate for? The normal #[test] test infrastructure exists on stable Rust. The only time I know of when one would need the test crate is for benchmarks. But I don't see any benchmarks in this PR.

---

system-configuration/src/network_configuration.rs, line 425 at r13 (raw file):

#[cfg(all(feature = "nightly", test))]
fn test_network_service() {

I'm not sure exactly what you are trying to achieve here. but if you are looking at making this function a test when the nightly feature flag is set you probably want to look at cfg_attr, #[cfg_attr(feature = "nightly", test)] would tag this function as a test if the feature is set. At the moment the cfg only conditionally includes this function iff the feature is set and the compile is happening under cargo test, it is never marked as a test.

But then again, I can make this test run fine on stable, what is it that you need nightly for?

---

Comments from Reviewable

[faern]

Ping @luozijun :) Felt like we were pretty close to merge with this PR.

---

Review status: 0 of 6 files reviewed at latest revision, 15 unresolved discussions.

---

Comments from Reviewable