[issue10141] SocketCan support

[Charles-François Natali]

Charles-François Natali <neologix at free.fr> added the comment:

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.

----------

_______________________________________
Python tracker <report at bugs.python.org>
<http://bugs.python.org/issue10141>
_______________________________________