Re: [PATCH v2 0/6] scsi: Some seq_file cleanups/optimizations

Re: [PATCH v2 0/6] scsi: Some seq_file cleanups/optimizations

[Finn Thain]

I have one reservation about this patch series.

For example, the changes,

-	seq_printf(m, "%s", p);
+	seq_puts(m, p);

These calls are not equivalent because the bounds check is not the same. 
seq_puts will fail when m->count + strlen(p) == m->size.

seq_write() does the same check as seq_puts() but the other routines vary.

There's a similar situation with the changes,

-	seq_puts(m, "x");
+	seq_putc(m, 'x');

Have you considered what the implications might be? Are there any?

-- 

On Wed, 3 Dec 2014, Rasmus Villemoes wrote:

> These patches mostly replace seq_printf with simpler and faster
> equivalents, e.g. seq_printf(m, "something") => seq_puts(m,
> "something") and seq_printf(m, "\n") => seq_putc(m, '\n). But before
> my Coccinelle scripts could be unleashed I had to clean up an
> unnecessary macro.
> 
> The patches don't change the semantics of the code (well, that's the
> idea anyway), but should make it slightly smaller and faster.
> 
> v2: Redone on top of git://git.infradead.org/users/hch/scsi-queue.git drivers-for-3.19
> 
> Rasmus Villemoes (6):
>   scsi: Remove SPRINTF macro
>   scsi/advansys: Replace seq_printf with seq_puts
>   scsi/aha152x: Replace seq_printf with seq_puts
>   scsi: misc:  Replace seq_printf with seq_puts
>   scsi: misc: Merge consecutive seq_puts calls
>   scsi: misc: Print single-character strings with seq_putc
> 
>  drivers/scsi/BusLogic.c             |  10 +-
>  drivers/scsi/NCR5380.c              |  20 ++-
>  drivers/scsi/advansys.c             | 142 ++++++++---------
>  drivers/scsi/aha152x.c              | 295 ++++++++++++++++++------------------
>  drivers/scsi/aic7xxx/aic79xx_proc.c |  38 +++--
>  drivers/scsi/aic7xxx/aic7xxx_proc.c |  24 +--
>  drivers/scsi/arm/fas216.c           |   6 +-
>  drivers/scsi/atari_NCR5380.c        |   4 +-
>  drivers/scsi/atp870u.c              |   5 +-
>  drivers/scsi/dc395x.c               |  79 +++++-----
>  drivers/scsi/dpt_i2o.c              |   2 +-
>  drivers/scsi/eata_pio.c             |   2 +-
>  drivers/scsi/esas2r/esas2r_main.c   |   2 +-
>  drivers/scsi/gdth_proc.c            |  24 +--
>  drivers/scsi/in2000.c               |  18 +--
>  drivers/scsi/ips.c                  |   7 +-
>  drivers/scsi/megaraid.c             |   2 +-
>  drivers/scsi/nsp32.c                |  41 +++--
>  drivers/scsi/pcmcia/nsp_cs.c        |  50 +++---
>  drivers/scsi/qla2xxx/qla_dfs.c      |   8 +-
>  drivers/scsi/scsi_proc.c            |  22 +--
>  drivers/scsi/scsi_trace.c           |   6 +-
>  drivers/scsi/wd33c93.c              |  18 +--
>  drivers/scsi/wd7000.c               |  41 +++--
>  24 files changed, 412 insertions(+), 454 deletions(-)
> 
> 

Re: [PATCH v2 0/6] scsi: Some seq_file cleanups/optimizations

[Rasmus Villemoes]

On Thu, Jan 29 2015, Finn Thain <fthain@telegraphics.com.au> wrote:

> I have one reservation about this patch series.
>
> For example, the changes,
>
> -	seq_printf(m, "%s", p);
> +	seq_puts(m, p);
>
> These calls are not equivalent because the bounds check is not the same. 
> seq_puts will fail when m->count + strlen(p) == m->size.
>

So will seq_printf:

int seq_vprintf(struct seq_file *m, const char *f, va_list args)
{
	int len;

	if (m->count < m->size) {
		len = vsnprintf(m->buf + m->count, m->size - m->count, f, args);
		if (m->count + len < m->size) {
			m->count += len;
			return 0;
		}
	}
	seq_set_overflow(m);
	return -1;
}

The return value from vsnprintf("%s", p) is by definition the length of
the string p. Yes, vsnprintf may write some of the bytes from the
string to the buffer, but those are effectively discarded if they don't
all fit, since m->count is not updated.

> There's a similar situation with the changes,
>
> -	seq_puts(m, "x");
> +	seq_putc(m, 'x');

It's true that this may cause 'x' to be printed which it might not have
been before. I think this is a bug in seq_puts - it should use <= for
its bounds check. OTOH, seq_printf probably needs to continue using <,
because if the return value is == m->size-m->count, vsnprintf will have
truncated the output, overwriting the last byte with a '\0'.

> Have you considered what the implications might be? Are there any?

I must admit I hadn't thought that deeply about it before, but now it
seems that my patches can only end up utilizing m->buf a bit better
(well, 8 bits, to be precise). If I understand the whole seq_*
interface, overflow will just cause a larger buffer to be allocated and
all the print functions to be called again.

Steven, you've been doing some cleanup in this area, among other things
trying to make all the seq_* functions return void. Could you fill me in
on the status of that?

Rasmus

Re: [PATCH v2 0/6] scsi: Some seq_file cleanups/optimizations

[Steven Rostedt]

On Thu, 29 Jan 2015 10:16:16 +0100
Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:

> Steven, you've been doing some cleanup in this area, among other things
> trying to make all the seq_* functions return void. Could you fill me in
> on the status of that?

Yes, the entire seq_*() operations are ambiguous in how they handle
filling the buffers. Don't worry about side effects of using one seq
operation over another (I highly doubt anyone will notice).

I had to stop doing the cleanups to work on other things, but I have
patches to make all seq operations perform the same (and also use the
new seq_buf infrastructure).

And, please ignore any return value from the seq operations. If you
want to know if the buffer is full use seq_has_overflowed() to find out.

I'll try to continue this clean up as a side project.

-- Steve