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

Printing function for VXLAN GPE and NSH #490

Closed
wants to merge 1 commit into from

Conversation

bugyo
Copy link
Contributor

@bugyo bugyo commented Nov 15, 2015

This pull request is the repost of #487.
I squashed the verbose commit logs and rebased on the current master branch.
Would it be acceptable?


I Added verbose output for encapsulated packets with Generic Protocol Extension for VXLAN and Network Service Header.
they are described in following documents.

draft-quinn-vxlan-gpe-04.txt ( https://datatracker.ietf.org/doc/draft-quinn-vxlan-gpe/ )
draft-ietf-sfc-nsh-01.txt ( http://datatracker.ietf.org/doc/draft-ietf-sfc-nsh/ )
I think that researchers and developers related on Service Function Chaining will be happy if this patch is merged.

@infrastation
Copy link
Member

draft-quinn-vxlan-gpe has been replaced with draft-ietf-nvo3-vxlan-gpe, does this code still stand for the latest revision?

@bugyo
Copy link
Contributor Author

bugyo commented Nov 15, 2015

Yes, draft-ietf-nvo3-vxlan-gpe did not change the alignment of the header. Only the latest draft allows MPLS payload. I will commit a supported version.

@bugyo bugyo force-pushed the nsh-o-vxlan-gpe branch 2 times, most recently from f823638 to 2c21490 Compare November 16, 2015 15:05
@bugyo
Copy link
Contributor Author

bugyo commented Nov 16, 2015

OK, now this pull request stands for the latest draft.

@infrastation
Copy link
Member

Thank you, please allow some time for somebody to review in better detail. Meanwhile it would help if you could add a make check test case for it based on a sample packet capture.

@bugyo
Copy link
Contributor Author

bugyo commented Nov 17, 2015

I also think that. I will add a test case and am waiting for some comments. Thank you.

@bugyo
Copy link
Contributor Author

bugyo commented Nov 30, 2015

OK, I added a test case for my code.

@fxlb
Copy link
Member

fxlb commented Dec 14, 2015

Build test: there are some warnings.
See CONTRIBUTING file part 4).

@fxlb
Copy link
Member

fxlb commented Dec 14, 2015

Please update according to f25ed1f.

@bugyo
Copy link
Contributor Author

bugyo commented Dec 23, 2015

OK, I updated the code according to f25ed1f and correct warnings with -Wall -Wmissing-prototypes -Wstrict-prototypes. Thank you.

@bugyo
Copy link
Contributor Author

bugyo commented Dec 26, 2015

This pull request merged a upa's contribution which enable to display MD-type 2 NSH Header.

@fxlb
Copy link
Member

fxlb commented Jan 7, 2016

Thanks, I will review in a few days.

@fxlb
Copy link
Member

fxlb commented Feb 1, 2016

Some comments after a first review:

A. A bound test was missing in print-vxlan.c, added in 20c9007. Also added a #define for more readability. Please update print-vxlan-gpe.c in accordance with this commit.
Same for print-nsh.c.

B. print-nsh.c, line 89: The loop is only for MD Type == 0x01 ?

C. Which code for MD Type == 0x02?

D. print-nsh.c, line 90: before extracting 32 bits you need to check for remaining data with f.e. ND_TCHECK2.

E. The usual way to print the VXLAN-GPE flags is:

static const struct tok vxlan_gpe_flags [] = {
    { 0x08, "I" },
    { 0x04, "P" },
    { 0x01, "O" },
    { 0, NULL }
};

[...]

    ND_PRINT((ndo, "flags [%s], ", bittok2str_nosep(vxlan_gpe_flags,
              "none", flags)));

Same for print-nsh.c.

F. print-nsh.c, line 109 & print-vxlan-gpe.c, line 98: Is it truncated? Not just invalid next protocol?

G. As the output depend on vflag, you should add more test cases with f.e.:

nsh-over-vxlan-gpe nsh-over-vxlan-gpe.pcap nsh-over-vxlan-gpe.out -t
nsh-over-vxlan-gpe-v nsh-over-vxlan-gpe.pcap nsh-over-vxlan-gpe-v.out -t -v
nsh-over-vxlan-gpe-vv nsh-over-vxlan-gpe.pcap nsh-over-vxlan-gpe-vv.out -t -vv
nsh-over-vxlan-gpe-vvv nsh-over-vxlan-gpe.pcap nsh-over-vxlan-gpe-vvv.out -t -vvv

H. print-nsh.c & print-vxlan-gpe.c: Remove space at end of line 5.

I. X < ndo->ndo_vflag tests: We usually test with: ndo->ndo_vflag > X

@bugyo
Copy link
Contributor Author

bugyo commented Feb 1, 2016

First, Thank you for your review.

A. A bound test was missing in print-vxlan.c, added in 20c9007. Also added a #define for more readability. Please update print-vxlan-gpe.c in accordance with this commit.
Same for print-nsh.c.

Sure. I will correct.

B. print-nsh.c, line 89: The loop is only for MD Type == 0x01 ?
C. Which code for MD Type == 0x02?

This code is for both MD Types 0x01 and 0x02. They have the same syntax except for MD Type 0x01 fixes Length field == 0x06. But I noticed that the draft specifies the format for Optional Variable Length Metadata. I will support it.

E. The usual way to print the VXLAN-GPE flags is:

I will correct my code using bittok2str_nosep.

F. print-nsh.c, line 109 & print-vxlan-gpe.c, line 98: Is it truncated? Not just invalid next protocol?

Yes, It just invalid or unknown next protocol.
I will correct my code like print-gre.c.

G. As the output depend on vflag, you should add more test cases with f.e.:

Yes.

H. print-nsh.c & print-vxlan-gpe.c: Remove space at end of line 5.

Yes.

I. X < ndo->ndo_vflag tests: We usually test with: ndo->ndo_vflag > X

OK.

@fxlb fxlb self-assigned this Feb 2, 2016
@bugyo bugyo force-pushed the nsh-o-vxlan-gpe branch 3 times, most recently from 8d0c191 to f585762 Compare February 21, 2016 15:04
and Network Service Header.

This code stands for following internet drafts:

- VXLAN GPE: draft-ietf-nvo3-vxlan-gpe-01
- NSH: draft-ietf-sfc-nsh-01
@bugyo
Copy link
Contributor Author

bugyo commented Feb 21, 2016

I fixed my code according to the comment.

@fxlb
Copy link
Member

fxlb commented Feb 22, 2016

Thanks. Please give me a few days to check once again.

@fxlb
Copy link
Member

fxlb commented Mar 22, 2016

Merged with space editing.
Thanks for your work and patience!

@fxlb fxlb closed this Mar 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants