Commit 1f4aace6 authored by NeilBrown's avatar NeilBrown Committed by Linus Torvalds

fs/seq_file.c: simplify seq_file iteration code and interface

The documentation for seq_file suggests that it is necessary to be able
to move the iterator to a given offset, however that is not the case.
If the iterator is stored in the private data and is stable from one
read() syscall to the next, it is only necessary to support first/next
interactions.  Implementing this in a client is a little clumsy.

 - if ->start() is given a pos of zero, it should go to start of
   sequence.

 - if ->start() is given the name pos that was given to the most recent
   next() or start(), it should restore the iterator to state just
   before that last call

 - if ->start is given another number, it should set the iterator one
   beyond the start just before the last ->start or ->next call.

Also, the documentation says that the implementation can interpret the
pos however it likes (other than zero meaning start), but seq_file
increments the pos sometimes which does impose on the implementation.

This patch simplifies the interface for first/next iteration and
simplifies the code, while maintaining complete backward compatability.
Now:

 - if ->start() is given a pos of zero, it should return an iterator
   placed at the start of the sequence

 - if ->start() is given a non-zero pos, it should return the iterator
   in the same state it was after the last ->start or ->next.

This is particularly useful for interators which walk the multiple
chains in a hash table, e.g.  using rhashtable_walk*.  See
fs/gfs2/glock.c and drivers/staging/lustre/lustre/llite/vvp_dev.c

A large part of achieving this is to *always* call ->next after ->show
has successfully stored all of an entry in the buffer.  Never just
increment the index instead.  Also:

 - always pass &m->index to ->start() and ->next(), never a temp
   variable

 - don't clear ->from when ->count is zero, as ->from is dead when
   ->count is zero.

Some ->next functions do not increment *pos when they return NULL.  To
maintain compatability with this, we still need to increment m->index in
one place, if ->next didn't increment it.  Note that such ->next
functions are buggy and should be fixed.  A simple demonstration is

   dd if=/proc/swaps bs=1000 skip=1

Choose any block size larger than the size of /proc/swaps.  This will
always show the whole last line of /proc/swaps.

This patch doesn't work around buggy next() functions for this case.

[neilb@suse.com: ensure ->from is valid]
  Link: http://lkml.kernel.org/r/87601ryb8a.fsf@notabene.neil.brown.nameSigned-off-by: default avatarNeilBrown <neilb@suse.com>
Acked-by: Jonathan Corbet <corbet@lwn.net>	[docs]
Tested-by: default avatarJann Horn <jannh@google.com>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: Kees Cook <keescook@chromium.org>
Signed-off-by: default avatarAndrew Morton <akpm@linux-foundation.org>
Signed-off-by: default avatarLinus Torvalds <torvalds@linux-foundation.org>
parent 4cdfffc8
...@@ -66,23 +66,39 @@ kernel 3.10. Current versions require the following update ...@@ -66,23 +66,39 @@ kernel 3.10. Current versions require the following update
The iterator interface The iterator interface
Modules implementing a virtual file with seq_file must implement a simple Modules implementing a virtual file with seq_file must implement an
iterator object that allows stepping through the data of interest. iterator object that allows stepping through the data of interest
Iterators must be able to move to a specific position - like the file they during a "session" (roughly one read() system call). If the iterator
implement - but the interpretation of that position is up to the iterator is able to move to a specific position - like the file they implement,
itself. A seq_file implementation that is formatting firewall rules, for though with freedom to map the position number to a sequence location
example, could interpret position N as the Nth rule in the chain. in whatever way is convenient - the iterator need only exist
Positioning can thus be done in whatever way makes the most sense for the transiently during a session. If the iterator cannot easily find a
generator of the data, which need not be aware of how a position translates numerical position but works well with a first/next interface, the
to an offset in the virtual file. The one obvious exception is that a iterator can be stored in the private data area and continue from one
position of zero should indicate the beginning of the file. session to the next.
A seq_file implementation that is formatting firewall rules from a
table, for example, could provide a simple iterator that interprets
position N as the Nth rule in the chain. A seq_file implementation
that presents the content of a, potentially volatile, linked list
might record a pointer into that list, providing that can be done
without risk of the current location being removed.
Positioning can thus be done in whatever way makes the most sense for
the generator of the data, which need not be aware of how a position
translates to an offset in the virtual file. The one obvious exception
is that a position of zero should indicate the beginning of the file.
The /proc/sequence iterator just uses the count of the next number it The /proc/sequence iterator just uses the count of the next number it
will output as its position. will output as its position.
Four functions must be implemented to make the iterator work. The first, Four functions must be implemented to make the iterator work. The
called start() takes a position as an argument and returns an iterator first, called start(), starts a session and takes a position as an
which will start reading at that position. For our simple sequence example, argument, returning an iterator which will start reading at that
position. The pos passed to start() will always be either zero, or
the most recent pos used in the previous session.
For our simple sequence example,
the start() function looks like: the start() function looks like:
static void *ct_seq_start(struct seq_file *s, loff_t *pos) static void *ct_seq_start(struct seq_file *s, loff_t *pos)
...@@ -101,11 +117,12 @@ implementations; in most cases the start() function should check for a ...@@ -101,11 +117,12 @@ implementations; in most cases the start() function should check for a
"past end of file" condition and return NULL if need be. "past end of file" condition and return NULL if need be.
For more complicated applications, the private field of the seq_file For more complicated applications, the private field of the seq_file
structure can be used. There is also a special value which can be returned structure can be used to hold state from session to session. There is
by the start() function called SEQ_START_TOKEN; it can be used if you wish also a special value which can be returned by the start() function
to instruct your show() function (described below) to print a header at the called SEQ_START_TOKEN; it can be used if you wish to instruct your
top of the output. SEQ_START_TOKEN should only be used if the offset is show() function (described below) to print a header at the top of the
zero, however. output. SEQ_START_TOKEN should only be used if the offset is zero,
however.
The next function to implement is called, amazingly, next(); its job is to The next function to implement is called, amazingly, next(); its job is to
move the iterator forward to the next position in the sequence. The move the iterator forward to the next position in the sequence. The
...@@ -121,9 +138,13 @@ complete. Here's the example version: ...@@ -121,9 +138,13 @@ complete. Here's the example version:
return spos; return spos;
} }
The stop() function is called when iteration is complete; its job, of The stop() function closes a session; its job, of course, is to clean
course, is to clean up. If dynamic memory is allocated for the iterator, up. If dynamic memory is allocated for the iterator, stop() is the
stop() is the place to free it. place to free it; if a lock was taken by start(), stop() must release
that lock. The value that *pos was set to by the last next() call
before stop() is remembered, and used for the first start() call of
the next session unless lseek() has been called on the file; in that
case next start() will be asked to start at position zero.
static void ct_seq_stop(struct seq_file *s, void *v) static void ct_seq_stop(struct seq_file *s, void *v)
{ {
......
...@@ -90,23 +90,22 @@ EXPORT_SYMBOL(seq_open); ...@@ -90,23 +90,22 @@ EXPORT_SYMBOL(seq_open);
static int traverse(struct seq_file *m, loff_t offset) static int traverse(struct seq_file *m, loff_t offset)
{ {
loff_t pos = 0, index; loff_t pos = 0;
int error = 0; int error = 0;
void *p; void *p;
m->version = 0; m->version = 0;
index = 0; m->index = 0;
m->count = m->from = 0; m->count = m->from = 0;
if (!offset) { if (!offset)
m->index = index;
return 0; return 0;
}
if (!m->buf) { if (!m->buf) {
m->buf = seq_buf_alloc(m->size = PAGE_SIZE); m->buf = seq_buf_alloc(m->size = PAGE_SIZE);
if (!m->buf) if (!m->buf)
return -ENOMEM; return -ENOMEM;
} }
p = m->op->start(m, &index); p = m->op->start(m, &m->index);
while (p) { while (p) {
error = PTR_ERR(p); error = PTR_ERR(p);
if (IS_ERR(p)) if (IS_ERR(p))
...@@ -123,20 +122,15 @@ static int traverse(struct seq_file *m, loff_t offset) ...@@ -123,20 +122,15 @@ static int traverse(struct seq_file *m, loff_t offset)
if (pos + m->count > offset) { if (pos + m->count > offset) {
m->from = offset - pos; m->from = offset - pos;
m->count -= m->from; m->count -= m->from;
m->index = index;
break; break;
} }
pos += m->count; pos += m->count;
m->count = 0; m->count = 0;
if (pos == offset) { p = m->op->next(m, p, &m->index);
index++; if (pos == offset)
m->index = index;
break; break;
}
p = m->op->next(m, p, &index);
} }
m->op->stop(m, p); m->op->stop(m, p);
m->index = index;
return error; return error;
Eoverflow: Eoverflow:
...@@ -160,7 +154,6 @@ ssize_t seq_read(struct file *file, char __user *buf, size_t size, loff_t *ppos) ...@@ -160,7 +154,6 @@ ssize_t seq_read(struct file *file, char __user *buf, size_t size, loff_t *ppos)
{ {
struct seq_file *m = file->private_data; struct seq_file *m = file->private_data;
size_t copied = 0; size_t copied = 0;
loff_t pos;
size_t n; size_t n;
void *p; void *p;
int err = 0; int err = 0;
...@@ -223,16 +216,12 @@ ssize_t seq_read(struct file *file, char __user *buf, size_t size, loff_t *ppos) ...@@ -223,16 +216,12 @@ ssize_t seq_read(struct file *file, char __user *buf, size_t size, loff_t *ppos)
size -= n; size -= n;
buf += n; buf += n;
copied += n; copied += n;
if (!m->count) {
m->from = 0;
m->index++;
}
if (!size) if (!size)
goto Done; goto Done;
} }
/* we need at least one record in buffer */ /* we need at least one record in buffer */
pos = m->index; m->from = 0;
p = m->op->start(m, &pos); p = m->op->start(m, &m->index);
while (1) { while (1) {
err = PTR_ERR(p); err = PTR_ERR(p);
if (!p || IS_ERR(p)) if (!p || IS_ERR(p))
...@@ -243,8 +232,7 @@ ssize_t seq_read(struct file *file, char __user *buf, size_t size, loff_t *ppos) ...@@ -243,8 +232,7 @@ ssize_t seq_read(struct file *file, char __user *buf, size_t size, loff_t *ppos)
if (unlikely(err)) if (unlikely(err))
m->count = 0; m->count = 0;
if (unlikely(!m->count)) { if (unlikely(!m->count)) {
p = m->op->next(m, p, &pos); p = m->op->next(m, p, &m->index);
m->index = pos;
continue; continue;
} }
if (m->count < m->size) if (m->count < m->size)
...@@ -256,29 +244,33 @@ ssize_t seq_read(struct file *file, char __user *buf, size_t size, loff_t *ppos) ...@@ -256,29 +244,33 @@ ssize_t seq_read(struct file *file, char __user *buf, size_t size, loff_t *ppos)
if (!m->buf) if (!m->buf)
goto Enomem; goto Enomem;
m->version = 0; m->version = 0;
pos = m->index; p = m->op->start(m, &m->index);
p = m->op->start(m, &pos);
} }
m->op->stop(m, p); m->op->stop(m, p);
m->count = 0; m->count = 0;
goto Done; goto Done;
Fill: Fill:
/* they want more? let's try to get some more */ /* they want more? let's try to get some more */
while (m->count < size) { while (1) {
size_t offs = m->count; size_t offs = m->count;
loff_t next = pos; loff_t pos = m->index;
p = m->op->next(m, p, &next);
p = m->op->next(m, p, &m->index);
if (pos == m->index)
/* Buggy ->next function */
m->index++;
if (!p || IS_ERR(p)) { if (!p || IS_ERR(p)) {
err = PTR_ERR(p); err = PTR_ERR(p);
break; break;
} }
if (m->count >= size)
break;
err = m->op->show(m, p); err = m->op->show(m, p);
if (seq_has_overflowed(m) || err) { if (seq_has_overflowed(m) || err) {
m->count = offs; m->count = offs;
if (likely(err <= 0)) if (likely(err <= 0))
break; break;
} }
pos = next;
} }
m->op->stop(m, p); m->op->stop(m, p);
n = min(m->count, size); n = min(m->count, size);
...@@ -287,11 +279,7 @@ ssize_t seq_read(struct file *file, char __user *buf, size_t size, loff_t *ppos) ...@@ -287,11 +279,7 @@ ssize_t seq_read(struct file *file, char __user *buf, size_t size, loff_t *ppos)
goto Efault; goto Efault;
copied += n; copied += n;
m->count -= n; m->count -= n;
if (m->count) m->from = n;
m->from = n;
else
pos++;
m->index = pos;
Done: Done:
if (!copied) if (!copied)
copied = err; copied = err;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment