dpdk - fix checking if any packet has actually been parsed

[TheSableCZ]

No description provided.

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 40.61%. Comparing base (ebb8d77) to head (bc2024b).
Report is 6 commits behind head on master.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #225      +/-   ##
==========================================
+ Coverage   40.55%   40.61%   +0.05%     
==========================================
  Files         104      104              
  Lines       10110    10120      +10     
  Branches     1493     1492       -1     
==========================================
+ Hits         4100     4110      +10     
  Misses       5145     5145              
  Partials      865      865              

Flag	Coverage Δ
tests	40.61% <ø> (+0.05%)	⬆️

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

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

[cejkato2]

This solution is not correct.

Let's analyze it, dpdk-ring for simplicity:

1. we create opt with PacketBlock, I expect the opt.pblock->cnt is analyzed to 0
2. loop L200 to L209 iterates over i, which is not used inside parse_packet
3. let's assume we received 10 packets -> pkts_read_ = 10
4. let's assume packet 0. is unknown not parsed
5. parse_packet() uses opt->pblock->cnt (probably initialized to 0) as index to the array opt->pblock->pkts
6. for packet 0 we can return, e.g., L725 or L737 (parser.cpp) but nothing changes opt->pblock->cnt (increment is L775)
7. for the next packets until pkts_read_ we are still on the same packet and nothing is processed - we loose everything -> this is not expected behavior and this fix cannot solve it.
   It is worth noting that if any packet index > 0 and < pkts_read_ is unknown/invalid and parsers returns without incrementing opt->pblock->cnt we get Result::PARSED from return opt.pblock->cnt ? Result::PARSED : Result::NOT_PARSED; L214, but any packet after the unknown one is skipped (because opt->pblock->cnt is not incremented).

Is it ensured somewhere, that opt->packet_valid is initialized to false? Is it used somewhere to distinguish theparsed and not_parsed individual packets? (the packetblock may contain both valid and invalid)

[Lukas955]

This solution is not correct.

Let's analyze it, dpdk-ring for simplicity:

1. we create opt with PacketBlock, I expect the opt.pblock->cnt is analyzed to 0
2. loop L200 to L209 iterates over i, which is not used inside parse_packet
3. let's assume we received 10 packets -> pkts_read_ = 10
4. let's assume packet 0. is unknown not parsed
5. parse_packet() uses opt->pblock->cnt (probably initialized to 0) as index to the array opt->pblock->pkts
6. for packet 0 we can return, e.g., L725 or L737 (parser.cpp) but nothing changes opt->pblock->cnt (increment is L775)
7. for the next packets until pkts_read_ we are still on the same packet and nothing is processed - we loose everything -> this is not expected behavior and this fix cannot solve it.
   It is worth noting that if any packet index > 0 and < pkts_read_ is unknown/invalid and parsers returns without incrementing opt->pblock->cnt we get Result::PARSED from return opt.pblock->cnt ? Result::PARSED : Result::NOT_PARSED; L214, but any packet after the unknown one is skipped (because opt->pblock->cnt is not incremented).

Is it ensured somewhere, that opt->packet_valid is initialized to false? Is it used somewhere to distinguish theparsed and not_parsed individual packets? (the packetblock may contain both valid and invalid)

I think the solution in this pull request IS correct.

ad 1) pblock->cnt is initialized to zero here (workers.cpp:61)
ad 6) Not incrementing counter is correct behaviour. This ensures that packets that cannot be parsed by the parser, are ignored.
ad 7) You interpretation is not correct. The next packet in the dpdk-ring loop is passed to the parser and if it is successfully parsed, the counter will be incremented here. Keep on mind that PacketBlock filled by the parser is different from burst received by the input module. In other words, only successfully parsered packets are placed in the PacketBlock.

opt->packet_valid is something old and used only by PCAP input module.

[cejkato2]

Perfect teamwork! :-)

Ok, I found the most confusing that other parse functions had doxygen comment and their input parameters are the first ones. Contrary, parse_packet() unfortunately has the first parameter opt as output and the doxygen was missing at all...
I created some draft of the doc, maybe it will help with the future investigation. Anyway, this part of the source codes is a candidate for refactoring...

Additionally, I've created an update for the new package release and I'm waiting for a test build on COPR in @CESNET/NEMEA-testing.