Conversation
yehonatanz
left a comment
yehonatanz
left a comment
There was a problem hiding this comment.
Clean and simple :)
However, I'd like to have module level functions (not methods), this would be much more comfortable.
Consult Omer and me about it, since that's bound to be a rather delicate case of singleton, and might require slight modifications after we address the packaging of marine.
shaharyarn
force-pushed
the
feature/expose-wrapper-functions
branch
from
a3f8c75 to
0e4a23f
Compare
tomlegkov
left a comment
tomlegkov
left a comment
There was a problem hiding this comment.
Do we want tests for the API? @yehonatanz
marine/simple_marine.py
Outdated
| import sys | ||
| import os.path | ||
|
|
||
| marine_instance = {} |
There was a problem hiding this comment.
Why is more than one instance needed? I'm pretty sure it actually won't work (only one instance per process)
There was a problem hiding this comment.
Also what is the use case of loading Marine from multiple paths?
There was a problem hiding this comment.
Based on recommendations changed so that marine is loaded from default .so location (DL_LIBRARY_PATH).
shaharyarn
force-pushed
the
feature/macros
branch
from
35f627d to
65cc141
Compare
shaharyarn
force-pushed
the
feature/expose-wrapper-functions
branch
from
89098a5 to
83e39a5
Compare
Static analysis is fine for me in this case, tests seem like a waste of effort. |
shaharyarn
force-pushed
the
feature/expose-wrapper-functions
branch
from
83e39a5 to
5f3d051
Compare
yehonatanz
left a comment
yehonatanz
left a comment
There was a problem hiding this comment.
Looks great, but there are common typing errors (assuming your None pr is merged).
Mostly encap_type and the result dict.
| packet: bytes, | ||
| fields: Optional[List[str]] = None, | ||
| macros: Optional[Dict[str, List[str]]] = None, | ||
| encapsulation_type: int = encap_consts.ENCAP_ETHERNET, |
There was a problem hiding this comment.
Those are changes that are indeed related to the encap-detection PR, which is why the typing changes are made there.
| packet: bytes, | ||
| bpf: Optional[str] = None, | ||
| display_filter: Optional[str] = None, | ||
| encapsulation_type: int = encap_consts.ENCAP_ETHERNET, |
| self, | ||
| packet: bytes, | ||
| fields: Optional[List[str]] = None, | ||
| encapsulation_type: int = encap_consts.ENCAP_ETHERNET, |
| fields: Optional[List[str]] = None, | ||
| encapsulation_type: int = encap_consts.ENCAP_ETHERNET, | ||
| macros: Optional[Dict[str, List[str]]] = None, | ||
| ) -> Dict[str, str]: |
There was a problem hiding this comment.
Dict[str, Optional[str]]
There was a problem hiding this comment.
Those are changes that are related to the empty-fields-type PR so it would be nice if they were noted there as I indeed forgot to switch the typing :)
| fields: Optional[List[str]] = None, | ||
| encapsulation_type: int = encap_consts.ENCAP_ETHERNET, | ||
| macros: Optional[Dict[str, List[str]]] = None, | ||
| ) -> (bool, Dict[str, str]): |
There was a problem hiding this comment.
Dict[str, Optional[str]]
| packet: bytes, | ||
| bpf: Optional[str] = None, | ||
| display_filter: Optional[str] = None, | ||
| encapsulation_type: int = encap_consts.ENCAP_ETHERNET, |
shaharyarn
force-pushed
the
feature/macros
branch
from
796c9ea to
4bcae36
Compare
shaharyarn
force-pushed
the
feature/expose-wrapper-functions
branch
from
5f3d051 to
c811d0d
Compare
3 participants
No description provided.