Generic HDLC interface continued
Hi,
I just want to return to the generic HDLC ioctl interface issue.
Currently (in 2.5) we have:
struct ifreq
{
char ifrn_name[IFNAMSIZ];
union {
... struct if_settings {
unsigned int type; /* Type of physical device or protocol */
union {
/* {atm/eth/dsl}_settings anyone ? */
union hdlc_settings {
raw_hdlc_proto raw_hdlc;
cisco_proto cisco;
fr_proto fr;
fr_proto_pvc fr_pvc;
}ifsu_hdlc;
union line_settings {
sync_serial_settings sync;
te1_settings te1;
}ifsu_line;
} ifs_ifsu;
} *ifru_settings; } ifr_ifru;
}
As the previous discussion showed, the ifs_ifsu union is just an artificial structure - it's used only for the purpose of filling the space in if_settings, as both sides (user and kernel) use the members hdlc_settings or line_settings with their real length (as opposed to the union length).
This situation creates some problems. First, IMHO the presence of artificial union is a problem (one could think we're really using the union and not variable length int type with *_proto / *_settings).
Second, the user-level utility has to supply the full length union if it want to query the kernel (with IF_GET_PROTO etc) - as it doesn't know what protocol has been set. It might not be a problem now, but will be when a bigger member struct is added (imagine an ioctl setting /getting a long encryption key etc).
Third, we are using complicated data types instead of simple flat ones, which negatively affect code readability.
Solution:
struct ifreq
{
char ifrn_name[IFNAMSIZ];
union ifr_ifru {
...
struct {
int type;
int size;
union {
raw_hdlc_proto *;
cisco_proto *;
etc_proto *;
sync_serial_settings *;
etc_line_settings *;Addressing the second problem (unknown data length) requires the caller (user-space utils) to supply allocated space size. The kernel would update the size to reflect the actual amount of data required, allowing the caller to allocate more space and try again (or ignore the unknown interface).
What is important here is that inner union consists of pointers to *_proto / *_settings structs and not of the structs themselves.
Another solution - using a different ifreq structs for different tasks (something like the sockaddr_*) - sort of:
struct ifreq_raw_hdlc_proto {
char ifrn_name[IFNAMSIZ];
int type;
int size;
raw_hdlc_proto settings;
};struct ifreq_cisco_proto {
char ifrn_name[IFNAMSIZ];
int type;
int size;
raw_hdlc_proto settings;
};The first 3 members would be common to all tasks (we would use a macro for that, of course).
Comments?
Signature · 2 lines
Krzysztof Halasa Network Administrator
<disclaimer>
It's ok for me to trim the Cc: list if noboy objects.
Don't hesistate to ask for clarification if my english comes from Mars. </disclaimer>
Krzysztof Halasa <[email protected]> :
[...]
Quoted 5 lines
Addressing the second problem (unknown data length) requires the caller (user-space utils) to supply allocated space size. The kernel would update the size to reflect the actual amount of data required, allowing the caller to allocate more space and try again (or ignore the unknown interface).
If size/limit of an underlying object is in a structure for other purpose than debygging, it means something (S) is working with an object and it (S) doesn't know what it is. Design proble: always work with an object whose identity you know or simply pass a reference to someone knowing better.
I don't see why a 'size' parameter is required. Thus I'm fine with the following (we are lucky enough that there is enough space in union ifr_ifru to hold ifru_settings):
struct ifreq
{
#define IFHWADDRLEN 6
#define IFNAMSIZ 16
union
{
char ifrn_name[IFNAMSIZ]; /* if name, e.g. "en0" */
} ifr_ifrn;
union {
struct sockaddr ifru_addr;
struct sockaddr ifru_dstaddr;
struct sockaddr ifru_broadaddr;
struct sockaddr ifru_netmask;
struct sockaddr ifru_hwaddr;
short ifru_flags;
int ifru_ivalue;
int ifru_mtu;
struct ifmap ifru_map;
char ifru_slave[IFNAMSIZ]; /* Just fits the size */
char ifru_newname[IFNAMSIZ];
char * ifru_data;
struct {
int type;
union {
raw_hdlc_proto *;
...
sync_serial_settings *;
etc_line_settings *;
}
} ifru_settings;
} ifr_ifru;
};Note however that struct ifreq on amphetamin (wrt lines of code) doesn't improve readability for everybody. That's a slightly different problem.
Quoted 2 lines
What is important here is that inner union consists of pointers to
*_proto/*_settingsstructs and not of the structs themselves.
Agree on this.
Quoted 2 lines
Another solution - using a different ifreq structs for different tasks (something like the sockaddr_*) - sort of:
[...]
I am not too fond of this and, again, what are these 'size' for ? If it's supposed to replace/duplicate ifreq, the 'settings' part should be a pointer imho. Same reason as before: size change => compatibility problems for tools (we have sources but downgrading tools when returning to previous kernel sucks).
Signature · 1 line
Ueimor
Quoted 11 lines · Francois Romieu
Quoted 5 lines
Addressing the second problem (unknown data length) requires the caller (user-space utils) to supply allocated space size. The kernel would update the size to reflect the actual amount of data required, allowing the caller to allocate more space and try again (or ignore the unknown interface).
If size/limit of an underlying object is in a structure for other purpose than debygging, it means something (S) is working with an object and it (S) doesn't know what it is.
Design proble: always work with an object whose
identity you know or simply pass a reference to someone knowing better.
Not exactly. The caller always knows meaning of the returned value (or it reports error etc). The caller doesn't just know size of the value _in_advance_, as it isn't constant. Still, meaning of the variable portion of the data is defined by the constant part.
It won't be a problem if allocating space is a kernel task.
Quoted 1 line
struct ifreq
...
Quoted 12 lines
struct {
int type;
union {
raw_hdlc_proto *;
...
sync_serial_settings *;
etc_line_settings *;
}
} ifru_settings;
} ifr_ifru;
};Quoted 2 lines
Note however that struct ifreq on amphetamin (wrt lines of code) doesn't improve readability for everybody. That's a slightly different problem.
Of course, I didn't want to include all the member structs directly in ifreq definition (in the header file). I just did it for the purpose of discussion.
Quoted 7 lines
Quoted 2 lines
What is important here is that inner union consists of pointers to
*_proto/*_settingsstructs and not of the structs themselves.Agree on this.
Quoted 2 lines
Another solution - using a different ifreq structs for different tasks (something like the sockaddr_*) - sort of:
Quoted 7 lines
[...]
I am not too fond of this and, again, what are these 'size' for ? If it's supposed to replace/duplicate ifreq, the 'settings' part should be a pointer imho. Same reason as before: size change => compatibility problems for tools (we have sources but downgrading tools when returning to previous kernel sucks).
Yes. That's why I want to finally have a well-designed interface, with no need for changes in the future (except as needed for new interfaces/protocols etc).
That's why I want to do it in 2.5 and then downport it to 2.4 and possibly even to 2.2.
Signature · 2 lines
Krzysztof Halasa Network Administrator
Krzysztof Halasa <[email protected]> :
[...]
Quoted 4 lines
Not exactly. The caller always knows meaning of the returned value (or it reports error etc). The caller doesn't just know size of the value _in_advance_, as it isn't constant. Still, meaning of the variable portion of the data is defined by the constant part.
The caller doesn't know size in advance but he gets 'type' and 'size' at the same time. Why shouldn't 'size' be deduced from 'type' ?
Signature · 1 line
Ueimor
Quoted 7 lines · Francois Romieu
Quoted 4 lines
Not exactly. The caller always knows meaning of the returned value (or it reports error etc). The caller doesn't just know size of the value _in_advance_, as it isn't constant. Still, meaning of the variable portion of the data is defined by the constant part.
The caller doesn't know size in advance but he gets 'type' and 'size' at the same time. Why shouldn't 'size' be deduced from 'type' ?
The caller don't know the result size. It mallocs some space which is long enough for usual things. Now, we have new super protocol with 100 KB of config data, and the caller requests settings of that protocol.
With "size" variable, the caller signals that there are 500 bytes for the result, and the kernel tells it that 100 KB are required, and that it's BTW the super protocol itself. The caller can allocate more space and try again, or just fail if it doesn't know the protocol.
Without "size" variable, the kernel tries to copy 100 KB to random space, and we get results ranging from SEGV to mysterious behavior.
Please note that there is no call which could return the protocol (or interface type) without copying the whole data - thus the caller (utility) is unable to skip unknown interfaces.
Signature · 2 lines
Krzysztof Halasa Network Administrator
Krzysztof Halasa <[email protected]> :
[...]
Quoted 3 lines
Please note that there is no call which could return the protocol (or interface type) without copying the whole data - thus the caller (utility) is unable to skip unknown interfaces.
Ok, the 'type' attribute isn't enough.
I feel like we are trying to do two things at the same time:
a) the size of the allocated area isn't required if we need to do something real with the data: if size doesn't match what is expected, we loose anyway
b) if we don't care about the copied data, we actually ask for the subtype of the interface
-> a) and b) are two different operations imho.
Signature · 1 line
Ueimor
Quoted 10 lines · Francois Romieu
Ok, the 'type' attribute isn't enough.
I feel like we are trying to do two things at the same time:
a) the size of the allocated area isn't required if we need to do something real with the data: if size doesn't match what is expected, we loose anyway
b) if we don't care about the copied data, we actually ask for the subtype of the interface-> a) and b) are two different operations imho.
Depends on the point of view, but generally it might be true. However, it is quite practical to do a+b in one call:
- utilities will always do it in exactly that order,
- we don't need to worry about races (process A gets type/size; process B changes protocol; process A gets invalid data).
Signature · 2 lines
Krzysztof Halasa Network Administrator