rusticata / pcap-parser

PCAP/PCAPNG file format parser written in pure Rust. Fast, zero-copy, safe.
Other
104 stars 24 forks source link

Don't make trait PcapReadIterator generic over R #10

Closed asdfuser closed 3 years ago

asdfuser commented 3 years ago

R is not used in PcapReadIterator and having the trait generic over R complicates using the Boxes returned from create_reader()

Removing R makes something like this possible:

let file = File::open(path)?;
let mut reader = match pcap_parser::create_reader(4096, file) {
    Ok(reader) => reader,
    Err(PcapError::HeaderNotRecognized) => {
        // Try gzip
    let file = File::open(path)?;
        let mut gz = GzDecoder::new(file);
        pcap_parser::create_reader(4096, gz)?
    },
    Err(e) => bail!(e),
};

I think this would be a breaking change and I'm not sure if I missed any use case that needs this parameter. cargo build && cargo test run fine and I'm able to read pcap, pcapng and gzipped pcaps now.

Thanks for your work on pcap-parser and let me know if I should change anything.

chifflier commented 3 years ago

Hi,

After looking again at PcapReaderIterator, I also have the feeling the generic parameter is unnecessary, and can't remember of any specific reason for adding it.

That said, I'm also wondering if the creater_reader function should return a Box. That seems complex, I think it should return something like impl PcapReaderIterator and let the caller decide if it should use a Box or not.

I'm running more tests, but will merge the PR soon it it works. Thanks for raising the point, and the PR!

Note that the current code did not prevent prevent the use case you mentioned. For an example, see this code from pcap-analyzer (the reader being used here)

chifflier commented 3 years ago

Merged, thanks! (I edited the commit for rustfmt, so github did not detect the merge automatically)

It did not break my crates based on pcap-parser, however as this is an API change, I'll ship it in 0.11.0

asdfuser commented 3 years ago

Great, thanks! Looking forward to 0.11

And you're totally right, I didn't think think about using a trait object as the generic parameter. I tried returning impl PcapReaderIterator, but I don't think this will work here since the return type is decided at runtime and not at compile time.

chifflier commented 3 years ago

0.11 is a bit delayed for now, I am working on a rewrite of macro-based parsers to functions, which lead me to some refactoring (and also some gain in performances that I'm evaluating to keep code readable). It should be a matter of days, unless uncovering new problems.

About the function return type, I tried several things but couldn't get a satisfying result. Indeed, the impl PcapReaderIterator cannot be used directly since the return type is not static, so I'll leave it for now.

chifflier commented 3 years ago

0.11 has just been uploaded to crates.io, including this fix and several other changes and improvements