lkml.org 
[lkml]   [2002]   [Feb]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] HDLC patch for 2.5.5 (0/3)
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/

\
 
 \ /
  Last update: 2005-03-22 13:24    [W:0.066 / U:14.568 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site