Support Multiple Comments in Packet Object #4798
Conversation
…riter classes for back compatibility
|
Hi @gpotter2 , this is my first contribution to the repo. Could you please review this PR? Let me know if there's anything I should fix. Thanks! |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4798 +/- ##
==========================================
+ Coverage 80.08% 81.03% +0.95%
==========================================
Files 365 365
Lines 89027 89052 +25
==========================================
+ Hits 71296 72166 +870
+ Misses 17731 16886 -845
🚀 New features to boost your workflow:
|
|
Hi again @gpotter2, just following up on this PR. We have a need for this feature in our project, so I’d really appreciate a review when you have the time. Please let me know if any changes are needed. Thanks in advance! |
scapy/utils.py
Outdated
|
|
||
| def __init__(self, filename, fdesc=None, magic=None): # type: ignore | ||
| # type: (str, IO[bytes], bytes) -> None | ||
| def __init__(self, filename, fdesc=None, magic=None, comments=None): # type: ignore |
There was a problem hiding this comment.
This looks weird. I don't understand why you would be passing comments to the reader?
There was a problem hiding this comment.
This was likely my oversight — it’s not actually necessary. I’ll go ahead and remove it.
scapy/utils.py
Outdated
| @@ -1802,7 +1803,12 @@ def _read_options(self, options): | |||
| "%d !" % len(options)) | |||
| raise EOFError | |||
| if code != 0 and 4 + length <= len(options): | |||
| opts[code] = options[4:4 + length] | |||
| if code in [1, 2988, 2989, 19372, 19373]: | |||
There was a problem hiding this comment.
These are option codes defined in the pcapng file format docs, and it's possible for multiple codes to be present.
There was a problem hiding this comment.
Got it. Could you add the link you just sent as a comment? Thanks.
There was a problem hiding this comment.
Added it as a comment just now 👍
scapy/utils.py
Outdated
| p.comment = comment | ||
| p.comments = comments | ||
| if p.comments is None: | ||
| p.comment = comment |
There was a problem hiding this comment.
I don't get this. I would expect to only set comments (?)
There was a problem hiding this comment.
This part is included for backward compatibility. If comments is None, we assign comment = comment to ensure existing usage doesn't break.
There was a problem hiding this comment.
If I understand it well you're already using a property for comment to take the last value of comments. this should already take care of compatibility, so I don't understand why it's necessary to put the two.
There was a problem hiding this comment.
You're right. this isn't necessary anymore, so I'm removing it. Thanks for pointing it out!
|
Thanks for the PR and sorry for the delay of review. I have a few questions regarding the code. |
Let me know if there's anything else I should take care of before this can be merged. Appreciate your time! |
scapy/utils.py
Outdated
| @@ -2211,7 +2225,8 @@ def write_packet(self, | |||
| comment=comment, | |||
scapy/utils.py
Outdated
| @@ -2194,6 +2207,7 @@ def write_packet(self, | |||
| wirelen = caplen | |||
|
|
|||
| comment = getattr(packet, "comment", None) | |||
scapy/utils.py
Outdated
| @@ -2366,6 +2381,7 @@ def _write_packet(self, | |||
| comment=None, # type: Optional[bytes] | |||
scapy/utils.py
Outdated
| @@ -2600,6 +2617,7 @@ def _write_packet(self, # type: ignore | |||
| comment=None, # type: Optional[bytes] | |||
|
I also personally would have kept comments in the same argument index as comment, rather than adding it at the end. |
That's a fair point, and I understand the value of maintaining positional consistency. In this case, since we changed both the name and the type (comment: |
The pcapng format specification explicitly allows for multiple comments to be associated with a packet via repeated opt_comment fields. This change brings Scapy closer to that standard, making it easier to work with multi-comment pcapng traces and enabling richer annotations in packet analysis.
Checklist:
cd test && ./run_testsortox)