msantos / pkt

Erlang network protocol library
http://blog.listincomprehension.com/search/label/epcap
BSD 3-Clause "New" or "Revised" License
150 stars 44 forks source link

Modules per protocol #8

Closed msantos closed 10 years ago

msantos commented 10 years ago

@ates @josemic

I've pushed some mechanical changes to split up pkt into separate modules per protocol. @ates is this sort of like what you were thinking of?

https://github.com/msantos/pkt/tree/dev

Comments?

ates commented 10 years ago

Yes, I like it.

I also propose remove the prefix from the internal functions, e.g. tcp_options -> options, ...

Will start working to fix some problems in sctp codec which were found today.

ates commented 10 years ago

I believe tests should be reorganized too in the same way.

msantos commented 10 years ago

Thanks!

josemic commented 10 years ago

I like it. I am using it!

Should "pkt.erl" be the interface to be used by the application or should e.g. pkt_tcp's functions be directly called from application code?

In case pkt is the interface, then some functions are missing in pkt.erl e.g.:

tcp_options(Options) ->
     pkt_tcp:options(Options).

For backward compatibility this might be useful:

link_type(DLT) ->
     pkt:dlt(DLT).
msantos commented 10 years ago

@josemic great, thanks for trying it!

I added a tcp_options/1 at first but it seemed awkward. We would need to add an ipv4_options as well. The other protocols will have their own specific functions too, like pkt:ether_type/1.

I also tried:

options({tcp, Bin}) -> pkt_tcp:options(Bin);
options(#tcp{} = H}) -> pkt_tcp:options(H);
options({ipv4, Bin}) -> pkt_ipv4:options(Bin);
options(#ipv4{} = H) -> pkt_ipv:options(H).

Not sure if that is much better:

pkt:options({tcp, pkt:options(#tcp{opt = <<1,2,3,4,5,6>>})}).

So I left it out until I could think of something. Any ideas?

Sure I can re-add link_type/1. There is a difference in behaviour between dlt/1 and link_type/1:

I modified sniff in epcap to use dlt/1. If I re-add it, should it have the old behaviour?

etnt commented 10 years ago

I would prefer to have one module containing the pkt API.

msantos commented 10 years ago

@etnt agreed, it's just a matter of naming. At the moment, I sort of prefer having a pkt:tcp_options/1, pkt:ipv4_options/1, pkt:ether_type/1 rather than putting it into a tuple like pkt:options({ipv4, Bin})

etnt commented 10 years ago

I also prefer pkt:tcp_options/1.

josemic commented 10 years ago

I also prefer to have "pkt" as interface module.