* [PATCH] sysctl: remove impossible condition check @ 2011-06-01 13:15 Lucas De Marchi 2011-06-01 13:25 ` Jesper Juhl 0 siblings, 1 reply; 8+ messages in thread From: Lucas De Marchi @ 2011-06-01 13:15 UTC (permalink / raw) To: linux-kernel; +Cc: Nick Piggin, Christoph Hellwig, Lucas De Marchi Signed-off-by: Lucas De Marchi <lucas.demarchi@profusion.mobi> --- fs/proc/proc_sysctl.c | 5 +---- 1 files changed, 1 insertions(+), 4 deletions(-) diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c index 2e5d3ec..98e82d4 100644 --- a/fs/proc/proc_sysctl.c +++ b/fs/proc/proc_sysctl.c @@ -52,11 +52,8 @@ static struct ctl_table *find_in_table(struct ctl_table *p, struct qstr *name) { int len; for ( ; p->procname; p++) { - - if (!p->procname) - continue; - len = strlen(p->procname); + if (len != name->len) continue; -- 1.7.5.2 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] sysctl: remove impossible condition check 2011-06-01 13:15 [PATCH] sysctl: remove impossible condition check Lucas De Marchi @ 2011-06-01 13:25 ` Jesper Juhl 2011-06-01 14:11 ` Lucas De Marchi 0 siblings, 1 reply; 8+ messages in thread From: Jesper Juhl @ 2011-06-01 13:25 UTC (permalink / raw) To: Lucas De Marchi; +Cc: linux-kernel, Nick Piggin, Christoph Hellwig On Wed, 1 Jun 2011, Lucas De Marchi wrote: > Signed-off-by: Lucas De Marchi <lucas.demarchi@profusion.mobi> > --- > fs/proc/proc_sysctl.c | 5 +---- > 1 files changed, 1 insertions(+), 4 deletions(-) > > diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c > index 2e5d3ec..98e82d4 100644 > --- a/fs/proc/proc_sysctl.c > +++ b/fs/proc/proc_sysctl.c > @@ -52,11 +52,8 @@ static struct ctl_table *find_in_table(struct ctl_table *p, struct qstr *name) > { > int len; > for ( ; p->procname; p++) { > - > - if (!p->procname) > - continue; > - > len = strlen(p->procname); > + > if (len != name->len) > continue; How about compacting it even further by getting rid of the 'len' variable as well? Like this: Signed-off-by: Jesper Juhl <jj@chaosbits.net> --- proc_sysctl.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c index f50133c..bd7f7af 100644 --- a/fs/proc/proc_sysctl.c +++ b/fs/proc/proc_sysctl.c @@ -49,17 +49,11 @@ out: static struct ctl_table *find_in_table(struct ctl_table *p, struct qstr *name) { - int len; for ( ; p->procname; p++) { - - if (!p->procname) - continue; - - len = strlen(p->procname); - if (len != name->len) + if (strlen(p->procname) != name->len) continue; - if (memcmp(p->procname, name->name, len) != 0) + if (memcmp(p->procname, name->name, name->len) != 0) continue; /* I have a match */ -- Jesper Juhl <jj@chaosbits.net> http://www.chaosbits.net/ Don't top-post http://www.catb.org/jargon/html/T/top-post.html Plain text mails only, please. ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] sysctl: remove impossible condition check 2011-06-01 13:25 ` Jesper Juhl @ 2011-06-01 14:11 ` Lucas De Marchi 2011-06-02 13:40 ` Eric W. Biederman 0 siblings, 1 reply; 8+ messages in thread From: Lucas De Marchi @ 2011-06-01 14:11 UTC (permalink / raw) To: Jesper Juhl; +Cc: linux-kernel, Nick Piggin, Christoph Hellwig, Al Viro [ CC'ing Al Viro ] On Wed, Jun 1, 2011 at 10:25 AM, Jesper Juhl <jj@chaosbits.net> wrote: > How about compacting it even further by getting rid of the 'len' variable > as well? > Like this: > > Signed-off-by: Jesper Juhl <jj@chaosbits.net> > --- > proc_sysctl.c | 10 ++-------- > 1 file changed, 2 insertions(+), 8 deletions(-) > > diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c > index f50133c..bd7f7af 100644 > --- a/fs/proc/proc_sysctl.c > +++ b/fs/proc/proc_sysctl.c > @@ -49,17 +49,11 @@ out: > > static struct ctl_table *find_in_table(struct ctl_table *p, struct qstr *name) > { > - int len; > for ( ; p->procname; p++) { > - > - if (!p->procname) > - continue; > - > - len = strlen(p->procname); > - if (len != name->len) > + if (strlen(p->procname) != name->len) > continue; > > - if (memcmp(p->procname, name->name, len) != 0) > + if (memcmp(p->procname, name->name, name->len) != 0) > continue; > > /* I have a match */ > Looking again at the code, I'm wondering if this is not actually a bug. There might be entries with procname == NULL, meaning they are not mirrored in /proc. What seems wrong is the condition in the for(). It should stop when all fields are 0 (meaning the end of the table) instead of stopping when procname is NULL. > > -- > Jesper Juhl <jj@chaosbits.net> http://www.chaosbits.net/ > Don't top-post http://www.catb.org/jargon/html/T/top-post.html > Plain text mails only, please. > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] sysctl: remove impossible condition check 2011-06-01 14:11 ` Lucas De Marchi @ 2011-06-02 13:40 ` Eric W. Biederman 2011-06-02 13:51 ` Lucas De Marchi 0 siblings, 1 reply; 8+ messages in thread From: Eric W. Biederman @ 2011-06-02 13:40 UTC (permalink / raw) To: Lucas De Marchi Cc: Jesper Juhl, linux-kernel, Nick Piggin, Christoph Hellwig, Al Viro Lucas De Marchi <lucas.demarchi@profusion.mobi> writes: > [ CC'ing Al Viro ] > > On Wed, Jun 1, 2011 at 10:25 AM, Jesper Juhl <jj@chaosbits.net> wrote: >> How about compacting it even further by getting rid of the 'len' variable >> as well? >> Like this: >> >> Signed-off-by: Jesper Juhl <jj@chaosbits.net> >> --- >> proc_sysctl.c | 10 ++-------- >> 1 file changed, 2 insertions(+), 8 deletions(-) >> >> diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c >> index f50133c..bd7f7af 100644 >> --- a/fs/proc/proc_sysctl.c >> +++ b/fs/proc/proc_sysctl.c >> @@ -49,17 +49,11 @@ out: >> >> static struct ctl_table *find_in_table(struct ctl_table *p, struct qstr *name) >> { >> - int len; >> for ( ; p->procname; p++) { >> - >> - if (!p->procname) >> - continue; >> - >> - len = strlen(p->procname); >> - if (len != name->len) >> + if (strlen(p->procname) != name->len) >> continue; >> >> - if (memcmp(p->procname, name->name, len) != 0) >> + if (memcmp(p->procname, name->name, name->len) != 0) >> continue; >> >> /* I have a match */ >> > > > Looking again at the code, I'm wondering if this is not actually a > bug. There might be entries with procname == NULL, meaning they are > not mirrored in /proc. What seems wrong is the condition in the for(). > It should stop when all fields are 0 (meaning the end of the table) > instead of stopping when procname is NULL. It is not a bug. The condition was originally p->ctlname then it became p->ctlname || p->procname and then finally I was able to kill ctl_name. What you see is a left over that didn't get removed. This is also the second time in the last couple of weeks someone has sent this patch. There is some ongoing work to make sysctl scale better that with any luck should be ready for 3.1. Decide which version of this patch you like and please resend, and I will add this to my sysctl tree. Thank you, Eric ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] sysctl: remove impossible condition check 2011-06-02 13:40 ` Eric W. Biederman @ 2011-06-02 13:51 ` Lucas De Marchi 2011-06-02 16:00 ` Lucas De Marchi 0 siblings, 1 reply; 8+ messages in thread From: Lucas De Marchi @ 2011-06-02 13:51 UTC (permalink / raw) To: Eric W. Biederman Cc: Jesper Juhl, linux-kernel, Nick Piggin, Christoph Hellwig, Al Viro On Thu, Jun 2, 2011 at 10:40 AM, Eric W. Biederman <ebiederm@xmission.com> wrote: >> Looking again at the code, I'm wondering if this is not actually a >> bug. There might be entries with procname == NULL, meaning they are >> not mirrored in /proc. What seems wrong is the condition in the for(). >> It should stop when all fields are 0 (meaning the end of the table) >> instead of stopping when procname is NULL. > > It is not a bug. The condition was originally p->ctlname then > it became p->ctlname || p->procname and then finally I was able to > kill ctl_name. > > What you see is a left over that didn't get removed. > Alright then. > This is also the second time in the last couple of weeks someone has > sent this patch. > > There is some ongoing work to make sysctl scale better that with > any luck should be ready for 3.1. Decide which version of this > patch you like and please resend, and I will add this to my > sysctl tree. There's also the same thing in this file, in scan() function. I'll add it to the previous patch and resend. thanks Lucas De Marchi ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] sysctl: remove impossible condition check 2011-06-02 13:51 ` Lucas De Marchi @ 2011-06-02 16:00 ` Lucas De Marchi [not found] ` <1307684165-14742-1-git-send-email-lucas.demarchi@profusion.mobi> 0 siblings, 1 reply; 8+ messages in thread From: Lucas De Marchi @ 2011-06-02 16:00 UTC (permalink / raw) To: Eric W. Biederman Cc: Jesper Juhl, linux-kernel, Nick Piggin, Christoph Hellwig, Al Viro, Lucas De Marchi Signed-off-by: Lucas De Marchi <lucas.demarchi@profusion.mobi> --- fs/proc/proc_sysctl.c | 14 ++------------ 1 files changed, 2 insertions(+), 12 deletions(-) diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c index f50133c..6cbcb1e 100644 --- a/fs/proc/proc_sysctl.c +++ b/fs/proc/proc_sysctl.c @@ -49,17 +49,11 @@ out: static struct ctl_table *find_in_table(struct ctl_table *p, struct qstr *name) { - int len; for ( ; p->procname; p++) { - - if (!p->procname) - continue; - - len = strlen(p->procname); - if (len != name->len) + if (strlen(p->procname) != name->len) continue; - if (memcmp(p->procname, name->name, len) != 0) + if (memcmp(p->procname, name->name, name->len) != 0) continue; /* I have a match */ @@ -223,10 +217,6 @@ static int scan(struct ctl_table_header *head, ctl_table *table, for (; table->procname; table++, (*pos)++) { int res; - /* Can't do anything without a proc name */ - if (!table->procname) - continue; - if (*pos < file->f_pos) continue; -- 1.7.5.2 ^ permalink raw reply related [flat|nested] 8+ messages in thread
[parent not found: <1307684165-14742-1-git-send-email-lucas.demarchi@profusion.mobi>]
* Re: [PATCH] sysctl: remove impossible condition check [not found] ` <1307684165-14742-1-git-send-email-lucas.demarchi@profusion.mobi> @ 2011-06-10 7:56 ` Jesper Juhl 2011-06-12 6:02 ` Eric W. Biederman 0 siblings, 1 reply; 8+ messages in thread From: Jesper Juhl @ 2011-06-10 7:56 UTC (permalink / raw) To: Lucas De Marchi Cc: Andrew Morton, Eric W. Biederman, Jesper Juhl, linux-kernel, Nick Piggin, Christoph Hellwig, Al Viro, Lucas De Marchi On Fri, 10 Jun 2011, Lucas De Marchi wrote: > Remove checks for conditions that will never happen. If procname is NULL > the loop would already had bailed out, so there's no need to check it > again. > > At the same time this also compacts the function find_in_table() by > refactoring it to be easier to read. > > Signed-off-by: Lucas De Marchi <lucas.demarchi@profusion.mobi> Looks good to me. Reviewed-by: Jesper Juhl <jj@chaosbits.net> -- Jesper Juhl <jj@chaosbits.net> http://www.chaosbits.net/ Don't top-post http://www.catb.org/jargon/html/T/top-post.html Plain text mails only, please. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] sysctl: remove impossible condition check 2011-06-10 7:56 ` Jesper Juhl @ 2011-06-12 6:02 ` Eric W. Biederman 0 siblings, 0 replies; 8+ messages in thread From: Eric W. Biederman @ 2011-06-12 6:02 UTC (permalink / raw) To: Jesper Juhl Cc: Lucas De Marchi, Andrew Morton, linux-kernel, Nick Piggin, Christoph Hellwig, Al Viro Jesper Juhl <jj@chaosbits.net> writes: > On Fri, 10 Jun 2011, Lucas De Marchi wrote: > >> Remove checks for conditions that will never happen. If procname is NULL >> the loop would already had bailed out, so there's no need to check it >> again. >> >> At the same time this also compacts the function find_in_table() by >> refactoring it to be easier to read. >> >> Signed-off-by: Lucas De Marchi <lucas.demarchi@profusion.mobi> > > Looks good to me. > > Reviewed-by: Jesper Juhl <jj@chaosbits.net> Applied thanks all. Eric ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2011-06-12 6:02 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-06-01 13:15 [PATCH] sysctl: remove impossible condition check Lucas De Marchi
2011-06-01 13:25 ` Jesper Juhl
2011-06-01 14:11 ` Lucas De Marchi
2011-06-02 13:40 ` Eric W. Biederman
2011-06-02 13:51 ` Lucas De Marchi
2011-06-02 16:00 ` Lucas De Marchi
[not found] ` <1307684165-14742-1-git-send-email-lucas.demarchi@profusion.mobi>
2011-06-10 7:56 ` Jesper Juhl
2011-06-12 6:02 ` Eric W. Biederman
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.