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

IP options and TCP #21

Merged
merged 13 commits into from
Sep 8, 2016
Merged

IP options and TCP #21

merged 13 commits into from
Sep 8, 2016

Conversation

cristiklein
Copy link
Contributor

Hi,

I added some infrastructure for parsing IP options, with an example of an IP option that I was interested in. I also added support for decoding TCP/IPv4 packets.

Cheers,
Cristian

@codecov-io
Copy link

codecov-io commented Sep 2, 2016

Current coverage is 82.10% (diff: 84.46%)

Merging #21 into master will increase coverage by 2.79%

@@             master        #21   diff @@
==========================================
  Files            11         13     +2   
  Lines           411        531   +120   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits            326        436   +110   
- Misses           85         95    +10   
  Partials          0          0          

Powered by Codecov. Last update 552472d...9c34cd1

@kisom
Copy link
Owner

kisom commented Sep 2, 2016

Hello @cristiklein, thanks for the PR. Can you add tests and add yourself to the AUTHORS file?

@cristiklein
Copy link
Contributor Author

Hello @kisom . Good point. It even helped me find a few bugs in my original contribution. 😄

return packet

def __len__(self):
return self.udp_header_size + len(self.payload)
Copy link
Owner

Choose a reason for hiding this comment

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

This doesn't seem right. Can you add a test to check the length of the packet?

@cristiklein
Copy link
Contributor Author

@kisom Nice catch!

BTW, I hope I'm not opening a can of worms, but I tend to dislike the fact that the payload is stored hexlified, which means that it needs to be divided by two for some length computations. I would rather store everything as original bytes and let the application present the data as it sees fit.

@kisom
Copy link
Owner

kisom commented Sep 7, 2016

@cristiklein I don't have a strong preference either way; when I first wrote this, the hexlified payload was better for what I was doing (and for debugging). I guess the biggest question now would be whether anyone is relying on the payload being hexlified now.

@cristiklein
Copy link
Contributor Author

I am leaning towards de-hexlification, due to the fact that upper decoding layers need to call unhexlify, which adds overhead. Then again, I don't currently have a particular need for performance and the library works very well for me as it is now.

There is also the option of a "soft transition". Having a new field, payload_bin that stores bytes and changing payload to a property that hexlifies the bytes on-the-fly. I can prepare a patch, if this solution sounds acceptable.

@kisom
Copy link
Owner

kisom commented Sep 8, 2016

That sounds like a good way forward. It should probably go in a separate PR, though. Does that work for you?

@cristiklein
Copy link
Contributor Author

Of course, separate PR. Will open one, once I find some time.

2016-09-08 15:24 GMT+02:00 Kyle Isom notifications@github.com:

That sounds like a good way forward. It should probably go in a separate
PR, though. Does that work for you?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#21 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABlXoSyl6ekmQjg4heJdjdsJsfgQjctmks5qoAxzgaJpZM4Jzevf
.

@kisom
Copy link
Owner

kisom commented Sep 8, 2016

Okay, thanks. I'll merge this now, then.

@kisom kisom merged commit 8b3e2b4 into kisom:master Sep 8, 2016
@kisom
Copy link
Owner

kisom commented Sep 8, 2016

Follow up: this was released as v0.12.0 and is now on PyPI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants