dtn7 / dtn7-rs

Rust implementation of a DTN based on RFC 9171
Other
75 stars 20 forks source link

dtntrigger: Add support for multiple arg command lines #2

Closed mattbradbury closed 4 years ago

mattbradbury commented 4 years ago

Got another small tweak for you.

I wanted to be able to execute shell scripts, and since the scripts themselves aren't executable binaries, you need to call the shell interpreter first.

Modified dtntrigger to allow commands such as:

./dtntrigger --command "/bin/bash ./test.sh test_arg" -e dtn://example/

gh0st42 commented 4 years ago

You don't need to call the shell interpreter first, making your shell script executable (chmod a+x script.sh) such as the one in tests/sink.sh is enough. But you are right, providing command line arguments to the command is not possible currently.

Your solution splits only at a single whitespace ' ', could you please change your pull request to use the split_whitespace function? Then it should even work if a user provides multiple whitespaces or tabs between parameters.

Also, what was the reason to replace the ? behind output? Passing the execution error upwards has the same effect and seems clearer so I'd prefer less code.

Why an unwrap after the next call? A ? would be cleaner, shorter and easier to read.

mattbradbury commented 4 years ago

You don't need to call the shell interpreter first, making your shell script executable (chmod a+x script.sh) such as the one in tests/sink.sh is enough. But you are right, providing command line arguments to the command is not possible currently.

Ha!! I did all this because I thought it only wanted to call binaries! I mistyped my test.sh as: #/bin/bash instead of #!/bin/bash. Of course it worked from the command line since I was running bash.

Your solution splits only at a single whitespace ' ', could you please change your pull request to use the split_whitespace function? Then it should even work if a user provides multiple whitespaces or tabs between parameters.

Fixed.

Also, what was the reason to replace the ? behind output? Passing the execution error upwards has the same effect and seems clearer so I'd prefer less code.

Here's the errors before unwrapping: (malformed test.sh) Encountered an error: other os error (bad path to test.sh) Encountered an error: entity not found

After: (malformed test.sh) Error executing command: Exec format error (os error 8) (bad path to test.sh) Error executing command: No such file or directory (os error 2)

I originally wrote the match to figure out why my shell script wasn't working correctly. I was thinking it needed absolute paths maybe, or needed to be a binary. I needed a clearer understanding of what was broken than what I was receiving.

Why an unwrap after the next call? A ? would be cleaner, shorter and easier to read.

I get this compile error when I try to do that. the trait std::convert::From<std::option::NoneError> is not implemented for ws::result::Error

I've added a check to clap to check for empty string.

If you can figure out the Result inter dependencies to throw just one Result type from several different commands, I'd love to see how you do it.

gh0st42 commented 4 years ago

Regarding the error and ws interop that is a problem. For a library, I'd write a custom error that implements or derives the correct from implementations and use my own error handling code or maybe map the error to something meaningful. But for this little helper, I guess it's okay to go this way and sometimes handle the errors directly as you suggested.

A final change would be nice to avoid the match arms by just using unwrap_or_else to handle the error case directly. A suggestion of this is attached should be attached to your PR.

Thank you!

mattbradbury commented 4 years ago

Still getting my head around all the idiomatic Rustisms. Thanks for the feedback.

match removed, unwrap_or_else is in now.

gh0st42 commented 4 years ago

thanks for your work!