Add High-level API #8

LuoZijun · 2018-02-20T03:39:38Z

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

pronebird · 2018-02-20T11:27:17Z

system-configuration/src/network_configuration.rs

+        let mut setup_domain_name: Option<String> = None;
+        let mut setup_server_addresses: Option<Vec<IpAddr>> = None;
+
+        if let Some(value) = store.get(CFString::new(&format!("State:/Network/Service/{}/DNS", self.id()))) {


If State and Setup branches do the same perhaps this can be turned into a loop or maybe a helper function.

faern · 2018-02-20T17:14:47Z

@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 :)

faern

Please run the latest rustfmt on all code as well, we try to stick to having everything formatted following that standard.

faern · 2018-02-20T17:58:56Z

system-configuration/src/network_configuration.rs

+use std::mem;
+use std::net::IpAddr;
+
+use core_foundation_sys::base::kCFAllocatorDefault;


The core-foundation crate re-exports the corresponding -sys modules. So you don't need to depend directly on core-foundation-sys. You could import it as use core_foundation::base::kCFAllocatorDefault;

LuoZijun · 2018-02-21T00:49:16Z

@faern

When PR #9 has merged, i will remove system-configuration-sys changes and rebase.

faern · 2018-02-21T09:29:22Z

system-configuration/src/network_configuration.rs

+pub struct SCNetworkInterfaceMTU {
+    cur: u32,
+    min: u32,
+    max: u32,


I read in the documentation that min and max can be unavailable for some interfaces, and then a negative value is returned. I see you just cast the signed integers to unsigned ones. It would be more Rust idiomatic to represent the missing values with an Option<u32>. And then where you obtain them from the OS you have a safety check instead of just casting, something like:

SCNetworkInterfaceMTU { cur: current as u32, min: if min < 0 { None } else { Some(min as u32) } max: if max < 0 { None } else { Some(max as u32) } }

faern · 2018-02-21T09:33:04Z

system-configuration/src/network_configuration.rs

+
+#[derive(Debug)]
+pub struct SCNetworkInterfaceMTU {
+    cur: u32,


I'm not a big fan of abbreviations where they are not very standard ones or not really needed to reduce bloat. Here I would argue we would benefit from calling it current to be more user friendly. In other words, we can call it cur when interacting with the sys layer, but current in the abstraction.

faern · 2018-02-21T09:34:50Z

system-configuration/src/network_configuration.rs

+    max: u32,
+}
+
+impl SCNetworkInterfaceMTU {


This struct is used purely as a data struct, it does not carry any logic with it. Then we could simplify things by making all fields in it public and remove the impl. That would allow a user to just access the data directly instead of going through getters.

faern · 2018-02-21T11:41:48Z

@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 · 2018-02-21T12:34:30Z

system-configuration/src/network_configuration.rs

+            let path = CFString::new(&format!("Setup:/Network/Service/{}/DNS", self.id()));
+
+            if !store.set(path, dictionary) {
+                // FIXME: should rollback ?


It seems that SCDynamicStoreSetMultiple would be more appropriate for setting multiple values.

@faern is it implemented in the store?

ps: I don't insist on changing this now. Just a thought for future refactoring and optimization.

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

set ServerAddresses and DomainName .

LuoZijun · 2018-02-21T12:59:07Z

system-configuration/src/network_configuration.rs

+        return None;
+    }
+
+    pub fn service(&self) -> Option<SCNetworkService> {


this is better ?

pub fn service(&self, session_name: &str) -> Option<SCNetworkService>

LuoZijun · 2018-02-21T12:59:25Z

system-configuration/src/network_configuration.rs

+        return None;
+    }
+
+    pub fn interface(&self) -> Option<SCNetworkInterface> {


this is better ?

pub fn interface(&self, session_name: &str) -> Option<SCNetworkInterface>

LuoZijun · 2018-02-21T12:59:52Z

system-configuration/src/network_configuration.rs

+        return None;
+    }
+
+    pub fn router(&self) -> Option<IpAddr> {


this is better ?

pub fn router(&self, session_name: &str) -> Option<IpAddr>

LuoZijun · 2018-02-21T13:00:22Z

system-configuration/src/network_configuration.rs

+pub struct SCNetworkService(pub SCNetworkServiceRef);
+
+impl SCNetworkService {
+    pub fn list() -> Vec<SCNetworkService> {


this is better ?

pub fn list(session_name: &str) -> Vec<SCNetworkService> {

faern

I have to go now. Not done reviewing, will continue looking tomorrow!

faern · 2018-02-21T14:36:13Z

system-configuration/src/network_configuration.rs

+    pub current: u32,
+    /// the minimum MTU setting for the interface. If negative, the minimum setting could not
+    /// be determined.
+    pub min: Option<u32>,


This can't be negative with the current representation, so the documentation is slightly off. You can change "If negative" to "If None" to make it correct. Same goes for max.

faern · 2018-02-21T14:46:38Z

system-configuration/src/network_configuration.rs

+
+/// MTU
+#[derive(Debug)]
+pub struct SCNetworkInterfaceMTU {


For abbreviations, such as MTU, the usually preferred way is to only capitalize the first letter (Mtu). For example in the standard library we have Ipv4Addr, not IPv4Addr. The same holds for SCNetworkServiceDNS. See this: https://github.com/rust-lang/rfcs/blob/master/text/0430-finalizing-naming-conventions.md#fine-points

I just now realize that with this argument we should not prefix types with SC..., but rather with Sc.... However core-foundation prefixes with CF so it makes a lot of sense to be consistent with them, plus it's a lot more consistent with the actual Apple API, so I'm fully OK with keeping SC....

@faern do you suggest we should leave the uppercase prefix but switch the rest to the CamelCase? I.e SCNetworkServiceDns instead of SCNetworkServiceDNS?

Yes. I suggest we should keep the SC prefix to be consistent with core-foundation and that we should follow the standard in all other instances.

faern · 2018-02-21T15:08:27Z

system-configuration/src/network_configuration.rs

+}
+
+/// Network service object.
+pub struct SCNetworkService(pub SCNetworkServiceRef);


I think all of these types should be declared according to the standard used in core-foundation, with the macros there. Compare with how I have done it for SCDynamicStore: https://github.com/mullvad/system-configuration-rs/blob/master/system-configuration/src/dynamic_store.rs#L137

That way we'll get automatic and correct Drop implementation and lots of helper methods.

faern · 2018-02-21T22:41:26Z

system-configuration/src/network_configuration.rs

+    /// Returns primary network service
+    pub fn service(&self) -> Option<SCNetworkService> {
+        if let Some(service_id) = SCNetworkGlobal::query("ng_service", "PrimaryService") {
+            for _service in SCNetworkService::list() {


Please only prefix a variable name with an underscore if the variable is intentionally unused. If underscores are used on used variables they become easy to confuse with unused ones. Plus then the linting fails, if this variable ever becomes unused the lint won't help in detecting that.

faern · 2018-02-21T22:50:28Z

system-configuration/src/network_configuration.rs

+}
+
+/// Global network object
+pub struct SCNetworkGlobal;


Since this struct does not hold any data it basically just becomes a namespace to attach methods to. I would argue that the methods in this struct would suit better on the structs they return.

// So instead of SCNetworkGlobal::service(&self) -> Option<SCNetworkService> // we could have SCNetworkService::global(store: &SCDynamicStore) -> Option<Self>

I think that would make more sense, as it would become a kind of constructor on the type it relates to.

Note the extra store argument. I saw you asked in another thread if the session_name should be passed in instead of hardcoded. I think it would be nice to take it one step further and let the user pass in their dynamic store instance instead of letting this method create their own. I'm not super familiar with the dynamic stores or the names of their instances, but it would likely be cleanest if an application had one dynamic store instance that it used for everything, which we can only achieve by passing in that instance.

The query method could just be a standalone function in this module since it would be required in more than one struct impl.

The query method could just be a standalone function in this module since it would be required in more than one struct impl.

that's two method query have diffence type.

faern · 2018-02-21T23:15:16Z

system-configuration/src/network_configuration.rs

+        let mut services = Vec::new();
+
+        for id in array.get_all_values().iter() {
+            let id_ptr: CFStringRef = *id as _;


Here you treat id as a CFStringRef, but you have defined array: CFArray<SCNetworkServiceRef>, so at least one of them must be wrong :P. According to the documentation for SCNetworkSetGetServiceOrder it returns service identifiers, so I guess you want CFArray<CFString>?

If you just get the iterator for an array directly, without calling get_all_values then you get the correct type out of the iterator directly instead of just *const c_void which you then have to cast.

Yeah, your are right :)

faern · 2018-02-21T23:18:17Z

system-configuration/src/network_configuration.rs

+            let path = CFString::new(&format!("Setup:/Network/Service/{}/DNS", self.id()));
+
+            if !store.set(path, dictionary) {
+                // FIXME: should rollback ?


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

faern · 2018-02-21T23:22:36Z

system-configuration/src/network_configuration.rs

+    }
+
+    /// Returns the DNS infomation on this network service
+    pub fn dns(&self) -> SCNetworkServiceDNS {


I'm a bit ambivalent about this method and set_dns. They are nice helpers indeed, but they are not at all an abstraction of the SCNetworkConfiguration API. They use self solely to get the network service ID, they don't call a single function in system_configuration_sys::network_configuration::.

EDIT: Keep them for now and we'll see how everything fits together when the other feedback has been fixed.

pronebird · 2018-02-22T09:46:37Z

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 · 2018-02-26T09:14:52Z

system-configuration/src/network_configuration.rs

+
+    /// Returns the DNS infomation on this network service
+    pub fn dns(&self) -> SCNetworkServiceDns {
+        let store = SCDynamicStoreBuilder::new("ns_dns").build();


Here are still internally created dynamic stores. I think it's better we let these be passed in. So the consumer can decide the name and so they can reuse the same store for multiple things.

Sounds good.

faern · 2018-02-26T09:15:02Z

system-configuration/src/network_configuration.rs

+    state_domain_name: Option<String>,
+    setup_domain_name: Option<String>,
+    state_server_addresses: Option<Vec<IpAddr>>,
+    setup_server_addresses: Option<Vec<IpAddr>>,


I thought I had already commented on this, but now I can't find that comment so maybe I forgot to submit it. Or if I already did write it, sorry for repeating myself. Anyway I think it's a bit sub-optimal to have to send in and get out the state_domain_name and setup_domain_name as (Option<String>, Option<String>). Which is which is not obvious from the type etc. Also state and setup store the same type of information, so I think it could be nicer modeled something like:

pub struct SCNetworkServiceDns { pub state: DnsSetting, pub setup: DnsSetting, } pub struct DnsSetting { pub domain_name: Option<String>, pub server_addresses: Vec<IpAddr>, }

Or would this cause problems somewhere?
Note that I made all fields public again. Since these are pure data structs I think we can just allow direct access. I also made Option<Vec<IpAddr>> into Vec<IpAddr>, I thought we could just model no addresess as an empty vector instead of a None, would that have downsides?

Note that I made all fields public again. Since these are pure data structs I think we can just allow direct access. I also made Option<Vec> into Vec, I thought we could just model no addresess as an empty vector instead of a None, would that have downsides?

Personally, I think Option<Vec<IpAddr>> is better than Vec<IpAddr>, when we setting dns with Empty (like networksetup -setdnsservers "Wi-Fi" "Empty"), we do not need alloc one Vec .
But if you insist on that, i can change that.

Creating an empty Vec does not do any allocation in Rust unless I'm remembering wrong. So I would definitely not worry about the performance aspect of it.

But yes if we want to be able to detect the difference of no DNS set vs empty list of DNS set, then we need this to be an Option<..>. So keep it an Option<..>, that is then also same as the type of argument to set_dns_state_addresses method, so that is good.

faern · 2018-02-26T09:18:57Z

system-configuration/src/network_configuration.rs

+                        dict.find2(&CFString::from_static_string("ServerAddresses"))
+                    {
+                        let addrs = unsafe { CFType::wrap_under_get_rule(addrs) };
+                        if let Some(addrs) = addrs.downcast::<CFArray<CFString>>() {


This can actually lead to a panic. It's a bug that one can downcast to CFArray<T> since it never checks that all items in the array are actually of type T. See this issue: servo/core-foundation-rs#158

The safer way would be to addrs.downcast::<CFArray<CFType>>() and then for each element addr.downcast_into::<CFString>. This would actually add a check that each item is a CFString. It can still theoretically panic if an item in the array is not a CFType at all, but we can't reliably check against that and have to assume it is. It could just as well be equally valid to assume they are CFStrings, but this at least adds one level of sanity check :)

Oh, right ...

faern · 2018-02-26T10:04:47Z

system-configuration/src/network_configuration.rs

+    }
+
+    /// Setting DNS on this network service
+    pub fn set_dns(&self, dns: SCNetworkServiceDns) -> bool {


This method only sets the Setup part of the DNS, never the State. Since a SCNetworkServiceDns is passed in one could easily think it would set both. And there is also the possible confusion about if a setup_server_addresses: None should not change the addresses at all (current implementation) or change it into no addresses.

Expressing "update structs", structs that can conditionally update some fields, can be hard. And it usually ends up with multiple layers of Option<...>. To avoid that and take a more direct approach I'm wondering if the following might be a desirable way?:

set_dns_setup_domain_name(&mut self, domain_name: Option<String>) set_dns_state_domain_name(&mut self, domain_name: Option<String>) set_dns_setup_addresses(&mut self, addresses: Option<Vec<IpAddr>>) set_dns_state_addresses(&mut self, addresses: Option<Vec<IpAddr>>)

Where a None argument in both cases removes the corresponding "ServerAddresses"/"DomainName" key from the store.

What do you think @pronebird?

faern · 2018-02-26T10:11:23Z

system-configuration/src/network_configuration.rs

+
+        array
+            .get_all_values()
+            .iter()


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.

faern · 2018-02-26T10:14:51Z

system-configuration/src/network_configuration.rs

+
+impl SCNetworkInterface {
+    /// Returns primary network interface
+    pub fn global(store: &SCDynamicStore) -> Option<Self> {


Could we not implement this method as SCNetworkService::global(store).interface()? That would avoid iterating all interfaces. Or can there be a case where the primary interface is not attached to a service, and thus that implementation would return something different from this one?

that's make sense :)

faern · 2018-02-26T10:18:52Z

system-configuration/src/network_configuration.rs

+
+    /// Returns the BSD interface or device name
+    pub fn name(&self) -> Option<String> {
+        self.bsd_name()


So name is just an alias for bsd_name? Why do we need two methods doing the same thing? Would be enough with one of them. Naming it just name is probably good since that is equal to the name method on SCNetworkService and since the documentation can say "BSD interface or device name" to explain the same thing.

faern · 2018-02-26T10:20:13Z

system-configuration/src/network_configuration.rs

+    }
+
+    /// Returns the network interface type
+    pub fn type_(&self) -> Option<String> {


Naming the method interface_type would IMO be preferred over having a trailing _ to not collide with the keyword. This would also be more in line with what the underlying ffi function is named.

faern · 2018-02-26T10:21:48Z

system-configuration/src/network_configuration.rs

+        unsafe {
+            let str_ptr = SCNetworkInterfaceGetInterfaceType(self.0);
+            if str_ptr.is_null() {
+                None


The SCNetworkInterfaceGetInterfaceType documentation does not mention it can return NULL. So it's probably safe to assume it doesn't and change the return type here to -> String directly.

faern · 2018-02-26T10:22:59Z

system-configuration/src/network_configuration.rs

+        unsafe {
+            let str_ptr = SCNetworkInterfaceGetHardwareAddressString(self.0);
+            if str_ptr.is_null() {
+                None


Same as the type. The documentation does not mention it can return NULL. Have you seen a case where it does? Otherwise it would probably be safe to just return String instead of Option<String>.

On my macbook, the network service iPhone USB 's interface havn't hwaddr.

Ok. Then I would say their documentation is dangerous, not warning about null pointers. But sure, then it should be an Option<..>! :)

Yeah, that's true.

faern · 2018-03-07T09:13:03Z

system-configuration/src/network_configuration.rs

+        if let Some(service_id) = global_query(store, "PrimaryService") {
+            for service in SCNetworkService::list() {
+                if service.id() == service_id {
+                    return Some(service);


You can probably use SCNetworkServiceCopy and give it the ID instead of listing all services and comparing with the ID. Should be more efficient.

I suggest exposing a constructor pub fn from_id(service_id: &CFString) -> Option<Self> and then make use of that one in here.

Please add unit tests to this new constructor to make sure it correctly handles both a valid and an invalid service_id correctly.

faern · 2018-03-07T09:28:51Z

system-configuration/src/network_configuration.rs

+    }
+
+    /// Returns all available network services for the specified preferences.
+    pub fn list() -> Vec<SCNetworkService> {


Documentation says "for the specified preferences", but it's impossible to specify preferences since they are hardcoded. Remove that part of the documentation maybe?

I have just add &SCPreferences arg.

pub fn list(prefs: &SCPreferences) -> Vec<SCNetworkService>

faern · 2018-03-07T09:31:50Z

system-configuration/src/network_configuration.rs

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


I think you can specify let array: CFArray<SCNetworkService> = ... directly and thus not having to do this mapping at all. I'm not 100% sure about the retain count here, have to make sure that becomes correct.

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.

faern · 2018-03-07T10:10:37Z

system-configuration/src/network_configuration.rs

+                let service_ref = unsafe {
+                    CFType::wrap_under_get_rule(SCNetworkServiceCopy(
+                        prefs,
+                        id.as_concrete_TypeRef(),


Here you could make use of the from_id constructor I suggested above if you add it.

Okay, that's a good idea.

faern · 2018-03-07T10:15:49Z

system-configuration/src/network_configuration.rs

+        for item in array.iter() {
+            if let Some(id) = item.downcast::<CFString>() {
+                let service_ref = unsafe {
+                    CFType::wrap_under_get_rule(SCNetworkServiceCopy(


SCNetworkServiceCopy returns a SCNetworkServiceRef directly. So you can just do SCNetworkService::wrap_under_get_rule(..), no need to go via CFType and downcast.

On the other hand, the function can return NULL. So you'll have to check for that before calling wrap_under_get_rule. The current code can reach undefined behavior if called on a non-existent service.

faern · 2018-03-07T10:18:05Z

@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 · 2018-03-07T10:32:05Z

@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 · 2018-03-07T11:34:09Z

@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 · 2018-03-07T13:14:45Z

@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 · 2018-03-12T09:37:35Z

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 · 2018-03-12T09:37:55Z

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

Comments from Reviewable

pronebird · 2018-03-12T09:38:46Z

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

Comments from Reviewable

faern · 2018-03-12T11:56:02Z

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 · 2018-03-12T12:17:29Z

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 · 2018-03-12T12:25:42Z

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 · 2018-03-12T14:23:58Z

@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 · 2018-03-12T23:58:55Z

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 · 2018-03-14T19:25:40Z

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 · 2018-03-17T17:57:46Z

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 · 2018-03-19T20:33:34Z

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 · 2018-04-05T09:11:51Z

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

LuoZijun force-pushed the patch-1 branch from d7f7154 to a35a48c Compare February 20, 2018 03:50

pronebird reviewed Feb 20, 2018

View reviewed changes

faern mentioned this pull request Feb 20, 2018

Simplify bindings and add SCPreferences + SCNetworkConfiguration #9

Merged

faern requested changes Feb 20, 2018

View reviewed changes

faern requested changes Feb 21, 2018

View reviewed changes

LuoZijun force-pushed the patch-1 branch 3 times, most recently from 76eb9da to c41a1a5 Compare February 21, 2018 12:18

pronebird reviewed Feb 21, 2018

View reviewed changes

LuoZijun force-pushed the patch-1 branch 3 times, most recently from 311e66c to 1bb3afb Compare February 21, 2018 12:56

LuoZijun commented Feb 21, 2018

View reviewed changes

faern requested changes Feb 21, 2018

View reviewed changes

faern self-assigned this Feb 26, 2018

faern requested changes Feb 26, 2018

View reviewed changes

LuoZijun force-pushed the patch-1 branch from 563646b to d57eef8 Compare February 28, 2018 02:49

faern requested changes Mar 7, 2018

View reviewed changes

faern mentioned this pull request Mar 7, 2018

Remove BoxFuture from core and replace with manual future impl mullvad/jsonrpc-client-rs#30

Merged

faern mentioned this pull request Mar 12, 2018

Add preferences abstraction #12

Merged

LuoZijun force-pushed the patch-1 branch from 799ed30 to a75deee Compare March 13, 2018 05:45

Add High-level API

7e8771b

LuoZijun force-pushed the patch-1 branch from a75deee to 7e8771b Compare March 19, 2018 07:19

Add High-level API #8

Are you sure you want to change the base?

Add High-level API #8

Conversation

LuoZijun commented Feb 20, 2018 • edited by faern Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

faern commented Feb 20, 2018

faern left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LuoZijun commented Feb 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

faern commented Feb 21, 2018

pronebird Feb 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

faern left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

faern Feb 21, 2018 • edited Loading

Choose a reason for hiding this comment

pronebird commented Feb 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

faern commented Mar 7, 2018

LuoZijun commented Mar 7, 2018

faern commented Mar 7, 2018

LuoZijun commented Mar 7, 2018

pronebird commented Mar 12, 2018

pronebird commented Mar 12, 2018

pronebird commented Mar 12, 2018

faern commented Mar 12, 2018

LuoZijun commented Feb 20, 2018 •

edited by faern

Loading

LuoZijun commented Feb 21, 2018 •

edited

Loading

pronebird Feb 21, 2018 •

edited

Loading

faern Feb 21, 2018 •

edited

Loading