rust-pcap / pcap

Rust language pcap library
Apache License 2.0
595 stars 138 forks source link

Change ToRawFd impl from Active to Activate #288

Closed robs-zeynet closed 1 year ago

robs-zeynet commented 1 year ago

The underlying file descriptor in an Capture can be converted to a raw fd, but so can one from an offline Capture, so change the type declaration to reflect that.

I think this is a simple, safe and correct change but feedback welcome.

robs-zeynet commented 1 year ago

Miss read the documentation and this doesn't work :-(

robs-zeynet commented 1 year ago

It's really annoying that this doesn't work :-(

#include <stdio.h>
#include <stdlib.h>
#include <pcap.h>

#ifndef BUFLEN 
#define BUFLEN 4096
#endif

int main(int argc, char* argv[]) {
        char errbuf[BUFLEN];

        if (argc != 2) {
                fprintf(stderr, "Need filname\n");
                exit(1);
        }
        pcap_t * pcap = pcap_open_offline(argv[1], errbuf);
        printf("Pcap opened: %d and pcap_fileno() = %d \n", pcap, pcap_fileno(pcap));
        return 0;
}

[robs@fedora pcap]$ ./a.out out.pcap Pcap opened: 33778064 and pcap_fileno() = -1

Stargateur commented 1 year ago

what would be your use case ?

robs-zeynet commented 1 year ago

I use offline pcap files for testing, so imagine I have a function I'd like to test:

fn count_syns<T: pcap::Activated + ?Sized)(capture: Capture<T>) -> usize {
    let mut count = 0; 
    while let Some(pkt) = capture.next() {
        //  count +1 if syn
     }
    count
}

Then I can run it 'for real' with a live capture or test it with an offline capture

assert_eq!( count_syns( Capture::from_file("test.pcap")), 24)

Recently, I've needed to all a call to poll() in my code, which would/should work perfectly with offline captures as there is an underlying file descriptor that could be poll()'d and should always appear ready for reading, but the underlying pcap libraries just return -1 for pcap_fileno() for an offline capture rather than the actual file descriptor... which means I have to rewrite a bunch of code :-(

If anyone has a theory about how to work around this, I'd love to hear it, but in the mean time my testing abstraction just appears broken :-(

Stargateur commented 1 year ago

poll is obsolete, epoll is way better (if you use trigger mode), anyway, it's perfectly possible a disk file is not available immediately, thus with modern IO that very rare.

You could try to open the file yourself:

use std::fs::File;
use std::os::fd::{AsRawFd};

fn main() -> std::io::Result<()> {
    let mut toto = File::open("toto.pcap")?;
    let raw_fd = f.as_raw_fd();
    let toto = pcap::from_raw_fd(raw_fd)?;
}
robs-zeynet commented 1 year ago

I think you're missing (multiple) points.

1) I'm actually using the polling crate (https://docs.rs/polling/latest/polling/) which runs the best select/poll/epoll/kqueue/etc. for each OS. Certainly on Linux epoll() is better than poll(), but pcap compiles for a lot of platforms and to say that "poll is obsolete" trivially can't be true on operating systems that don't support epoll().

2) The entire point of my design was the production workflows (with an active capture) and the test workflows (with an offline capture) follow the exact same code paths. This has lots of benefits in terms of ease and completeness of testing, but this issue around as_raw_fd() breaks that. Now I have to have a bunch of places in my code where it says:

if capture is Capture<Active> {
   // do a production thing
}  else {
  // do a testing thing
}

Which is a bunch of extra code and creates an opportunity for bugs. All I wanted was for pcap to return the underlying file descriptor in the file read for an offline capture and then it could participate in the poll/epoll/etc. correctly.

Does that make sense?