Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32.1k
bpo-30987 - Support for ISO-TP protocol in SocketCAN#2956
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
the-knights-who-say-ni commentedJul 31, 2017
Hello, and thanks for your contribution! I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed thePSF contributor agreement (CLA). Unfortunately we couldn't find an account corresponding to your GitHub username onbugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please followthe steps outlined in the CPython devguide to rectify this issue. Thanks again to your contribution and we look forward to looking at it! |
After second thoughts, only the |
pylessard commentedAug 3, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Pull request is re-open. All dependencies over the third party project have been removed. |
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.
Thanks for your contribution. Your patch looks mostly fine, good work! I added notes for some style nit picks and a missingifdef check.
Modules/socketmodule.c Outdated
| @@ -6993,6 +7055,8 @@ PyInit__socket(void) | |||
| PyModule_AddIntMacro(m, CAN_SFF_MASK); | |||
| PyModule_AddIntMacro(m, CAN_EFF_MASK); | |||
| PyModule_AddIntMacro(m, CAN_ERR_MASK); | |||
| PyModule_AddIntMacro(m, CAN_ISOTP); | |||
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 needs a#ifdef CAN_ISOTP.
Modules/socketmodule.c Outdated
| @@ -1913,6 +1927,54 @@ getsockaddrarg(PySocketSockObject *s, PyObject *args, | |||
| Py_DECREF(interfaceName); | |||
| return 1; | |||
| } | |||
| #endif | |||
| #if defined(CAN_ISOTP) | |||
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.
#ifdef CAN_ISOTP
Modules/socketmodule.c Outdated
| @@ -1869,7 +1882,9 @@ getsockaddrarg(PySocketSockObject *s, PyObject *args, | |||
| } | |||
| #endif | |||
| #if defined(AF_CAN) && defined(CAN_RAW) && defined(CAN_BCM) | |||
| #if defined(AF_CAN) | |||
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.
Please use ifdef for simple checks:#ifdef AF_CAN
Lib/test/test_socket.py Outdated
| def testBind(self): | ||
| try: | ||
| with socket.socket(socket.PF_CAN, socket.SOCK_DGRAM, socket.CAN_ISOTP) as s: | ||
| addr = (self.interface,0x123,0x456) |
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.
Style nit pick:addr = self.interface, 0x123, 0x456
Lib/test/test_socket.py Outdated
| # most systems limit IFNAMSIZ to 16, take 1024 to be sure | ||
| with socket.socket(socket.PF_CAN, socket.SOCK_DGRAM, socket.CAN_ISOTP) as s: | ||
| self.assertRaisesRegex(OSError, 'interface name too long', | ||
| s.bind, ('x' * 1024,1,2)) |
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.
Please use
with self.assertRaisesRegex(OSError, 'interface name too long'): s.bind(('x' * 1024, 1, 2))to make it more readable.
bedevere-bot commentedAug 26, 2017
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
I didn't expect the Spanish Inquisition! |
bedevere-bot commentedAug 26, 2017
Nobody expects the Spanish Inquisition! @tiran: please review the changes made to this pull request. |
Your PR is still missing a space after comma in |
pylessard commentedAug 27, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@tiran Changes done sir! |
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.
perfect! 🥇
* Added support for CAN_ISOTP protocol* Added unit tests for CAN ISOTP* Updated documentation for ISO-TP protocol* Removed trailing whitespace in documentation* Added blurb NEWS.d file* updated Misc/ACKS* Fixed broken unit test that was using isotp const outside of skippable section* Removed dependecy over third party project* Added implementation for getsockname + unit tests* Missing newline at end of ACKS file* Accidentally inserted a type in ACKS file* Followed tiran changes review#1 recommendations* Added spaces after comma
Uh oh!
There was an error while loading.Please reload this page.
This Pull Request add the support of CAN ISO-TP address extensions in the socket module by enhancing
getsockaddrargandgetsockname.Note to core developpers : I believe this pull request should be backported back to Python 3.3 where SocketCAN support have been added.
https://bugs.python.org/issue30987