Add partial implementation of mount by lizhuohua · Pull Request #46 · mesalock-linux/mesabox

lizhuohua · 2018-08-22T02:27:23Z

Most LSB features are supported, except -v (print diagnostic messages), -n (mount without writing in /etc/mtab), and -s (ignore mount options not supported by a file system type). Also, user mountable devices are not supported yet.

Some tests need root privilege, they are marked as #[ignore]. To run them, use the following command: sudo cargo test root_test_mount -- --ignored --test-threads 1. (We have to disable parallel tests, because the tests will mount the same device.)

codecov · 2018-08-22T05:23:11Z

Codecov Report

Merging #46 into master will decrease coverage by 0.93%.
The diff coverage is 2.28%.

@@            Coverage Diff             @@
##           master      #46      +/-   ##
==========================================
- Coverage   19.95%   19.01%   -0.94%     
==========================================
  Files          46       47       +1     
  Lines        4680     5027     +347     
  Branches     1081     1183     +102     
==========================================
+ Hits          934      956      +22     
- Misses       3355     3663     +308     
- Partials      391      408      +17

Impacted Files	Coverage Δ
libmesabox/src/lib.rs	`59.01% <ø> (ø)`	⬆️
libmesabox/src/lsb/mount/mod.rs	`2.28% <2.28%> (ø)`
libmesabox/src/posix/cat.rs	`35.01% <0%> (-4.75%)`	⬇️
libmesabox/src/gnu/arch.rs	`71.42% <0%> (-3.58%)`	⬇️
libmesabox/src/gnu/base32/common.rs	`56.25% <0%> (ø)`	⬆️
libmesabox/src/posix/head.rs	`46.82% <0%> (-0.34%)`	⬇️
libmesabox/src/posix/echo.rs	`51.25% <0%> (+1.25%)`	⬆️
libmesabox/src/posix/sh/ast.rs	`27.52% <0%> (+0.07%)`	⬆️
libmesabox/src/posix/sh/mod.rs	`31.53% <0%> (+0.24%)`	⬆️
... and 8 more

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 17de43b...2f51827. Read the comment docs.

…ring tests

mssun · 2018-09-04T18:50:23Z

Can you refactor your code according to the dummy utility here? #47

Arcterus

I've reviewed like half of this PR. I don't have time to finish the rest right now, so I'm just giving you my current feedback. I'll try to get around to reviewing the rest this week.

Arcterus · 2018-09-18T01:48:22Z

 tar_util = ["libmesabox/tar_util"]
 lsb = [
-    "tar_util"
+    "mount", "tar_util"


Please split into multiple lines.

Arcterus · 2018-09-18T01:49:01Z

 tar_util = ["tar", "globset"]
 lsb = [
-    "tar"
+    "mount", "tar"


Please split into multiple lines.

Arcterus · 2018-09-18T01:49:46Z

+extern crate clap;
+extern crate lazy_static;
+extern crate libc;
+extern crate nix;


Please separate extern crate and use statements by an empty line.

Arcterus · 2018-09-18T01:52:39Z

+}
+
+impl Uuid {
+    fn new() -> MountResult<Self> {


The code for this and Label (and maybe more that I haven't looked at yet) is almost exactly the same. With how minor the differences are, the common code should be split out into a separate function.

Now I combined Label and Uuid together

Arcterus · 2018-09-18T01:53:24Z

+        Ok(Self { path_map: path_map })
+    }
+
+    fn get_device_path(&self, input: &OsString) -> MountResult<&Path> {


Same issue. This is almost exactly the same as for Label.

Arcterus · 2018-09-18T02:05:44Z

+    fn new(path: &str) -> MountResult<Self> {
+        // all of these files should exist and can be read, but just in case
+        let file = fs::File::open(path).or(Err(MountError::OpenFileError(String::from(path))))?;
+        let mut entries = Vec::new();


Use vec![] instead of Vec::new().

Arcterus · 2018-09-18T02:07:40Z

+        let file = fs::File::open(path).or(Err(MountError::OpenFileError(String::from(path))))?;
+        let mut entries = Vec::new();
+        for line in BufReader::new(file).lines() {
+            let line = line.or(Err(MountError::FSDescFileFormatError(String::from(path))))?;


I don't think these files are guaranteed to be valid UTF-8, so I think using Strings here is wrong.

You are right. If these files contain invalid UTF-8, the program will return an error. Now I use read_until to read them in bytes.

Arcterus · 2018-09-18T02:11:40Z

+                }
+                Some(_) => {}
+            }
+            let a: Vec<&str> = line.split_whitespace().collect();


Rather than constructing a Vec, you can just use the iterator from line.split_whitespace() to get the data and if it returns None at some point than clearly you have less than six (6) columns. You can just do .count() after getting the data and see if there are exactly two (2) left columns.

Arcterus · 2018-09-18T02:12:29Z

+        Ok(Self { entries: entries })
+    }
+
+    fn get_output(&self, fs_type: &OsString) -> String {


Again, String seems wrong because the filesystem directory doesn't need to be valid UTF-8.

The returned string is used for output, and it is generated by to_string_lossy. I guess this is the only way to get a printable OsString.

Arcterus · 2018-09-18T02:15:36Z

+
+impl MountOptions {
+    fn from_matches(matches: &clap::ArgMatches) -> MountResult<Self> {
+        let mut mount_list: Vec<Box<Mountable + Send>> = Vec::new();


I would recommend trying to find another way to store the data, as this setup requires dynamic dispatch. If you can't, I suppose it's fine for this utility as this part doesn't need to be super fast.

Now I defined an enum to do the dispatching.

lizhuohua and others added 3 commits August 21, 2018 19:16

mesabox: partially implement mount

34b38c8

lsb/mount: add tests

37611f4

lsb/mount: fix compile error on stable

b6de5b3

lizhuohua added 3 commits August 22, 2018 15:02

lsb/mount: use to_string_lossy method to eliminate invalid Unicode error

1a47d9f

tests/lsb/mount: remove hardcode device image, now it is generated du…

9ac55c2

…ring tests

lsb/mount: use lazy_static to generate option map

eecb068

lizhuohua added 8 commits September 4, 2018 16:27

lsb/mount: fix the order of entries in Cargo.toml

b698881

lsb/mount: refactor code according to the dummy utility

9751f15

lsb/mount: polish code

7db42e1

lsb/mount: use match expression in from_matches()

d970eb4

lsb/mount: turn to use match expressions

df7afcb

lsb/mount: reduce the use of clone

715a4bb

lsb/mount: fix bug and polish code

41c5433

tests/lsb/mount: add tests for remount

2c6a884

Arcterus suggested changes Sep 18, 2018

View reviewed changes

lsb/mount: implement change requests

2f51827

Conversation

lizhuohua commented Aug 22, 2018

Uh oh!

codecov bot commented Aug 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mssun commented Sep 4, 2018

Uh oh!

Arcterus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Aug 22, 2018 •

edited

Loading