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, mluis, neologix, ogait87, orsenthil, rosslagerwall, slanden, vstinner
Date 2011-09-23.21:33:35
SpamBayes Score 3.8857806e-16
Marked as misclassified No
Message-id <CAH_1eM1SEuiTGUcmVhbCk9bxvma8xPk_2i1+sqN0X6wogeRz8w@mail.gmail.com>
In-reply-to <1316737061.33.0.353797692488.issue10141@psf.upfronthosting.co.za>
Content
>  - 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).
Files
File name Uploaded
socketcan_v5.patch neologix, 2011-09-23.21:33:31
History
Date User Action Args
2011-09-23 21:33:37neologixsetrecipients: + neologix, amaury.forgeotdarc, orsenthil, vstinner, giampaolo.rodola, slanden, rosslagerwall, ogait87, mluis
2011-09-23 21:33:35neologixlinkissue10141 messages
2011-09-23 21:33:35neologixcreate