RFC: configuring net interfaces
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 8 lines · Francois Romieu
Review
Quoted 6 lines
(quoted diff) +6
-0+struct hdlc_physical /* 10 bytes */ +{ + unsigned int interface; + unsigned int clock_rate; + unsigned short clock_type; +};What do you mean with 'interface' ?
That's a physical interface like V.35 or RS232.
Quoted 14 lines
[...]
Review
Quoted 11 lines
(quoted diff) +11
-0+struct fr_protocol /* 12 bytes */ +{ + unsigned short lmi_type; + unsigned short t391; + unsigned short t392; + unsigned short n391; + unsigned short n392; + unsigned short n393; +}; + +* n200, t200 ?
What's that?
Quoted 1 line
Do we put the crc type here ?
I don't think so. Frame Relay uses only standard CRC. Correct me if I'm wrong.
Quoted 6 lines
Quoted 6 lines
/*
* Interface request structure used for socket
* ioctl's. All interface ioctl's must have parameter(quoted diff) +0
-0@@ -95,6 +121,9 @@ char ifru_slave[IFNAMSIZ]; /* Just fits the size */ char ifru_newname[IFNAMSIZ];
Quoted 8 lines
Review line 128
Quoted 6 lines
(quoted diff) +3
-0char * ifru_data; + struct hdlc_physical hdlc_phy; + struct hdlc_protocol hdlc_proto; + struct fr_protocol fr_proto; } ifr_ifru; };All the structs or just an union of ?
An (existing) union, of course. That's part of ifmap struct, with meaning determined by ioctl command code.
Quoted 1 line
That is: is an SIOCXXX allowed to poke into different structs or not ?
Only one struct at a time :-)
Quoted 10 lines
include/linux/sockios.h line 80
Quoted 8 lines
include/linux/sockios.h +2
-0@@ -76,6 +76,12 @@#define SIOCSIFDIVERT 0x8945/* Set frame diversion options */ #define SIOCETHTOOL 0x8946/* Ethtool interface */ +#define SIOCSHDLC_PHY 0x8947/* set physical HDLC iface */ +#define SIOCGHDLC_PHY 0x8948/* get physical HDLC iface */ What type of operation do you mean by get/set ?
Get/set all device characteristics described by the struct. For example, get/set interface type, clock type and rate. Or, with other command(s), get/set Frame-Relay related parameters.
Quoted 3 lines
While we're here, could we agree on the notion of raw hdlc, i.e. :
- no address, no command. crc present (ARPHRD_RAWHDLC?);
- no address, no command, no crc (ARPHRD_PATHOLOGICHDLC?).
Do we really need another ARPHRD for that?
BTW: What protocol(s) use such CRC-less HDLC?
Signature · 2 lines
Krzysztof Halasa Network Administrator
Quoted 5 lines · Krzysztof Halasa
What about a patch like this:
That would move interface configuration out of private ioctl range, making it universal for all types of interfaces (now, we have different configuration mechanisms even between different HDLC cards).
Yes! This would be great.
Review
Quoted 6 lines
(quoted diff) +6
-0+struct hdlc_physical /* 10 bytes */ +{ + unsigned int interface; + unsigned int clock_rate; + unsigned short clock_type; +};
I guess 'interface' means media type (e.g. V.35, RS-232, X.21, etc.). Maybe it would be more intuitive to call it 'media'. What do you think?
Also, for synchronous cards that have built-in DSU/CSU's (such as the Cylades-PC300/TE), it's also necessary to configure T1/E1 parameters (e.g. line code, frame mode, active channels, etc.). Should we make these parameters also available here or keep them in the driver realm?? I think we should have them here too, but maybe you see some problem with this that I don't. Please advice.
Review
Quoted 4 lines
(quoted diff) +4
-0+struct hdlc_protocol /* 4 bytes */ +{ + unsigned int proto; +};
What are the possible protocols to be set here?? I imagine PPP, Cisco HDLC, Raw HDLC, Frame Relay, X.25, and ... ?? Is that it??
Review
Quoted 9 lines
(quoted diff) +9
-0+struct fr_protocol /* 12 bytes */ +{ + unsigned short lmi_type; + unsigned short t391; + unsigned short t392; + unsigned short n391; + unsigned short n392; + unsigned short n393; +};
So we would have hdlc_protocol->proto set to PROTO_FR, and then the details about Frame Relay would be set in this separate structure. Is that what you have in mind??
include/linux/sockios.h line 84
Quoted 12 lines
include/linux/sockios.h +6
-0@@ -76,6 +76,12 @@#define SIOCSIFDIVERT 0x8945/* Set frame diversion options */ #define SIOCETHTOOL 0x8946/* Ethtool interface */ +#define SIOCSHDLC_PHY 0x8947/* set physical HDLC iface */ +#define SIOCGHDLC_PHY 0x8948/* get physical HDLC iface */ +#define SIOCSHDLC_PROTO 0x8949/* set HDLC protocol */ +#define SIOCGHDLC_PROTO 0x894A/* get HDLC protocol */ +#define SIOCSFR_PROTO 0x894B/* set Frame-Relay protocol */ +#define SIOCGFR_PROTO 0x894C/* get Frame-Relay protocol */
Maybe it's a better idea to have just two ioctl's here (GET and SET), and have "subioctl's" inside the structure passed to the HDLC layer (and defined by the HDLC layer). This would allow changes in the HDLC layer without having to change sockios.h (you'd still have to change HDLC's code and definitions, but this would be more self-contained). Again, this may be better, or maybe not. What do you think?
Regards,
Ivan
Quoted 6 lines · Ivan Passos
Maybe it's a better idea to have just two ioctl's here (GET and SET), and have "subioctl's" inside the structure passed to the HDLC layer (and defined by the HDLC layer). This would allow changes in the HDLC layer without having to change
sockios.h(you'd still have to change HDLC's code and definitions, but this would be more self-contained). Again, this may be better, or maybe not. What do you think?
That's essentially what's happening with ethtool
(include/linux/ethtool.h in 2.4.3-pre8)
Signature · 3 lines
Jeff Garzik | May you have warm words on a cold evening, Building 1024 | a full moon on a dark night, MandrakeSoft | and a smooth road all the way to your door.
Quoted 10 lines · Jeff Garzik
Quoted 6 lines · Ivan Passos
Maybe it's a better idea to have just two ioctl's here (GET and SET), and have "subioctl's" inside the structure passed to the HDLC layer (and defined by the HDLC layer). This would allow changes in the HDLC layer without having to change
sockios.h(you'd still have to change HDLC's code and definitions, but this would be more self-contained). Again, this may be better, or maybe not. What do you think?That's essentially what's happening with ethtool
(include/linux/ethtool.hin 2.4.3-pre8)
Then maybe it's really a better idea, after all. ;)
Krzysztof, please let us know what you think.
Regards,
Ivan
Quoted 2 lines · Ivan Passos
I guess 'interface' means media type (e.g. V.35, RS-232, X.21, etc.). Maybe it would be more intuitive to call it 'media'. What do you think?
Probably.
Quoted 6 lines
Also, for synchronous cards that have built-in DSU/CSU's (such as the Cylades-PC300/TE), it's also necessary to configure T1/E1 parameters (e.g. line code, frame mode, active channels, etc.). Should we make these parameters also available here or keep them in the driver realm?? I think we should have them here too, but maybe you see some problem with this that I don't.
No problem. That was just an example.
Quoted 7 lines
Review
Quoted 4 lines
(quoted diff) +4
-0+struct hdlc_protocol /* 4 bytes */ +{ + unsigned int proto; +};What are the possible protocols to be set here?? I imagine PPP, Cisco HDLC, Raw HDLC, Frame Relay, X.25, and ... ?? Is that it??
Exactly.
Quoted 5 lines
Quoted 5 lines
(quoted diff) +5
-0+struct fr_protocol /* 12 bytes */ +{ + unsigned short lmi_type; + unsigned short t391; + unsigned short t392;
Quoted 8 lines
Review
Quoted 4 lines
(quoted diff) +4
-0+ unsigned short n391; + unsigned short n392; + unsigned short n393; +};So we would have
hdlc_protocol->protoset toPROTO_FR, and then the details about Frame Relay would be set in this separate structure. Is that what you have in mind??
Exactly. With defaults filled in by the (some) driver.
Quoted 6 lines
Maybe it's a better idea to have just two ioctl's here (GET and SET), and have "subioctl's" inside the structure passed to the HDLC layer (and defined by the HDLC layer). This would allow changes in the HDLC layer without having to change
sockios.h(you'd still have to change HDLC's code and definitions, but this would be more self-contained). Again, this may be better, or maybe not. What do you think?
I think it would make it more complicated. ioctl namespace is huge enough. Without changing ifmap struct size (which would require recompilation of all programs using it - ip, ifconfig etc), the *_protocol/physical structures can only be 16 bytes long each (8 shorts or 4 ints) - I would rather not put command code there.
Of course, the structs I outlined are just an example, which need checking for max and min values and then setting variable length (int/short/char).
Signature · 2 lines
Krzysztof Halasa Network Administrator
Quoted 9 lines · Jeff Garzik
Quoted 6 lines
Maybe it's a better idea to have just two ioctl's here (GET and SET), and have "subioctl's" inside the structure passed to the HDLC layer (and defined by the HDLC layer). This would allow changes in the HDLC layer without having to change
sockios.h(you'd still have to change HDLC's code and definitions, but this would be more self-contained). Again, this may be better, or maybe not. What do you think?That's essentially what's happening with ethtool
(include/linux/ethtool.hin 2.4.3-pre8)
Right. While I don't think ethernet-only interface is our ultimate goal, I'll look at it again to see if I can stole some idea there.
Signature · 2 lines
Krzysztof Halasa Network Administrator
Quoted 9 lines · Krzysztof Halasa
Quoted 7 lines · Jeff Garzik
Quoted 6 lines
Maybe it's a better idea to have just two ioctl's here (GET and SET), and have "subioctl's" inside the structure passed to the HDLC layer (and defined by the HDLC layer). This would allow changes in the HDLC layer without having to change
sockios.h(you'd still have to change HDLC's code and definitions, but this would be more self-contained). Again, this may be better, or maybe not. What do you think?
Quoted 5 lines
Quoted 2 lines
That's essentially what's happening with ethtool
(include/linux/ethtool.hin 2.4.3-pre8)Right. While I don't think ethernet-only interface is our ultimate goal, I'll look at it again to see if I can stole some idea there.
I'm not suggesting you modify ethtool for your needs :) But ethtool
perfectly illustrates the technique of using a single socket ioctl (SIOCETHTOOL) to extend a set of standard, domain-specific ioctls (ETHTOOL_xxx) to Linux networking drivers.
Jeff
Quoted 4 lines · Jeff Garzik
I'm not suggesting you modify ethtool for your needs :) But ethtool
perfectly illustrates the technique of using a single socket ioctl (SIOCETHTOOL) to extend a set of standard, domain-specific ioctls (ETHTOOL_xxx) to Linux networking drivers.
I know. The problem is I don't see here any advantages over many SIOCxxx command codes, while there are disadvantages.
Simple things are IMHO better, and ioctl was designed to handle many simple commands (instead of one complex).
Am I missing something?
Signature · 2 lines
Krzysztof Halasa Network Administrator
Quoted 6 lines · Krzysztof Halasa
Quoted 4 lines · Jeff Garzik
I'm not suggesting you modify ethtool for your needs :) But ethtool
perfectly illustrates the technique of using a single socket ioctl (SIOCETHTOOL) to extend a set of standard, domain-specific ioctls (ETHTOOL_xxx) to Linux networking drivers.
Quoted 6 lines
I know. The problem is I don't see here any advantages over many SIOCxxx command codes, while there are disadvantages.
Simple things are IMHO better, and ioctl was designed to handle many simple commands (instead of one complex).Am I missing something?
IMHO we should get away from adding hardware-type-specific ioctls from the generic SIOCxxx list.
Sure it is easy to dump a bunch of new ioctls into sockios.h. But having "one big header that covers all hardware types and ioctl situations" does not seem like the right solution to me.
First of all, you as the HDLC subsystem maintainer have a lot more control over what goes into include/linux/hdlc.h than include/linux/sockios.h. New SIOCxxxx ioctls should not be added on a whim, but after examination of the issues involved. Making a mistake and adding a bad/pointless SIOCxxxx ioctl means you are stuck with it for a long time. The same applies to ioctls in hdlc.h of course -- but the key distinction is that you are 100% sure of the issues involved because changes are localized to your own domain.
Further, each change to sockios.h affects a LOT of code, both in and outside the kernel. Localizing your changes also localizes the effects of bad namespace and ioctl choices (and bugs, though in this case that would be rare).
Finally, I have this (perhaps crazy) idea that we should move in the direction of removing ALL hardware-related ioctls from sockios.h, making SIOCxxxx the domain of generic network device ioctls.
Comments welcome. IMHO domain-specific ioctls are a better direction than the current make-sockios.h-huge-with-new-ioctls approach.
Regards,
Jeff
P.S. It would be awesome if you would consider CC'ing [email protected]. Some developers who might have valuable input do not subscribe to linux-kernel, or are not able to read all of the net-related linux-kernel messages.
Quoted 4 lines · Jeff Garzik
First of all, you as the HDLC subsystem maintainer have a lot more control over what goes into
include/linux/hdlc.hthaninclude/linux/sockios.h. New SIOCxxxx ioctls should not be added on a whim, but after examination of the issues involved.
Right. The same applies to config_xxx structures.
This is why we're talking about it here.
Quoted 5 lines
Making a mistake
and adding a bad/pointless SIOCxxxx ioctl means you are stuck with it for a long time. The same applies to ioctls inhdlc.hof course -- but the key distinction is that you are 100% sure of the issues involved because changes are localized to your own domain.
I don't see the real difference. SIOCxxx is just a name+value pair, everybody can monitor SIOCxxx changes etc.
Quoted 4 lines
Further, each change to
sockios.haffects a LOT of code, both in and outside the kernel. Localizing your changes also localizes the effects of bad namespace and ioctl choices (and bugs, though in this case that would be rare).
We should then design it the right way :-)
Quoted 3 lines
Finally, I have this (perhaps crazy) idea that we should move in the direction of removing ALL hardware-related ioctls from
sockios.h, making SIOCxxxx the domain of generic network device ioctls.
What are these "generic network device ioctls"? Is add-an-IP-address a generic enough? Not all devices use IP.
Is set-interface-speed a generic command? Most devices have something like "speed", clock rate or something like this.
The question is: where would you like to move these ioctls to?
Quoted 2 lines
Comments welcome. IMHO domain-specific ioctls are a better direction than the current make-sockios.h-huge-with-new-ioctls approach.
I think we should separate two things there:
- the place (files) where SIOCxxx values are defined
- the way we use ioctl call.
The first question is less important (files are just files, both sockios.h and protocol header files are acceptable I think). We just have to make sure ioctls are unique across the kernel - current sockios.h does the job, but we can, for example, use a prefix like #define HDLC_PREFIX 0xHD7C0000 and then
#define SIOC_add-HDLC-something HDLC_PREFIX | 0x0001
(It looks more complicated and thus we should avoid it I think).
The second question looks more important, as it influences the actual code being used.
I just believe something like:
struct ifreq {
name;
union {
struct ifru_addr;
struct ifru_ipv6_something;
...
struct hdlc_physical;
struct eth_physical;
struct fr_protocol;
...
}ifru;
}and calling ioctl with:
ifreq.name = "qwe0";
ifreq.ifru.fr_protocol.t391 = 20;
ifreq.ifru.fr_protocol.n293 = 5;
ioctl(s, SIOC_SET_FR_PROTO_PARAMETERS, &ifreq);is technically much better than:
struct ifreq {
name;
void *data;
}you have to call it with:
proto = malloc();
ifreq.name = "qwe0";
ifreq.data = proto;
(int*)proto = SIOC_SET_FR_PROTO_PARAMETERS;
(fr_protocol)(((int*)proto) + 1).fr_protocol.t391 = 20;
(fr_protocol)(((int*)proto) + 1).fr_protocol.n393 = 5;
ioctl(s, SIOC_FR_PROTO, &ifreq);and it ends in the protocol driver:do_ioctl(dev, *ifreq, int cmd)
{
verify_area(..., ifreq.data, sizeof(int));
if (*(int*)ifreq.data == SIOC_SET_FR_PROTO_PARAMETERS) {
verify_area(ifreq.data + sizeof(int), sizeof(fr_protocol));
etc.Your comments?
In short and general: I think the quality (and thus simplicity) of the actual code is more important than creating mechanisms designed to fight future possible binary incompatibility issues, especially issues with helper utils like ifconfig which can be easily rebuild from source.
Of course, we have to design good code and this is why we're all here.
Signature · 2 lines
Krzysztof Halasa Network Administrator
Krzysztof Halasa <[email protected]> écrit :
[...]
Quoted 6 lines
Quoted 2 lines
Comments welcome. IMHO domain-specific ioctls are a better direction than the current make-sockios.h-huge-with-new-ioctls approach.
I think we should separate two things there:
- the place (files) where SIOCxxx values are defined
- the way we use ioctl call.
(1) and (2) may be related:
no sub-ioctl (2) + scattered files (1) = *ouch*
[...]
Quoted 8 lines
you have to call it with:
proto = malloc(); ifreq.name = "qwe0"; ifreq.data = proto; (int*)proto = SIOC_SET_FR_PROTO_PARAMETERS; (fr_protocol)(((int*)proto) + 1).fr_protocol.t391 = 20; (fr_protocol)(((int*)proto) + 1).fr_protocol.n393 = 5; ioctl(s, SIOC_FR_PROTO, &ifreq);
Variant:
struct sub_req sub; sub.fr_protocol.t391 = 20; sub.fr_protocol.n293 = 5; sub.sub_ioctl = SIOC_SET_FR_PROTO_PARAMETERS; ifreq.name = "qwe0"; ifreq.data = ⊂ ioctl(s, SIOC_FR_PROTO, &ifreq);
At least it avoids digging at a special position in a structure to know the expected operation and the underlying structure.
struc sub_req {
int sub_ioctl;
union {
struct fr_protocol fr_prot;
...
struct xx_protocol xx_prot;
}
}struct if_req {
int name;
struct sub_req sub;
}It could avoid a flat name-space. Opinion anyone ?
Signature · 1 line
Ueimor
Quoted 6 lines · Francois Romieu
Quoted 3 lines
I think we should separate two things there:
- the place (files) where SIOCxxx values are defined
- the way we use ioctl call.(1) and (2) may be related:
no sub-ioctl (2) + scattered files (1) = *ouch*
Sure.
Quoted 9 lines
Variant:
struct sub_req sub; sub.fr_protocol.t391 = 20; sub.fr_protocol.n293 = 5; sub.sub_ioctl = SIOC_SET_FR_PROTO_PARAMETERS; ifreq.name = "qwe0"; ifreq.data = ⊂ ioctl(s, SIOC_FR_PROTO, &ifreq);
Yes, it's a little nicer than my second variant. But it's still more complicated than the first one and I'm not sure if doing that is worth it
Quoted 2 lines
struc sub_req {
int sub_ioctl;... as we lose 4 bytes here (currently the union of structs in ifreq is limited to 16 bytes)
Quoted 6 lines
union {
struct fr_protocol fr_prot;
...
struct xx_protocol xx_prot;
}
}What might be actually better than my first variant, is a variable-length data:
struct ifreq {
char name[16];
union {
...
struct {
int sub_command;
int data_length;
void *data;
}
}ifru;
}... while "data" would be fr_protocol, eth_physical etc.
It's (of course) more complicated, but there is a gain:
- we can have different size requests (from 0 bytes to, say, 100KB)
- we split SIOC namespace into domains
- the core ioctl handler can still "verify" data area for the underlying driver
Signature · 2 lines
Krzysztof Halasa Network Administrator
Krzysztof Halasa <[email protected]> écrit :
[...]
Quoted 8 lines
But it's still more complicated than the first one and I'm not sure if doing that is worth it
Quoted 2 lines
struc sub_req { int sub_ioctl;... as we lose 4 bytes here (currently the union of structs in ifreq is limited to 16 bytes)
I missed that. Point taken.
[...]
Quoted 12 lines
struct ifreq {
char name[16];
union {
...
struct {
int sub_command;
int data_length;
void *data;
}
}ifru;
}Quoted 4 lines
... while "data" would be
fr_protocol,eth_physicaletc.It's (of course) more complicated, but there is a gain:
- we can have different size requests (from 0 bytes to, say, 100KB)
Fine with me (some day we'll surely end passing those data via a read if we need 300Mo but we're not there :o) ).
[Other points]
Yes.
Signature · 1 line
Ueimor
From: "Krzysztof Halasa" <[email protected]>
Review
Quoted 6 lines
(quoted diff) +6
-0+struct hdlc_physical /* 10 bytes */ +{ + unsigned int interface; + unsigned int clock_rate; + unsigned short clock_type; +};
What about encoding (NRZ/NRZI)?
Plus I think the CRC type would be a good idea for raw HDLC mode. (CRC-16, CRC-32, no CRC)
Paul Fulghum
[email protected]
Krzysztof Halasa <[email protected]> écrit :
[...]
Quoted 1 line
That's a physical interface like V.35 or RS232.
Ok.
[...]
Quoted 3 lines
Quoted 1 line
* n200, t200 ?
What's that?
Parameters for retransmission of a trame specified in Q922. t200 is the timeout value and n200 the maximal number of retransmissions. They can be negocied and default to t200=1,5s, n200=3.
Quoted 4 lines
Quoted 1 line
Do we put the crc type here ?
I don't think so. Frame Relay uses only standard CRC. Correct me if I'm wrong.
Ok.
[...]
Quoted 6 lines
Quoted 3 lines
While we're here, could we agree on the notion of raw hdlc, i.e. :
- no address, no command. crc present (ARPHRD_RAWHDLC?);
- no address, no command, no crc (ARPHRD_PATHOLOGICHDLC?).Do we really need another ARPHRD for that?
BTW: What protocol(s) use such CRC-less HDLC?
No protocol I know. Sometime one just want to verify how a driver performs when he receives badly fscked data. One can live without it.
Signature · 1 line
Ueimor
Quoted 11 lines · Paul Fulghum
Review
Quoted 6 lines
(quoted diff) +6
-0+struct hdlc_physical /* 10 bytes */ +{ + unsigned int interface; + unsigned int clock_rate; + unsigned short clock_type; +};What about encoding (NRZ/NRZI)?
Plus I think the CRC type would be a good idea for raw HDLC mode. (CRC-16, CRC-32, no CRC)
Here - I think so. + good defaults.
Signature · 2 lines
Krzysztof Halasa Network Administrator
Quoted 3 lines · Francois Romieu
Parameters for retransmission of a trame specified in Q922. t200 is the timeout value and n200 the maximal number of retransmissions. They can be negocied and default to t200=1,5s, n200=3.
Hmm... I've taken a look at it, but it seems to me that they are only used with "acknowledged multiple frame operation". Isn't it for ISDN only? With Frame Relay, we rather use unacknowledged transfers and UI frames.
Of course, if we have an implementation using t200, n200 or other parameters, they should be added to the structure.
Signature · 2 lines
Krzysztof Halasa Network Administrator
Quoted 10 lines
Quoted 3 lines
Parameters for retransmission of a trame specified in Q922. t200 is the timeout value and n200 the maximal number of retransmissions. They can be negocied and default to t200=1,5s, n200=3.
Hmm... I've taken a look at it, but it seems to me that they are only used with "acknowledged multiple frame operation". Isn't it for ISDN only? With Frame Relay, we rather use unacknowledged transfers and UI frames.
Of course, if we have an implementation using t200, n200 or other parameters, they should be added to the structure.
It makes sense to me to defer adding the extra parameters (as you suggest) until there is actual support for SVCs or acknowledged mode over PVCs. The majority of what I've seen is PVC/UI.
Paul Fulghum [email protected]
Microgate Corporation www.microgate.com