Re: [PATCH] seq_file: Fix seq_putc() to be consistent with seq_puts()

[Petr Mladek]

On Fri 2014-09-26 14:37:21, Steven Rostedt wrote:
> I noticed that seq_putc() is not consistent with the way seq_puts() is
> when it comes to filling the buffer.
> 
> Lets look at the two functions doing basically the same thing:
> 
> 	seq_putc('A');
> 
> and
> 
> 	seq_puts("A");
> 
> Now if m->count is 1 less than m->size, the two do different things.
> The seq_putc() will write the character, return success, but if a
> seq_overflow() is called on the seq_file *m, it will return true.
> 
> For seq_puts(), the character is not written, count is set to size, it
> returns failure (-1), and like seq_putc(), seq_overflow() will
> also return true.

Great catch!

> Note, technically, both should act the exact same way, because
> seq_puts() does not add the ending null character:
> 
> int seq_puts(struct seq_file *m, const char *s)
> {
>         int len = strlen(s);
>         if (m->count + len < m->size) {
>                 memcpy(m->buf + m->count, s, len);
>                 m->count += len;
>                 return 0;
>         }
>         seq_set_overflow(m);
>         return -1;
> }
> 
> len is just the number of characters in "A" and does not include the
> nul terminator. The memcpy, would only copy one character for the
> example above. But if m->count is 1 less than m->size, it would not do
> the copy and would set the overflow and return failure.
> 
> Looking at seq_putc():
> 
> int seq_putc(struct seq_file *m, char c)
> {
>         if (m->count < m->size) {
>                 m->buf[m->count++] = c;
>                 return 0;
>         }
>         return -1;
> }
> 
> If m->count is one less than m->size, the if condition will succeed,
> the buffer will be updated, and success would be returned. But as
> overflow is determined by m->count == m->size, that would be true.
> 
> Note, I also noticed that seq_puts() behaves slightly different than
> other seq_*() functions, in that it does not fill the buffer when it
> overflows, but just sets the m->count to size. I'm not sure if this is
> incorrect or not as there's bogus data within the buffer that is
> encompassed by m->count. This isn't that important as the seq_file()
> which uses this code, on overflow, simply frees the entire contents of
> m->buf, and updates the size by a power of two the next go around, and
> it doesn't care if there's bogus data in an overflowed buffer or not.

Hmm, this inconsistency seems to be in more functions. I would divide
it into three categories:

a) Functions that writes valid data until the end of the buffer
   and returns -1 when the operation makes it full (m->count ==
   m->size) or when they are not able to write at all:

	seq_bitmap()
	seq_bitmap_list()


b) Functions that writes the full buffer but they report -1 only
   when they cannot write at all:

	set_putc()


c) Functions that leave mess at the end of the buffer when they could
   not write all data; they mark it as full and return -1 when this happens:

	set_puts()
	seq_put_decimal_ull()
	seq_put_decimal_ll()
	seq_write()


This patch moves set_putc() from b) to c) because it leaves mess in the
last byte.


The semantic looks weird in all categories:

add a) They report error even when there was not a real overflow.
    Well, they do not know if there was overflow because
    bitmap_scnprintf() does not report if it shrunk
    the data or not.

add b) It does not report error even when seq_overflow() might later
    report overflow.

add c) Any overflow invalidates the whole buffer because there might
    be mess at the end. The overflow is not checked by read functions,
    so they allow to read the mess.


I think that reasonable semantic would be:

  + either signalize overflow another way (extra struct member and
    leave count to point to the valid data) or always fill valid
    data until the end of the buffer.

    The second variant might be problem if we add only half of
    an escape sequence.


  + always return error when seq_overflow() would return overflow;
    in fact, the full buffer means that the last write operation
    was most likely incomplete

    Also we might rename it from "overflow" to "full" or "filled"
    or "sealed" or "complete" that might describe the state more
    precisely because there is not a real overflow in some situations


Best Regards,
Petr

 
> But, I'm trying to write code that both the seq_file and trace_seq can
> share and these issues will come to the surface and make a difference.
> 
> Anyway, this patch is just to make seq_putc() match the behavior of
> seq_puts() which does have visibility to the other subsystems.
> 
> Signed-off-by: Steven Rostedt <rostedt@xxxxxxxxxxx>
> ---
> diff --git a/fs/seq_file.c b/fs/seq_file.c
> index 3857b720cb1b..4cdd900c6a0c 100644
> --- a/fs/seq_file.c
> +++ b/fs/seq_file.c
> @@ -686,10 +686,11 @@ EXPORT_SYMBOL(seq_open_private);
>  
>  int seq_putc(struct seq_file *m, char c)
>  {
> -	if (m->count < m->size) {
> +	if (m->count + 1 < m->size) {
>
>  		m->buf[m->count++] = c;
>  		return 0;
>  	}
> +	seq_set_overflow(m);
>  	return -1;
>  }
>  EXPORT_SYMBOL(seq_putc);

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/