Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Document troubleshooting of packets using wireshark #4391

Merged
merged 8 commits into from
Jul 17, 2024

Conversation

danieldspx
Copy link
Contributor

Description

Add documentation on how to troubleshoot quic packets using wireshark

Testing

This is not a change to the library itself, just a change in a sample program that uses MsQuic.

Documentation

This change the Diagnostics documentation adding a description on how to troubleshoot packets using wireshark.

@danieldspx danieldspx requested a review from a team as a code owner July 6, 2024 19:02
@danieldspx
Copy link
Contributor Author

@microsoft-github-policy-service agree

Copy link
Member

@nibanks nibanks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thanks for doing this! Just a few nits

docs/Diagnostics.md Outdated Show resolved Hide resolved
src/tools/sample/sample.c Outdated Show resolved Hide resolved
Co-authored-by: Nick Banks <nibanks@microsoft.com>
nibanks
nibanks previously approved these changes Jul 7, 2024
@nibanks nibanks added Area: Documentation external Proposed by non-MSFT labels Jul 7, 2024
@danieldspx
Copy link
Contributor Author

@nibanks The code is failing to compile and I assume this is because the sample is .c and the code a added is C++. Should we turn the code into C like QUIC_TLS_SECRETS ClientSecrets = {0}; and change the WriteSslKeyLogFile to receive a pointer instead of a reference? Or can we just change sample to a .cpp extension? I think the later would be simpler

@nibanks
Copy link
Member

nibanks commented Jul 8, 2024

Please convert to C

docs/Diagnostics.md Outdated Show resolved Hide resolved
docs/Diagnostics.md Outdated Show resolved Hide resolved
@anrossi
Copy link
Contributor

anrossi commented Jul 9, 2024

This looks good. I've made a couple comments on the documentation. Fix those and I'll approve as well.
Thanks for taking the time to make this PR!

@danieldspx
Copy link
Contributor Author

Sorry for the delay on this guys, I'll update them as soon as I have some free time. I plan doing this tomorrow.

@danieldspx
Copy link
Contributor Author

Finally had some free time, I ported the code back to C and tested locally and everything seems to be working just fine. Any changes please let me know. If no changes are required and the pipeline pass we can merge this. Thanks guys.

Also @nibanks it would be a good idea to add the https://microsoft.github.io/msquic/msquicdocs/ website to this repo.

@nibanks
Copy link
Member

nibanks commented Jul 15, 2024

Looks like every Windows build failed:

D:\a\msquic\msquic\src\tools\sample\sample.c(229,36): error C2065: '_SH_DENYNO': undeclared identifier

@nibanks
Copy link
Member

nibanks commented Jul 15, 2024

Looks like every Windows build failed:

D:\a\msquic\msquic\src\tools\sample\sample.c(229,36): error C2065: '_SH_DENYNO': undeclared identifier

I think you need to include share.h.

@danieldspx
Copy link
Contributor Author

Added as required. I cannot test this on my machine since I don't have windows installed but I did the same way other places where including it.

Copy link

codecov bot commented Jul 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.00%. Comparing base (db66ab9) to head (69e0db2).
Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4391      +/-   ##
==========================================
- Coverage   85.02%   85.00%   -0.03%     
==========================================
  Files          56       56              
  Lines       15457    15457              
==========================================
- Hits        13143    13139       -4     
- Misses       2314     2318       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@nibanks nibanks merged commit 45f9bb0 into microsoft:main Jul 17, 2024
472 of 477 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants