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.

Sign up for GitHub

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

Jump to bottom

Filter for packetcapture service #273

Merged
merged 4 commits into from
Mar 12, 2020
Merged

Filter for packetcapture service #273

merged 4 commits into from
Mar 12, 2020

Conversation

riccardo-rusca
Copy link
Contributor

the filter is tcp dump like (especially the syntax to insert it). In particular from the string of the filter inserted by the user, it will generate the eBPF code which will then be translated into C code to be injected into the dp file

@riccardo-rusca riccardo-rusca requested a review from a team as a code owner February 17, 2020 19:21
frisso
frisso requested changes Feb 18, 2020
View reviewed changes
@frisso
Copy link
Contributor

frisso commented Feb 18, 2020

Riccardo, very good work.
I would suggest to align with the latest master (there has been some changes made recently by Davide here), then take care of the comments. Overall, looks good.
Thanks!

@frisso
Copy link
Contributor

frisso commented Feb 19, 2020

@DavideAG Feel free to add your review.

@riccardo-rusca
Copy link
Contributor Author

I've done some fix according to the comments, are still missing:

  • rst file for filter documentation
  • debug possibility of the filter C code

@DavideAG
Copy link
Contributor

@DavideAG Feel free to add your review.

Thank you, I'm doing it

DavideAG
DavideAG suggested changes Feb 19, 2020
View reviewed changes
Copy link
Contributor

@DavideAG DavideAG left a comment

Choose a reason for hiding this comment

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

Good job Riccardo!
I really like what you did to improve this cube but I have some questions about it:

  • how do you acquire the packet capture timestamp

  • how do you manage the operation of ebpf programs so that you can create different capture configurations (ingress only, egress only, both or none)

@riccardo-rusca
Copy link
Contributor Author

riccardo-rusca commented Feb 21, 2020 •
edited
Loading

I've updated the code with new fixes:

  • I've added a new documentation for describing the filter procedure
  • I've added a debug logger for print the C code of the filter
  • I've fixed the problem with the timestamp as suggested by Davide, and now I take the timestamp in the fast path
  • I've deleted some not needed structure
  • I've corrected an error that arised the first time service start, so I moved the default filter injection in the updatefilter() function and now everything work right

DavideAG
DavideAG suggested changes Feb 22, 2020
View reviewed changes
Copy link
Contributor

@DavideAG DavideAG left a comment

Choose a reason for hiding this comment

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

There are some mistakes, you have to correct them before you go on.

Why did you edit 94 files in this PR?
Maybe you didn't rebase your branch correctly

@frisso
Copy link
Contributor

frisso commented Feb 22, 2020

Riccardo, please rebase your code with master. It looks there are a lot of files that look modified, but that you didn't touch at all. This makes the review (and not only the review) really hard.

@riccardo-rusca riccardo-rusca force-pushed the ric-filter branch 3 times, most recently from 6f81c12 to e1a6a1b Compare February 22, 2020 20:48
@riccardo-rusca riccardo-rusca force-pushed the ric-filter branch 3 times, most recently from 6c12c41 to fd0518d Compare March 3, 2020 16:15
@riccardo-rusca riccardo-rusca force-pushed the ric-filter branch 3 times, most recently from ed4e877 to 864b5aa Compare March 5, 2020 21:28
@riccardo-rusca riccardo-rusca force-pushed the ric-filter branch 4 times, most recently from 6ad9102 to d2c73d3 Compare March 10, 2020 15:36
@frisso frisso self-requested a review March 11, 2020 16:41
frisso
frisso approved these changes Mar 11, 2020
View reviewed changes
Copy link
Contributor

@frisso frisso left a comment

Choose a reason for hiding this comment

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

/lgtm
Thanks, Riccardo!

palexster
palexster approved these changes Mar 11, 2020
View reviewed changes
Copy link
Collaborator

@palexster palexster left a comment

Choose a reason for hiding this comment

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

/lgtm

@frisso frisso merged commit f199ac1 into polycube-network:master Mar 12, 2020
@frisso frisso mentioned this pull request Mar 12, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DavideAG DavideAG DavideAG requested changes

@palexster palexster palexster approved these changes

@frisso frisso frisso approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@riccardo-rusca @frisso @DavideAG @palexster