[WIP] Add initial implementation for netcat #28

rdzhou · 2018-07-16T02:41:25Z

Main features of netcat is added. The rest remains to be added.

mssun · 2018-07-16T06:49:20Z

Please add [WIP] prefix in the title if you think it is not ready for reviewing/merging.
The PR didn't pass the CI because of testing (try to run cargo test first). You can add a testing file (test_nc.rs) to bypass the check.

Arcterus

I only commented on repeated issues once, so several occur more than once.

Arcterus · 2018-07-16T16:20:55Z

src/networking/nc/mod.rs

+extern crate libc;
+extern crate socket2;
+
+use std;


You shouldn't need to include this.

Arcterus · 2018-07-16T16:22:06Z

src/networking/nc/mod.rs

+use mio::{Events, Event, Poll, Ready, PollOpt, Token};
+use libc::{AF_UNSPEC, AF_INET, AF_INET6, AF_UNIX};
+use std::io;
+use std::net::{SocketAddr};


If there's only one member that you are trying to use from a module, usually you'd just do use std::net::SocketAddr instead of use std::net::{SocketAddr}.

Arcterus · 2018-07-16T16:23:35Z

src/networking/nc/mod.rs

+        ))
+}
+
+fn build_ports(ports: &str) -> Result<Vec<u16>, MesaError>{


If you include super::Result, you should be able to just do Result<Vec<u16>>. You could also just do super::Result<Vec<u16>>.

Since there are not just one type of Result, I think it's better to keep all the Result consistent and make Ok and Err explicit.

Arcterus · 2018-07-16T16:32:44Z

src/networking/nc/mod.rs

+    let port_list = match ports.parse::<u16>() {
+        Ok(port) => port,
+        Err(_) => {
+            return mesaerr_result(&format!("invalid port[s] {}", ports));


I think that you should create a type like the following rather than using mesaerr_result() everywhere:

use std::num::ParseIntError; #[derive(Debug, Fail)] enum NcError { #[fail(display = "invalid port(s) {}: {}", ports, err)] ParsePort { #[cause] err: ParseIntError, ports: String, }, // more errors go here }

You can then change this function to something like:

fn build_ports(ports: &str) -> super::Result<Vec<u16>> { ports .parse::<u16>() .map(|port| vec![port]) .map_err(|e| NcError::ParsePort { err: e, ports: ports.to_string() }) }

Arcterus · 2018-07-16T16:43:43Z

src/networking/nc/mod.rs

+    Ok(vec!(port_list))
+}
+
+fn warn(msg: &str) {


This would be fine if the utility was written normally (i.e. as a standalone binary), but because mesabox can be included into other projects as a library, we need to be able to write to custom stdin/stdout/stderr (as in, we need to be able to write to a Vec<u8> or perhaps to a File rather than Stderr). This issue is primarily what UtilSetup is meant to solve. This function should instead be:

fn warn<S: UtilSetup>(setup: &mut S, msg: &str) { let _ = write!(setup.error(), "{}", msg); }

I haven't fully checked where you use warn() (so what I'm about to say may not fully apply), but there is also display_msg!(), which prints the name of the utility + whatever you want to write to the given output stream (e.g. display_msg!(setup.error(), "{}", msg)).

I probably need to document the structure of the framework somewhere to avoid this sort of confusion.

Arcterus · 2018-07-16T17:18:07Z

src/networking/nc/mod.rs

+                s_addr.clone().unwrap()
+            } else {
+                let nf = NamedTempFile::new()?;
+                let path = String::from(nf.path().to_str().unwrap());


Rather than a String, this should most likely be a Path or PathBuf. You might be able to get Path to work by giving NcOptions a lifetime and not taking ownership of ArgMatches (i.e. use &mut ArgMatches instead).

Path won't work since nc doesn't only work for unix. For TCP/UDP, the type is not path.

Isn’t unix_dg_tmp_socket just used for Unix sockets though? I scanned through the code briefly and didn’t notice anything that indicated otherwise, but let me know if I missed something.

Yes unix_dg_tmp_socket is only for unix socket. However, after I changed it to Path, I found that I had to immediately convert it to string because the functions consuming it also handle TCP/UDP and cannot take Path. Besides, no Path-related action is performed so there is no benefit to use Path.

What exactly requires conversion into a string? I believe I have this working fine using Paths locally (although to avoid some allocations the code would probably need to be restructured to not return NcOptions).

The benefit of using Path is you don't need to (most likely uselessly) check if the path is valid UTF-8 before converting it into a string.

I know what you mean now. Thank you for the explanation.

Arcterus · 2018-07-16T17:18:56Z

src/networking/nc/mod.rs

+
+        let mut unix_dg_tmp_socket = String::new();
+
+        /* Get name of temporary socket for unix datagram client */


I think most people use // for single-line comments in Rust.

Arcterus · 2018-07-16T17:54:15Z

src/networking/nc/mod.rs

+    opts: &'a NcOptions,
+    poll: Poll,
+    net_interest: Ready,
+    event_stdin: EventedFd<'a>,


Because of UtilSetup, this will need to check setup.input().raw_fd() rather than just using stdin. If the result is None, we can just assume there is no input (it's either something like Vec<u8>, which has no file descriptor, or io::Empty).

Arcterus · 2018-07-16T17:56:35Z

src/networking/nc/mod.rs

+            unix_dg_tmp_socket = if s_addr.is_some() {
+                s_addr.clone().unwrap()
+            } else {
+                let nf = NamedTempFile::new()?;


Does this actually work? I'd assume the temporary file would be dropped and deleted at the end of scope.

Yes. It works.

Arcterus · 2018-07-16T17:57:31Z

src/networking/nc/mod.rs

+        self.netinbufpos >= BUFSIZE
+    }
+
+    fn remove_stdin(&mut self) -> std::io::Result<()> {


Should probably use io::Result<()> instead of std::io::Result<()>.

mssun

There are a lot of space to improve your code quality. Please try to polish it multiple times.

mssun · 2018-07-16T18:08:19Z

build.rs

@@ -31,6 +31,7 @@ fn create_util_map() -> HashMap<&'static str, &'static str> {
    hashmap.insert("head", "posix");
    hashmap.insert("sh", "posix");
    hashmap.insert("sleep", "posix");
+    hashmap.insert("nc", "networking");


I think this line should be put with ping (which is in the "networking section).

mssun · 2018-07-16T18:09:46Z

src/networking/nc/mod.rs

+        let mut portlist = vec!();
+        let lflag = matches.is_present("l");
+        let mut host = String::from("127.0.0.1");
+        let uport:String;


There should be a space after :.

let uport: String;

codecov · 2018-08-06T00:23:12Z

Codecov Report

Merging #28 into master will decrease coverage by 1.98%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master      #28      +/-   ##
==========================================
- Coverage   19.95%   17.96%   -1.99%     
==========================================
  Files          46       47       +1     
  Lines        4680     5198     +518     
  Branches     1081     1274     +193     
==========================================
  Hits          934      934              
- Misses       3355     3873     +518     
  Partials      391      391

Impacted Files	Coverage Δ
libmesabox/src/lib.rs	`59.01% <ø> (ø)`	⬆️
libmesabox/src/networking/nc/mod.rs	`0% <0%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c5674c4...d98eb5c. Read the comment docs.

Fix portrange.

mssun · 2018-09-01T05:06:56Z

I plan to refactor your code. Could you help me to write some integration tests? Thanks.

Arcterus suggested changes Jul 16, 2018

View reviewed changes

mssun reviewed Jul 16, 2018

View reviewed changes

mssun changed the title ~~Add initial implementation for netcat~~ [WIP] Add initial implementation for netcat Jul 18, 2018

rdzhou force-pushed the master branch 2 times, most recently from 0bf5436 to e5f036d Compare July 26, 2018 08:06

rdzhou added 9 commits August 5, 2018 16:36

Add netcat placeholder.

c1ad27e

Add dependencies for nc.

1cb5afc

Implement helper for generating error.

398eace

Remove all warning.

2be7403

Clean code.

8e25124

Add description.

24abf66

Remove sflag.

fb1f561

Fix some minor points according to the comments.

6994b06

Minor fix for UtilSetup.

ee5879c

rdzhou force-pushed the master branch from 5d8e2d0 to ee5879c Compare August 5, 2018 23:43

Fix compilation error.

ea8965a

rdzhou and others added 8 commits August 5, 2018 19:45

Fix integration.

c7325fd

Fix portrange.

Before modifying to generic ncoption.

b2cd093

Fix timeout.

09a92d3

Polish code and bug fix in clap construction

588dc73

Refactor build_ports without regex

70520a0

Delete trailling white spaces and unused comments

7f0e2cf

Add test for build_ports

09b508c

Add create_app to make things clearer

dfe8b0c

networking/nc: wrap long help message

d98eb5c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Add initial implementation for netcat #28

[WIP] Add initial implementation for netcat #28

rdzhou commented Jul 16, 2018

mssun commented Jul 16, 2018

Arcterus left a comment

Arcterus Jul 16, 2018

rdzhou Jul 31, 2018

Arcterus Jul 16, 2018

rdzhou Jul 31, 2018

Arcterus Jul 16, 2018

rdzhou Aug 5, 2018

Arcterus Jul 16, 2018

Arcterus Jul 16, 2018 •

edited

Loading

Arcterus Jul 16, 2018 •

edited

Loading

rdzhou Aug 5, 2018

Arcterus Aug 6, 2018

rdzhou Aug 6, 2018

Arcterus Aug 7, 2018

rdzhou Aug 12, 2018

Arcterus Jul 16, 2018

rdzhou Aug 5, 2018

Arcterus Jul 16, 2018

rdzhou Aug 5, 2018

Arcterus Jul 16, 2018

rdzhou Jul 31, 2018

Arcterus Jul 16, 2018

mssun left a comment

mssun Jul 16, 2018

rdzhou Jul 31, 2018

mssun Jul 16, 2018

rdzhou Jul 31, 2018

codecov bot commented Aug 6, 2018 •

edited

Loading

mssun commented Sep 1, 2018


		let mut unix_dg_tmp_socket = String::new();

		/* Get name of temporary socket for unix datagram client */

[WIP] Add initial implementation for netcat #28

Are you sure you want to change the base?

[WIP] Add initial implementation for netcat #28

Conversation

rdzhou commented Jul 16, 2018

mssun commented Jul 16, 2018

Arcterus 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

Arcterus Jul 16, 2018 • edited Loading

Choose a reason for hiding this comment

Arcterus Jul 16, 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

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

mssun 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

codecov bot commented Aug 6, 2018 • edited Loading

Codecov Report

mssun commented Sep 1, 2018

Arcterus Jul 16, 2018 •

edited

Loading

Arcterus Jul 16, 2018 •

edited

Loading

codecov bot commented Aug 6, 2018 •

edited

Loading