onvm_pkt_parse_ip fix by dennisafa · Pull Request #104 · sdnfv/openNetVM

dennisafa · 2019-04-17T17:57:01Z

We've been parsing string based IP's in reverse order, this aims to fix that functionality and make modifications to NF's using this function as needed.

Summary:

Previously, we have been converting IP's in string form to IPv4 format in reverse order (IPv4(ip[3], ip[2], ip[1], ip[0]) this PR modifies this to parse in proper format (IPv4(ip[0], ip[1], ip[2], ip[3])

Usage:

This PR includes
Resolves issues
Breaking API changes
Internal API changes	X
Usability improvements
Bug fixes	X
New functionality
New NF/onvm_mgr args
Changes to starting NFs	X
Dependency updates
Web stats updates

Merging notes:

Dependencies:
NF's seperate from nf_router use this functionality. We must assure those are modified to match this ordering of IPv4.

TODO before merging :

PR is ready for review
Find other NF's that use this function and fix

Test Plan:

Tested this with Pktgen with nf_router running. Assured that IP address being sent from Pktgen matched the properly parsed IP address in the route.conf file.

Review:

Sanity checks, assigned to @koolzz @kevindweb
- Run make in /onvm and /examples directories
Code style, assigned to @koolzz @kevindweb
- Run linter
- Check everything style related
Code design, assigned to @koolzz @kevindweb
- Check for memory leaks
- Check that code is reusable
- Code is clean, functions are concise
- Verify that edge cases properly handled
Performance, assigned to @koolzz @kevindweb
- Run Speed Tester NF, report performance.
Documentation, assigned to @koolzz @kevindweb
- Check if the new changes are well documented, in both code and READMEs

onvm · 2019-04-17T17:57:09Z

In response to PR creation

CI Message

Your results will arrive shortly

onvm · 2019-04-17T18:04:03Z

In response to PR creation

CI Message

Run successful see results:
[Results from nimbnode30]
Median TX pps for Speed Tester: 35178613

Linter Failed

examples/aes_decrypt/aes.h:176: #endif line should be "#endif // AES_H" [build/header_guard] [5]
Total errors found: 1
examples/aes_encrypt/aes.h:185: #endif line should be "#endif // AES_H" [build/header_guard] [5]
Total errors found: 1
examples/flow_table/flow_table.h:63: #endif line should be "#endif // FLOW_TABLE_H" [build/header_guard] [5]
Total errors found: 1
examples/flow_table/msgbuf.h:71: #endif line should be "#endif // MSGBUF_H" [build/header_guard] [5]
Total errors found: 1
examples/flow_table/openflow.h:969: #endif line should be "#endif // OPENFLOW_H" [build/header_guard] [5]
examples/flow_table/openflow.h:50: Using deprecated casting style. Use static_cast(...) instead [readability/casting] [4]
examples/flow_table/openflow.h:569: Extra space before ( in function call [whitespace/parens] [4]
examples/flow_table/openflow.h:634: Extra space before ( in function call [whitespace/parens] [4]
examples/flow_table/openflow.h:771: Extra space before ( in function call [whitespace/parens] [4]
examples/flow_table/openflow.h:804: Extra space before ( in function call [whitespace/parens] [4]
examples/flow_table/openflow.h:865: Extra space before ( in function call [whitespace/parens] [4]
examples/flow_table/openflow.h:926: Extra space before ( in function call [whitespace/parens] [4]
Total errors found: 8
examples/flow_table/sdn.c:334: { should almost always be at the end of the previous line [whitespace/braces] [4]
examples/flow_table/sdn.c:365: Should have a space between // and comment [whitespace/comments] [4]
Total errors found: 2
examples/flow_table/sdn.h:104: #endif line should be "#endif // SDN_H" [build/header_guard] [5]
Total errors found: 1
examples/flow_table/sdn_pkt_list.h:122: #endif line should be "#endif // SDN_PKT_LIST_H" [build/header_guard] [5]
Total errors found: 1
examples/flow_table/setupconn.h:53: #endif line should be "#endif // SETUPCONN_H" [build/header_guard] [5]
Total errors found: 1
examples/nf_router/nf_router.c:173: Lines should be <= 120 characters long [whitespace/line_length] [5]
Total errors found: 1
onvm/onvm_mgr/onvm_init.c:107: { should almost always be at the end of the previous line [whitespace/braces] [4]
onvm/onvm_mgr/onvm_init.c:114: { should almost always be at the end of the previous line [whitespace/braces] [4]
onvm/onvm_mgr/onvm_init.c:116: { should almost always be at the end of the previous line [whitespace/braces] [4]
Total errors found: 3
onvm/onvm_mgr/onvm_pkt.c:68: Are you taking an address of a cast? This is dangerous: could be a temp var. Take the address before doing the cast, rather than after [runtime/casting] [4]
Total errors found: 1
onvm/onvm_nflib/onvm_common.h:366: #endif line should be "#endif // ONVM_COMMON_H" [build/header_guard] [5]
Total errors found: 1
onvm/onvm_nflib/onvm_config_common.h:208: #endif line should be "#endif // ONVM_CONFIG_COMMON_H" [build/header_guard] [5]
Total errors found: 1
onvm/onvm_nflib/onvm_msg_common.h:61: #endif line should be "#endif // ONVM_MSG_COMMON_H" [build/header_guard] [5]
Total errors found: 1
onvm/onvm_nflib/onvm_nflib.c:523: Line ends in whitespace. Consider deleting these extra spaces. [whitespace/end_of_line] [4]
Total errors found: 1
onvm/onvm_nflib/onvm_pkt_common.c:98: Are you taking an address of a cast? This is dangerous: could be a temp var. Take the address before doing the cast, rather than after [runtime/casting] [4]
Total errors found: 1
onvm/onvm_nflib/onvm_sc_common.h:70: #endif line should be "#endif // ONVM_SC_COMMON_H" [build/header_guard] [5]
onvm/onvm_nflib/onvm_sc_common.h:70: Should have a space between // and comment [whitespace/comments] [4]
Total errors found: 2
onvm/onvm_nflib/onvm_sc_mgr.h:79: #endif line should be "#endif // ONVM_SC_MGR_H" [build/header_guard] [5]
Total errors found: 1

koolzz · 2019-04-17T18:28:48Z

As briefly discussed don't overwrite the packet data, just use the function when you need to compare.

dennisafa · 2019-04-17T18:29:23Z

As brifly discussed don't overwrite the packet data, just use the function when you need to compare.

Fixed, looking at other NF's doing the comparison and updating accordingly

twood02 · 2019-04-19T00:24:39Z

is your ordering now the same as in this code? https://github.com/FOXNEOAdvancedTechnology/MinimalDPDKExamples/blob/master/minimal_tx/minimal_tx.c#L68

dennisafa · 2019-04-19T00:44:45Z

is your ordering now the same as in this code? https://github.com/FOXNEOAdvancedTechnology/MinimalDPDKExamples/blob/master/minimal_tx/minimal_tx.c#L68

Yes, it is the same. It follows what the IPv4 function is doing here: http://doc.dpdk.org/api/rte__ip_8h.html#a052e2a96de0562a034776fa758255650

koolzz

Tested, works as expected. Although missing a small change. Fix it and I'll merge/

load_balancer line 549 (this address is parsed using the affected onvm_pkt_parse_ip function)
change ip->dst_addr = lb->server[flow_info->dest].d_ip; to ip->dst_addr = rte_cpu_to_be_32(lb->server[flow_info->dest].d_ip;) (we have to send in bid endian)

dennisafa · 2019-04-20T22:21:07Z

Should be good to go

koolzz · 2019-04-20T23:57:01Z

Just in case @onvm

onvm · 2019-04-20T23:57:09Z

Just in case @onvm

CI Message

Your results will arrive shortly

onvm · 2019-04-21T00:04:06Z

Just in case @onvm

CI Message

Run successful see results:
[Results from nimbnode30]
Median TX pps for Speed Tester: 35095158

Linter Failed

examples/aes_decrypt/aes.h:176: #endif line should be "#endif // AES_H" [build/header_guard] [5]
Total errors found: 1
examples/aes_encrypt/aes.h:185: #endif line should be "#endif // AES_H" [build/header_guard] [5]
Total errors found: 1
examples/arp_response/arp_response.c:284: Lines should be <= 120 characters long [whitespace/line_length] [5]
Total errors found: 1
examples/flow_table/flow_table.h:63: #endif line should be "#endif // FLOW_TABLE_H" [build/header_guard] [5]
Total errors found: 1
examples/flow_table/msgbuf.h:71: #endif line should be "#endif // MSGBUF_H" [build/header_guard] [5]
Total errors found: 1
examples/flow_table/openflow.h:969: #endif line should be "#endif // OPENFLOW_H" [build/header_guard] [5]
examples/flow_table/openflow.h:50: Using deprecated casting style. Use static_cast(...) instead [readability/casting] [4]
examples/flow_table/openflow.h:569: Extra space before ( in function call [whitespace/parens] [4]
examples/flow_table/openflow.h:634: Extra space before ( in function call [whitespace/parens] [4]
examples/flow_table/openflow.h:771: Extra space before ( in function call [whitespace/parens] [4]
examples/flow_table/openflow.h:804: Extra space before ( in function call [whitespace/parens] [4]
examples/flow_table/openflow.h:865: Extra space before ( in function call [whitespace/parens] [4]
examples/flow_table/openflow.h:926: Extra space before ( in function call [whitespace/parens] [4]
Total errors found: 8
examples/flow_table/sdn.c:334: { should almost always be at the end of the previous line [whitespace/braces] [4]
examples/flow_table/sdn.c:365: Should have a space between // and comment [whitespace/comments] [4]
Total errors found: 2
examples/flow_table/sdn.h:104: #endif line should be "#endif // SDN_H" [build/header_guard] [5]
Total errors found: 1
examples/flow_table/sdn_pkt_list.h:122: #endif line should be "#endif // SDN_PKT_LIST_H" [build/header_guard] [5]
Total errors found: 1
examples/flow_table/setupconn.h:53: #endif line should be "#endif // SETUPCONN_H" [build/header_guard] [5]
Total errors found: 1
examples/nf_router/nf_router.c:173: Lines should be <= 120 characters long [whitespace/line_length] [5]
Total errors found: 1
onvm/onvm_mgr/onvm_init.c:107: { should almost always be at the end of the previous line [whitespace/braces] [4]
onvm/onvm_mgr/onvm_init.c:114: { should almost always be at the end of the previous line [whitespace/braces] [4]
onvm/onvm_mgr/onvm_init.c:116: { should almost always be at the end of the previous line [whitespace/braces] [4]
Total errors found: 3
onvm/onvm_mgr/onvm_pkt.c:68: Are you taking an address of a cast? This is dangerous: could be a temp var. Take the address before doing the cast, rather than after [runtime/casting] [4]
Total errors found: 1
onvm/onvm_nflib/onvm_common.h:366: #endif line should be "#endif // ONVM_COMMON_H" [build/header_guard] [5]
Total errors found: 1
onvm/onvm_nflib/onvm_config_common.h:208: #endif line should be "#endif // ONVM_CONFIG_COMMON_H" [build/header_guard] [5]
Total errors found: 1
onvm/onvm_nflib/onvm_msg_common.h:61: #endif line should be "#endif // ONVM_MSG_COMMON_H" [build/header_guard] [5]
Total errors found: 1
onvm/onvm_nflib/onvm_nflib.c:523: Line ends in whitespace. Consider deleting these extra spaces. [whitespace/end_of_line] [4]
Total errors found: 1
onvm/onvm_nflib/onvm_pkt_common.c:98: Are you taking an address of a cast? This is dangerous: could be a temp var. Take the address before doing the cast, rather than after [runtime/casting] [4]
Total errors found: 1
onvm/onvm_nflib/onvm_sc_common.h:70: #endif line should be "#endif // ONVM_SC_COMMON_H" [build/header_guard] [5]
onvm/onvm_nflib/onvm_sc_common.h:70: Should have a space between // and comment [whitespace/comments] [4]
Total errors found: 2
onvm/onvm_nflib/onvm_sc_mgr.h:79: #endif line should be "#endif // ONVM_SC_MGR_H" [build/header_guard] [5]
Total errors found: 1

dennisafa added 2 commits April 17, 2019 12:44

Modifying pkt helper file and nf_router

fa55114

Modifying pkt helper file

6f29fe9

dennisafa added 2 commits April 17, 2019 13:26

Style fix

2dbb96c

Style fix

c95287d

dennisafa added 3 commits April 17, 2019 13:34

Reverse ip fix (all Nf's)

dd65f07

load generator fix

69e4ab2

Remove pcap file

5c1f76f

koolzz self-requested a review April 17, 2019 20:17

koolzz added the bug fix 🐛 label Apr 17, 2019

koolzz requested changes Apr 20, 2019

View reviewed changes

Small fix to load balancer

acefe35

koolzz approved these changes Apr 20, 2019

View reviewed changes

koolzz merged commit 9b7efba into sdnfv:develop Apr 21, 2019

dennisafa deleted the pkt_ip_fix branch April 27, 2019 02:51

koolzz added this to the ONVM 19.05 Release milestone May 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

onvm_pkt_parse_ip fix#104

onvm_pkt_parse_ip fix#104
koolzz merged 8 commits intosdnfv:developfrom
dennisafa:pkt_ip_fix

dennisafa commented Apr 17, 2019 •

edited

Loading

Uh oh!

onvm commented Apr 17, 2019

Uh oh!

onvm commented Apr 17, 2019

Uh oh!

koolzz commented Apr 17, 2019 •

edited

Loading

Uh oh!

dennisafa commented Apr 17, 2019

Uh oh!

twood02 commented Apr 19, 2019

Uh oh!

dennisafa commented Apr 19, 2019

Uh oh!

koolzz left a comment •

edited

Loading

Uh oh!

dennisafa commented Apr 20, 2019

Uh oh!

koolzz commented Apr 20, 2019

Uh oh!

onvm commented Apr 20, 2019

Uh oh!

onvm commented Apr 21, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

dennisafa commented Apr 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary:

Merging notes:

Test Plan:

Review:

Uh oh!

onvm commented Apr 17, 2019

CI Message

Uh oh!

onvm commented Apr 17, 2019

CI Message

Linter Failed

Uh oh!

koolzz commented Apr 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dennisafa commented Apr 17, 2019

Uh oh!

twood02 commented Apr 19, 2019

Uh oh!

dennisafa commented Apr 19, 2019

Uh oh!

koolzz left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dennisafa commented Apr 20, 2019

Uh oh!

koolzz commented Apr 20, 2019

Uh oh!

onvm commented Apr 20, 2019

CI Message

Uh oh!

onvm commented Apr 21, 2019

CI Message

Linter Failed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dennisafa commented Apr 17, 2019 •

edited

Loading

koolzz commented Apr 17, 2019 •

edited

Loading

koolzz left a comment •

edited

Loading