There are two redundant assignments in the traverse() function, because
the while loop will break after these two assignments, and after that
the variable index will be assigned to m->index again.
Signed-off-by: Peng Donglin <[email protected]>
---
fs/seq_file.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/fs/seq_file.c b/fs/seq_file.c
index eea09f6..2298656 100644
--- a/fs/seq_file.c
+++ b/fs/seq_file.c
@@ -120,14 +120,12 @@ static int traverse(struct seq_file *m, loff_t offset)
if (pos + m->count > offset) {
m->from = offset - pos;
m->count -= m->from;
- m->index = index;
break;
}
pos += m->count;
m->count = 0;
if (pos == offset) {
index++;
- m->index = index;
break;
}
p = m->op->next(m, p, &index);
--
1.9.1
On Sat, 2018-02-10 at 23:59 +0800, Donglin Peng wrote:
> There are two redundant assignments in the traverse() function, because
> the while loop will break after these two assignments, and after that
> the variable index will be assigned to m->index again.
[]
> diff --git a/fs/seq_file.c b/fs/seq_file.c
[]
> @@ -120,14 +120,12 @@ static int traverse(struct seq_file *m, loff_t offset)
> if (pos + m->count > offset) {
> m->from = offset - pos;
> m->count -= m->from;
> - m->index = index;
> break;
> }
> pos += m->count;
> m->count = 0;
> if (pos == offset) {
> index++;
> - m->index = index;
> break;
> }
> p = m->op->next(m, p, &index);
Of course this looks correct, but how
are you _absolutely sure_ about this?
Perhaps the m->op->stop(m, p) call below
the break, which takes m as an argument,
needs an updated m->index.
Unless you can verify _all_ the indirect
paths that stop() can take, this patch may
not be correct.
On Sat, Feb 10, 2018 at 10:04:23AM -0800, Joe Perches wrote:
> > @@ -120,14 +120,12 @@ static int traverse(struct seq_file *m, loff_t offset)
> > if (pos + m->count > offset) {
> > m->from = offset - pos;
> > m->count -= m->from;
> > - m->index = index;
> > break;
> > }
> > pos += m->count;
> > m->count = 0;
> > if (pos == offset) {
> > index++;
> > - m->index = index;
> > break;
> > }
> > p = m->op->next(m, p, &index);
>
> Of course this looks correct, but how
> are you _absolutely sure_ about this?
>
> Perhaps the m->op->stop(m, p) call below
> the break, which takes m as an argument,
> needs an updated m->index.
Not only that, but ->next might also look at m->index.
This is not performance critical; don't try to optimise it.
Programmers waste enormous amounts of time thinking about, or worrying
about, the speed of noncritical parts of their programs, and these
attempts at efficiency actually have a strong negative impact when
debugging and maintenance are considered. We should forget about small
efficiencies, say about 97% of the time: premature optimization is the
root of all evil. Yet we should not pass up our opportunities in that
critical 3%. -- Donald Knuth
On Sun, Feb 11, 2018 at 9:02 AM, Matthew Wilcox <[email protected]> wrote:
> On Sat, Feb 10, 2018 at 10:04:23AM -0800, Joe Perches wrote:
>> > @@ -120,14 +120,12 @@ static int traverse(struct seq_file *m, loff_t offset)
>> > if (pos + m->count > offset) {
>> > m->from = offset - pos;
>> > m->count -= m->from;
>> > - m->index = index;
>> > break;
>> > }
>> > pos += m->count;
>> > m->count = 0;
>> > if (pos == offset) {
>> > index++;
>> > - m->index = index;
>> > break;
>> > }
>> > p = m->op->next(m, p, &index);
>>
>> Of course this looks correct, but how
>> are you _absolutely sure_ about this?
>>
>> Perhaps the m->op->stop(m, p) call below
>> the break, which takes m as an argument,
>> needs an updated m->index.
>
> Not only that, but ->next might also look at m->index.
I think there is no chance to call op->next, because the loop will
break immediately
after the assignment.