Add skeleton reimplementation of sandboxfs in Rust #23

jmmv · 2018-06-04T20:51:11Z

This change adds a barebones reimplementation of sandboxfs in Rust.
As things are now, the new version does nothing: it just provides
trivial command-line processing and mounts an empty FUSE file
system.

However, this change puts all pieces in place to continue development
of this new version. In particular, this adjusts the Travis CI
configuration to support running the exact same integration tests as
we run for the Go implementation against the new code, and currently
blacklists all of them because the new code passes none.

taralx · 2018-06-04T21:17:57Z

.travis.yml

+    # For testing the Rust implementation, we only need to build the tests using
+    # one Go version.
+    - env: DO=rust
+      go: 1.8


Given that stable is 1.10, are you sure you want to pin back to 1.8?

Note that this is an exclusion and it's actually saying "for the Rust build, don't bother using Go 1.8". Thanks Travis for making this so obvious!

I guess I should update the configuration to use 1.10 now, but let's not conflate that change with this review ;-)

taralx · 2018-06-04T21:19:35Z

admin/travis-build.sh

+  # pass all integration tests.  This blacklist keeps track of which those are.
+  # Ideally, by the time this alternative implementation is ready, this list
+  # will be empty and we can then refactor this file to just test one version.
+  local blacklist=(


I feel like there should be an issue tracking the progress on this blacklist that is linked from this comment.

Maybe. But this project (including the previous implementation) is all experimental and under active development. I don't see much value in tracking this in a bug at this point.

taralx · 2018-06-04T21:20:03Z

admin/travis-build.sh

+    TestSignal_QueuedWhileInUse
+  )
+
+  # TODO(jmmv): Replace by a Bazel-based build once the Rust rules are


Link to an open issue in a tracker somewhere?

taralx · 2018-06-04T21:20:47Z

admin/travis-build.sh

+
+  # Compute the list of tests to run by comparing the full list of tests that
+  # we got from the test program and taking out all blacklisted tests.  This is
+  # O(n^2), sure, but it doesn't matter: the list is small and this will go away


You could use associative arrays if you know it's bash.

Unfortunately, macOS's ancient /bin/bash (3.2.57) doesn't support this feature and we do have a Travis build on macOS. (I haven't double-checked if Travis installs a newer bash; maybe it does, but I'm lazy and don't think it's super-important to improve this.)

taralx · 2018-06-04T21:21:48Z

admin/travis-build.sh

+  done
+  set -x
+
+  [ "${#valid[@]}" -gt 0 ] || return 0  # Only run tests if any are valid.


This check is unnecessary. The for loop won't do anything on an empty array.

Yeah... I'm not even sure why I wrote this. I must have had some extra code in the past that evaluated to non-zero and caused set -e to fail the build.

Ah, I remember now. Removed this line and Travis started failing again:

./admin/travis-build.sh: line 179: valid[@]: unbound variable

I've seen bugs in macOS's ancient bash that cause this, but I'm a bit surprised to see that Linux is failing as well.

taralx · 2018-06-04T21:37:34Z

src/main.rs

+
+impl From<io::Error> for MainError {
+    fn from(err: io::Error) -> Self {
+        MainError::Runtime(format!("{}", err))


io::Error should probably be embedded instead of flattened. Consider using withoutboats's failure crate to automate this logic.

Still trying to figure this one out... but because GitHub didn't let me batch replies... writing this reply to avoid confusion.

taralx · 2018-06-04T22:16:46Z

src/main.rs

+/// Obtains the program name from the execution's first argument, or returns a
+/// default if the program name cannot be determined for whatever reason.
+fn program_name(args: &[String], default: &'static str) -> String {
+    args.get(0)


Long chains like this are hard to read. Instead, break out of the monad early and use regular code.

Good. Somehow I thought this was common style in Rust as I've seen it in many places... but glad to see I'm not the only one thinking it's hard to read.

I've replaced this with a match chain... which maybe is also hard?

taralx · 2018-06-04T22:18:07Z

src/main.rs

+        .unwrap_or(default.to_string())
+}
+
+/// Prints program usage information to stdout.


Usage is generally printed to stderr, not stdout.

No. That gets in the way of sandboxfs --help | grep foo, which is very frustrating. Note that I'm only printing usage when explicitly asked to, and when that's the case, the output belongs in stdout because that's what the user asked; it's not an error.

Self-citation: http://julio.meroh.net/2013/08/cli-design-requesting-and-offering-help.html

taralx · 2018-06-04T22:20:10Z

src/main.rs

+            process::exit(1);
+        },
+        Err(MainError::Usage(message)) => {
+            eprintln!("Usage error: {}", message);


I'm not sure this message needs to be prefixed.

taralx · 2018-06-04T22:21:44Z

src/main.rs

+        },
+        Err(MainError::Usage(message)) => {
+            eprintln!("Usage error: {}", message);
+            eprintln!("Type {} --help for more information", program);


Why not just print the usage at this point?

Because I find it incredibly annoying to have a long usage message printed when I didn't ask for one. Such a long message clobbers the actual problem that triggered this because that first line vanishes among the noise.

jmmv · 2018-06-15T21:21:56Z

Alright. I think I have addressed everything. But oh what a mess GitHub is. I can't tell why I couldn't bundle all replies to your commits in one "review step" and instead had to reply to them individually. I'm sure you have received too many notifications...

taralx · 2018-06-16T20:20:11Z

src/lib.rs

+        .collect::<Vec<&OsStr>>();
+    let fs = SandboxFS::new();
+    info!("Mounting file system onto {:?}", mount_point);
+    fuse::mount(fs, &mount_point, &options)


Ugh, this is a bug in their API. They're requesting &P instead of P, but forgot to specify ?Sized. Honestly, they should probably not be requesting &P, since they're indirecting via AsRef.

taralx · 2018-06-16T20:23:47Z

src/main.rs

-        Err(MainError::Usage(message)) => {
-            eprintln!("Usage error: {}", message);
+    if let Err(err) = safe_main(&program, &args[1..]) {
+        if let Some(err) = err.cause().downcast_ref::<UsageError>() {


You can downcast_ref the error directly.

I can't figure out what you mean by this... (and especially not after a month since I dealt with this code).

remove .cause()

I see. Done.

jmmv · 2018-07-25T18:12:58Z

Thanks again for reviewing this and apologies for the long delay in getting back to this change. Have been mostly on vacation since then.

taralx · 2018-07-25T19:44:14Z

src/lib.rs

+        .collect::<Vec<&OsStr>>();
+    let fs = SandboxFS::new();
+    info!("Mounting file system onto {:?}", mount_point);
+    fuse::mount(fs, &mount_point, &options)


Alternatively you could just add ?Sized to their constraint: P: AsRef<Path>+?Sized

This change adds a barebones reimplementation of sandboxfs in Rust. As things are now, the new version does nothing: it just provides trivial command-line processing and mounts an empty FUSE file system. However, this change puts all pieces in place to continue development of this new version. In particular, this adjusts the Travis CI configuration to support running the exact same integration tests as we run for the Go implementation against the new code, and currently blacklists all of them because the new code passes none.

jmmv · 2018-07-31T01:28:45Z

Ping @mhlopko for Bazel project review.

hlopko

Thought I've lgtmed this already.

kevincox · 2018-08-21T15:36:27Z

src/main.rs

+        },
+        None => default,
+    }
+}


Personally I find using the methods on Option much easier to understand: https://rust.godbolt.org/z/sGTXht

You can also return a &str to save an allocation but it probably isn't worth it in this case: https://rust.godbolt.org/z/7wGCfT

kevincox · 2018-08-21T15:39:28Z

src/main.rs

+        &matches.free[0]
+    } else {
+        return Err(Error::from(UsageError { message: "invalid number of arguments".to_string() }));
+    };


Personally I think it would be easier to read with the failure first.

if matches.free.len() != 1 { return Err(...) } let mount_point = &matches.free[0];

kevincox · 2018-08-21T15:40:43Z

src/main.rs

+use std::env;
+use std::path::Path;
+use std::process;
+use std::result::Result;


This is in the prelude and shouldn't have to be imported explicitly.

taralx requested changes Jun 4, 2018

View reviewed changes

jmmv requested a review from hlopko June 15, 2018 21:22

taralx requested changes Jun 16, 2018

View reviewed changes

taralx approved these changes Jul 25, 2018

View reviewed changes

jmmv and others added 4 commits July 26, 2018 11:43

Address review comments

0766585

Address more review comments

a52ced4

Remove cause() call per review comments

c15c30b

jmmv force-pushed the rust branch from 90d4bc4 to 2f5a682 Compare July 26, 2018 15:43

Adjust code and editor to 100 columns per Rust formatting guidelines

dfdfb33

jmmv force-pushed the rust branch from 2f5a682 to dfdfb33 Compare July 26, 2018 16:38

jmmv mentioned this pull request Jul 26, 2018

Add a trivial root directory implementation #24

Merged

hlopko approved these changes Aug 3, 2018

View reviewed changes

jmmv merged commit 0b78fae into bazelbuild:master Aug 3, 2018

jmmv deleted the rust branch August 3, 2018 15:48

kevincox reviewed Aug 21, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add skeleton reimplementation of sandboxfs in Rust #23

Add skeleton reimplementation of sandboxfs in Rust #23

jmmv commented Jun 4, 2018

taralx Jun 4, 2018

jmmv Jun 15, 2018

taralx Jun 4, 2018

jmmv Jun 15, 2018

taralx Jun 4, 2018

jmmv Jun 15, 2018

taralx Jun 4, 2018

jmmv Jun 15, 2018

taralx Jun 4, 2018

jmmv Jun 15, 2018

jmmv Jun 15, 2018

taralx Jun 4, 2018

jmmv Jun 15, 2018

taralx Jun 4, 2018

jmmv Jun 15, 2018

taralx Jun 4, 2018

jmmv Jun 15, 2018

taralx Jun 4, 2018

taralx Jun 4, 2018

jmmv Jun 15, 2018

jmmv commented Jun 15, 2018

taralx Jun 16, 2018

taralx Jun 16, 2018

jmmv Jul 25, 2018

taralx Jul 25, 2018

jmmv Jul 25, 2018

jmmv commented Jul 25, 2018

taralx Jul 25, 2018

jmmv commented Jul 31, 2018

hlopko left a comment

kevincox Aug 21, 2018

kevincox Aug 21, 2018

kevincox Aug 21, 2018

Add skeleton reimplementation of sandboxfs in Rust #23

Add skeleton reimplementation of sandboxfs in Rust #23

Conversation

jmmv commented Jun 4, 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

jmmv commented Jun 15, 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

jmmv commented Jul 25, 2018

Choose a reason for hiding this comment

jmmv commented Jul 31, 2018

hlopko 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