Message144476
> - 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). |
|
Date |
User |
Action |
Args |
2011-09-23 21:33:37 | neologix | set | recipients:
+ neologix, amaury.forgeotdarc, orsenthil, vstinner, giampaolo.rodola, slanden, rosslagerwall, ogait87, mluis |
2011-09-23 21:33:35 | neologix | link | issue10141 messages |
2011-09-23 21:33:35 | neologix | create | |
|