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

Add method to match layers by type. #282

Closed
wants to merge 1 commit into from
Closed

Conversation

kleptog
Copy link

@kleptog kleptog commented Aug 15, 2016

Using the new .get() method you can quickly find the IP layer in a packet.
With the .getall() method you can retrieve a list, in case there are
multiple.

It would have been nice to use the getitem interface, but it is already
used for another purpose.

Fixes: #165

@coveralls
Copy link

coveralls commented Aug 15, 2016

Coverage Status

Coverage decreased (-0.1%) to 83.765% when pulling 43074db on kleptog:getitem into 6410c98 on kbandla:master.

@kleptog
Copy link
Author

kleptog commented Aug 15, 2016

This patch depends on the previous one, will rebase when the other is merged.

Using the new .get() method you can quickly find the IP layer in a packet.
With the .getall() method you can retrieve a list, in case there are
multiple.

It would have been nice to use the __getitem__ interface, but it is already
used for another purpose.

Fixes: kbandla#165
@coveralls
Copy link

coveralls commented Sep 1, 2016

Coverage Status

Coverage increased (+0.08%) to 84.686% when pulling 571f366 on kleptog:getitem into d12567a on kbandla:master.

@brifordwylie
Copy link
Collaborator

brifordwylie commented Sep 2, 2016

So I really like @obormot suggestion on #165 to use the contains and getitem methods, which would mean this kind of intuitive usage.

eth = Ethernet(buf)
if UDP in eth:
    udp = pkt[UDP]

Right now dpkt.py already overloads the getitem method for getting an attribute...

    def __getitem__(self, k):
        try:
            return getattr(self, k)
        except AttributeError:
            raise KeyError

I don't actually like the the current getitem functionality. dpkt has attribute based classes and that's how everyone uses it. So retrieving the attributes with a 'dictionary' interface just muddies the waters (BTW: all 132 dpkt tests pass fine without the getitem interface).

This is a big change, so I'll like @kbandla and @obormot to weigh in here, but for my $0.02 I'm going to suggest something mildly radical.

  1. Remove the current getitem functionality
  2. Use getitem and contains as @obormot suggested

Also no matter what we do I don't like the 'getall' functionality, it seems superfluous and I feel like code using it will want to know/differentiate between the 'outer' IP packet and the 'inner' IP packet, so for example.. I feel like this snippet below is intuitive and natural, I'm not sure how I would code this up with a list of 2 IPs from getall()

if IP in eth:
     ip = eth[IP]
     if IP in ip:
            print('Interesting.. I have an encapsulated IP packet')

@obormot
Copy link
Collaborator

obormot commented Sep 4, 2016

@brifordwylie I agree completely, this is how scapy does it BTW. I think we want __getitem__ to return the first instance of the given layer (if there are multiple); and __contains__ obviously to check if the given layer is present at all.

getall could be useful for grouping fields of the same type in a list (e.g. to make a list of SSL ciphersuites, where each is an instance of a CipherSuite class), if the protocol parser didn't group them inside a wrapper field (which it should do..). I agree it'll confuse encapsulation. I would not add it as a core library function.

@brifordwylie
Copy link
Collaborator

Closing this PR as it's been superseded by #584.

Thanks @obormot 🥇

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.

Check if a given dpkt-layer is part of the packet
4 participants