rust-pcap / pcap

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

change `PacketStream` codec to public #228

Closed aviramha closed 2 years ago

aviramha commented 2 years ago

Hi, I have a use case where I'm using PacketStream but I want to access the underlying codec which is moved into it. I guess there are two options - make it public or make a inner_mut method for it. I thought making it public would be easier as it's a "user" given struct and implementation, which means that they accessing it directly shouldn't cause any issues. Let me know if there's a better approach here.

Wojtek242 commented 2 years ago

Hi @aviramha! Thanks for the PR. Can you please share some more details about your use case? I'm hesitant to make anything public that wasn't public before, because it's essentially an irreversible decision. Perhaps there is a better approach to your use case? Can you not just make the codec return the packets and use that?

aviramha commented 2 years ago

@Wojtek242

struct ConnectionManager {
    sessions: SessionMap,
    connection_index: ConnectionID,
    ports: HashSet<u16>,
}

pub struct TCPManagerCodec {
    connection_manager: ConnectionManager,
}

impl TCPManagerCodec {
    fn new() -> Self {
        TCPManagerCodec {
            connection_manager: ConnectionManager::new(),
        }
    }

    fn add_ports(&mut self, ports: &[u16]) -> Vec<u16> {
        self.connection_manager.add_ports(ports)
    }
}

impl PacketCodec for TCPManagerCodec {
    type Type = Vec<DaemonMessage>;

    fn decode(&mut self, packet: pcap::Packet) -> Result<Self::Type, pcap::Error> {
        let res = match EthernetPacket::new(packet.data) {
            Some(packet) => self
                .connection_manager
                .handle_packet(&packet)
                .unwrap_or(vec![]),
            _ => vec![],
        };
        Ok(res)
    }
}

I want to call add_ports after passing the codec to the stream. I can't access the codec so it's either making public or adding a new method that returns a reference to the codec. either way is fine, I just thought making it public is okay as the user supplies and codes it anyway..

Stargateur commented 2 years ago

Today I working on some change that affect stream https://github.com/rust-pcap/pcap/pull/229

I think a good way to implement this is what I did for PacketIter:

impl<S: Activated + ?Sized, C> From<PacketStream<S, C>> for (Capture<S>, C) {
  fn from(stream: PacketStream<S, C>) -> Self {
      (stream.inner.into_inner(), stream.codec)
  }
}

I was also think we could have a method as_codec() and as_mut_codec but be aware I expect we will remove codec when we have GaTs

@Wojtek242 said what I going to add in my second message ^^

Wojtek242 commented 2 years ago

This seems more stateful than what a codec was ultimately designed for. The Codec trait assumes it just decodes, not handles the packet. The function next called on the stream object which calls the codec will return the decoded object of type PacketCodec::Type. I think that for your use case you may want to implement a trivial codec that just passes the Packet through and instead operate on the returned value rather than doing it all within the codec.

Wojtek242 commented 2 years ago

See the streamecho.rs example for usage. That example makes a copy in its codec, but if a reference is all you need then perhaps that might work as well.

aviramha commented 2 years ago

Okay, I'll go that route. Thanks!

Stargateur commented 2 years ago

to be clear, in the codec you just allocate and then you can implement whatever you need in the loop https://github.com/rust-pcap/pcap/blob/main/examples/streamecho.rs#L42 here it's a echo but that where you want to implement your logic.