rust-pcap / pcap

Rust language pcap library
Apache License 2.0
620 stars 142 forks source link

Implement pcap_major/minor_version() #235

Closed noselasd closed 2 years ago

noselasd commented 2 years ago

Getting the pcap_major_version()/pcap_minor_version() of a dump file can be handy.

noselasd commented 2 years ago

I'm concern with i32 this function return an int this is not always an i32 this could make rust_pcap not compile on some target. I would prefer at least a cast to i32. Or we return a c_int and let the user handle this.

I would like you add a fn version(&self) ->(i32, i32) that return both. (utils)

Makes sense - yes. Done. And added a cast to i32. I think this should be ok c_int is i32 on all platforms rust knows of today, and even more so for the platforms libpcap can run on.

Stargateur commented 2 years ago

I think you misunderstand, I wanted to add version() not remove the two others method. (version() would simply call the other two)

About the i32 I saw we already have a c_int exposed https://docs.rs/pcap/0.9.2/src/pcap/lib.rs.html#898-901 so add your code should not break already existing workflow. I only found some micro controller that are 16 bits, but I don't think pcap could run on them anyway so I think it's a non problem for now.

Wojtek242 commented 2 years ago

Thanks for the PR @noselasd!

I'm not sure what the problem with i32 is. If c_int has a smaller bitwidth it's safe, it's only a problem if it has more, which it could in principle according to spec. Rust decided to default c_int to i32 (https://doc.rust-lang.org/std/os/raw/type.c_int.html) so I think that's good enough for us.

Once you reintroduce major_version and minor_version and make version call the pcap functions it can be merged.

noselasd commented 2 years ago

I think you misunderstand, I wanted to add version() not remove the two others method. (version() would simply call the other two)

Aye, I didn't quite get that .Added them back 😄

Stargateur commented 2 years ago

I'm not sure what the problem with i32 is. If c_int has a smaller bitwidth it's safe, it's only a problem if it has more, which it could in principle according to spec. Rust decided to default c_int to i32 (doc.rust-lang.org/std/os/raw/type.c_int.html) so I think that's good enough for us.

It's not about safety but about compilation playground:

fn foo(_: u16) {}

fn bar(_: u32) {}

fn main() {
    foo(42u32);
    bar(84u64);
}