Created on 2010-10-19 00:49 by slanden, last changed 2014-03-21 17:03 by neologix.
Python lacks support for the AF_CAN family of sockets ()
Looks like an interesting feature request. What are the packages or external libraries which may rely on this and how do they deal with SocketCan support at moment for their requirements? (That thread mentions several, but some details may help further).
BTW, this can go in Python 3.2 if it gets in before Nov 2nd week. It wont be going into Python 2.x series.
forgot to include this:
SocketCan is the (currently) Linux-specific Berkley-socket CAN implementation created by Volkswagen. Alot of previous CAN implementations use a rather inferior (explained in can.txt in the Linux sources) character device approach.
So the use case for programs that use SocketCan is raw sockets in C, as it is a rather specialty form of interface. But considering that it fits fairly well into an existing interface, it seems appropriate to add to python.
I clicked 2.7 because that was what the patch was written for.
Our personal use case is for an IVI dash, to access vehicle devices that output familiar metrics like speed, tach, fuel levels, seat belts, etc over CAN.
* PF_CAN family of sockets
The patch looks good at first glance, but is there a way to test the feature?
There is this "simple" program that can be easily adapted to test the interface.
The patch looks good to me.
A couple remarks:
- could you please post it as a Mercurial diff? (it makes it easier to
review and apply)
@@ -1151,6 +1151,25 @@ makesockaddr(int sockfd, struct sockaddr
}
+ return Py_BuildValue("sh",
+ ifname,
+ a->can_family);
1) a->can_family would be better as a 'i' to be consistent with the
rest of the code
2) you should use the FS encoding for the interface name, e.g.
return Py_BuildValue("O&i",
PyUnicode_DecodeFSDefault
ifname,
a->can_family);
(you can have a look at socket_if_nameindex for an example).
@@ -1486,6 +1505,43 @@ getsockaddrarg(PySocketSockObject *s, Py
if (!PyArg_ParseTuple(args, "s", &interfaceName))
return 0;
3) same thing here, you should use
if (!PyArg_ParseTuple(args, "O&", PyUnicode_FSConverter,
&interfaceName))
return 0;
(see socket_if_nametoindex for an example)
4)
@@ -1486,6 +1505,43 @@ getsockaddrarg(PySocketSockObject *s, Py
+ if (!strcmp(interfaceName, "any")) {
+ ifr.ifr_ifindex = 0;
To be consistent with the convention used IPv4/IPv6
INADDR_ANY/in6addr_any, I would prefer if you replaced the any
interface name by "" (empty string)
5)
@@ -69,6 +69,20 @@ typedef int socklen_t;
+#ifdef HAVE_LINUX_CAN_H
+#include <linux/can.h>
+#ifndef PF_CAN
+# define PF_CAN 29
+#endif
+#ifndef AF_CAN
+# define AF_CAN PF_CAN
+#endif
+#endif
I don't see how PF_CAN or AF_CAN could not be defined lin <linux/can.h>.
And if they're not defined, it's probably not a good idea to define
them arbitrarily...
6)
+#ifdef HAVE_LINUX_CAN_H
+ struct sockaddr_can* can;
+#endif
it should be struct sockaddr_can can here, not a pointer.
7)
- you should update Doc/library/socket.rst
8)
- could you add a test for SocketCan in Lib/test/socket.py ?
It would be nice to have a basic test which can be run on all the
kernels supporting PF_CAN (typically just creating a socket, binding
to it and closing it), and another more complete test if there's a CAN
interface (sending and receiving a packet, probably using the
loopback).
Take into account that SOCK_RAW are priviledged and restricted to root
or CAP_SYS_RAW.
This patch should be easy to verify and includes documentation and the test cases.
ogait87@mypc:~/cpython$ hg sum
parent: 71435:6f9d917df541 tip
Fix test_multiprocessing failure under Windows.
branch: default
commit: 7 modified
update: (current)
Thanks for updating your patch.
I've done a review, available here:
I think that this time i have included almost every proposed changes.
For the unitTest I did not included the condition "os.getuid() == 0" because of this information in the Linux documentation "3.3 network security issues (capabilities)". For example, the Ubuntu 10.10 does not require to be root to access socketCAN network interfaces.
For the "return Py_BuildValue("O&h", PyUnicode_DecodeFSDefault, ifname, a->can_family);" i have kept the "h" because in the header file the "a->can_family" is defined as an short int.
Here's an updated patch, with more tests.
Please review!
socketcan_v4.patch:
- dummy question: why an address is a tuple with 1 string instead of just the string? Does AF_UNIX also uses a tuple of 1 string?
- the example should also use struct.pack() to create the frame, I don't like hardcoded BLOB
- in test_socket: _have_socket_can() interprets permission denied as "CAN is not supported", it would be nice to provide a better skip message. Create maybe a decorator based?
- _have_socket_can(): you may move s.close() outside the try block (add maybe a "else:" block?) because you may hide a real bug in .close()
- data += b'\0' * (8 - can_dlc): I prefer data = data.ljust(8, '\x00')
- you might add frame encoder/decoder in your example
- if (!strcmp(PyBytes_AS_STRING(interfaceName), "")) hum..... PyBytes_GET_SIZE(intername)==0 should be enough
- you truncate the interface name, it can be surprising, I would prefer an error (e.g. "interface name too long: 20 characters, the maximum is 10 characters" ?)
- (oh no! don't include horrible configure diff in patches for the bug tracker :-p)
In which Linux version was CAN introduced?
> - dummy question: why an address is a tuple with 1 string instead of just
> the string? Does AF_UNIX also uses a tuple of 1 string?
I think the reason behind the tuple is future proofing.
Here's the definition of `struct sockaddr_can` in my Linux box's headers:
"""
/**
* struct sockaddr_can - the sockaddr structure for CAN sockets
* @can_family: address family number AF_CAN.
* @can_ifindex: CAN network interface index.
* @can_addr: protocol specific address information
*/
struct sockaddr_can {
sa_family_t can_family;
int can_ifindex;
union {
/* transport protocol class address information (e.g. ISOTP) */
struct { canid_t rx_id, tx_id; } tp;
/* reserved for future CAN protocols address information */
} can_addr;
};
"""
By making it a tuple, it will be easier to extend the address that
must be passed to bind(2), should it ever evolve, in a backward
compatible way. Well, that's just a guess (I'm by no means a SocketCAN
expert :-).
> - the example should also use struct.pack() to create the frame, I don't
> like hardcoded BLOB
Done.
> - in test_socket: _have_socket_can() interprets permission denied as "CAN
> is not supported", it would be nice to provide a better skip message. Create
> maybe a decorator based?
AFAICT, it shouldn't fail with EPERM or so.
Also, I'm not sure what the message would look like, and it's probably
a bit overkill.
> - _have_socket_can(): you may move s.close() outside the try block (add
> maybe a "else:" block?) because you may hide a real bug in .close()
Changed that.
> - data += b'\0' * (8 - can_dlc): I prefer data = data.ljust(8, '\x00')
Hum... Done.
> - you might add frame encoder/decoder in your example
Done.
> - if (!strcmp(PyBytes_AS_STRING(interfaceName), "")) hum.....
> PyBytes_GET_SIZE(intername)==0 should be enough
Done.
> - you truncate the interface name, it can be surprising, I would prefer an
> error (e.g. "interface name too long: 20 characters, the maximum is 10
> characters" ?)
I changed that, and added a test. Also, note that AF_PACKET suffers
from the same problem.
I'll submit a separate patch.
> - (oh no! don't include horrible configure diff in patches for the bug
> tracker :-p)
Yeah, I usually take care of that, but forgot this time.
> In which Linux version was CAN introduced?
>
Apparently, 2.6.25. Note that we don't need
@support.requires_linux_version() though, it should be catched by
HAVE_SOCKET_CAN (also, you can't use it as a class decorator...).
Here's the updated patch. It passes on all the buildbots (of course,
it's only relevant on Linux).
So, Victor, what do you think of the last version?
This patch has been lingering for quite some time, and it's really a
cool feature.
I don't have much to say about the patch, given that I don't know anything about CAN and my system doesn't appear to have a "vcan0" interface. I think it's ok to commit and refine later if something turns out insufficient.
> I don't have much to say about the patch, given that I don't know
> anything about CAN and my system doesn't appear to have a "vcan0"
> interface.
Ah, thanks! Can you add a comment about that in test_socket.py?
I can confirm that all tests pass ok on my Linux system (kernel
2.6.38.8-desktop-5.mga).
New changeset e767318baccd by Charles-François Natali in branch 'default':
Issue #10141: socket: add SocketCAN (PF_CAN) support. Initial patch by Matthias
New changeset a4af684bb54e by Victor Stinner in branch 'default':
Issue #10141: Don't use hardcoded frame size in example, use struct.calcsize()
Committed.
Matthias, Tiago, thanks!
From python-dev:
"""
I work on Ubuntu Jaunty for my cpython development work - an old version, I
know, but still quite serviceable and has worked well for me over many months.
With the latest default cpython repository, however, I can't run the regression
suite because the socket module now fails to build:
gcc -pthread -fPIC -g -O0 -Wall -Wstrict-prototypes -IInclude -I.
-I./Include -I/usr/local/include -I/home/vinay/projects/python/default -c
/home/vinay/projects/python/default/Modules/socketmodule.c
-o build/temp.linux-i686-3.3-pydebug/home/vinay/projects/python/default
/Modules/socketmodule.o
.../Modules/socketmodule.c: In function
‘makesockaddr’:
.../Modules/socketmodule.c:1224: error: ‘AF_CAN’ undeclared (first use in
this function)
.../Modules/socketmodule.c:1224: error: (Each undeclared identifier is
reported only once
.../Modules/socketmodule.c:1224: error: for each function it appears in.)
.../Modules/socketmodule.c: In function
‘getsockaddrarg’:
.../Modules/socketmodule.c:1610: error: ‘AF_CAN’ undeclared (first use in
this function)
.../Modules/socketmodule.c: In function ‘getsockaddrlen’:
.../Modules/socketmodule.c:1750: error: ‘AF_CAN’ undeclared (first use in
this function)
On this system, AF_CAN *is* defined, but in linux/socket.h, not in sys/socket.h.
From what I can see, sys/socket.h includes bits/socket.h which includes
asm/socket.h, but apparently linux/socket.h isn't included.
"""
Vinay, what happens if you replace in Modules/socketmodule.h:
"""
#ifdef HAVE_LINUX_CAN_H
#include <linux/can.h>
#endif
"""
with
"""
#ifdef HAVE_LINUX_CAN_H
#include <linux/socket.h>
#include <linux/can.h>
#endif
"""
I added the line in the suggested place:
#ifdef HAVE_LINUX_TIPC_H
# include <linux/tipc.h>
#endif
#ifdef HAVE_LINUX_CAN_H
#include <linux/socket.h> /* the line I added - line 76 */
#include <linux/can.h>
#endif
#ifdef HAVE_LINUX_CAN_RAW_H
#include <linux/can/raw.h>
#endif
Strangely, it seems to make no difference! I copied out the actual gcc line from the make file and ran it from the command line to just pre-process:
gcc -pthread -fPIC -g -O0 -Wall -Wstrict-prototypes -IInclude -I. -I./Include -I/usr/local/include -I/home/vinay/projects/python/default -E /home/vinay/projects/python/default/Modules/socketmodule.c >/tmp/tmp.c
(I assume /usr/include is always searched, as it's not explicitly named in a -I parameter.)
Looking at the tmp.c file produced, here's the relevant section:
#/can.h" 1 3 4
After the tipc.h include, the can.h include comes next, as if I hadn't added the linux/socket.h line at all! What is this I don't even ...
I pasted the whole socketmodule.h file to a gist here:
Tell me I'm not going mad!
Just to test, I added the full absolute path name in socketmodule.h:
#ifdef HAVE_LINUX_CAN_H
#include "/usr/include/linux/socket.h"
#include <linux/can.h>
#endif
Now, the snippet from pre-processing is:
#/socket.h" 1
# 77 "/home/vinay/projects/python/default/Modules/socketmodule.h" 2
# 1 "/usr/include/linux/can.h" 1 3 4
# 41 "/usr/include/linux/can.h" 3 4
This implies that the linux/socket.h file is not being read at all. First part of linux/socket.h is:
#ifndef _LINUX_SOCKET_H
#define _LINUX_SOCKET_H
Ok, I found that linux/socket.h *is* actually included earlier, from /usr/include/linux/netlink.h. However, the AF_CAN definition appears only under the condition
#if defined(__KERNEL__) || !defined(__GLIBC__) || (__GLIBC__ < 2)
...
#define AF_CAN 29 /* Controller Area Network */
...
#endif /* not kernel and not glibc */
#endif /* _LINUX_SOCKET_H */
which would imply that on this system at least, the AF_CAN definition is supposed to come from elsewhere. I did a recursive grep in /usr/include: the only place AF_CAN is defined is linux/socket.h.
> which would imply that on this system at least, the AF_CAN definition is
> supposed to come from elsewhere.
Yes, from <bits/socket.h>.
Looks like a crappy libc version: <linux/can.h> is present, but AF_CAN is not defined.
Just for fun, is PF_CAN defined?
You might try the following in configure.in:
"""
# On Linux, can.h and can/raw.h require sys/socket.h
AC_CHECK_HEADERS(linux/can.h linux/can/raw.h,,,[
#ifdef HAVE_SYS_SOCKET_H
#include <sys/socket.h>
#ifndef AF_CAN
# error "AF_CAN not defined"
#endif
#endif
])
"""
PF_CAN is defined by
#define PF_CAN AF_CAN
in linux/socket.h :-(
Making the change in configure.in didn't lead to any change: no error when I ran configure (which is ./configure --with-py-debug in my case), and when I build, the same error (AF_CAN undefined) occurs. Just to eliminate any extraneous variables and be absolutely sure, the program
#include <sys/socket.h>
#include <stdio.h>
int main(int argc, char ** argv)
{
printf("AF_CAN is %d\n", AF_CAN);
}
also fails with the same error.
> Making the change in configure.in didn't lead to any change: no error
> when I ran configure
Did you run autoconf (or autoreconf) before?
> Did you run autoconf (or autoreconf) before?
D'oh! I didn't realise I had to, though in retrospect it should have been obvious ... autoconf isn't even installed on my system, and I can't install it at the moment because of some problem with the Ubuntu repos for Jaunty.
Here's a better patch.
It only touches socketmodule.c, so no need for autoconf, just use make.
If it works on your box, I'll test it on the custom buildbots before pushing it for good (if I learned one thing, it's to never underestimate the potential for broken headers/libc).
That did the trick - I can now build the socket module. Thanks!
I noticed that the module initialisation code uses #ifdef AF_CAN and #ifdef PF_CAN rather than #ifdef HAVE_LINUX_CAN_H :-)
New changeset d4ce850b06b7 by Charles-François Natali in branch 'default':
Issue #10141: fix socketmodule compilation on Linux systems with <linux/can.h>
Working fine on the buildbots and Vinay's box, closing!
New changeset df427bf067d7 by Vinay Sajip in branch '3.4':
Issue #10141: updated new usages of AF_CAN to be in #ifdef AF_CAN rather than #ifdef HAVE_LINUX_CAN_H to allow compilation on older Linuxes.
Vinay, your change just reverted this:
Basically, AF_CAN being defined doesn't imply that CAN_RAW is defined.
So compilation will now fail - again - on those hosts.
Sorry if I messed up - I just applied the same logic as I thought you had done earlier (replacing #ifdef HAVE_LINUX_CAN_H with #ifdef AF_CAN), and everything compiled OK after my changes.
Are you saying that an additional clause for CAN_RAW being defined should be added around where it is used? Would that sort things out? I'd rather not just revert my change, as that would mean I couldn't compile the SSL module.
> Are you saying that an additional clause for CAN_RAW being defined should
be added around where it is used? Would that sort things out?
Yes.
> I'd rather not just revert my change, as that would mean I couldn't
compile the SSL module.
I don't get it: how could the previous code prevent the SSL module from
being built?
What error were you getting?
> What error were you getting?
That AF_CAN was undefined (even though HAVE_LINUX_CAN_H is). This is on Ubuntu Jaunty, which I use for my Python core development.
Note the change you made in d4ce850b06b7 to fix this problem when it occurred before - it's the same approach as my recent change that we're talking about. It seems that in 3.4, some more instances of AF_CAN usage were added, but wrapped in #ifdef HAVE_LINUX_CAN_H, which led to the new compilation failures.
I'll make the changes to take CAN_RAW into account.
New changeset 9dc199b921eb by Vinay Sajip in branch '3.4':
Issue #10141, Issue 20065: Changed #if to take CAN_RAW into account.
New changeset 20cced06acdd by Vinay Sajip in branch 'default':
Issue #10141, Issue 20065: Merged from 3.4.
> That AF_CAN was undefined (even though HAVE_LINUX_CAN_H is). This is on
Ubuntu Jaunty, which I use for my Python core development.
How dear...
The latest change should be OK.