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

BGP: Handle multiple capabilities in one Optional Parameter #480

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kbandla
Copy link
Owner

@kbandla kbandla commented Jul 13, 2020

  • Fixes BGP (v4) Open typed packet failing to parse. #118 (please see issue for details)
  • Introduce capabilities property (for cases where there are >1 capablity)
  • Keep older capability property for backward compatibility
  • Added __bgp10 unittest, which broke the original parser

* Fixes #118 (please see issue for details)
* Introduce `capabilities` property (for cases where there are >1 capablity)
* Keep older `capability` property for backward compatibility
* Added `__bgp10` unittest, which broke the original parser
@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 89.288% when pulling a7bdc52 on issue118 into bc0fc98 on master.

Copy link
Contributor

@amgadhanafy amgadhanafy left a comment

Choose a reason for hiding this comment

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

I believe this fix will be useful in multiple locations that have similar issues (single value instead of list), I believe making a generic way to apply it is useful

Copy link
Collaborator

@obormot obormot left a comment

Choose a reason for hiding this comment

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

minor comments.. looks good overall 👍


def __bytes__(self):
caps = b''.join(map(bytes, self.capabilities))
self.cap_len = len(caps)
Copy link
Collaborator

Choose a reason for hiding this comment

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

unused?
also prob shouldn't create new fields inside this method

self.capability = self.capabilities[0]

def __len__(self):
return self.__hdr_len__ + sum(map(len, self.capabilities))
Copy link
Collaborator

Choose a reason for hiding this comment

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

calling len() on each instance of capability would cause serialization to bytes, then taking the resulting length. would something like sum(cap.len for cap in self.capabilities) achieve the same result?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Alternatively you could override the __len__ method to return that header element?

while self.data:
capability = self.Capability(self.data)
l.append(capability)
self.data = self.data[self.__hdr_len__+capability.len:]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe pep8 would want whitespace around +

@amgadhanafy
Copy link
Contributor

Hi @kbandla ,
Any chance to get this merged soon?

self.capability = self.capabilities[0]

def __len__(self):
return self.__hdr_len__ + sum(map(len, self.capabilities))
Copy link
Collaborator

Choose a reason for hiding this comment

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

This doesn't cover the case where the type is AUTHENTICATION, as capabilities won't exist (and it doesn't count the length of the authentication data).

return self.__hdr_len__ + sum(map(len, self.capabilities))

def __bytes__(self):
caps = b''.join(map(bytes, self.capabilities))
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will also fail if this is an authentication parameter. Perhaps some variant of caps = bytes(self.data) if not isinstance(self.data, list) else b''.join(map(bytes, self.data))?

@crocogorical crocogorical mentioned this pull request Jan 31, 2021
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.

BGP (v4) Open typed packet failing to parse.
5 participants