[PATCH] 9p: Don't use binary sysctl numbers.

[PATCH] 9p: Don't use binary sysctl numbers.

[Eric W. Biederman]

The recent 9p commit: bd238fb431f31989898423c8b6496bc8c4204a86
that supposedly only moved files also introduced a new 9p sysctl
interface that did not properly register it's sysctl binary numbers,
and since it was only for debugging clearly did not need a binary fast
path in any case.  So this patch just remove the binary numbers.

See Documentation/sysctl/ctl_unnumbered.txt for more details.

While I was at it I cleaned up the sysctl initializers a little as
well so there is less to read.

Cc: Latchesar Ionkov <lucho@ionkov.net>
Cc: Eric Van Hensbergen <ericvh@gmail.com>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
 net/9p/sysctl.c |   21 ++++++++-------------
 1 files changed, 8 insertions(+), 13 deletions(-)

diff --git a/net/9p/sysctl.c b/net/9p/sysctl.c
index e7fe706..8b61027 100644
--- a/net/9p/sysctl.c
+++ b/net/9p/sysctl.c
@@ -28,15 +28,10 @@
 #include <linux/init.h>
 #include <net/9p/9p.h>
 
-enum {
-	P9_SYSCTL_NET = 487,
-	P9_SYSCTL_DEBUG = 1,
-};
-
-static ctl_table p9_table[] = {
+static struct ctl_table p9_table[] = {
 #ifdef CONFIG_NET_9P_DEBUG
 	{
-		.ctl_name       = P9_SYSCTL_DEBUG,
+		.ctl_name       = CTL_UNNUMBERED,
 		.procname       = "debug",
 		.data           = &p9_debug_level,
 		.maxlen         = sizeof(int),
@@ -44,21 +39,21 @@ static ctl_table p9_table[] = {
 		.proc_handler   = &proc_dointvec
 	},
 #endif
-	{	.ctl_name	= 0 },
+	{},
 };
 
-static ctl_table p9_net_table[] = {
+static struct ctl_table p9_net_table[] = {
 	{
-		.ctl_name	= P9_SYSCTL_NET,
+		.ctl_name	= CTL_UNNUMBERED,
 		.procname	= "9p",
 		.maxlen		= 0,
 		.mode		= 0555,
 		.child		= p9_table,
 	},
-	{	.ctl_name	= 0 },
+	{},
 };
 
-static ctl_table p9_ctl_table[] = {
+static struct ctl_table p9_ctl_table[] = {
 	{
 		.ctl_name	= CTL_NET,
 		.procname	= "net",
@@ -66,7 +61,7 @@ static ctl_table p9_ctl_table[] = {
 		.mode		= 0555,
 		.child		= p9_net_table,
 	},
-	{	.ctl_name	= 0 },
+	{},
 };
 
 static struct ctl_table_header *p9_table_header;
-- 
1.5.1.1.181.g2de0

CTL_UNNUMBERED (Re: [PATCH] 9p: Don't use binary sysctl numbers.)

[Alexey Dobriyan]

On Sat, Jul 21, 2007 at 12:53:19PM -0600, Eric W. Biederman wrote:
> The recent 9p commit: bd238fb431f31989898423c8b6496bc8c4204a86
> that supposedly only moved files also introduced a new 9p sysctl
> interface that did not properly register it's sysctl binary numbers,
> and since it was only for debugging clearly did not need a binary fast
> path in any case.  So this patch just remove the binary numbers.
>
> See Documentation/sysctl/ctl_unnumbered.txt for more details.
>
> While I was at it I cleaned up the sysctl initializers a little as
> well so there is less to read.

> --- a/net/9p/sysctl.c
> +++ b/net/9p/sysctl.c
> @@ -28,15 +28,10 @@

> -enum {
> -	P9_SYSCTL_NET = 487,
> -	P9_SYSCTL_DEBUG = 1,
> -};
> -
> -static ctl_table p9_table[] = {
> +static struct ctl_table p9_table[] = {
>  #ifdef CONFIG_NET_9P_DEBUG
>  	{
> -		.ctl_name       = P9_SYSCTL_DEBUG,
> +		.ctl_name       = CTL_UNNUMBERED,

That's separate patch but CTL_UNNUMBERED must die, because it's totally
unneeded. If you don't want sysctl(2) interface just SKIP ->ctl_name
initialization and save one line for something useful.

	{
		.procname       = "prove_locking",
		.data		= &prove_locking,
		.maxlen		= sizeof(int),
		.mode		= 0644,
		.proc_handler	= &proc_dointvec,
	},

Or too late for -rc1?

Re: CTL_UNNUMBERED (Re: [PATCH] 9p: Don't use binary sysctl numbers.)

[Andrew Morton]

On Sun, 22 Jul 2007 00:57:09 +0400 Alexey Dobriyan <adobriyan@gmail.com> wrote:

> On Sat, Jul 21, 2007 at 12:53:19PM -0600, Eric W. Biederman wrote:
> > The recent 9p commit: bd238fb431f31989898423c8b6496bc8c4204a86
> > that supposedly only moved files also introduced a new 9p sysctl
> > interface that did not properly register it's sysctl binary numbers,
> > and since it was only for debugging clearly did not need a binary fast
> > path in any case.  So this patch just remove the binary numbers.
> >
> > See Documentation/sysctl/ctl_unnumbered.txt for more details.
> >
> > While I was at it I cleaned up the sysctl initializers a little as
> > well so there is less to read.
> 
> > --- a/net/9p/sysctl.c
> > +++ b/net/9p/sysctl.c
> > @@ -28,15 +28,10 @@
> 
> > -enum {
> > -	P9_SYSCTL_NET = 487,
> > -	P9_SYSCTL_DEBUG = 1,
> > -};
> > -
> > -static ctl_table p9_table[] = {
> > +static struct ctl_table p9_table[] = {
> >  #ifdef CONFIG_NET_9P_DEBUG
> >  	{
> > -		.ctl_name       = P9_SYSCTL_DEBUG,
> > +		.ctl_name       = CTL_UNNUMBERED,
> 
> That's separate patch but CTL_UNNUMBERED must die, because it's totally
> unneeded. If you don't want sysctl(2) interface just SKIP ->ctl_name
> initialization and save one line for something useful.
> 
> 	{
> 		.procname       = "prove_locking",
> 		.data		= &prove_locking,
> 		.maxlen		= sizeof(int),
> 		.mode		= 0644,
> 		.proc_handler	= &proc_dointvec,
> 	},
> 
> Or too late for -rc1?

It might be too late for -rc1 but it isn't too late for 2.6.23.

This affects a userspace interface.  Let's get it right please,
no rush.

Re: CTL_UNNUMBERED (Re: [PATCH] 9p: Don't use binary sysctl numbers.)

[Eric W. Biederman]

Alexey Dobriyan <adobriyan@gmail.com> writes:

>
> That's separate patch but CTL_UNNUMBERED must die, because it's totally
> unneeded. If you don't want sysctl(2) interface just SKIP ->ctl_name
> initialization and save one line for something useful.

As for the 9p code it doesn't seem to need or want a real binary
interface.  The 9p debug code picking of a semi-random number and not
patching it into sysctl.h like it should for a binary interface is
an implementation bug, and a maintenance problem.

Further it is a classic example of the silliness that goes on
when people actually try and add to the binary interface.

So not assigning a binary number very much looks like the right thing
to do for 9p.

I expect if the change had not happened in a mega patch to 9p that
seems to have changed everything the addition of a new user space
interface would more likely have been caught in a code review.



Now to the issue of using CTL_UNNUMBERED versus knowing that the magic
value is zero and we can just leave it uninitialized.  I don't much
care but given how often people who are not actively watching this
mess up I tend to prefer the explicit value.  It is a practical
question of how do we get the word out that we should not expand the
binary interface anymore. 

The only really practical way I can see us doing better then we are
today is to have a separate tree that maps binary numbers into ascii
strings and so we remove the ctl_name field entirely from ctl_table. 

That way people attempting to assign binary numbers using old
conventions will have code that doesn't even compile, and the
developers themselves are more likely to spot the problem.

Eric

Re: CTL_UNNUMBERED (Re: [PATCH] 9p: Don't use binary sysctl numbers.)

[Eric Van Hensbergen]

On 7/21/07, Eric W. Biederman <ebiederm@xmission.com> wrote:
> Alexey Dobriyan <adobriyan@gmail.com> writes:
>
> >
> > That's separate patch but CTL_UNNUMBERED must die, because it's totally
> > unneeded. If you don't want sysctl(2) interface just SKIP ->ctl_name
> > initialization and save one line for something useful.
>
> As for the 9p code it doesn't seem to need or want a real binary
> interface.  The 9p debug code picking of a semi-random number and not
> patching it into sysctl.h like it should for a binary interface is
> an implementation bug, and a maintenance problem.
>

Now that -rc1 is out, lets talk a bit more about this.  Lucho can you
provide some level of justification of why you went for a sysctl
interface versus something directly accessible within the file system
-- that would seem more on-par with the 9p philosophy.

Perhaps its time for a general cleanup of the debug_level stuff -- it
was always ugly to have it as a global, but there was just no clear
way to have the session structure available everywhere we use it.

               -eric

Re: CTL_UNNUMBERED (Re: [PATCH] 9p: Don't use binary sysctl numbers.)

[Latchesar Ionkov]

It doesn't really matter (for me) whether it is sysctl or sysfs
interface. The sysctl approach seemed easier to implement. If the
consensus is to use sysfs, I'll send a patch (for 2.6.24).

Sorry for the incorrect implementation, I guess I stole the code from
unappropriate place :)

Thanks,
    Lucho

On 7/23/07, Eric Van Hensbergen <ericvh@gmail.com> wrote:
> On 7/21/07, Eric W. Biederman <ebiederm@xmission.com> wrote:
> > Alexey Dobriyan <adobriyan@gmail.com> writes:
> >
> > >
> > > That's separate patch but CTL_UNNUMBERED must die, because it's totally
> > > unneeded. If you don't want sysctl(2) interface just SKIP ->ctl_name
> > > initialization and save one line for something useful.
> >
> > As for the 9p code it doesn't seem to need or want a real binary
> > interface.  The 9p debug code picking of a semi-random number and not
> > patching it into sysctl.h like it should for a binary interface is
> > an implementation bug, and a maintenance problem.
> >
>
> Now that -rc1 is out, lets talk a bit more about this.  Lucho can you
> provide some level of justification of why you went for a sysctl
> interface versus something directly accessible within the file system
> -- that would seem more on-par with the 9p philosophy.
>
> Perhaps its time for a general cleanup of the debug_level stuff -- it
> was always ugly to have it as a global, but there was just no clear
> way to have the session structure available everywhere we use it.
>
>                -eric
>

Re: CTL_UNNUMBERED (Re: [PATCH] 9p: Don't use binary sysctl numbers.)

[Eric Van Hensbergen]

On 7/23/07, Latchesar Ionkov <lucho@ionkov.net> wrote:
> It doesn't really matter (for me) whether it is sysctl or sysfs
> interface. The sysctl approach seemed easier to implement. If the
> consensus is to use sysfs, I'll send a patch (for 2.6.24).
>
> Sorry for the incorrect implementation, I guess I stole the code from
> unappropriate place :)
>

I think this is appropriate for a "fix" submission to the 2.6.23-rc
series.  If you don't have the bandwidth right now, I'll look at
reworking it, or at the very least just removing the sysctl interface.

         -eric

Re: CTL_UNNUMBERED (Re: [PATCH] 9p: Don't use binary sysctl numbers.)

[Eric W. Biederman]

"Eric Van Hensbergen" <ericvh@gmail.com> writes:

> On 7/23/07, Latchesar Ionkov <lucho@ionkov.net> wrote:
>> It doesn't really matter (for me) whether it is sysctl or sysfs
>> interface. The sysctl approach seemed easier to implement. If the
>> consensus is to use sysfs, I'll send a patch (for 2.6.24).
>>
>> Sorry for the incorrect implementation, I guess I stole the code from
>> unappropriate place :)
>>
>
> I think this is appropriate for a "fix" submission to the 2.6.23-rc
> series.  If you don't have the bandwidth right now, I'll look at
> reworking it, or at the very least just removing the sysctl interface.

For something this simple I don't see a problem with using sysctl
as it is simple and stupid, and generally easy to implement.  If you
had value per filesystem you would want things as mount options or
someplace else but for a global tunable sysctl is decent (although it
doesn't do much for locking).

The practical issue is that sysctl has two places it shows up.

/proc/sys/...  (very unix/plan9 ish)
sys_sysctl()  (not very unixish and not barely used).

Currently allocating binary numbers so you can use the sys_sysctl
interface is not required.  And it doesn't look interesting in your
case.

So all I'm asking is that you don't allocate binary numbers for
sys_sysctl.  As odds are no one is going to use them anyway.

The binary numbers you are using were not reserved in sysctl.h
so they were not reserved properly (a bug).

My patch simply removes the attempt to provide binary numbers for
sys_sysctl(), but keeps the good unixish /proc/sys parts.

If sysfs provides some cool widgetry that really helps go for it
but I would actually be surprised in this context if it does.

Eric

Re: CTL_UNNUMBERED (Re: [PATCH] 9p: Don't use binary sysctl numbers.)

[Alexey Dobriyan]

On Sat, Jul 21, 2007 at 04:36:03PM -0600, Eric W. Biederman wrote:
> Alexey Dobriyan <adobriyan@gmail.com> writes:
> > That's separate patch but CTL_UNNUMBERED must die, because it's totally
> > unneeded. If you don't want sysctl(2) interface just SKIP ->ctl_name
> > initialization and save one line for something useful.

[9p and .ctl_name]

> Now to the issue of using CTL_UNNUMBERED versus knowing that the magic
> value is zero and we can just leave it uninitialized.  I don't much
> care but given how often people who are not actively watching this
> mess up I tend to prefer the explicit value.

mmm, from my own experience the quickest way to add sysctl to kernel is
to _copy_ from other place and tweak variable name and string itself.
I'm sure that's what evebody is doing.

And once you're copy-pasting, existence of CTL_UNNUMBERED or absence of
it doesn't matter. Propagating of CTL_UNNUMBERED or empty .ctl_name
depends entirely on the place from where you're copy-pasting.

And since it doesn't matter, we'd better save one line :)

> It is a practical
> question of how do we get the word out that we should not expand the
> binary interface anymore.

Article on LWN, scary comment near struct ctl_table::ctl_name, tree-wide
removal .ctl_name initializers, periodic grepping of -mm and -rc diffs.
I promise to grep next -mm patch and submit CTL_UNNUMBERED removal :)

> The only really practical way I can see us doing better then we are
> today is to have a separate tree that maps binary numbers into ascii
> strings and so we remove the ctl_name field entirely from ctl_table.

Not sure how to do this cleanly and without flag-day. And it's
additional code.

> That way people attempting to assign binary numbers using old
> conventions will have code that doesn't even compile, and the
> developers themselves are more likely to spot the problem.