On Tue, Feb 09, 2010 at 06:03:30PM +0100, Oliver Hartkopp wrote:
> Kurt Van Dijck wrote:
> > On Tue, Feb 09, 2010 at 10:33:05AM +0100, Wolfgang Grandegger wrote:
>
>
> >> Anyhow, there is no
> >> user of this code. Why should we add it?
>
> >
> > The newly introduce 'code' does not add features, nor does it add
> > 'functions', but it add 'flexibility'.
>
[...]
> Hi Kurt,
>
> if it's ok for you, i would like to move the discussion to your intended
> extensions ...
* first things first: the patch is white-space mangled too. sorry for
that. I attached a better one below.
* The proposed patch is seperate from any protocol extension. It's about
binary compatibility.
To visualize my problem, I included my current version of
sockaddr_can below. Note that when you would extend sockaddr_can in this or
any other way, you'll want the patch in order to not recompile every
userspace binary.
* Since I'm not the only contributor, I'm not in a position to force
such things, now or later :-). If the discussion is moved, I'll have
to accept that.
>
> AFAIK in J1939 you have broadcast announce messages (BAM) that send on two
> different CAN identifiers in one direction. This could be handled within the
> union.
In fact, there's a lot more.
J1939 introduces a network addressing (both source & destination).
And this addressing tables (comparable to arp tables I think) should be
kept in kernel.
The CAN id's are then seperated into SRC & DST fields, and a PGN
(iso11783 naming). SRC & DST is comparable to IP, in the way that is a
mapped address from an 'ISONAME' (iso11783 naming). The PGN is roughly
to be compared with a TCP port number, but again, roughly.
So, J1939 much better matches the kernel-to-userspace API sendto,
recvfrom, .... using all a 'struct sockaddr_xxx' derivate.
I could use a seperate struct, but don't think that's an elegant
solution.
The transport protocol indeed used 2 'PGN's, but they are fixed. the
transport protocol must thus be serialized, but that's a different
thing.
>
> What are your proposed new structure elements for a J1939 socketcan address?
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;
/* J1939 address information */
struct {
/*
* any of these parameters that are used in connect()
* can be overruled by using sendto()
*/
/* name: bind() sets source, connect sets dest */
uint64_t name; /* bind != connect */
/* pgn & priority: can be set by bind & connect */
uint32_t pgn;
uint8_t priority;
/*
* 'sa' is used in bind() & sendto(),
* 'da' is used in connect() & sendto()
*/
uint8_t sa; /* the originator address */
uint8_t da; /* the destination address */
} j1939;
/* reserved for future CAN protocols address information */
} can_addr;
};
this is my current working version.
Now, this is a preliminary thing, and only for j1939/iso11783.
>
> Regards,
> Oliver
Regards,
Kurt
>
---
This patch solves a problem that 'regular' socket types
do require the full struct sockaddr_can be present, while using only the
first few fields.
It does so by not testing on sizeof() but using a newly introduced macro
'partial_struct_size', which returns the size of the struct up to the
requested struct member.
When socketcan is compiled with bigger struct sockaddr_can, It will not
pose any problems with userspace binaries that got compiled with older
revisions of struct sockaddr_can.
Please not that the 'partial_struct_size' macro can only work when the
struct in only added to, not modified, which is the case here.
Signed-off-by: Kurt Van Dijck <[email protected]>
---
Index: include/socketcan/can/core.h
===================================================================
--- include/socketcan/can/core.h (revision 1124)
+++ include/socketcan/can/core.h (working copy)
@@ -52,6 +52,15 @@
#endif
};
+
+/*
+ * partial_struct_size
+ * macro to find the size of a struct up to a requested member (inclusive)
+ */
+#define partial_struct_size(member, type) \
+ (offsetof(typeof(type), member) + \
+ sizeof(((typeof(type) *)(0))->member))
+
/* function prototypes for the CAN networklayer core (af_can.c) */
extern int can_proto_register(struct can_proto *cp);
Index: net/can/isotp.c
===================================================================
--- net/can/isotp.c (revision 1124)
+++ net/can/isotp.c (working copy)
@@ -826,7 +826,7 @@
int err = 0;
int notify_enetdown = 0;
- if (len < sizeof(*addr))
+ if (len < partial_struct_size(can_addr.tp, *addr))
return -EINVAL;
if (addr->can_addr.tp.rx_id == addr->can_addr.tp.tx_id)
Index: net/can/bcm.c
===================================================================
--- net/can/bcm.c (revision 1124)
+++ net/can/bcm.c (working copy)
@@ -1610,6 +1610,9 @@
if (bo->bound)
return -EISCONN;
+ if (len < partial_struct_size(can_ifindex, *addr))
+ return -EINVAL;
+
/* bind a device to this socket */
if (addr->can_ifindex) {
struct net_device *dev;
Index: net/can/raw.c
===================================================================
--- net/can/raw.c (revision 1124)
+++ net/can/raw.c (working copy)
@@ -354,7 +354,7 @@
int err = 0;
int notify_enetdown = 0;
- if (len < sizeof(*addr))
+ if (len < partial_struct_size(can_ifindex, *addr))
return -EINVAL;
lock_sock(sk);
Index: net/can/bcm-prior-2-6-22.c
===================================================================
--- net/can/bcm-prior-2-6-22.c (revision 1124)
+++ net/can/bcm-prior-2-6-22.c (working copy)
@@ -1475,6 +1475,9 @@
if (bo->bound)
return -EISCONN;
+ if (len < partial_struct_size(can_ifindex, *addr))
+ return -EINVAL;
+
/* bind a device to this socket */
if (addr->can_ifindex) {
struct net_device *dev;
_______________________________________________
Socketcan-core mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/socketcan-core
