radareorg / r2pipe.rs

Rust crate for r2pipe
Other
45 stars 19 forks source link

provide a proper default for R2PipeSpawnOptions #58

Closed jcreekmore closed 1 year ago

jcreekmore commented 1 year ago

The derived Default implementation for R2PipeSpawnOptions created an empty args and an empty exepath. That is almost certainly not what was intended since the example that showed its use looked like this:

let opts = R2PipeSpawnOptions {
        exepath: "radare2".to_owned(),
        ..Default::default()
    };

implying that the Default implementation would provide a reasonable default and that you could just override the options that you wanted to override. However, if you tried to do something like the following:

let opts = R2PipeSpawnOptions {
        args: vec!["-2"],
        ..Default::default()
    };

to just disable the warnings spat out on stderr, R2Pipe would fail to spawn since the exepath was empty. This is the case since the spawn function would default initialize the exepath if you didn't provide any R2PipeSpawnOptions by passing None, but simply took what you passed in if you passed Some(R2PipeSpawnOptions)

By providing a proper Default implementation, this allows the previous method of overriding the args for the options without having to also override the exepath. It also simplifies the handling when you do not provide any options.

Checklist

Description

meme commented 1 year ago

Thanks for catching this and providing an in-depth explanation, I agree this is an oversight. Looks good.