-
-
Notifications
You must be signed in to change notification settings - Fork 30.3k
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
bpo-37085: Expose SocketCAN bcm_msg_head flags #13646
bpo-37085: Expose SocketCAN bcm_msg_head flags #13646
Conversation
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.
tests?
Could you clarify what you mean, @auvipy? These are just binding the C Thanks! |
Expose the CAN_BCM SocketCAN constants used in the bcm_msg_head struct flags (provided by <linux/can/bcm.h>) under the socket library. This adds the following constants with a CAN_BCM prefix: * SETTIMER * STARTTIMER * TX_COUNTEVT * TX_ANNOUNCE * TX_CP_CAN_ID * RX_FILTER_ID * RX_CHECK_DLC * RX_NO_AUTOTIMER * RX_ANNOUNCE_RESUME * TX_RESET_MULTI_IDX * RX_RTR_FRAME * CAN_FD_FRAME
baab234
to
6d1d880
Compare
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 certainly needs tests, at least checking availability as done for the other socket.CAN_BCM_*
constants in test_socket.BasicCANTest.testBCMConstants()
.
Modules/socketmodule.c
Outdated
@@ -7668,6 +7668,42 @@ PyInit__socket(void) | |||
PyModule_AddIntConstant(m, "CAN_BCM_RX_STATUS", RX_STATUS); | |||
PyModule_AddIntConstant(m, "CAN_BCM_RX_TIMEOUT", RX_TIMEOUT); | |||
PyModule_AddIntConstant(m, "CAN_BCM_RX_CHANGED", RX_CHANGED); | |||
#ifdef SETTIMER |
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.
Why these separate #ifdef
checks for each constant? Are they not either all available together, or not? More specifically, isn't the #ifdef HAVE_LINUX_CAN_BCM_H
check enough to ensure their availability?
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 is not necessarily true for all the constants, namely the backwards-compatible introduction of the CAN_FD_FRAME
flag, which was added in 6f3b911d5f29b98752e5da86a295210c0c4f4e14
as part of the the 4.8.x
kernel series.
The other constants were always present since the SocketCAN drivers were mainlined in 2.6.25
.
Should I just have the #ifdef
on CAN_FD_FRAME
then? I had added the other ones for consistency.
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.
Should I just have the #ifdef on CAN_FD_FRAME then? I had added the other ones for consistency.
IMO that would be better, yes. And perhaps add a note about the availability of CAN_FD_FRAME
in the docs.
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 |
The CAN_FD_FRAME flag was introduced in the 4.8 kernel, while the other ones were present since SocketCAN drivers were mainlined in 2.6.25. As such, it is probably unnecessary to guard against these constants being missing.
The CAN_BCM_CAN_FD_FRAME constant mirrors the availability in the Linux kernel. This constant was introduced in Linux 4.8 kernels.
Lib/test/test_socket.py
Outdated
socket.CAN_BCM_TX_RESET_MULTI_IDX | ||
socket.CAN_BCM_RX_RTR_FRAME | ||
|
||
@unittest.skipUnless(hasattr(socket, "CAN_BCM_CAN_FD_FRAME"), |
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.
Is it better to parse the kernel version from platform.release
as a way of checking whether this exists?
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.
Yes, using the test.support.requires_linux_version
decorator.
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.
And don't forget to also use the same decorator as above, i.e.:
@unittest.skipUnless(hasattr(socket, "CAN_BCM"),
'socket.CAN_BCM required for this test.')
@requires_linux_version(4, 8)
def testBCMConstantsSupportsFD(self):
# ...
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.
Also the method name for the separate test is unclear to me ("supports FD"). Either of testBCMConstantsLinux48
or testBCMConstantsFD
seems better.
Use support.requires_linux_version to guard BCM flag CAN_FD_FRAME, which is only present on Linux >= 4.8.
I have made the requested changes; please review again |
Thanks for making the requested changes! @taleinat: please review the changes made to this pull request. |
@@ -0,0 +1 @@ | |||
Added support for the Linux SocketCAN Broadcast Manager API constants. |
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.
Some BCM constants were already there; perhaps more precisely describe the nature of the added constants/flags.
Thanks, @karlding! This is looking good, I have just a small comment on the NEWS entry. |
The CI tests are failing due to the |
I ran the tests with this PR on an Ubuntu 18.10 installation and they passed. Perhaps get access to a host running Ubuntu 16.04 to reproduce and diagnose the issue. |
So correct me if I'm wrong, but I believe this is because the Xenial headers included in Looking at #define LINUX_VERSION_CODE 263335 which corresponds to the following ( (((4) << 16) + ((4) << 8) + (167)) And even after updating #define LINUX_VERSION_CODE 263349 which corresponds to (((4) << 16) + ((4) << 8) + (181)) Despite running a
I'm not too familiar with packaging, but if I'm reading this Ubuntu kernel release schedule page correctly, this seems to indicate that they ship And according to this AskUbuntu post, in Ubuntu LTS releases, it appears that they are not upgraded with the HWE kernels. So while the kernel may support it, the headers provided by the repos don't provide the necessary userspace API. |
@karlding, great research and write-up! I think the existing implementation approach, exposing the flags only when they are available at the C level, is good. It's just the tests that need to be fixed. In this case even just not testing for this flag would be fine IMO, but here I'd definitely want another core dev's opinion. So please remove it from the test, make sure tests are passing, and we'll wait for another reviewer. @serhiy-storchaka, any chance for a quick review of this, perhaps? |
This constant doesn't exist on Ubuntu 16.04 LTS, since linux-libc-dev ships 4.4.x headers from the repos, while the feature was added in the 4.8.x kernel series. However, Ubuntu 16.04 supports the 4.15.x kernel series, but since linux-libc-dev isn't updated past 4.4.x headers, there is no guarantee that /usr/include/linux/can/bcm.h contains the proper constant for the userspace API, despite having a supported kernel.
@karlding, which current OS-s do support the |
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.
LGTM, just fix some spacing.
Modules/socketmodule.c
Outdated
PyModule_AddIntConstant(m, "CAN_BCM_RX_ANNOUNCE_RESUME", RX_ANNOUNCE_RESUME); | ||
PyModule_AddIntConstant(m, "CAN_BCM_TX_RESET_MULTI_IDX", TX_RESET_MULTI_IDX); | ||
PyModule_AddIntConstant(m, "CAN_BCM_RX_RTR_FRAME", RX_RTR_FRAME); | ||
#ifdef CAN_FD_FRAME |
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_FD_FRAME | |
#ifdef CAN_FD_FRAME |
Doc/library/socket.rst
Outdated
@@ -374,6 +374,9 @@ Constants | |||
|
|||
.. availability:: Linux >= 2.6.25. | |||
|
|||
.. note:: | |||
The :data:`CAN_BCM_CAN_FD_FRAME` flag is only available on Linux >= 4.8. |
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.
The :data:`CAN_BCM_CAN_FD_FRAME` flag is only available on Linux >= 4.8. | |
The :data:`CAN_BCM_CAN_FD_FRAME` flag is only available on Linux >= 4.8. |
AFAIK, the only OS besides Linux which supports CAN_BCM is NetBSD starting from version 8. But our NetBSD buildbots were broken and removed a long time ago. |
I was thinking of other Linux distros. Actually, I myself checked that it is available on Ubuntu 18.10 (see previous comment). I suggest to check on 18.04 (since it's LTS), and if found there, enable the removed test just on Ubuntu >= 18.04. |
I checked on Ubuntu 18.04, and after installing
With that being said, is there a non-deprecated way to check for Ubuntu versions (ie. besides |
Fix the sphinx warning message that is causing the CI failure: Content block expected for the "note" directive; none found. This is done by increasing the indentation to place the text in a block.
The constant should also be available on Debian Stretch and beyond, since |
That's a good question, I hadn't realized it had been deprecated without any replacement.
No, I can't find an example of this in our codebase. |
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 think it can be merged without test for CAN_BCM_CAN_FD_FRAME.
Also, should this be backported, since the docs (incorrectly) indicate that all the BCM constants are available on older versions? |
It may be backported to 3.8 (ask @ambv for this), but not to 3.7. |
@ambv: Please replace |
GH-15049 is a backport of this pull request to the 3.8 branch. |
Expose the CAN_BCM SocketCAN constants used in the bcm_msg_head struct flags (provided by <linux/can/bcm.h>) under the socket library. This adds the following constants with a CAN_BCM prefix: * SETTIMER * STARTTIMER * TX_COUNTEVT * TX_ANNOUNCE * TX_CP_CAN_ID * RX_FILTER_ID * RX_CHECK_DLC * RX_NO_AUTOTIMER * RX_ANNOUNCE_RESUME * TX_RESET_MULTI_IDX * RX_RTR_FRAME * CAN_FD_FRAME The CAN_FD_FRAME flag was introduced in the 4.8 kernel, while the other ones were present since SocketCAN drivers were mainlined in 2.6.25. As such, it is probably unnecessary to guard against these constants being missing. (cherry picked from commit 31c4fd2) Co-authored-by: karl ding <karlding@users.noreply.github.com>
Shouldn't we add a note about this in "What's New"? |
Expose the CAN_BCM SocketCAN constants used in the bcm_msg_head struct flags (provided by <linux/can/bcm.h>) under the socket library. This adds the following constants with a CAN_BCM prefix: * SETTIMER * STARTTIMER * TX_COUNTEVT * TX_ANNOUNCE * TX_CP_CAN_ID * RX_FILTER_ID * RX_CHECK_DLC * RX_NO_AUTOTIMER * RX_ANNOUNCE_RESUME * TX_RESET_MULTI_IDX * RX_RTR_FRAME * CAN_FD_FRAME The CAN_FD_FRAME flag was introduced in the 4.8 kernel, while the other ones were present since SocketCAN drivers were mainlined in 2.6.25. As such, it is probably unnecessary to guard against these constants being missing. (cherry picked from commit 31c4fd2) Co-authored-by: karl ding <karlding@users.noreply.github.com>
Expose the CAN_BCM SocketCAN constants used in the bcm_msg_head struct flags (provided by <linux/can/bcm.h>) under the socket library. This adds the following constants with a CAN_BCM prefix: * SETTIMER * STARTTIMER * TX_COUNTEVT * TX_ANNOUNCE * TX_CP_CAN_ID * RX_FILTER_ID * RX_CHECK_DLC * RX_NO_AUTOTIMER * RX_ANNOUNCE_RESUME * TX_RESET_MULTI_IDX * RX_RTR_FRAME * CAN_FD_FRAME The CAN_FD_FRAME flag was introduced in the 4.8 kernel, while the other ones were present since SocketCAN drivers were mainlined in 2.6.25. As such, it is probably unnecessary to guard against these constants being missing.
Expose the CAN_BCM SocketCAN constants used in the bcm_msg_head struct flags (provided by <linux/can/bcm.h>) under the socket library. This adds the following constants with a CAN_BCM prefix: * SETTIMER * STARTTIMER * TX_COUNTEVT * TX_ANNOUNCE * TX_CP_CAN_ID * RX_FILTER_ID * RX_CHECK_DLC * RX_NO_AUTOTIMER * RX_ANNOUNCE_RESUME * TX_RESET_MULTI_IDX * RX_RTR_FRAME * CAN_FD_FRAME The CAN_FD_FRAME flag was introduced in the 4.8 kernel, while the other ones were present since SocketCAN drivers were mainlined in 2.6.25. As such, it is probably unnecessary to guard against these constants being missing.
Expose the CAN_BCM SocketCAN constants used in the bcm_msg_head struct flags (provided by <linux/can/bcm.h>) under the socket library. This adds the following constants with a CAN_BCM prefix: * SETTIMER * STARTTIMER * TX_COUNTEVT * TX_ANNOUNCE * TX_CP_CAN_ID * RX_FILTER_ID * RX_CHECK_DLC * RX_NO_AUTOTIMER * RX_ANNOUNCE_RESUME * TX_RESET_MULTI_IDX * RX_RTR_FRAME * CAN_FD_FRAME The CAN_FD_FRAME flag was introduced in the 4.8 kernel, while the other ones were present since SocketCAN drivers were mainlined in 2.6.25. As such, it is probably unnecessary to guard against these constants being missing.
Expose the CAN_BCM SocketCAN constants used in the bcm_msg_head struct
flags (provided by <linux/can/bcm.h>) under the socket library.
This adds the following constants with a CAN_BCM prefix:
https://bugs.python.org/issue37085