On 28 Mar 2001, Krzysztof Halasa wrote:
>
> 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.
> +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.
> +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??
> +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??
> --- linux-2.4.orig/include/linux/sockios.h Sun Nov 12 04:02:40 2000
> +++ linux-2.4/include/linux/sockios.h Wed Mar 28 16:35:23 2001
> @@ -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
Ivan Passos wrote:
> 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)
--
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.
On Wed, 28 Mar 2001, Jeff Garzik wrote:
> Ivan Passos wrote:
> > 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)
Then maybe it's really a better idea, after all. ;)
Krzysztof, please let us know what you think.
Regards,
Ivan
Ivan Passos <[email protected]> writes:
> 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.
> 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.
> > +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.
> > +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??
Exactly. With defaults filled in by the (some) driver.
> 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).
--
Krzysztof Halasa
Network Administrator
Jeff Garzik <[email protected]> writes:
> > 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)
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.
--
Krzysztof Halasa
Network Administrator
On 29 Mar 2001, Krzysztof Halasa wrote:
> Jeff Garzik <[email protected]> writes:
>
> > > 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)
>
> 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
Jeff Garzik <[email protected]> writes:
> 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?
--
Krzysztof Halasa
Network Administrator
On 1 Apr 2001, Krzysztof Halasa wrote:
> Jeff Garzik <[email protected]> writes:
> > 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?
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.
Jeff Garzik <[email protected]> writes:
> 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.
Right. The same applies to config_xxx structures.
This is why we're talking about it here.
> 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.
I don't see the real difference. SIOCxxx is just a name+value pair,
everybody can monitor SIOCxxx changes etc.
> 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).
We should then design it the right way :-)
> 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?
> 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.
--
Krzysztof Halasa
Network Administrator
Krzysztof Halasa <[email protected]> ?crit :
[...]
> > 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*
[...]
> 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 ?
--
Ueimor
Francois Romieu <[email protected]> writes:
> > 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.
> 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
> struc sub_req {
> int sub_ioctl;
... as we lose 4 bytes here (currently the union of structs in ifreq
is limited to 16 bytes)
> 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
--
Krzysztof Halasa
Network Administrator
Krzysztof Halasa <[email protected]> ?crit :
[...]
> But it's still more complicated than the first one and I'm not sure
> if doing that is worth it
>
> > 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.
[...]
> 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)
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.
--
Ueimor