ax25.h unsigned long type for ax25 timers

PATCH In review Bernard Pidoux 4 participants 9 messages Jun 17, 2008 – Jun 28, 2008

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Bernard Pidoux

author

2008-06-17 13:04 UTC #0 lore raw

PATCH 1/1 ax25.h unsigned long type for ax25 timers

In linux/include/linux/ax25.h
ax25_info_struct timers types remain unsigned

struct ax25_info_struct {
	unsigned int	n2, n2count;
	unsigned int	t1, t1timer;
	unsigned int	t2, t2timer;
	unsigned int	t3, t3timer;
	unsigned int	idle, idletimer;
        .....
};

while in linux/include/net/ax25.h timers are unsigned long according to kernel 2.6 timers.

typedef struct ax25_cb {
        .....
        struct timer_list       t1timer, t2timer, t3timer, idletimer;
        unsigned long           t1, t2, t3, idle, rtt;
        ....

Although ax25_info_struct is not much used it is refered at least into libax25 and xfbb BBS application program.

It seems thus reasonable to make the change.

Signed-off-by Bernard Pidoux <[email protected]>

---
 include/linux/ax25.h |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

include/linux/ax25.h +4 -4

index 56c11f0..75282bb 100644
@@ -97,10 +97,10 @@ struct ax25_info_struct_deprecated {
 
 struct ax25_info_struct {
 	unsigned int	n2, n2count;
-	unsigned int	t1, t1timer;
-	unsigned int	t2, t2timer;
-	unsigned int	t3, t3timer;
-	unsigned int	idle, idletimer;
+	unsigned long	t1, t1timer;
+	unsigned long	t2, t2timer;
+	unsigned long	t3, t3timer;
+	unsigned long	idle, idletimer;
 	unsigned int	state;
 	unsigned int	rcv_q, snd_q;
 	unsigned int	vs, vr, va, vs_max;

Signature · 1 line

1.5.5

David Miller

maintainer

2008-06-18 05:30 UTC #1 lore raw

From: Bernard Pidoux <[email protected]>
Date: Tue, 17 Jun 2008 15:04:38 +0200

Quoted 21 lines

In linux/include/linux/ax25.h
ax25_info_struct timers types remain unsigned
struct ax25_info_struct {
	unsigned int	n2, n2count;
	unsigned int	t1, t1timer;
	unsigned int	t2, t2timer;
	unsigned int	t3, t3timer;
	unsigned int	idle, idletimer;
        .....
};
while in linux/include/net/ax25.h timers are unsigned long according to kernel 2.6 timers.
typedef struct ax25_cb {
        .....
        struct timer_list       t1timer, t2timer, t3timer, idletimer;
        unsigned long           t1, t2, t3, idle, rtt;
        ....

Quoted 6 lines

Although ax25_info_struct is not much used it is refered at least into libax25 and xfbb BBS application program.
It seems thus reasonable to make the change.
Signed-off-by Bernard Pidoux <[email protected]>

Unfortunately this datastructure is exported to userspace, and therefore we cannot change the structure layout without breaking userspace.

We cannot, as a result, make this change.

The only way to fix this is to make a new fixed structure, and add new ax25 calls that accept this new structure.

Bernard Pidoux

author

2008-06-19 16:28 UTC #2 lore raw

David,

Unfortunately I don't quite understand exactly why a structure in user space cannot be changed. But this means that I have to make some effort to learn a bit more about the kernel structure and userspace. However, thank you for your explanation about how to fix the discrepancy between both timer structures. I will try to dig out more information on this subject.

Bernard Pidoux, f6bvp

Quoted 33 lines · David Miller

From: Bernard Pidoux <[email protected]>
Date: Tue, 17 Jun 2008 15:04:38 +0200
Quoted 21 lines
In linux/include/linux/ax25.h
ax25_info_struct timers types remain unsigned
struct ax25_info_struct {
	unsigned int	n2, n2count;
	unsigned int	t1, t1timer;
	unsigned int	t2, t2timer;
	unsigned int	t3, t3timer;
	unsigned int	idle, idletimer;
        .....
};
while in linux/include/net/ax25.h timers are unsigned long according to kernel 2.6 timers.
typedef struct ax25_cb {
        .....
        struct timer_list       t1timer, t2timer, t3timer, idletimer;
        unsigned long           t1, t2, t3, idle, rtt;
        ....
Quoted 7 lines
Although ax25_info_struct is not much used it is refered at least into libax25 and xfbb BBS application program.
It seems thus reasonable to make the change.
Signed-off-by Bernard Pidoux <[email protected]>

Quoted 11 lines

Unfortunately this datastructure is exported to userspace, and therefore we cannot change the structure layout without breaking userspace.
We cannot, as a result, make this change.
The only way to fix this is to make a new fixed structure, and add new ax25 calls that accept this new structure.

--
To unsubscribe from this list: send the line "unsubscribe linux-hams" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

David Miller

maintainer

2008-06-19 21:46 UTC #3 lore raw

From: Bernard Pidoux <[email protected]>
Date: Thu, 19 Jun 2008 18:28:46 +0200

Quoted 2 lines

Unfortunately I don't quite understand exactly why a structure in user space cannot be changed.

Because there are userland binaries already compiled out there using the current structure layout. If we change the kernel, those binaries stop using a datastructure that matches what the kernel is using. Such binaries will break.

In generic, because of this, you can never change the layout of a data structure exported to userland as a kernel interface.

Bernard Pidoux

author

2008-06-19 22:38 UTC #4 lore raw

David,

Your explanation is perfectly clear and I understand the problem now. Thank you very much for taking the time to explain me that.

Bernard Pidoux

Quoted 14 lines · David Miller

From: Bernard Pidoux <[email protected]>
Date: Thu, 19 Jun 2008 18:28:46 +0200
Quoted 1 line
Unfortunately I don't quite understand exactly why a structure in user space cannot be changed.
Because there are userland binaries already compiled out there using the current structure layout. If we change the kernel, those binaries stop using a datastructure that matches what the kernel is using. Such binaries will break.
In generic, because of this, you can never change the layout of a data structure exported to userland as a kernel interface.

Bernard Pidoux F6BVP

2008-06-20 13:17 UTC #5 lore raw

There have been in the past already some modifications of ax25_info_struct and the old one was renamed deprecated.

/* this will go away. Please do not export to user land */

struct ax25_info_struct_deprecated {
        unsigned int    n2, n2count;
        unsigned int    t1, t1timer;
        unsigned int    t2, t2timer;
        unsigned int    t3, t3timer;
        unsigned int    idle, idletimer;
        unsigned int    state;
        unsigned int    rcv_q, snd_q;
};

Three new items were included :

...

        unsigned int    vs, vr, va, vs_max;
        unsigned int    paclen;
        unsigned int    window;

};

At the same time case SIOCAX25GETINFOOLD:
was added in ax25_ioctl()
and would trigger a warning message.

/* old structure? */

                if (cmd == SIOCAX25GETINFOOLD) {
                        static int warned = 0;
                        if (!warned) {
                                printk(KERN_INFO "%s uses old 
SIOCAX25GETINFO\n",
                                        current->comm);
                                warned=1;
                        }

                        if (copy_to_user(argp, &ax25_info, 
sizeof(struct ax25_info_struct_deprecated))) {
                                res = -EFAULT;
                                break;
                        }
                } else {
                        if (copy_to_user(argp, &ax25_info, 
sizeof(struct ax25_info_struct))) {
                                res = -EINVAL;
                                break;
                        }
                }
                res = 0;
                break;

Here is my question :

Can't we remove the probably quite old ax25_info_struct_deprecated structure, and rename the present ax25_info_struct, ax25_info_struct_deprecated, then create the new ax25_info_struct with resized timers unsigned long ?

The second solution is to declare a new structure

* new ax25 info struct */

struct ax25_info_long_timers_struct {
        unsigned int    n2, n2count;
        unsigned long   t1, t1timer;
        unsigned long   t2, t2timer;
        unsigned long   t3, t3timer;
        unsigned long   idle, idletimer;
        unsigned int    state;
        unsigned int    rcv_q, snd_q;
        unsigned int    vs, vr, va, vs_max;
        unsigned int    paclen;
        unsigned int    window;
};

adding another case SIOCAX25GETNEWINFO
that would be defined
#define SIOCAX25GETNEWINFO (SIOCPROTOPRIVATE+14)

and trigger warning message by calls to SIOCAX25GETINFOOLD and SIOCAX25GETINFO.

It that the right way to do it ?

Bernard, f6bvp

Quoted 30 lines · Bernard Pidoux

David,
Your explanation is perfectly clear and I understand the problem now. Thank you very much for taking the time to explain me that.
Bernard Pidoux
Quoted 14 lines · David Miller
From: Bernard Pidoux <[email protected]>
Date: Thu, 19 Jun 2008 18:28:46 +0200
Quoted 1 line
Unfortunately I don't quite understand exactly why a structure in user space cannot be changed.
Because there are userland binaries already compiled out there using the current structure layout. If we change the kernel, those binaries stop using a datastructure that matches what the kernel is using. Such binaries will break.
In generic, because of this, you can never change the layout of a data structure exported to userland as a kernel interface.
Signature · 3 lines
To unsubscribe from this list: send the line "unsubscribe linux-hams" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

David Miller

maintainer

2008-06-28 02:33 UTC #6 lore raw

From: Bernard Pidoux F6BVP <[email protected]>
Date: Fri, 20 Jun 2008 15:17:39 +0200

Quoted 4 lines

The second solution is to declare a new structure
* new ax25 info struct */
struct ax25_info_long_timers_struct {

...

Quoted 9 lines

adding another case SIOCAX25GETNEWINFO
that would be defined
#define SIOCAX25GETNEWINFO (SIOCPROTOPRIVATE+14)
and trigger warning message by calls to SIOCAX25GETINFOOLD and SIOCAX25GETINFO.
It that the right way to do it ?

Yes.

Al

2008-06-19 19:38 UTC #7 lore raw

In reply to David Miller

David
My impression was that the proposed change did NOT change binary interfaces to the kernel, but correctly labeled fields for the first time specifically as "long" instead of the fuzzy definition as "int". While it's true that a non-386 system where "int" is not a "long" would be adversely affected, there appear not to be any of those (rare) systems running this software, so the point seems entirely moot. The proposed change improves strict type checking and makes a cleaner definition.
I say let it in.
Al

Quoted 47 lines · David Miller

From: Bernard Pidoux <[email protected]>
Date: Tue, 17 Jun 2008 15:04:38 +0200
Quoted 21 lines
In linux/include/linux/ax25.h
ax25_info_struct timers types remain unsigned
struct ax25_info_struct {
	unsigned int	n2, n2count;
	unsigned int	t1, t1timer;
	unsigned int	t2, t2timer;
	unsigned int	t3, t3timer;
	unsigned int	idle, idletimer;
        .....
};
while in linux/include/net/ax25.h timers are unsigned long according to kernel 2.6 timers.
typedef struct ax25_cb {
        .....
        struct timer_list       t1timer, t2timer, t3timer, idletimer;
        unsigned long           t1, t2, t3, idle, rtt;
        ....
Quoted 7 lines
Although ax25_info_struct is not much used it is refered at least into libax25 and xfbb BBS application program.
It seems thus reasonable to make the change.
Signed-off-by Bernard Pidoux <[email protected]>
Unfortunately this datastructure is exported to userspace, and therefore we cannot change the structure layout without breaking userspace.
We cannot, as a result, make this change.
The only way to fix this is to make a new fixed structure, and add new ax25 calls that accept this new structure.
--
To unsubscribe from this list: send the line "unsubscribe linux-hams" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Al

2008-06-19 20:35 UTC #8 lore raw

Actually on closer inspection, the items are reordered, which would break existing compiled programs, so I'll have to reverse myself on this. Sorry for the distraction.
Al

Quoted 60 lines · Al

David
My impression was that the proposed change did NOT change binary interfaces to the kernel, but correctly labeled fields for the first time specifically as "long" instead of the fuzzy definition as "int". While it's true that a non-386 system where "int" is not a "long" would be adversely affected, there appear not to be any of those (rare) systems running this software, so the point seems entirely moot. The proposed change improves strict type checking and makes a cleaner definition.
I say let it in.
Al
Quoted 47 lines · David Miller
From: Bernard Pidoux <[email protected]>
Date: Tue, 17 Jun 2008 15:04:38 +0200
Quoted 21 lines
In linux/include/linux/ax25.h
ax25_info_struct timers types remain unsigned
struct ax25_info_struct {
    unsigned int    n2, n2count;
    unsigned int    t1, t1timer;
    unsigned int    t2, t2timer;
    unsigned int    t3, t3timer;
    unsigned int    idle, idletimer;
        .....
};
while in linux/include/net/ax25.h timers are unsigned long according to kernel 2.6 timers.
typedef struct ax25_cb {
        .....
        struct timer_list       t1timer, t2timer, t3timer, idletimer;
        unsigned long           t1, t2, t3, idle, rtt;
        ....
Quoted 7 lines
Although ax25_info_struct is not much used it is refered at least into libax25 and xfbb BBS application program.
It seems thus reasonable to make the change.
Signed-off-by Bernard Pidoux <[email protected]>
Unfortunately this datastructure is exported to userspace, and therefore we cannot change the structure layout without breaking userspace.
We cannot, as a result, make this change.
The only way to fix this is to make a new fixed structure, and add new ax25 calls that accept this new structure.
Signature · 3 lines
To unsubscribe from this list: send the line "unsubscribe linux-hams" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Signature · 3 lines
To unsubscribe from this list: send the line "unsubscribe linux-hams" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html