Krzysztof Halasa <khc@pm.waw.pl> :
[...]
>
> > [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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
This archive was generated by hypermail 2b29 : Sat Feb 23 2002 - 21:00:14 EST