This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

Author neologix
Recipients amaury.forgeotdarc, giampaolo.rodola, neologix, ogait87, orsenthil, rosslagerwall, slanden
Date 2011-07-04.16:55:25
SpamBayes Score 4.352877e-09
Marked as misclassified No
Message-id <CAH_1eM2GF9yt+Vcys6N8sTDDVBUHw9gbZkVTrXW6fgzcrE5Tdg@mail.gmail.com>
In-reply-to <1309786184.27.0.443142198349.issue10141@psf.upfronthosting.co.za>
Content
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.
History
Date User Action Args
2011-07-04 16:55:27neologixsetrecipients: + neologix, amaury.forgeotdarc, orsenthil, giampaolo.rodola, slanden, rosslagerwall, ogait87
2011-07-04 16:55:26neologixlinkissue10141 messages
2011-07-04 16:55:25neologixcreate