Re: [PATCH] HDLC patch for 2.5.5 (0/3)

[Francois Romieu <romieu@cogenit.fr>]

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