Porting HttpAnalyzer to C++11 #1196

jpcofr · 2023-09-10T15:57:51Z

@seladb This is still work in progress.
Comments
I tested HttpAnalyzer using an small HTTP trace from Packet Life, it seems to work fine.
While I did changes for the std::map, the only correctness tests I performed were to compile and execute a manual test with a small trace. PcapPlusPlus/Tests/Packet++Test/Bin/Packet++Test compiles correctly but fails in my machine.
I believe that I am using a different OS version so I may need even to uplift this project to support my OS version if I want to run the tests.

ERROR: /home/me/PcapPlusPlus/Common++/src/OUILookup.cpp: initOUIDatabaseFromJson:72] Can't open OUI database: No such file or directory
OUILookup                          : FAILED (/home/me/PcapPlusPlus/Tests/Packet++Test/Tests/EthAndArpTests.cpp:16). Assert GREATER THAN failed:
   Actual:   -1
   Expected: 0
EthPacketCreation                  : PASSED
EthPacketPointerCreation           : PASSED
EthAndArpPacketParsing             : FAILED (/home/me/PcapPlusPlus/Tests/Packet++Test/Tests/EthAndArpTests.cpp:92). Assert NOT NULL failed:
   [buffer1] is NULL
ArpPacketCreation                  : FAILED (/home/me/PcapPlusPlus/Tests/Packet++Test/Tests/EthAndArpTests.cpp:140). Assert NOT NULL failed:
   [buffer1] is NULL
EthDot3LayerParsingTest            : FAILED (/home/me/PcapPlusPlus/Tests/Packet++Test/Tests/EthAndArpTests.cpp:154). Assert NOT NULL failed:
   [buffer1] is NULL
EthDot3LayerCreateEditTest         : FAILED (/home/me/PcapPlusPlus/Tests/Packet++Test/Tests/EthAndArpTests.cpp:175). Assert NOT NULL failed:
   [buffer1] is NULL

I need some help setting up the code prettifier to match the project's style.

I still need to check if this commit needs more test coverage.

I noticed in the file that there are many opportunities to use a formatting library like std::fmt or perhaps the std::format library in C++20 (e.g. simplify void printHostnames).

About the documentation
I noticed the following issues in the documentation:

In https://pcapplusplus.github.io/docs/install/macOS ./configure-mac_os_x.sh is not there. Is it still a valid Document?

https://pcapplusplus.github.io/docs/tests says "Run the tests from this directory. The executable is inside the Bin directory:..." What I found is that the examples get built to ./examples_bin/

seladb · 2023-09-11T08:19:12Z

@jpcofr thank you for working on this!

PcapPlusPlus/Tests/Packet++Test/Bin/Packet++Test compiles correctly but fails in my machine.
I believe that I am using a different OS version so I may need even to uplift this project to support my OS version if I want to run the tests.

You can find more details here on how to run the tests, what I think you're missing is run it from Packet++Test directory, like this:

seladb@seladb:~/PcapPlusPlus/Tests/Packet++Test$ Bin/Packet++Test

Let me know if it works now...

I need some help setting up the code prettifier to match the project's style.

Unfortunately there is no code prettifier we're using at the moment (definitely a TODO for us), just make sure you're using the same conventions like tabs and not spaces, { } are always in a new line, etc.

I still need to check if this commit needs more test coverage.

The HTTPAnalyzer already has tests here: https://github.com/seladb/PcapPlusPlus/blob/master/Tests/ExamplesTest/tests/test_httpanalyzer.py These is part of a python test suite that runs most of the examples in different scenarios and tests them. Since this PR shouldn't add functionality, I think once the existing tests pass we should be good to merge

I noticed in the file that there are many opportunities to use a formatting library like std::fmt or perhaps the std::format library in C++20 (e.g. simplify void printHostnames).

I'd like to avoid 3rd party libraries as much as possible, although we might consider using std::fmt in the future. As for C++20 - at this point we try to be compatible with C++11..

In https://pcapplusplus.github.io/docs/install/macOS ./configure-mac_os_x.sh is not there. Is it still a valid Document?

This is the documentation for the latest release 22.11 which has a custom build system. Since then we moved to CMake which is more standard. But we haven't released a new version yet, so if you want to look at the documentation for the next release you should switch from "22.11" to "Next" version:

Here is the link to the page you mentioned specifically: https://pcapplusplus.github.io/docs/next/install/macos

https://pcapplusplus.github.io/docs/tests says "Run the tests from this directory. The executable is inside the Bin directory:..." What I found is that the examples get built to ./examples_bin/

You may be right, let me take a look...

seladb · 2023-09-15T07:43:19Z

@jpcofr please notice CI fails, can you please fix the compilation issues?
https://github.com/seladb/PcapPlusPlus/actions/runs/6189207740?pr=1196

jpcofr · 2023-09-15T08:00:34Z

Right now I cannot prioritise fixing the CI issue. Is it ok if I close the pull request and reopen it when I can spend some time on this PR? @seladb

seladb · 2023-09-15T08:30:59Z

Right now I cannot prioritise fixing the CI issue. Is it ok if I close the pull request and reopen it when I can spend some time on this PR? @seladb

@jpcofr we can keep the PR open for now, if you can find time in the next few weeks maybe you can work on it

codecov · 2023-09-16T11:00:37Z

Codecov Report

Merging #1196 (4dcf51e) into dev (09141bb) will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##              dev    #1196      +/-   ##
==========================================
+ Coverage   82.64%   82.65%   +0.01%     
==========================================
  Files         157      157              
  Lines       20180    20180              
  Branches     7624     7620       -4     
==========================================
+ Hits        16677    16680       +3     
+ Misses       2883     2876       -7     
- Partials      620      624       +4

Flag	Coverage Δ
alpine317	`70.05% <ø> (ø)`
centos7	`73.42% <ø> (ø)`
fedora37	`69.88% <ø> (ø)`
macos-11	`61.29% <ø> (ø)`
macos-12	`61.34% <ø> (ø)`
macos-ventura	`61.32% <ø> (-0.01%)`	⬇️
mingw32	`?`
mingw64	`70.26% <ø> (-0.02%)`	⬇️
npcap	`83.20% <ø> (-0.05%)`	⬇️
ubuntu1804	`73.62% <ø> (ø)`
ubuntu2004	`70.74% <ø> (-0.01%)`	⬇️
ubuntu2204	`69.65% <ø> (+<0.01%)`	⬆️
ubuntu2204-icpx	`59.23% <ø> (+0.04%)`	⬆️
unittest	`82.65% <ø> (+0.01%)`	⬆️
windows-2019	`83.24% <ø> (-0.02%)`	⬇️
windows-2022	`83.25% <ø> (-0.02%)`	⬇️
winpcap	`83.21% <ø> (ø)`
zstd	`73.11% <ø> (-0.04%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 4 files with indirect coverage changes

Examples/HttpAnalyzer/main.cpp

Tests/ExamplesTest/expected_output/httpanalyzer_sanity.txt

Tests/ExamplesTest/expected_output/httpanalyzer_manyprotocols.txt

jpcofr · 2023-09-18T04:52:46Z

@seladb Is it possible to execute all targets without needing to push them to GitHub? It was quite cumbersome to debug all the targets, especially after extended wait times for the CI to complete.

seladb · 2023-09-18T06:52:08Z

@seladb Is it possible to execute all targets without needing to push them to GitHub? It was quite cumbersome to debug all the targets, especially after extended wait times for the CI to complete.

@jpcofr by all targets do you mean all platforms?
I'm not sure there is an easy way to run it on all platforms... however for a specific platform you should be able to build PcapPlusPlus and run all the tests locally

jpcofr · 2023-09-19T16:08:51Z

@seladb Yes, by 'target' I meant platform. I can test locally but I noticed that other platforms in the CI pipeline have more strict (or simply different) code checking styles. It was time consuming to debug for all of these given that the compilation time takes ~20 min.

seladb · 2023-09-21T05:13:35Z

@seladb Yes, by 'target' I meant platform. I can test locally but I noticed that other platforms in the CI pipeline have more strict (or simply different) code checking styles. It was time consuming to debug for all of these given that the compilation time takes ~20 min.

Yes, I agree it's not always easy to write multi-platform code, but being multi-platform is one of the big "selling points" of this project and we get a lot of good feedback about it

Examples/HttpAnalyzer/main.cpp

Tests/ExamplesTest/expected_output/httpanalyzer_sanity.txt

seladb · 2023-09-27T08:10:11Z

@jpcofr something is wrong because this PR contains a lot of changes that were done in previous PRs. Can you rebase your branch on top of dev?

jpcofr · 2023-09-28T06:16:51Z

@jpcofr something is wrong because this PR contains a lot of changes that were done in previous PRs. Can you rebase your branch on top of dev?

I branched from master instead of dev. I rebased the PR onto dev in this PR, still my fork's branch was not rebased. I think it is ok now...

Tests/ExamplesTest/expected_output/httpanalyzer_manyprotocols.txt

Examples/HttpAnalyzer/main.cpp

Other: Updates to the documentation Ported HttpAnalyzer to C++11 Other: Updates to the documentation

... first criterion is 'count', then, the second criterion 'hostname'

the amount of packets

to this branch.

jpcofr · 2023-10-01T13:42:47Z

@jpcofr something is wrong because this PR contains a lot of changes that were done in previous PRs. Can you rebase your branch on top of dev?

Apologies for the mess in this PR, I'm more used to Gerrit and the CI in my 8-to-5 to squash automatically etc. I created a squashed version of the changes in my fork under: dev_port_C11_HttpAnalizer_squashed. If you want I can request a PR for that and drop this one. I do think nothing worthy will be lost because most of the changes are meaningless...

seladb · 2023-10-02T06:50:49Z

@jpcofr something is wrong because this PR contains a lot of changes that were done in previous PRs. Can you rebase your branch on top of dev?

Apologies for the mess in this PR, I'm more used to Gerrit and the CI in my 8-to-5 to squash automatically etc. I created a squashed version of the changes in my fork under: dev_port_C11_HttpAnalizer_squashed. If you want I can request a PR for that and drop this one. I do think nothing worthy will be lost because most of the changes are meaningless...

No worries at all! We can leave this PR as is because we merge PRs using "squash and merge" so these changes will eventually become 1 git commit after the merge

jpcofr force-pushed the dev_port_C11_HttpAnalyzer branch 4 times, most recently from d12f5c3 to c94f695 Compare September 16, 2023 10:54

jpcofr force-pushed the dev_port_C11_HttpAnalyzer branch 4 times, most recently from 4724c40 to 5b4e058 Compare September 17, 2023 17:06

jpcofr commented Sep 18, 2023

View reviewed changes

Examples/HttpAnalyzer/main.cpp Show resolved Hide resolved

jpcofr commented Sep 18, 2023

View reviewed changes

Tests/ExamplesTest/expected_output/httpanalyzer_sanity.txt Outdated Show resolved Hide resolved

jpcofr commented Sep 18, 2023

View reviewed changes

Tests/ExamplesTest/expected_output/httpanalyzer_manyprotocols.txt Outdated Show resolved Hide resolved

jpcofr force-pushed the dev_port_C11_HttpAnalyzer branch 2 times, most recently from 0c5733f to 4c76009 Compare September 18, 2023 05:05

jpcofr marked this pull request as ready for review September 19, 2023 16:06

jpcofr requested a review from seladb as a code owner September 19, 2023 16:06

jpcofr force-pushed the dev_port_C11_HttpAnalyzer branch from 89faa0b to 8da31bf Compare September 19, 2023 16:09

seladb reviewed Sep 21, 2023

View reviewed changes

jpcofr force-pushed the dev_port_C11_HttpAnalyzer branch from e36e2f6 to 3721483 Compare September 27, 2023 17:42

seladb reviewed Sep 30, 2023

View reviewed changes

Tests/ExamplesTest/expected_output/httpanalyzer_manyprotocols.txt Outdated Show resolved Hide resolved

Examples/HttpAnalyzer/main.cpp Outdated Show resolved Hide resolved

Ported HttpAnalyzer to C++11

e8b6c58

Other: Updates to the documentation Ported HttpAnalyzer to C++11 Other: Updates to the documentation

jpcofr force-pushed the dev_port_C11_HttpAnalyzer branch from 89321cd to 2f4de3e Compare October 1, 2023 09:53

jpcofr added 9 commits October 1, 2023 13:34

Ported HttpAnalyzer to C++11

ea2091a

Ported HttpAnalyzer to C++11

9d2f2b5

Ported HttpAnalyzer to C++11

86fa264

Other: Updates to the documentation Ported HttpAnalyzer to C++11 Other: Updates to the documentation

Sorts hostnames in decreasing order...

4c617af

... first criterion is 'count', then, the second criterion 'hostname'

Ported HttpAnalyzer to C++11

23a6fe7

Porting HttpAnalyzer to C++11

eb1f12d

Sorting the method count in descending order

f419320

Ordering the HTTP method stats based on

689f2f8

the amount of packets

Removed unintended changes that didn't exist prior

c71894a

to this branch.

jpcofr force-pushed the dev_port_C11_HttpAnalyzer branch from 2f4de3e to c71894a Compare October 1, 2023 11:35

Revert a change in httpanalyzer_manyprotocols.txt

eebec62

seladb approved these changes Oct 3, 2023

View reviewed changes

Minor fixes

4dcf51e

seladb merged commit b331f3f into seladb:dev Oct 3, 2023
35 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Porting HttpAnalyzer to C++11 #1196

Porting HttpAnalyzer to C++11 #1196

jpcofr commented Sep 10, 2023

seladb commented Sep 11, 2023

seladb commented Sep 15, 2023

jpcofr commented Sep 15, 2023

seladb commented Sep 15, 2023

codecov bot commented Sep 16, 2023 •

edited

Loading

jpcofr commented Sep 18, 2023

seladb commented Sep 18, 2023

jpcofr commented Sep 19, 2023

seladb commented Sep 21, 2023

seladb commented Sep 27, 2023

jpcofr commented Sep 28, 2023

jpcofr commented Oct 1, 2023

seladb commented Oct 2, 2023

Porting HttpAnalyzer to C++11 #1196

Porting HttpAnalyzer to C++11 #1196

Conversation

jpcofr commented Sep 10, 2023

seladb commented Sep 11, 2023

seladb commented Sep 15, 2023

jpcofr commented Sep 15, 2023

seladb commented Sep 15, 2023

codecov bot commented Sep 16, 2023 • edited Loading

Codecov Report

jpcofr commented Sep 18, 2023

seladb commented Sep 18, 2023

jpcofr commented Sep 19, 2023

seladb commented Sep 21, 2023

seladb commented Sep 27, 2023

jpcofr commented Sep 28, 2023

jpcofr commented Oct 1, 2023

seladb commented Oct 2, 2023

codecov bot commented Sep 16, 2023 •

edited

Loading