Conversation
|
cc @matttbe |
|
@mozillazg wow, thank you very much for this big work! I didn't realise there was so much work to do on the It looks good to me. I will try to run it locally with various MPTCP traffic. One last thing: I noticed you added a commit in the |
|
I just tried, using the binary from: https://github.com/mozillazg/ptcpdump/actions/runs/11082221375 I had this error: (note that I'm running the tests from a small VM where there is no Docker, etc.) FYI, here is the trace presented by TCPDump: |
|
@matttbe Thanks for your review and test, I'll try running the same testing VM to fix the panic. |
Thanks! FYI, the You can place With the
|
|
@matttbe I believe the panic is fixed. you can download the latest binary from https://github.com/mozillazg/ptcpdump/actions/runs/11102065291. I tested with using ptcpdump to read all pcap files were generated via run packetdrill tests and
I'll support this case in the feature. #153
I'll add an new option to allow skip these checks in the feature. #58
Yes, ptcpdump doesn't support this case yet. It'll be supported in the feature. #154 |
|
@mozillazg thank you for the update! I had a quick look: no more crashes, thanks! I also took a bit more time to look at differences with TCPDump, and I noticed two issues (packets carrying data display a wrong DSS ( Here is an example when running mptcp_join.sh's ptcpdump: tcpdump (with If we do a "word diff" after having removed the headers (e.g. here), we can easily see the issue I mentioned, and a few more details:
Also, when running other subtests from
|
|
Thank you for having fixed the crash and opened the new tickets :) |
|
@matttbe Thanks for your review and testing! I'll fix the display related issues. |
|
Thank you! :) |
this issue is fixed and it was caused by gopacket: mozillazg/gopacket@2e5a32c
it is fixed.
These issues are fixed.
OK, I may improve it in the feature. |
Good catch! I just updated the upstream PR: gopacket/gopacket#66 I added you as a co-author, and I also fixed another similar issue with MP_TCPRST, see here. (The U flag should be set to 0 anyway)
Thanks! I confirm that!
Great, thanks! It looks excellent now!
Sure, I'm fine with or without. Probably easier to keep it. |
FYI. I updated it again to match the latest tcpdump: https://github.com/the-tcpdump-group/tcpdump/blob/master/tests/mptcp-fclose.out#L3-L5 Another mptcp related bug from gopacket was found: mozillazg/gopacket@112c3fb |
|
Thank you for the recent modifications!
Good catch! I just updated the gopacket PR with the same fix. |
Closes #148