Skip to content

LCM Defragmentation Support - #449

Merged
gconnell merged 3 commits into
google:masterfrom
gustafj:lcm_defrag_support
Jun 4, 2018
Merged

LCM Defragmentation Support#449
gconnell merged 3 commits into
google:masterfrom
gustafj:lcm_defrag_support

Conversation

@donkahlero

@donkahlero donkahlero commented May 8, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

Hello again!

This patch includes support for defragmentation of LCM packages, as specified in the LCM protocol description (https://lcm-proj.github.io/udp_multicast_protocol.html).

As my PR before, this patch is intended to be used in combination with an open PR (lcm-proj/lcm#220) towards the GitHub LCM repository that adds golang + gopacket support.

As you can see, I put the defragmenter into a package called defrag. My suggestion is that the IPv4 defragmenter is also moved there as well and is renamed to ipv4defrag.go (and ipv4defrag_test.go respectively). IMHO this would be a bit more cleaner in case new defragmenters should come in. If you agree, I would open another PR for that.

Thanks for your openness towards new protocols 👍 😎

@donkahlero

donkahlero commented Jun 1, 2018

Copy link
Copy Markdown
Contributor Author

Hello @gconnell!
I just wanted to kindly ask, if we somehow can get this in? :) Right now, we are using this already in production and it works fine :) But obviously, we do not want to have the dependency to my personal clone, but rather to this repository. Thanks!

@gconnell

gconnell commented Jun 1, 2018

Copy link
Copy Markdown
Contributor

Hey, Taco, sorry for the slow response on this; review just slipped my mind.

The biggest sticking point for me right now is the defrag subdirectory... what would you think of putting this code into something like defrag/lcmdefrag, so we don't have all the future defrag stuff in a single directory and run into a problem like we have now with layers being a catch-all?

@donkahlero

donkahlero commented Jun 1, 2018

Copy link
Copy Markdown
Contributor Author

No problem!

Yeah, that sounds like a good idea. I guess that this would then also apply for the ip4defrag packet (which IMHO should be under defrag as well)?

Let me amend on Monday! Have a nice weekend!

@gconnell

gconnell commented Jun 1, 2018

Copy link
Copy Markdown
Contributor

yep, sounds good. let's not do the ip4defrag switch as part of this pull (we'll want to do some silly type aliasing and stuff to not break compatibility), but I think long-term it's a fine idea.

@donkahlero

donkahlero commented Jun 1, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Yeah, I wasn't talking about this PR. Should definetely be a separate one!

donkahlero added a commit to gustafj/gopacket that referenced this pull request Jun 4, 2018
@donkahlero
Moved Defragmenter to New Folder Structure
1d07ce5 
As discussed in google#449, the
defragmenter was moved to a seperate package that will only contain the
LCM defragmenter. This is done in order to enable a more clean package
setup.
@donkahlero
donkahlero force-pushed the lcm_defrag_support branch from 1d07ce5 to 302008a Compare June 4, 2018 05:37
donkahlero added a commit to gustafj/gopacket that referenced this pull request Jun 4, 2018
@donkahlero
Moved Defragmenter to New Folder Structure
302008a 
As discussed in google#449, the
defragmenter was moved to a seperate package that will only contain the
LCM defragmenter. This is done in order to enable a more clean package
setup.
@donkahlero
donkahlero force-pushed the lcm_defrag_support branch from 302008a to 0f6fc69 Compare June 4, 2018 05:41
donkahlero added a commit to gustafj/gopacket that referenced this pull request Jun 4, 2018
@donkahlero
Moved Defragmenter to New Folder Structure
0f6fc69 
As discussed in google#449, the
defragmenter was moved to a seperate package that will only contain the
LCM defragmenter. This is done in order to enable a more clean package
setup.
@donkahlero
donkahlero force-pushed the lcm_defrag_support branch from 0f6fc69 to 49082dd Compare June 4, 2018 07:06
donkahlero added a commit to gustafj/gopacket that referenced this pull request Jun 4, 2018
@donkahlero
Moved Defragmenter to New Folder Structure
49082dd 
As discussed in google#449, the
defragmenter was moved to a seperate package that will only contain the
LCM defragmenter. This is done in order to enable a more clean package
setup.
@googlebot

googlebot commented Jun 4, 2018

Copy link
Copy Markdown
Collaborator

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

donkahlero added 3 commits June 4, 2018 09:16
@donkahlero
Add Support for LCM Defragmentation
2301c72 
This patch includes support for defragmentation of LCM packages, as
specified in the LCM protocol description
(https://lcm-proj.github.io/udp_multicast_protocol.html). This patch
code should also be used in combination with an upcoming PR towards the
GitHub LCM repository that adds golang + gopacket support.
@donkahlero
Fixed NextLayerType() Return
17d5b3e 
As the LCM defragmenter will build the defragmented package under the
header of the very first fragment, NextLayerType() should return the
matching LayerType() in case the message is not fragmented or the first
message of a fragmented one. Otherwise (all other fragmented messages),
the method should return LayerTypeFragment.
This patch fixes that.
@donkahlero
Moved Defragmenter to New Folder Structure
11f31cf 
As discussed in google#449, the
defragmenter was moved to a seperate package that will only contain the
LCM defragmenter. This is done in order to enable a more clean package
setup.
@donkahlero
donkahlero force-pushed the lcm_defrag_support branch from 49082dd to 11f31cf Compare June 4, 2018 07:16
@googlebot

googlebot commented Jun 4, 2018

Copy link
Copy Markdown
Collaborator

CLAs look good, thanks!

@donkahlero

donkahlero commented Jun 4, 2018

Copy link
Copy Markdown
Contributor Author

Ignore the mess that was in between here. Now it looks nice 😎

@gconnell
gconnell merged commit 055652d into google:master Jun 4, 2018
@gconnell

gconnell commented Jun 4, 2018

Copy link
Copy Markdown
Contributor

Great, thanks for the fast turn-arounds!

@donkahlero
donkahlero deleted the lcm_defrag_support branch June 4, 2018 15:44
traetox pushed a commit to traetox/gopacket that referenced this pull request Jan 2, 2019
@donkahlero @gconnell
Moved Defragmenter to New Folder Structure
5ba29b2 
As discussed in google#449, the
defragmenter was moved to a seperate package that will only contain the
LCM defragmenter. This is done in order to enable a more clean package
setup.
traetox pushed a commit to traetox/gopacket that referenced this pull request Jan 2, 2019
@donkahlero @gconnell
Moved Defragmenter to New Folder Structure
fe7f1b3 
As discussed in google#449, the
defragmenter was moved to a seperate package that will only contain the
LCM defragmenter. This is done in order to enable a more clean package
setup.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@donkahlero @gconnell @googlebot