Greetings,
the following patches try and address some of the previously
made remarks:
- avoid variable sized ifreq (Krzysztof Halasa);
- avoid untyped data in ifreq (Jeff Garzik);
- SIOCDEVICE/SIOCWANDEV, hdlc/raw_hdlc (ma pomme).
It's built on top of Krzysztof Halasa's patch applied to 2.5.5-pre1
(ftp://ftp.pm.waw.pl/pub/Linux/hdlc/experimental/hdlc-2.5.3.patch.gz).
[0/3]:
- SIOCDEVICE -> SIOCWANDEV conversion
- hdlc_proto -> raw_hdlc_proto
[1/3]:
- struct if_settings in struct ifreq becomes struct if_settings *;
- anonymous data pointer in struct if_settings is now a pointer to a
union of struct containing l2 parameters. These structs can be of
arbitrary size. So far, hdlc_settings is the only one available;
- struct hdlc_settings is declared in (new) include/linux/hdlc/ioctl.h.
The underlying settings (raw hdlc, cisco, fr) are moved from
include/linux/hdlc.h to here;
- shortcuts for accessing protocol specific settings are defined in
include/linux/hdlc/ioctl.h (shamelessly inspired from
#define ifr_map ifr_ifru.ifru_map and friends).
[2/3]:
- conversion of drivers/net/wan/hdlc_xxx.c files.
[3/3]:
- some device are converted (c101.c/dscc4.c/farsync.c/n2.c).
Remarks:
- As hdlc_{raw/cisco/fr/x25} doesn't need knowledge of struct ifreq, I would
happily pass them a pointer to a struct if_settings. This way the 2 stage
ioctl would be clearer imho.
- It compiles but I have to sacrifice a disk for 2.5 before it can be claimed
to work (TM).
- Patches are archived at <http://www.cogenit.fr/dscc4/hdlc-api/2.5.5-pre1/>.
Comments/code welcome.
diff -burpN linux-2.5.5-pre1-kh/drivers/net/wan/c101.c linux-2.5.5-pre1-ma_pomme/drivers/net/wan/c101.c
--- linux-2.5.5-pre1-kh/drivers/net/wan/c101.c Sun Feb 17 17:39:27 2002
+++ linux-2.5.5-pre1-ma_pomme/drivers/net/wan/c101.c Sun Feb 17 17:32:59 2002
@@ -189,7 +189,7 @@ static int c101_ioctl(struct net_device
return 0;
}
#endif
- if (cmd != SIOCDEVICE)
+ if (cmd != SIOCWANDEV)
return hdlc_ioctl(dev, ifr, cmd);
switch(ifr->ifr_settings.type) {
diff -burpN linux-2.5.5-pre1-kh/drivers/net/wan/dscc4.c linux-2.5.5-pre1-ma_pomme/drivers/net/wan/dscc4.c
--- linux-2.5.5-pre1-kh/drivers/net/wan/dscc4.c Sun Feb 17 17:39:27 2002
+++ linux-2.5.5-pre1-ma_pomme/drivers/net/wan/dscc4.c Sun Feb 17 17:32:59 2002
@@ -1073,7 +1073,7 @@ static int dscc4_ioctl(struct net_device
if (dev->flags & IFF_UP)
return -EBUSY;
- if (cmd != SIOCDEVICE)
+ if (cmd != SIOCWANDEV)
return -EOPNOTSUPP;
switch(ifr->ifr_settings.type) {
diff -burpN linux-2.5.5-pre1-kh/drivers/net/wan/farsync.c linux-2.5.5-pre1-ma_pomme/drivers/net/wan/farsync.c
--- linux-2.5.5-pre1-kh/drivers/net/wan/farsync.c Sun Feb 17 17:39:27 2002
+++ linux-2.5.5-pre1-ma_pomme/drivers/net/wan/farsync.c Sun Feb 17 17:32:59 2002
@@ -1240,7 +1240,7 @@ fst_ioctl ( struct net_device *dev, stru
return set_conf_from_info ( card, port, &info );
- case SIOCDEVICE:
+ case SIOCWANDEV:
switch ( ifr->ifr_settings.type )
{
case IF_GET_IFACE:
diff -burpN linux-2.5.5-pre1-kh/drivers/net/wan/hdlc_generic.c linux-2.5.5-pre1-ma_pomme/drivers/net/wan/hdlc_generic.c
--- linux-2.5.5-pre1-kh/drivers/net/wan/hdlc_generic.c Sun Feb 17 17:39:27 2002
+++ linux-2.5.5-pre1-ma_pomme/drivers/net/wan/hdlc_generic.c Sun Feb 17 17:33:07 2002
@@ -72,7 +72,7 @@ int hdlc_ioctl(struct net_device *dev, s
hdlc_device *hdlc = dev_to_hdlc(dev);
unsigned int proto;
- if (cmd != SIOCDEVICE)
+ if (cmd != SIOCWANDEV)
return -EINVAL;
switch(ifr->ifr_settings.type) {
diff -burpN linux-2.5.5-pre1-kh/drivers/net/wan/hdlc_raw.c linux-2.5.5-pre1-ma_pomme/drivers/net/wan/hdlc_raw.c
--- linux-2.5.5-pre1-kh/drivers/net/wan/hdlc_raw.c Sun Feb 17 17:39:21 2002
+++ linux-2.5.5-pre1-ma_pomme/drivers/net/wan/hdlc_raw.c Sun Feb 17 17:37:48 2002
@@ -49,7 +49,7 @@ int hdlc_raw_ioctl(hdlc_device *hdlc, st
if (ifr->ifr_settings.data_length < size)
return -ENOMEM; /* buffer too small */
if (copy_to_user(ifr->ifr_settings.data,
- &hdlc->state.hdlc.settings, size))
+ &hdlc->state.raw_hdlc.settings, size))
return -EFAULT;
ifr->ifr_settings.data_length = size;
return 0;
@@ -64,15 +64,15 @@ int hdlc_raw_ioctl(hdlc_device *hdlc, st
if (ifr->ifr_settings.data_length != size)
return -ENOMEM; /* incorrect data length */
- if (copy_from_user(&hdlc->state.hdlc.settings,
+ if (copy_from_user(&hdlc->state.raw_hdlc.settings,
ifr->ifr_settings.data, size))
return -EFAULT;
/* FIXME - put sanity checks here */
hdlc_detach(hdlc);
- result=hdlc->attach(hdlc, hdlc->state.hdlc.settings.encoding,
- hdlc->state.hdlc.settings.parity);
+ result=hdlc->attach(hdlc, hdlc->state.raw_hdlc.settings.encoding,
+ hdlc->state.raw_hdlc.settings.parity);
if (result) {
hdlc->proto = -1;
return result;
diff -burpN linux-2.5.5-pre1-kh/drivers/net/wan/n2.c linux-2.5.5-pre1-ma_pomme/drivers/net/wan/n2.c
--- linux-2.5.5-pre1-kh/drivers/net/wan/n2.c Sun Feb 17 17:39:27 2002
+++ linux-2.5.5-pre1-ma_pomme/drivers/net/wan/n2.c Sun Feb 17 17:32:59 2002
@@ -260,7 +260,7 @@ static int n2_ioctl(struct net_device *d
return 0;
}
#endif
- if (cmd != SIOCDEVICE)
+ if (cmd != SIOCWANDEV)
return hdlc_ioctl(dev, ifr, cmd);
switch(ifr->ifr_settings.type) {
diff -burpN linux-2.5.5-pre1-kh/include/linux/hdlc.h linux-2.5.5-pre1-ma_pomme/include/linux/hdlc.h
--- linux-2.5.5-pre1-kh/include/linux/hdlc.h Sun Feb 17 17:39:24 2002
+++ linux-2.5.5-pre1-ma_pomme/include/linux/hdlc.h Sun Feb 17 17:38:27 2002
@@ -53,7 +53,7 @@ typedef struct {
typedef struct {
unsigned short encoding;
unsigned short parity;
-}hdlc_proto;
+}raw_hdlc_proto;
#define LMI_DEFAULT 0 /* Default (current) setting */
@@ -251,8 +251,8 @@ typedef struct hdlc_device_struct {
}cisco;
struct {
- hdlc_proto settings;
- }hdlc;
+ raw_hdlc_proto settings;
+ }raw_hdlc;
struct {
struct ppp_device pppdev;
diff -burpN linux-2.5.5-pre1-kh/include/linux/sockios.h linux-2.5.5-pre1-ma_pomme/include/linux/sockios.h
--- linux-2.5.5-pre1-kh/include/linux/sockios.h Sun Feb 17 17:39:27 2002
+++ linux-2.5.5-pre1-ma_pomme/include/linux/sockios.h Sun Feb 17 16:39:23 2002
@@ -81,7 +81,7 @@
#define SIOCGMIIREG 0x8948 /* Read MII PHY register. */
#define SIOCSMIIREG 0x8949 /* Write MII PHY register. */
-#define SIOCDEVICE 0x894A /* get/set netdev parameters */
+#define SIOCWANDEV 0x894A /* get/set netdev parameters */
/* ARP cache control calls. */
/* 0x8950 - 0x8952 * obsolete calls, don't re-use */
diff -burpN linux-2.5.5-pre1-kh/net/core/dev.c linux-2.5.5-pre1-ma_pomme/net/core/dev.c
--- linux-2.5.5-pre1-kh/net/core/dev.c Sun Feb 17 17:39:27 2002
+++ linux-2.5.5-pre1-ma_pomme/net/core/dev.c Sun Feb 17 16:40:52 2002
@@ -2111,7 +2111,7 @@ static int dev_ifsioc(struct ifreq *ifr,
cmd == SIOCGMIIPHY ||
cmd == SIOCGMIIREG ||
cmd == SIOCSMIIREG ||
- cmd == SIOCDEVICE) {
+ cmd == SIOCWANDEV) {
if (dev->do_ioctl) {
if (!netif_device_present(dev))
return -ENODEV;
@@ -2277,7 +2277,7 @@ int dev_ioctl(unsigned int cmd, void *ar
*/
default:
- if (cmd == SIOCDEVICE ||
+ if (cmd == SIOCWANDEV ||
(cmd >= SIOCDEVPRIVATE &&
cmd <= SIOCDEVPRIVATE + 15)) {
dev_load(ifr.ifr_name);
Francois Romieu wrote:
> [0/3]:
> - SIOCDEVICE -> SIOCWANDEV conversion
> - hdlc_proto -> raw_hdlc_proto
patch looks ok to me.
> diff -burpN linux-2.5.5-pre1-kh/include/linux/sockios.h linux-2.5.5-pre1-ma_pomme/include/linux/sockios.h
> --- linux-2.5.5-pre1-kh/include/linux/sockios.h Sun Feb 17 17:39:27 2002
> +++ linux-2.5.5-pre1-ma_pomme/include/linux/sockios.h Sun Feb 17 16:39:23 > -#define SIOCDEVICE 0x894A /* get/set netdev parameters */
> +#define SIOCWANDEV 0x894A /* get/set netdev parameters */
a better comment might be nice...
--
Jeff Garzik | "Why is it that attractive girls like you
Building 1024 | always seem to have a boyfriend?"
MandrakeSoft | "Because I'm a nympho that owns a brewery?"
| - BBC TV show "Coupling"
Hi,
Francois Romieu <[email protected]> writes:
> [0/3]:
Looks ok for me.
> - SIOCDEVICE -> SIOCWANDEV conversion
But I wonder if that's what we really want. IIRC, the conclusion was
that we need a uniform interface for both LAN and WAN (with the ETHTOOL
being replaced eventually by it). That was why I changed it to "DEVICE"
the first time (for me personally, there is no problem with that).
> [1/3]:
> - struct if_settings in struct ifreq becomes struct if_settings *;
> - anonymous data pointer in struct if_settings is now a pointer to a
> union of struct containing l2 parameters. These structs can be of
> arbitrary size. So far, hdlc_settings is the only one available;
These two are IMHO bad :-(
The union would then be as big as the largest struct.
What's worse, the size would change when we add larger struct (i.e.
new protocol), causing binary incompatibility. With my patch, if we
add a new protocol (struct), existing utils would work.
The whole
+struct hdlc_settings {
+ union {
+ raw_hdlc_proto raw_hdlc;
+ cisco_proto cisco;
+ fr_proto fr;
+ fr_proto_pvc fr_pvc;
+ sync_serial_settings sync;
+ te1_settings te1;
+ } hdlcs_hdlcu;
+};
idea is IMHO bad - we need a variable size, separate structures for
separate protocols/interfaces. I think we should avoid a union at all costs.
Creating the new ioctl.h file seem ok to me.
Remember that i.e. sync_serial_settings are not related to any HDLC
protocol - that's just a physical interface which could be used for
things like SDLC, transparent transmissions etc. - or even for async
things (the synchronous interface can work in async mode as well).
> [2/3]:
> - conversion of drivers/net/wan/hdlc_xxx.c files.
>
> [3/3]:
> - some device are converted (c101.c/dscc4.c/farsync.c/n2.c).
IMHO we could wait with the real code until the interface is stable.
> Remarks:
> - As hdlc_{raw/cisco/fr/x25} doesn't need knowledge of struct ifreq, I would
> happily pass them a pointer to a struct if_settings. This way the 2 stage
> ioctl would be clearer imho.
Yes. The previous implementation depends on dev->do_ioctl and the
whole "PRIVATE" ioctls idea. When it's dead, we no longer require the
cmd/ifreq/etc.
--
Krzysztof Halasa
Network Administrator
Krzysztof Halasa <[email protected]> :
[...]
>
> > [1/3]:
> > - struct if_settings in struct ifreq becomes struct if_settings *;
> > - anonymous data pointer in struct if_settings is now a pointer to a
> > union of struct containing l2 parameters. These structs can be of
> > arbitrary size. So far, hdlc_settings is the only one available;
>
> These two are IMHO bad :-(
> The union would then be as big as the largest struct.
-> Does it hurt ? Why ?
> What's worse, the size would change when we add larger struct (i.e.
> new protocol), causing binary incompatibility. With my patch, if we
> add a new protocol (struct), existing utils would work.
I agree there's a way for an application to cause binary incompatibility if
it does:
struct userspace_foo {
struct if_settings frob;
int nitz;
} bar;
If size of struct if_settings changes (increases OR decreases), access to
bar.nitz doesn't work as expected.
But:
in hdlc_xxx_ioctl, only knowledge of the protocol-related member of the union
hdlcs_hdlcu is required. Nowhere does the code depend on size of if_settings.
-> Why would an application need to depend on it ?
Something like this comment in the kernel sources may help:
/*
* The size of this structure MAY change. If your code depends on it, it's
* broken. It doesn't need to depend on it. Change your mind.
*/
With this patch, only loosely written utils would break in the future.
Others wouldn't notice a new protocol has been added.
[...]
> Remember that i.e. sync_serial_settings are not related to any HDLC
> protocol - that's just a physical interface which could be used for
> things like SDLC, transparent transmissions etc. - or even for async
> things (the synchronous interface can work in async mode as well).
/me runs to the closer phone box and turns into Solution Man
include/linux/hdlc/ioctl.h:
[...]
struct hdlc_settings {
union {
/* sync_serial_settings removed */
raw_hdlc_proto raw_hdlc;
cisco_proto cisco;
fr_proto fr;
fr_proto_pvc fr_pvc;
te1_settings te1;
} hdlcs_hdlcu;
};
include/linux/whatever/ioctl.h:
[...]
struct whatever_settings {
union {
/* sync_serial_settings is back */
sync_serial_settings sync;
fancy_settings fancy;
}
};
include/linux/if.h:
[...]
struct if_settings
{
unsigned int type; /* Type of physical device or protocol */
union {
struct hdlc_settings ifsu_hdlc;
struct whatever_settings ifsu_whatever;
} ifs_ifsu;
};
As long as the application only accesses its data and doesn't try to embed
the variable sized kernel structure into its own, it won't break here either.
[...]
> > [3/3]:
> > - some device are converted (c101.c/dscc4.c/farsync.c/n2.c).
>
> IMHO we could wait with the real code until the interface is stable.
It helps me to see how the interface behaves. :o)
--
Ueimor
On Sun, 17 Feb 2002, Jeff Garzik wrote:
> Francois Romieu wrote:
> > [0/3]:
> > - SIOCDEVICE -> SIOCWANDEV conversion
> > - hdlc_proto -> raw_hdlc_proto
>
> patch looks ok to me.
Jeff, are yu willing to merge these things?
Linus
Francois Romieu <[email protected]> writes:
> I agree there's a way for an application to cause binary incompatibility if
> it does:
>
> struct userspace_foo {
> struct if_settings frob;
> int nitz;
> } bar;
>
> If size of struct if_settings changes (increases OR decreases), access to
> bar.nitz doesn't work as expected.
I assumed it's union and not a struct, you're right.
> But:
> in hdlc_xxx_ioctl, only knowledge of the protocol-related member of the
> union
> hdlcs_hdlcu is required. Nowhere does the code depend on size of if_settings.
I see now, t seems I haven't read the patches carefully enough.
Now... You just want to introduce an artificial struct which contains
only the union... Why? We could use just the union instead (?).
struct hdlc_settings {
union {
/* sync_serial_settings removed */
raw_hdlc_proto raw_hdlc;
cisco_proto cisco;
fr_proto fr;
fr_proto_pvc fr_pvc;
te1_settings te1;
} hdlcs_hdlcu;
};
Still, te1_settings are interface-related :-) Ok, I assume it goes
to the following:
> include/linux/whatever/ioctl.h:
> [...]
> struct whatever_settings {
> union {
> /* sync_serial_settings is back */
> sync_serial_settings sync;
> fancy_settings fancy;
> }
> };
>
> include/linux/if.h:
> [...]
> struct if_settings
> {
> unsigned int type; /* Type of physical device or protocol */
> union {
> struct hdlc_settings ifsu_hdlc;
> struct whatever_settings ifsu_whatever;
> } ifs_ifsu;
> };
>
> As long as the application only accesses its data and doesn't try to embed
> the variable sized kernel structure into its own, it won't break here either.
Yes, the compiler would compile that. Anyway, don't you think it's
a little messy? Void * pointers are IMHO not that evil.
Not sure about that, I have to think on it...
--
Krzysztof Halasa
Network Administrator
Linus Torvalds wrote:
>
> On Sun, 17 Feb 2002, Jeff Garzik wrote:
> > Francois Romieu wrote:
> > > [0/3]:
> > > - SIOCDEVICE -> SIOCWANDEV conversion
> > > - hdlc_proto -> raw_hdlc_proto
> >
> > patch looks ok to me.
>
> Jeff, are yu willing to merge these things?
Sure.
FWIW it looks like Krzysztof Halasa has some good points, so a "[BK
PATCH]" may be delayed a day or two before coming to you, I'm betting...
Jeff
--
Jeff Garzik | "Why is it that attractive girls like you
Building 1024 | always seem to have a boyfriend?"
MandrakeSoft | "Because I'm a nympho that owns a brewery?"
| - BBC TV show "Coupling"
Krzysztof Halasa <[email protected]> :
[...]
> Now... You just want to introduce an artificial struct which contains
> only the union... Why?
Copy-paste abuse.
> We could use just the union instead (?).
Yes. I'll try that tonite.
[...]
> Yes, the compiler would compile that. Anyway, don't you think it's
> a little messy? Void * pointers are IMHO not that evil.
Once the union in a struct disappears it should be minimal.
Regarding void * against union/union * I feel like minimal type checking is
better than none.
--
Ueimor
Francois Romieu wrote:
>
> Krzysztof Halasa <[email protected]> :
> [...]
> > Now... You just want to introduce an artificial struct which contains
> > only the union... Why?
>
> Copy-paste abuse.
>
> > We could use just the union instead (?).
>
> Yes. I'll try that tonite.
>
> [...]
> > Yes, the compiler would compile that. Anyway, don't you think it's
> > a little messy? Void * pointers are IMHO not that evil.
>
> Once the union in a struct disappears it should be minimal.
>
> Regarding void * against union/union * I feel like minimal type checking is
> better than none.
Agreed... the more type checking you can provide, the better.
Note that if it is possible to avoid the union in a manner that follows
the VFS inode cleanup, feel free to that too... Basically each private
info struct looks like
struct foo_inode {
/* ... driver-private stuff here ... */
struct inode vfs_inode; /* last member of struct */
};
Making it the last member of the structure ensures that people are not
tempted to do gross typecasting based on assumptions that the
"superclass" type is always located at the beginning of the subclass
substructure. (I don't know how well that applies to this case, just a
suggestion...)
Jeff
--
Jeff Garzik | "Why is it that attractive girls like you
Building 1024 | always seem to have a boyfriend?"
MandrakeSoft | "Because I'm a nympho that owns a brewery?"
| - BBC TV show "Coupling"
Jeff Garzik <[email protected]> :
[...]
> Making it the last member of the structure ensures that people are not
> tempted to do gross typecasting based on assumptions that the
> "superclass" type is always located at the beginning of the subclass
> substructure. (I don't know how well that applies to this case, just a
> suggestion...)
Afaiks, not to hdlc or not today. :o)
So far SIOCGOFIGURE mainly benefits to hdlc family of protocols. IMHO
if_settings.type should be enough (famous words) to make the difference
between the various l2/l1 informations. Others protocols will still be able
to store their data in a strict order behind it when/if they want to.
Patch of the day. Same layout as the last time. Applies after 2.5.5 +
latest Krzysztof's patch. Krzysztof, does diff-2.5.5-kh-mapomme-1 address
your issues ?
<URL:http://www.cogenit.fr/dscc4/hdlc-api/2.5.5/diff-2.5.5-kh-mapomme-0>
<URL:http://www.cogenit.fr/dscc4/hdlc-api/2.5.5/diff-2.5.5-kh-mapomme-1>
<URL:http://www.cogenit.fr/dscc4/hdlc-api/2.5.5/diff-2.5.5-kh-mapomme-2>
<URL:http://www.cogenit.fr/dscc4/hdlc-api/2.5.5/diff-2.5.5-kh-mapomme-3>
No more signal until tomorrow.
--
Ueimor
Jeff Garzik <[email protected]> writes:
> Sure.
>
> FWIW it looks like Krzysztof Halasa has some good points, so a "[BK
> PATCH]" may be delayed a day or two before coming to you, I'm betting...
Anyway, I can't even touch it until March, 23. I think it's now best
to apply the patches from Francois on top of my original patch.
We could eventually make some small correction(s) if I really still
have good points there, when I'm back in Poland.
--
Krzysztof Halasa out
Network Administrator
Krzysztof Halasa wrote:
>
> Jeff Garzik <[email protected]> writes:
>
> > Sure.
> >
> > FWIW it looks like Krzysztof Halasa has some good points, so a "[BK
> > PATCH]" may be delayed a day or two before coming to you, I'm betting...
>
> Anyway, I can't even touch it until March, 23. I think it's now best
> to apply the patches from Francois on top of my original patch.
> We could eventually make some small correction(s) if I really still
> have good points there, when I'm back in Poland.
Sounds good to me too.
Thanks,
Jeff
--
Jeff Garzik | "UNIX enhancements aren't."
Building 1024 | -- says /usr/games/fortune
MandrakeSoft |