Added CLI options with argh and implemented cli capturing

[rtfmkiesel]

As mentioned in #136, it should be possible to operate packetry via the terminal. This PR tries to address that and a bit more :P

A little warning first: I'm a total rust newbie, and the code is probably highly unoptimized and may contain flat-out bad code. The capturing stuff was pieced together by reading ui.rs. I've tested this on Linux and with the newest branch, the one which already has packetry-cli.

New Stuff

• First, I implemented cli arguments using argh. (It was the first simple-looking crate I found)
• The optional argument of a pcap file will still open in the GUI
• The version and test-cynthion commands were ported from your code
• The command devices will search for and list all Cynthions found in a table. For this, the dependency tabled was added. (technically optional)
• The capture command does what it says. For stopping captures with CTRL+C, the dependency ctrlc was added.

Previews

Here are some CLI previews:

$ ./target/debug/packetry --help
Usage: packetry [<filename>] [<command>] [<args>]

packetry - a fast, intuitive USB 2.0 protocol analysis application for use with Cynthion

Positional Arguments:
  filename          recording (pcap) to open in the GUI

Options:
  --help            display usage information

Commands:
  capture           Start packetry in CLI mode
  devices           List capture devices
  version           Print version information
  test-cynthion     Test an attached Cynthion

$ ./target/debug/packetry capture --help
Usage: packetry capture [-d <device>] [-s <speed>] [-o <output>]

Start packetry in CLI mode

Options:
  -d, --device      device (default: auto)
  -s, --speed       usb speed (default: low)
  -o, --output      output file (use '-' for stdout, default: cynthion.pcap)
  --help            display usage information

./target/debug/packetry-cli devices
+----------+------------------+---------+-----+---------+-----------------+
| name     | serial           | useable | bus | address | speeds          |
+----------+------------------+---------+-----+---------+-----------------+
| Cynthion | xxxxxxxxxxxxxxxx | Yes     | 3   | 20      | High, Full, Low |
+----------+------------------+---------+-----+---------+-----------------+

Changes

• I had to make some minimal changes. All println needed to become eprintln as the piping to, for example, Wireshark would not work. (stderr vs. stdout)
• I added some implementations to Speed, which enables me to look up the speed in a nicer fashion from the CLI argument.

[martinling]

Thank you for the PR!

Command line capture is definitely a feature we want to have. I have some issues with the way you've implemented it, but I do think you've done very well to get this working, given that you're both new to Rust and new to the project. I hope that the rest of my feedback manages to be helpful rather than overly discouraging.

The good news is that the code as it stands can be simplified quite a lot.

You're creating a capture database with create_capture, and decoding packets into it, but that's not actually necessary. It would be sufficient to take each packet and timestamp you get from stream_handle, and just feed them directly to the pcap::Writer, without ever constructing a capture read/write pair, or a decoder. There's no need to decode the packets, or to store them anywhere except into the output file, and doing so will just use up extra CPU and temporary storage.

The ctrlc_watchdog thread also isn't necessary. You should be able to move the original stop_handle into the closure that you pass to ctrlc::set_handler, and have that closure call the stop_handle.stop() method to end the capture.

The command line handling changes I'm not too sure about, and should perhaps be a separate PR. We've been using the built-in command line parsing provided by gio::Application for file handling and usage info, and I'm not sure if we want to switch away from that, or if so, whether argh is the library we'd want to choose for that. Right now this is using a mix of both, and that doesn't seem like a great idea.

I'm also a bit wary of accepting both subcommands (packetry capture etc) and filenames (packetry foo.pcap) at the top level, because it makes it ambiguous about whether something is supposed to be a subcommand or a filename (particularly one that doesn't have an extension). Unless we're going to have a specific subcommand for opening files (packetry open filename), I think we may want to stick to the --option approach.

I like the devices table, but the code generating it could use some improvement. Rather than creating a Vec for each field in DeviceInfo, you should be able to just construct a Vec<DeviceInfo> as you go.

One last small thing: the indentation is a bit all over the place in this PR. Please try to stick to the style used in the rest of the project (four spaces per indent).

[rtfmkiesel]

Thanks for the feedback. I really appreciate it.

Regarding the command line parsing, I can totally understand why this needs to be a separate PR, as your system right now works for you. For CLI capturing, however, I need a separate crate (from the beginning) as my code does not create a GTK app for obvious reasons, and your has_argument() only gives out booleans. Doing CLI parsing by hand with, for example, a get_argument() when there are perfectly working crates seems a bit too hacky. Based on rosetta-rs/argparse-rosetta-rs, I choose argh for this. However, I'm open to your suggestions.

Seeing your latest work with packetry-cli, I moved all my parsing to cli.rs.

• packetry works fully independently with the same arguments as before.
• packetry-cli contains all the arguments used for cli capturing and works independently.
  • If packetry-cli open file.pcap is executed, the GUI opens with the code you recently added by forwarding the filename as arg[1].

Please tell me if this is a direction that works for you.

Additionally:

• I've optimized the capturing (only one writer)
• I've removed the watchdog for the CTRL+C handler
• I've optimized the table generation
  • I had to modify the exception list in rust_licenses.py to accommodate for a higher version number
• I changed the indentation to spaces.

Looking forward to your feedback. I'm gonna change this PR to a draft in the meantime.

[rtfmkiesel]

This breaks the version and test-cynthion arguments via packetry-cli on Windows as those arguments only exist in packetry GUI. I could forward all args to the child process if no subcommand is applicable but then the CLI version would not list those as available commands.

I'll look into your suggestion of not using subcommands but rather --option tomorrow. Maybe there is an easy way of not having to maintain arguments in two different files.

[martinling]

This looks a lot better with the changes you've made already - nice!

On the issues about how to structure this going forwards, there's a few reasons behind the way things are done right now that really won't be obvious from just reading the code, so I should probably start by explaining the history a bit.

Backstory

The key thing to understand is that we'd been trying for a while to avoid having more than one binary target in the package. That was specifically due to some limitations of the Rust tooling, as follows.

Previously, we had multiple binary targets. Both test_cynthion.rs and test_replay.rs used to be standalone programs. To share code between main.rs and those additional programs, we had two options. Either:

1. Import the modules used by each program directly in its main file, with e.g. mod foo.
2. Import the modules by multiple programs into a src/lib.rs, make them public, and use that library in each program.

Option 1 turned out not to work well, because it produced a lot of spurious dead_code warnings whenever some code in those common modules wasn't used by whichever binary was currently being built. And we didn't want to just turn off all dead_code warnings, because they're actually very useful.

So we went with option 2 instead. But that required us to have a library crate, with a public API. And we learned that would automatically end up on docs.rs as soon as we made a release, whether we wanted it there or not! That would include a lot of internal things, that are still too in flux for us to commit to a public API for them yet. And we didn't really trust people not to start using that, and demanding support for it, even if we documented it as unstable and unsupported.

So as the deadline approached for the initial 0.1.0 release, I did a big push to get us down to a single binary target and get rid of the unwanted library target:

• Use procspawn crate to run replay test on macOS #100 made test_replay.rs into a normal unit test.
• Remove library target #105 moved test_cynthion.rs into the main binary, and removed the library target.

But that still left us with the problem of Windows, where we couldn't have command-line functionality on a GUI binary. So I wrote #154 to add packetry-cli as a purely standalone wrapper program; one that wouldn't have to share any actual code, but just provided a way to attach a console to the main GUI binary on Windows.

Next steps

The question of how things should be done in packetry and packetry-cli is entangled with those previous issues.

I quite like your approach of importing just mod pcap and mod backend into cli.rs, so that command-line capture can be implemented in that program, and so far at least, that seems to have worked without triggering the dead_code issue.

But invoking the wrapping code in the case of packetry-cli open is the wrong thing to do. We don't need any of that if we just want the GUI to open a file. We can just spawn the GUI without any console in that case, and at that point there's no need to have packetry-cli open filename, the user can just run packetry filename instead.

The case in which we need that wrapping code is where we want to get console output from the GUI binary. And currently that's only necessary when we want to run it with --help, --version or --test-cynthion.

So I think there's two basic options for how to proceed:

1. We recognise those options in packetry-cli, wrap the GUI binary, and forward the necessary arguments.
2. We put the code that implements those options into packetry-cli itself.

In the case of --help and --version, we're going to want those options to exist on both binaries. For Windows users, those options won't be usable on the GUI binary. But the version information is available in the About dialog in the GUI, so that's fine.

In the case of --test-cynthion, I think the aim should ultimately be to move that to packetry-cli. It's console-only functionality, and it's only part of the GUI binary for the historical reasons outlined above. The main issue I see with moving it is that we may run into the dead_code issue again.

If we can successfully move all the console functionality to packetry-cli, we can get rid of the wrapping code entirely.

As far as the argument handling goes, I think it makes sense to split this up, such that we use a Rust-native solution for packetry-cli, but the GUI program continues using the glib/gio argument parsing. The latter is important as it takes care of correctly parsing various location types into a gio::File that can be opened by the application. This already works nicely with loading captures from HTTPS URLs, SMB shares, MTP devices, etc - and we don't want to have to reimplement all that.

I've had a look at the other options listed on that rosetta-rs page, and I agree that argh looks like a good choice for the CLI side of things.

[rtfmkiesel]

Thanks for the backstory. It helped me understand the current problems.

I've removed open and added version to packetry-cli again.

You mentioned you didn't get dead_code issues. Even before my changes, I saw some, so I don't know why you didn't get any. Right now, they are definitely present. If I move test-cynthion to packetry-cli, I get even more, as I have to mod a lot more crates. I don't really know how to go forward here without changing some major stuff.

For me, the state that the CLI version is in works, as I only needed the Wireshark piping for a project, and I'd be more than happy if a developer with more Rust experience would take it from here. I honestly didn't open the PR to be an active developer on this project/feature as I don't really like Rust and don't have the time to like it more. There was just no excuse not to share the code. I hope this is somewhat understandable.

[martinling]

You mentioned you didn't get dead_code issues. Even before my changes, I saw some, so I don't know why you didn't get any. Right now, they are definitely present.

It looks like the latest release of Rust may have fixed the dead_code issue. Release 1.80.0 included a refactor of that lint (and brought with it some bugs, which 1.80.1 was just released to fix).

I was building your branch with 1.80.0, and didn't see any dead_code warnings. I've just tried with 1.79.0 and I get them again.

So it looks like they may have fixed this, such that it now understands the code is in use if any of the targets being built uses it. That would be really great news.

For me, the state that the CLI version is in works, as I only needed the Wireshark piping for a project, and I'd be more than happy if a developer with more Rust experience would take it from here. I honestly didn't open the PR to be an active developer on this project/feature as I don't really like Rust and don't have the time to like it more. There was just no excuse not to share the code. I hope this is somewhat understandable.

That's totally fine. I think you've done some great work on this and we can see about taking it forward from here.

Thank you!