Rust language reimplementation #105

majorz · 2017-06-28T13:43:00Z

Connects-To: #104
Change-Type: minor
---- Autogenerated Waffleboard Connection: Connects to #104

majorz · 2017-06-29T10:40:35Z

This new PR includes all of the Rust implementation code in a single commit.

This is the new flow of the application:

jbaldwinroberts

looking good!

jbaldwinroberts · 2017-06-29T10:53:01Z

Cargo.toml

@@ -0,0 +1,28 @@
+[package]
+name = "resin-wifi-connect"
+version = "0.1.0"


the version should match

will place a matching "2.0.8"

@majorz this will be 3.0.0 - the last coffeescript version we release will be 2.0.8

@josephroberts, it will be bumped to 3.0.0 by VersionBot automatically when 104-rust lands in master. So I think it is better to keep the Cargo version with the changelog one in sync.

jbaldwinroberts · 2017-06-29T10:53:10Z

Cargo.toml

+[package]
+name = "resin-wifi-connect"
+version = "0.1.0"
+authors = ["Joe Roberts <joe@resin.io>"]


add yourself

jbaldwinroberts · 2017-06-29T10:53:39Z

Cargo.toml

+env_logger = "0.4"
+
+[dependencies.network_manager]
+git = "https://github.com/resin-io-modules/network_manager"


we should release network_managed as a crate before merging this in
balena-io-modules/network-manager#57
balena-io-modules/network-manager#56

Sounds good - let's leave that for a next PR

jbaldwinroberts · 2017-06-29T10:57:56Z

src/config.rs

+
+    let interface: Option<String> = matches.value_of("interface").map_or_else(
+        || {
+            env::var("PORTAL_INTERFACE").ok()


why is there no default?

If no device is specified, we default to the first Wi-Fi device. Do we really need to default to 'wlan0'? For example on my laptop the internal one is called 'wlo1' and this one makes more sense as a default one for my system. Or I am missing something?

no, that makes total sense. ignore me :)

jbaldwinroberts · 2017-06-29T10:58:03Z

src/config.rs

+
+    let password: Option<String> = matches.value_of("password").map_or_else(
+        || {
+            env::var("PORTAL_PASSPHRASE").ok()


why is there no default?

It is the same as in the current implementation - the default is for creating an open hotspot.

jbaldwinroberts · 2017-06-29T10:59:01Z

.travis.yml

+  - if [ $TRAVIS_RUST_VERSION == nightly ]; then
+      cargo clippy -- -D warnings;
+    fi
+  - cargo build


travis should build for the required architectures, push to S3 and create the github releases

You need to update the docker file to always pull the latest release for the correct architecture. If this PR was merged today it would break resin-wifi-connect?

You need to update the readme. We also need to release the 2.x version with the warnings we spoke about in the arch call before this can be merged

As we talked on the chat about those items - the PR will go into 104-rust, not master and we will make the changes in consecutive PRs.

jbaldwinroberts · 2017-06-29T11:02:42Z

Cargo.lock

@@ -0,0 +1,1003 @@
+[root]
+name = "resin-wifi-connect"


should a binary have this lock file? I thought it was only recommended for libraries

It is the other way around: http://doc.crates.io/faq.html#why-do-binaries-have-cargolock-in-version-control-but-not-libraries

jbaldwinroberts · 2017-06-29T11:06:38Z

src/network.rs

+                    hotspot_connection = None;
+                }
+
+                let access_points = match get_access_points(&device) {


there is some code duplication throughout, for example this section is repeated on line 95. It would be good to go through and try to extract these parts into functions.

I have used this for go code - not sure if there is something similar for rust. I am surprised clippy does not catch this to be honest

There are actually some small changes (like a missing let in one of the cases) and maybe this is why clippy does not catch it. It is still a duplication, but I suggest that we leave it like that for now and address it all at once when we refactor that whole function with a better design.

jbaldwinroberts · 2017-06-29T11:08:45Z

src/server.rs

+unsafe impl Send for RequestSharedState {}
+unsafe impl Sync for RequestSharedState {}
+
+#[derive(Debug)]


can we remove this, you have the display trait implemented below

I tried to remove it and the compiler complains with required by "std::error::Error".

hmm, any idea why?

The Error trait is defined as dependent on the Debug and Display traits: pub trait Error: Debug + Display {...}.

abrodersen

A good start! Appears to be coming along nicely. :)

abrodersen · 2017-06-29T13:09:10Z

src/dnsmasq.rs

+
+use config::Config;
+
+pub fn start_dnsmasq(config: &Config, device: &Device) -> Result<Child, String> {


String errors make me sad. Have you looked at using error_chain?

We discussed that earlier on during one of the network_manager PRs, but left it for a later stage. @josephroberts, what's your opinion on the improved error reporting - should we pursue that before release or after?

I think we should release now and have this as one of the first things we "fix" - unless its like a couple of hours of work in which case we should do it

Definitely it will take more time to get it done in both repos. We will address it later on then.

abrodersen · 2017-06-29T13:11:54Z

src/network.rs

+                ssid,
+                password,
+            } => {
+                if hotspot_connection.is_some() {


I think this should be an if let Some(value)

nice catch!

abrodersen · 2017-06-29T13:16:09Z

src/network.rs

+        let command = match network_rx.recv() {
+            Ok(command) => command,
+            Err(e) => {
+                return exit_with_error(


I think instead of calling exit_with_error all over the place, it might make sense to convert all this main loop logic into a dispatch function that returns a Result. You can then use the handy ? operator inside the function to avoid all the matching. The main loop can then check for an Err result and send the exit command.

Yeah, I am not happy with that long function either. It definitely needs a better design and I was thinking that we can address that in the next development phase. Joe had some ideas about incorporating some form of state machine as well. @josephroberts, what's your opinion on this - should we try to come up with a better design for the network related thread function, or we should address that at a post-3.0 phase?

address it later on

abrodersen · 2017-06-29T13:19:05Z

src/network.rs

+    ssid: &str,
+    gateway: &Ipv4Addr,
+    password: &Option<&str>,
+) -> Result<Connection, String> {


Is this the rustfmt approved way of formatting a function? It looks really wonky to me.

Yes, this is how rustfmt is configured now for us to avoid right-side drifting (I am not really sure what is the current rustfmt default, since they improved many aspects of those recently). We need to catch up with rustfmt soon, since they introduced a new crate that runs on nightly only, which will be the maintained version: http://www.ncameron.org/blog/rustfmt-changes/

abrodersen · 2017-06-29T13:22:47Z

src/server.rs

+            format!("{}", request_state.gateway)
+        };
+
+        let host = req.headers.get::<headers::Host>().unwrap();


Could a client crash the server by not sending a Host header?

Another nice catch, will fix that.

abrodersen · 2017-06-29T13:23:14Z

src/server.rs

+        let host = req.headers.get::<headers::Host>().unwrap();
+
+        if host.hostname != gateway {
+            let url = Url::parse(&format!("http://{}/", gateway)).unwrap();


Same with sending a malformed Url

Will fix that too :)

I looked at this one and it will actually never fail, since the gateway is already parsed Ipv4Addr instance.

majorz · 2017-07-04T17:23:49Z

I updated the source code and recommitted with all the changes discussed above.

Additionally I ported the following functionality from the existing implementation:

Starting the NetworkManager service if stopped for some reason
Checking for Internet connectivity after connection to access point is established - when new connection is created using the hotspot and webserver. If connectivity is not established, the connection will be deleted and the webserver will continue to run waiting for new credentials to be added.

jbaldwinroberts · 2017-07-05T07:31:24Z

src/config.rs

+const DEFAULT_GATEWAY: &str = "192.168.42.1";
+const DEFAULT_DHCP_RANGE: &str = "192.168.42.2,192.168.42.254";
+const DEFAULT_SSID: &str = "ResinAP";
+const DEFAULT_TIMEOUT: &str = "15000";


We should specify that this timeout is in ms, either with a comment or by naming the variable DEFAULT_TIMEOUT_MS

Good point, I changed it to DEFAULT_TIMEOUT_MS.

jbaldwinroberts · 2017-07-05T07:32:06Z

src/config.rs

+
+pub fn get_config() -> Config {
+    let matches = App::new(env!("CARGO_PKG_NAME"))
+        .version(env!("CARGO_PKG_VERSION"))


I didn't know about CARGO_PKG_... - very nice :)

jbaldwinroberts · 2017-07-05T07:36:16Z

src/server.rs

+unsafe impl Send for RequestSharedState {}
+unsafe impl Sync for RequestSharedState {}
+
+#[derive(Debug)]


hmm, any idea why?

Connects-To: #104 Change-Type: major

majorz · 2017-07-05T11:20:34Z

In addition to the DEFAULT_TIMEOUT_MS change I added one fix for the travis builds:

When clippy was compiled with an older Rust nightly (like from the previous day), but no new clippy version was found, then clippy is not working with the new Rust nightly (because it links to the compiled Rust binary libs). In that case although a new clippy version is not available, we still need to recompile the current clippy version towards the new nightly. The check I did is run cargo clippy -V and then if the exit code is non-zero something is wrong (like the described case) and we force reinstall.

abrodersen

Looks good!

majorz requested review from emberian, jbaldwinroberts and abrodersen June 28, 2017 13:43

majorz force-pushed the 104-rust-binary branch 2 times, most recently from fc3e475 to 1a31569 Compare June 29, 2017 09:43

jbaldwinroberts reviewed Jun 29, 2017

View reviewed changes

abrodersen reviewed Jun 29, 2017

View reviewed changes

majorz force-pushed the 104-rust-binary branch from 1a31569 to 4667bf6 Compare July 4, 2017 17:18

jbaldwinroberts reviewed Jul 5, 2017

View reviewed changes

majorz force-pushed the 104-rust-binary branch 3 times, most recently from a82d236 to c5c6a55 Compare July 5, 2017 11:06

Rust language reimplementation

e5532e7

Connects-To: #104 Change-Type: major

majorz force-pushed the 104-rust-binary branch from c5c6a55 to e5532e7 Compare July 5, 2017 11:10

jbaldwinroberts approved these changes Jul 5, 2017

View reviewed changes

abrodersen approved these changes Jul 5, 2017

View reviewed changes

majorz merged commit e178987 into 104-rust Jul 7, 2017

majorz deleted the 104-rust-binary branch July 7, 2017 07:24


		use config::Config;

		pub fn start_dnsmasq(config: &Config, device: &Device) -> Result<Child, String> {

Rust language reimplementation #105

Rust language reimplementation #105

Conversation

majorz commented Jun 28, 2017 • edited by ghost Loading

majorz commented Jun 29, 2017 • edited Loading

jbaldwinroberts 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

majorz Jun 29, 2017 • 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

Choose a reason for hiding this comment

jbaldwinroberts Jun 29, 2017 • 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

abrodersen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbaldwinroberts Jun 29, 2017 • 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

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

majorz commented Jul 4, 2017 • 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

majorz commented Jul 5, 2017

abrodersen left a comment

Choose a reason for hiding this comment

majorz commented Jun 28, 2017 •

edited by ghost

Loading

majorz commented Jun 29, 2017 •

edited

Loading

majorz Jun 29, 2017 •

edited

Loading

jbaldwinroberts Jun 29, 2017 •

edited

Loading

jbaldwinroberts Jun 29, 2017 •

edited

Loading

majorz commented Jul 4, 2017 •

edited

Loading