[PATCH] ipc: Remove uses of return value of seq_printf/seq_puts

[PATCH] ipc: Remove uses of return value of seq_printf/seq_puts

[Joe Perches]

These functions are soon going to return void so remove the
return value uses.

Convert the return value to test seq_has_overflowed() instead.

Signed-off-by: Joe Perches <joe@perches.com>
---
 ipc/msg.c  | 34 ++++++++++++++++++----------------
 ipc/sem.c  | 26 ++++++++++++++------------
 ipc/shm.c  | 42 ++++++++++++++++++++++--------------------
 ipc/util.c |  6 ++++--
 4 files changed, 58 insertions(+), 50 deletions(-)

diff --git a/ipc/msg.c b/ipc/msg.c
index a7261d5..5b530ba 100644
--- a/ipc/msg.c
+++ b/ipc/msg.c
@@ -1015,22 +1015,24 @@ static int sysvipc_msg_proc_show(struct seq_file *s, void *it)
 	struct user_namespace *user_ns = seq_user_ns(s);
 	struct msg_queue *msq = it;
 
-	return seq_printf(s,
-			"%10d %10d  %4o  %10lu %10lu %5u %5u %5u %5u %5u %5u %10lu %10lu %10lu\n",
-			msq->q_perm.key,
-			msq->q_perm.id,
-			msq->q_perm.mode,
-			msq->q_cbytes,
-			msq->q_qnum,
-			msq->q_lspid,
-			msq->q_lrpid,
-			from_kuid_munged(user_ns, msq->q_perm.uid),
-			from_kgid_munged(user_ns, msq->q_perm.gid),
-			from_kuid_munged(user_ns, msq->q_perm.cuid),
-			from_kgid_munged(user_ns, msq->q_perm.cgid),
-			msq->q_stime,
-			msq->q_rtime,
-			msq->q_ctime);
+	seq_printf(s,
+		   "%10d %10d  %4o  %10lu %10lu %5u %5u %5u %5u %5u %5u %10lu %10lu %10lu\n",
+		   msq->q_perm.key,
+		   msq->q_perm.id,
+		   msq->q_perm.mode,
+		   msq->q_cbytes,
+		   msq->q_qnum,
+		   msq->q_lspid,
+		   msq->q_lrpid,
+		   from_kuid_munged(user_ns, msq->q_perm.uid),
+		   from_kgid_munged(user_ns, msq->q_perm.gid),
+		   from_kuid_munged(user_ns, msq->q_perm.cuid),
+		   from_kgid_munged(user_ns, msq->q_perm.cgid),
+		   msq->q_stime,
+		   msq->q_rtime,
+		   msq->q_ctime);
+
+	return seq_has_overflowed(s);
 }
 #endif
 
diff --git a/ipc/sem.c b/ipc/sem.c
index 9284211..7dbd359 100644
--- a/ipc/sem.c
+++ b/ipc/sem.c
@@ -2170,17 +2170,19 @@ static int sysvipc_sem_proc_show(struct seq_file *s, void *it)
 
 	sem_otime = get_semotime(sma);
 
-	return seq_printf(s,
-			  "%10d %10d  %4o %10u %5u %5u %5u %5u %10lu %10lu\n",
-			  sma->sem_perm.key,
-			  sma->sem_perm.id,
-			  sma->sem_perm.mode,
-			  sma->sem_nsems,
-			  from_kuid_munged(user_ns, sma->sem_perm.uid),
-			  from_kgid_munged(user_ns, sma->sem_perm.gid),
-			  from_kuid_munged(user_ns, sma->sem_perm.cuid),
-			  from_kgid_munged(user_ns, sma->sem_perm.cgid),
-			  sem_otime,
-			  sma->sem_ctime);
+	seq_printf(s,
+		   "%10d %10d  %4o %10u %5u %5u %5u %5u %10lu %10lu\n",
+		   sma->sem_perm.key,
+		   sma->sem_perm.id,
+		   sma->sem_perm.mode,
+		   sma->sem_nsems,
+		   from_kuid_munged(user_ns, sma->sem_perm.uid),
+		   from_kgid_munged(user_ns, sma->sem_perm.gid),
+		   from_kuid_munged(user_ns, sma->sem_perm.cuid),
+		   from_kgid_munged(user_ns, sma->sem_perm.cgid),
+		   sem_otime,
+		   sma->sem_ctime);
+
+	return seq_has_overflowed(s);
 }
 #endif
diff --git a/ipc/shm.c b/ipc/shm.c
index 19633b4..4eff4ca 100644
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -1342,25 +1342,27 @@ static int sysvipc_shm_proc_show(struct seq_file *s, void *it)
 #define SIZE_SPEC "%21lu"
 #endif
 
-	return seq_printf(s,
-			  "%10d %10d  %4o " SIZE_SPEC " %5u %5u  "
-			  "%5lu %5u %5u %5u %5u %10lu %10lu %10lu "
-			  SIZE_SPEC " " SIZE_SPEC "\n",
-			  shp->shm_perm.key,
-			  shp->shm_perm.id,
-			  shp->shm_perm.mode,
-			  shp->shm_segsz,
-			  shp->shm_cprid,
-			  shp->shm_lprid,
-			  shp->shm_nattch,
-			  from_kuid_munged(user_ns, shp->shm_perm.uid),
-			  from_kgid_munged(user_ns, shp->shm_perm.gid),
-			  from_kuid_munged(user_ns, shp->shm_perm.cuid),
-			  from_kgid_munged(user_ns, shp->shm_perm.cgid),
-			  shp->shm_atim,
-			  shp->shm_dtim,
-			  shp->shm_ctim,
-			  rss * PAGE_SIZE,
-			  swp * PAGE_SIZE);
+	seq_printf(s,
+		   "%10d %10d  %4o " SIZE_SPEC " %5u %5u  "
+		   "%5lu %5u %5u %5u %5u %10lu %10lu %10lu "
+		   SIZE_SPEC " " SIZE_SPEC "\n",
+		   shp->shm_perm.key,
+		   shp->shm_perm.id,
+		   shp->shm_perm.mode,
+		   shp->shm_segsz,
+		   shp->shm_cprid,
+		   shp->shm_lprid,
+		   shp->shm_nattch,
+		   from_kuid_munged(user_ns, shp->shm_perm.uid),
+		   from_kgid_munged(user_ns, shp->shm_perm.gid),
+		   from_kuid_munged(user_ns, shp->shm_perm.cuid),
+		   from_kgid_munged(user_ns, shp->shm_perm.cgid),
+		   shp->shm_atim,
+		   shp->shm_dtim,
+		   shp->shm_ctim,
+		   rss * PAGE_SIZE,
+		   swp * PAGE_SIZE);
+
+	return seq_has_overflowed(s);
 }
 #endif
diff --git a/ipc/util.c b/ipc/util.c
index 106bed0..4f726b7 100644
--- a/ipc/util.c
+++ b/ipc/util.c
@@ -837,8 +837,10 @@ static int sysvipc_proc_show(struct seq_file *s, void *it)
 	struct ipc_proc_iter *iter = s->private;
 	struct ipc_proc_iface *iface = iter->iface;
 
-	if (it == SEQ_START_TOKEN)
-		return seq_puts(s, iface->header);
+	if (it == SEQ_START_TOKEN) {
+		seq_puts(s, iface->header);
+		return seq_has_overflowed(s);
+	}
 
 	return iface->show(s, it);
 }

Re: [PATCH] ipc: Remove uses of return value of seq_printf/seq_puts

[Andrew Morton]

On Tue, 17 Feb 2015 11:44:48 -0800 Joe Perches <joe@perches.com> wrote:

> These functions are soon going to return void

That's news to me.

> so remove the
> return value uses.
> 
> Convert the return value to test seq_has_overflowed() instead.

Why not make seq_printf() return seq_has_overflowed()?

I'm sure it's all very sensible, but the changelogging is poor.  Perhaps
doing all this in a coherent patch series would be a better way.

Re: [PATCH] ipc: Remove uses of return value of seq_printf/seq_puts

[Joe Perches]

On Tue, 2015-02-17 at 14:52 -0800, Andrew Morton wrote:
> On Tue, 17 Feb 2015 11:44:48 -0800 Joe Perches <joe@perches.com> wrote:
> 
> > These functions are soon going to return void
> 
> That's news to me.

https://lkml.org/lkml/2014/11/4/393

> > so remove the
> > return value uses.
> > 
> > Convert the return value to test seq_has_overflowed() instead.
> 
> Why not make seq_printf() return seq_has_overflowed()?

Unnecessary overhead.

> I'm sure it's all very sensible, but the changelogging is poor.  Perhaps
> doing all this in a coherent patch series would be a better way.

The changes span too many subsystems.

These could just return 0 for the most part.

Re: [PATCH] ipc: Remove uses of return value of seq_printf/seq_puts

[Al Viro]

On Tue, Feb 17, 2015 at 02:52:46PM -0800, Andrew Morton wrote:
> On Tue, 17 Feb 2015 11:44:48 -0800 Joe Perches <joe@perches.com> wrote:
> 
> > These functions are soon going to return void
> 
> That's news to me.
> 
> > so remove the
> > return value uses.
> > 
> > Convert the return value to test seq_has_overflowed() instead.
> 
> Why not make seq_printf() return seq_has_overflowed()?

Because we are getting well-meaning folks who try to check that return value,
again and again, getting it wrong every time.   Typical idiocies:
	* return some kind of error out of ->show() on overflows.  Pointless
*and* wrong - only hard errors (== fail read(2) with that) should be
reported that way; the caller does detect overflow and retires with bigger
buffer just fine.
	* keep checking it after every sodding call of seq_...(), screwing
the cleanups up more often than not.  Pointless, unless you are doing some
seriously expensive calculations to produce something you are going to print.
seq_...() are no-ops in case when overflow has already happened.

seq_had_overflowed() is only for situations when you really want to skip
a serious amount of work generating the data that would end up being
discarded and recalculated again when the caller grabs a bigger buffer and
calls you again.  And more often than not it's an indication of ->show()
trying to do the work of iterator - e.g. when you have single_open() with
->show() printing the entire hash table of some sort all in one record.

Most of the time checking return value of seq_...() is better replaced with
not doing that.  And "must check return value and Do Something(tm)" is too
strong habit for enough people to cause recurring trouble.

Re: [PATCH] ipc: Remove uses of return value of seq_printf/seq_puts

[Joe Perches]

On Tue, 2015-02-17 at 23:16 +0000, Al Viro wrote:
> Most of the time checking return value of seq_...() is better replaced with
> not doing that.  And "must check return value and Do Something(tm)" is too
> strong habit for enough people to cause recurring trouble.

Does SEQ_SKIP still have value?

Re: [PATCH] ipc: Remove uses of return value of seq_printf/seq_puts

[Al Viro]

On Tue, Feb 17, 2015 at 04:09:44PM -0800, Joe Perches wrote:
> On Tue, 2015-02-17 at 23:16 +0000, Al Viro wrote:
> > Most of the time checking return value of seq_...() is better replaced with
> > not doing that.  And "must check return value and Do Something(tm)" is too
> > strong habit for enough people to cause recurring trouble.
> 
> Does SEQ_SKIP still have value?

Yes, it does, but it's not an error - it's an equivalent of "empty the buffer
before returning".  Basically, it's "I've decided that this entry shouldn't
produce anything".  Look at the caller:
                error = m->op->show(m, p);
                if (error < 0)
                        break;
                if (unlikely(error)) {
                        error = 0;
                        m->count = 0;
                }
Negatives are hard errors.  Positives (without distinction) are equivalent
to zero, except that we discard anything that might've been produced by
this call of ->show().  Another call site (one when we are trying to pack
more into buffer that already has some records in it) is
                size_t offs = m->count;
...
                err = m->op->show(m, p);
                if (seq_has_overflowed(m) || err) {
                        m->count = offs;
                        if (likely(err <= 0))
                                break;
                }
IOW, here we treat positive as "discard everything produced by this call of
->show(), ignore seq_has_overflowed() it might have triggered".  Might as
well have done
	if (err > 0) {
		m->count = offs; /* seq_has_overflowed() is false now */
		err = 0;
	}
	if (seq_has_overflowed(m) || err < 0) {
		m->count = offs;
		break;
	}
except that it'd cost more that way.

In principle, we could've provided seq_discard(m), but that would've required
keeping a snapshot of ->count in another field of struct seq_file, and that -
for a very rarely used thing.  And keep in mind that hard errors need to
be reported anyway, so it's not as if we could realistically make ->show()
return void.

Re: [PATCH] ipc: Remove uses of return value of seq_printf/seq_puts

[Joe Perches]

On Wed, 2015-02-18 at 00:27 +0000, Al Viro wrote:
> On Tue, Feb 17, 2015 at 04:09:44PM -0800, Joe Perches wrote:
> > On Tue, 2015-02-17 at 23:16 +0000, Al Viro wrote:
> > > Most of the time checking return value of seq_...() is better replaced with
> > > not doing that.  And "must check return value and Do Something(tm)" is too
> > > strong habit for enough people to cause recurring trouble.
> > 
> > Does SEQ_SKIP still have value?
> 
> Yes, it does, but it's not an error - it's an equivalent of "empty the buffer
> before returning".  Basically, it's "I've decided that this entry shouldn't
> produce anything".  Look at the caller:

Yeah.

I should have looked at the traverse function first.

>                 error = m->op->show(m, p);
>                 if (error < 0)
>                         break;
>                 if (unlikely(error)) {
>                         error = 0;
>                         m->count = 0;
>                 }

Basically all the show functions that I converted
to use seq_has_overflowed() should just return 0.

There are only 3 current uses of SEQ_SKIP

drivers/infiniband/hw/qib/qib_debugfs.c:119:            return SEQ_SKIP;
drivers/infiniband/hw/qib/qib_debugfs.c:177:            return SEQ_SKIP;
drivers/infiniband/hw/qib/qib_debugfs.c:183:            return SEQ_SKIP;

As far as I can tell, these uses would be fine
not using SEQ_SKIP and just emitting 0 instead
of nothing.