Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: redesign arg parsing with structopt #52

Closed
wants to merge 2 commits into from
Closed

Conversation

PaulGrandperrin
Copy link
Member

@drahnr, I played a little bit with how we could redesign the parsing of arguments.
I shuffled most of the code in a module in order to be able to not pull structopt as a dependency to the usage of the crate as a library.
Another solution could be to split the crate in two: honggfuzz-lib and honggfuzz-bin but I'd rather not.

Comment on lines +48 to +49
# requires the `cli` feature, but if it is forgotten, we want to fail compilation instead of silently skipping this binary
# we use conditionnal compilation to throw the error
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I would want to go this way, I'd prefer to split it into two crates and avoid the end user chores.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IDK, maintaining two crates, their synchronization, and making their doc reference each other in a non confusing way seems difficult too.

It would make sense to name the binary crate hfuzz though..

But the feature flag is not so bad, the user will try cargo install honggfuzz, will get a clear error stating that the flag is missing, and try again with cargo install honggfuzz --features=cli.

In the docs, the later command will be directly shown.

input: String,

/// which binary target to fuzz
target: String,
Copy link
Contributor

@drahnr drahnr Apr 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be in favour of changing this to --bin, since target is rather ambiguous with cargo having --target <TRIPLE>... Build for the target triple.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah yes, target is indeed confusing.
however, as of now, this is not an optional argument (a la --bin), but we could rename it as binary and it would be reflected in the help messages.
WDYT?

#[structopt(long)]
no_instr: bool,

/// args to target -- args to fuzzer
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Provides additional arguments to the underlying honggfuzz binary. See [honggfuzz usage](https//..) for details.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right now, the rightmost args given on the command line are forwarded to the fuzzed binary (child).
that's why to give args to the fuzzer (parent), it is currently needed to put them in an ENV var.

To keep that functionality, I wanted to introduce using -- as a way to separate the two kinds of args.

But maybe, the args to the fuzzed binary are not often used, if ever. Maybe it could be sacrificed in order to make things simpler.


/// args to target -- args to fuzzer
/// ( https://github.com/google/honggfuzz/blob/master/docs/USAGE.md )
args: Vec<String>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to have a few additional options here, that provide convenience i.e. --iterations=<N> and --exit-on-crash=<code> - these are very common for CI usage, so shortcuts are a nicety :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand your point, with this new code, those args could be put directly on the command line as you want.

Their documentation is from the upstream honggfuzz binary though...
I'll try to make it easy to access this help from the wrapper.

Maybe something like cargo hfuzz help inner-fuzzer would display the result of honggfuzz -h

Copy link
Contributor

@drahnr drahnr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I very much like the outcome of the structopt outline, I did the same but the structure I ended up with was slightly more complex since I tried to retain most of the initial semantics (i.e. keep the same set of commands).

Regarding adding the feature to avoid splitting the crate into two. I can't quite follow why this is prefered over the crate split, could you please shed some light? Thanks!

@PaulGrandperrin
Copy link
Member Author

PaulGrandperrin commented Apr 30, 2021

I very much like the outcome of the structopt outline, I did the same but the structure I ended up with was slightly more complex since I tried to retain most of the initial semantics (i.e. keep the same set of commands).

I tried too at first but I wasn't satisfied with the result..
I managed to keep the current CLI stable for quite a long time, so I won't feel bad breaking it in order to improve it.

@PaulGrandperrin
Copy link
Member Author

Regarding adding the feature to avoid splitting the crate into two. I can't quite follow why this is prefered over the crate split, could you please shed some light? Thanks!

I already partially answered in a previous comment, but I'm open to the idea and I agree that it would make sense. I just want things to be as easy as possible to the user.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants