Skip to content

bpo-42653: Add constants to socket module for IsoTp CAN protocol #26174

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.

Sign up for GitHub

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

Closed
wants to merge 9 commits into from
Closed

bpo-42653: Add constants to socket module for IsoTp CAN protocol #26174

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
0cf7cd7
Add import for linux/can/isotp.h
menschel May 16, 2021
17535b5
Add IsoTp constants in socketmodule.c
menschel May 19, 2021
1f7c2c5
Add test for isotp constants into unit test.
menschel May 19, 2021
bc9118c
Add entry to Doc/whatsnew/3.10.rst
menschel May 19, 2021
523bf9f
Fix a whitespace issue in test_socket.py
menschel May 19, 2021
eb283c2
Add issue number 42653 and blurb.
menschel May 19, 2021
2f19691
Fix: Setting of HAVE_LINUX_CAN_ISOTP_H by configure
menschel May 22, 2021
5860ad0
Fix: Change order of headers in configure
menschel May 22, 2021
8132cb1
Fix: Optionally include CAN_ISOTP_SF_BROADCAST
menschel May 23, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion Lib/test/test_socket.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2166,6 +2166,8 @@ def testCrucialConstants(self):
socket.CAN_ISOTP
socket.SOCK_DGRAM

@unittest.skipUnless(hasattr(socket, "SOL_CAN_ISOTP"),
'Constants from isotp.h required for this test.')
def testIsoTpConstants(self):
socket.SOL_CAN_ISOTP

Expand All @@ -2190,7 +2192,8 @@ def testIsoTpConstants(self):
socket.CAN_ISOTP_FORCE_RXSTMIN
socket.CAN_ISOTP_RX_EXT_ADDR
socket.CAN_ISOTP_WAIT_TX_DONE
socket.CAN_ISOTP_SF_BROADCAST
# This constant is new and not always available
# socket.CAN_ISOTP_SF_BROADCAST

# default values
socket.CAN_ISOTP_DEFAULT_FLAGS
Expand Down
3 changes: 3 additions & 0 deletions Modules/socketmodule.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7809,7 +7809,10 @@ PyInit__socket(void)
PyModule_AddIntMacro(m, CAN_ISOTP_FORCE_RXSTMIN);
PyModule_AddIntMacro(m, CAN_ISOTP_RX_EXT_ADDR);
PyModule_AddIntMacro(m, CAN_ISOTP_WAIT_TX_DONE);
#ifdef CAN_ISOTP_SF_BROADCAST
/* This constant is new and not always available */
PyModule_AddIntMacro(m, CAN_ISOTP_SF_BROADCAST);
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume the version of the kernel could be checked on buildtime, couldn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Technically yes, but there is no guarantee that this symbol really is there when the kernel version is above a specific version.
If you install the headers yourself with linux-headers-$(uname -r), the assumption works.

But if the installation of headers has been obscured, like with Raspberry Pi 's package raspberrypi-kernel-headers, this assumption no longer works. It just adds another assumption that the packages
raspberrypi-kernel-headers and raspberrypi-kernel are in sync.

Copy link
Contributor

@rumpelsepp rumpelsepp May 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Personally, I would argue that the mainline upstream kernel is the source of truth and this must be fixed downstream. No idea how the CPython project deals with these kinds of issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, lets wait for the Reviewer to comment on this. This should theoretically be @tiran.

Copy link
Contributor

Choose a reason for hiding this comment

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

aye.

#endif

/* default values */
PyModule_AddIntMacro(m, CAN_ISOTP_DEFAULT_FLAGS);
Expand Down