On Mon 29-09-14 16:08:21, Joe Perches wrote:
> The return values of seq_printf/puts/putc are frequently misused.
>
> Start down a path to remove all the return value uses of these
> functions.
>
> Make the static bool seq_overflow public along with a rename of
> the function to seq_is_full. Rename the still static
> seq_set_overflow to seq_set_full.
>
> Update the documentation to not show return types for seq_printf
> et al. Add a description of seq_is_full.
>
> Signed-off-by: Joe Perches <joe@xxxxxxxxxxx>
> ---
> Documentation/filesystems/seq_file.txt | 28 ++++++++++++++++------------
> fs/seq_file.c | 28 ++++++++++++++--------------
> include/linux/seq_file.h | 8 ++++++++
> 3 files changed, 38 insertions(+), 26 deletions(-)
>
> diff --git a/Documentation/filesystems/seq_file.txt b/Documentation/filesystems/seq_file.txt
> index 8ea3e90..e19c636 100644
> --- a/Documentation/filesystems/seq_file.txt
> +++ b/Documentation/filesystems/seq_file.txt
> @@ -180,27 +180,23 @@ output must be passed to the seq_file code. Some utility functions have
> been defined which make this task easy.
>
> Most code will simply use seq_printf(), which works pretty much like
> -printk(), but which requires the seq_file pointer as an argument. It is
> -common to ignore the return value from seq_printf(), but a function
> -producing complicated output may want to check that value and quit if
> -something non-zero is returned; an error return means that the seq_file
> -buffer has been filled and further output will be discarded.
> +printk(), but which requires the seq_file pointer as an argument.
>
> For straight character output, the following functions may be used:
>
> - int seq_putc(struct seq_file *m, char c);
> - int seq_puts(struct seq_file *m, const char *s);
> - int seq_escape(struct seq_file *m, const char *s, const char *esc);
> + seq_putc(struct seq_file *m, char c);
> + seq_puts(struct seq_file *m, const char *s);
> + seq_escape(struct seq_file *m, const char *s, const char *esc);
>
> The first two output a single character and a string, just like one would
> expect. seq_escape() is like seq_puts(), except that any character in s
> which is in the string esc will be represented in octal form in the output.
>
> -There is also a pair of functions for printing filenames:
> +There are also a pair of functions for printing filenames:
>
> - int seq_path(struct seq_file *m, struct path *path, char *esc);
> - int seq_path_root(struct seq_file *m, struct path *path,
> - struct path *root, char *esc)
> + seq_path(struct seq_file *m, struct path *path, char *esc);
> + seq_path_root(struct seq_file *m, struct path *path,
> + struct path *root, char *esc)
>
> Here, path indicates the file of interest, and esc is a set of characters
> which should be escaped in the output. A call to seq_path() will output
> @@ -209,6 +205,14 @@ root is desired, it can be used with seq_path_root(). Note that, if it
> turns out that path cannot be reached from root, the value of root will be
> changed in seq_file_root() to a root which *does* work.
>
> +A function producing complicated output may want to check
> + bool seq_is_full(struct seq_file *m);
> +and avoid further seq_<output> calls if true is returned.
> +
> +A true return from seq_is_full means that the seq_file buffer is full
> +and further output will be discarded. The seq_show function will attempt
> +to allocate a larger buffer and retry printing.
> +
>
> Making it all work
>
> diff --git a/fs/seq_file.c b/fs/seq_file.c
> index 3857b72..555aed6 100644
> --- a/fs/seq_file.c
> +++ b/fs/seq_file.c
> @@ -16,18 +16,18 @@
> #include <asm/uaccess.h>
> #include <asm/page.h>
>
> -
> /*
> - * seq_files have a buffer which can may overflow. When this happens a larger
> + * seq_files have a buffer which may overflow. When this happens a larger
> * buffer is reallocated and all the data will be printed again.
> * The overflow state is true when m->count == m->size.
> */
> -static bool seq_overflow(struct seq_file *m)
> +bool seq_is_full(struct seq_file *m)
> {
> return m->count == m->size;
> }
> +EXPORT_SYMBOL(seq_is_full);
>
> -static void seq_set_overflow(struct seq_file *m)
> +static void seq_set_full(struct seq_file *m)
> {
> m->count = m->size;
> }
> @@ -124,7 +124,7 @@ static int traverse(struct seq_file *m, loff_t offset)
> error = 0;
> m->count = 0;
> }
> - if (seq_overflow(m))
> + if (seq_is_full(m))
> goto Eoverflow;
I would keep seq_overflow() here. seq_is_full() should mean that the
data still fit into the buffer.
> if (pos + m->count > offset) {
> m->from = offset - pos;
> @@ -267,7 +267,7 @@ Fill:
> break;
> }
> err = m->op->show(m, p);
> - if (seq_overflow(m) || err) {
> + if (seq_is_full(m) || err) {
same here
> m->count = offs;
> if (likely(err <= 0))
> break;
> @@ -396,7 +396,7 @@ int seq_escape(struct seq_file *m, const char *s, const char *esc)
> *p++ = '0' + (c & 07);
> continue;
> }
> - seq_set_overflow(m);
> + seq_set_full(m);
I would keep seq_set_overflow() here because the data did not fit in.
> return -1;
> }
> m->count = p - m->buf;
> @@ -415,7 +415,7 @@ int seq_vprintf(struct seq_file *m, const char *f, va_list args)
> return 0;
> }
> }
> - seq_set_overflow(m);
> + seq_set_full(m);
Hmm, we do not know if the data are shrunken by vsnprintf(). I would
keep seq_set_overflow() here to be on the safe side.
> return -1;
> }
> EXPORT_SYMBOL(seq_vprintf);
> @@ -557,7 +557,7 @@ int seq_bitmap(struct seq_file *m, const unsigned long *bits,
> return 0;
> }
> }
> - seq_set_overflow(m);
> + seq_set_full(m);
same here
> return -1;
> }
> EXPORT_SYMBOL(seq_bitmap);
> @@ -573,7 +573,7 @@ int seq_bitmap_list(struct seq_file *m, const unsigned long *bits,
> return 0;
> }
> }
> - seq_set_overflow(m);
> + seq_set_full(m);
and here
> return -1;
> }
> EXPORT_SYMBOL(seq_bitmap_list);
> @@ -702,7 +702,7 @@ int seq_puts(struct seq_file *m, const char *s)
> m->count += len;
> return 0;
> }
> - seq_set_overflow(m);
> + seq_set_full(m);
and here
> return -1;
> }
> EXPORT_SYMBOL(seq_puts);
> @@ -736,7 +736,7 @@ int seq_put_decimal_ull(struct seq_file *m, char delimiter,
> m->count += len;
> return 0;
> overflow:
> - seq_set_overflow(m);
> + seq_set_full(m);
We should change one above contition from
if (m->count + 2 >= m->size) /* we'll write 2 bytes at least */
goto overflow;
to
if (m->count + 2 > m->size) /* we'll write 2 bytes at least */
goto overflow;
> return -1;
> }
> EXPORT_SYMBOL(seq_put_decimal_ull);
> @@ -746,7 +746,7 @@ int seq_put_decimal_ll(struct seq_file *m, char delimiter,
> {
> if (num < 0) {
> if (m->count + 3 >= m->size) {
> - seq_set_overflow(m);
> + seq_set_full(m);
> return -1;
We should change this to:
if (m->count + 3 > m->size) {
seq_set_overflow(m);
> }
> if (delimiter)
> @@ -774,7 +774,7 @@ int seq_write(struct seq_file *seq, const void *data, size_t len)
> seq->count += len;
> return 0;
> }
> - seq_set_overflow(seq);
> + seq_set_full(seq);
IMHO, the whole function should be:
void seq_write(struct seq_file *seq, const void *data, size_t len)
{
if (seq->count + len <= seq->size) {
memcpy(seq->buf + seq->count, data, len);
seq->count += len;
return 0;
}
seq_set_overflow(seq);
}
I mean that the overflow should be set only when there is a real overflow.
All in all, this patch should depend on the Steven's work and most of
it will be unused.
Best Regards,
Petr
> return -1;
> }
> EXPORT_SYMBOL(seq_write);
> diff --git a/include/linux/seq_file.h b/include/linux/seq_file.h
> index 52e0097..1f36da8 100644
> --- a/include/linux/seq_file.h
> +++ b/include/linux/seq_file.h
> @@ -43,6 +43,14 @@ struct seq_operations {
> #define SEQ_SKIP 1
>
> /**
> + * seq_is_full - check if the buffer associated to seq_file is full
> + * @m: the seq_file handle
> + *
> + * Returns true if the buffer is full
> + */
> +bool seq_is_full(struct seq_file *m);
> +
> +/**
> * seq_get_buf - get buffer to write arbitrary data to
> * @m: the seq_file handle
> * @bufp: the beginning of the buffer is stored here
> --
> 1.8.1.2.459.gbcd45b4.dirty
>
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
