-
Notifications
You must be signed in to change notification settings - Fork 273
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
base: master
Are you sure you want to change the base?
Conversation
* 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
There was a problem hiding this 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
There was a problem hiding this 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) |
There was a problem hiding this comment.
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)) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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:] |
There was a problem hiding this comment.
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 +
Hi @kbandla , |
self.capability = self.capabilities[0] | ||
|
||
def __len__(self): | ||
return self.__hdr_len__ + sum(map(len, self.capabilities)) |
There was a problem hiding this comment.
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)) |
There was a problem hiding this comment.
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))
?
capabilities
property (for cases where there are >1 capablity)capability
property for backward compatibility__bgp10
unittest, which broke the original parser