From: Petr Mladek <pmladek@suse.cz>
To: Joe Perches <joe@perches.com>
Cc: Steven Rostedt <rostedt@goodmis.org>,
Al Viro <viro@ZenIV.linux.org.uk>,
Andrew Morton <akpm@linux-foundation.org>,
Linus Torvalds <torvalds@linux-foundation.org>,
Christine Caulfield <ccaulfie@redhat.com>,
David Teigland <teigland@redhat.com>,
cluster-devel@redhat.com, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 3/7] dlm: Use seq_is_full - remove seq_printf returns
Date: Tue, 30 Sep 2014 12:34:35 +0200 [thread overview]
Message-ID: <20140930103435.GC2707@dhcp128.suse.cz> (raw)
In-Reply-To: <43590057bcb83846acbbcc1fe641f792b2fb7773.1412031505.git.joe@perches.com>
On Mon 29-09-14 16:08:23, Joe Perches wrote:
> The seq_printf return should be ignored and the
> seq_is_full function should be tested instead.
>
> Convert functions returning int to void where
> seq_printf is used.
>
> Signed-off-by: Joe Perches <joe@perches.com>
> ---
> fs/dlm/debug_fs.c | 248 +++++++++++++++++++++++++-----------------------------
> 1 file changed, 116 insertions(+), 132 deletions(-)
>
> diff --git a/fs/dlm/debug_fs.c b/fs/dlm/debug_fs.c
> index 1323c56..27c8335 100644
> --- a/fs/dlm/debug_fs.c
> +++ b/fs/dlm/debug_fs.c
> @@ -48,8 +48,8 @@ static char *print_lockmode(int mode)
> }
> }
>
> -static int print_format1_lock(struct seq_file *s, struct dlm_lkb *lkb,
> - struct dlm_rsb *res)
> +static void print_format1_lock(struct seq_file *s, struct dlm_lkb *lkb,
> + struct dlm_rsb *res)
> {
> seq_printf(s, "%08x %s", lkb->lkb_id, print_lockmode(lkb->lkb_grmode));
>
> @@ -68,21 +68,17 @@ static int print_format1_lock(struct seq_file *s, struct dlm_lkb *lkb,
> if (lkb->lkb_wait_type)
> seq_printf(s, " wait_type: %d", lkb->lkb_wait_type);
>
> - return seq_puts(s, "\n");
> + seq_puts(s, "\n");
> }
>
> -static int print_format1(struct dlm_rsb *res, struct seq_file *s)
> +static void print_format1(struct dlm_rsb *res, struct seq_file *s)
> {
> struct dlm_lkb *lkb;
> int i, lvblen = res->res_ls->ls_lvblen, recover_list, root_list;
> - int rv;
>
> lock_rsb(res);
>
> - rv = seq_printf(s, "\nResource %p Name (len=%d) \"",
> - res, res->res_length);
> - if (rv)
> - goto out;
> + seq_printf(s, "\nResource %p Name (len=%d) \"", res, res->res_length);
To keep the functionality, we should add
if(seq_overflow(s))
goto out;
> for (i = 0; i < res->res_length; i++) {
> if (isprint(res->res_name[i]))
> @@ -92,17 +88,16 @@ static int print_format1(struct dlm_rsb *res, struct seq_file *s)
> }
>
> if (res->res_nodeid > 0)
> - rv = seq_printf(s, "\"\nLocal Copy, Master is node %d\n",
> - res->res_nodeid);
> + seq_printf(s, "\"\nLocal Copy, Master is node %d\n",
> + res->res_nodeid);
> else if (res->res_nodeid == 0)
> - rv = seq_puts(s, "\"\nMaster Copy\n");
> + seq_puts(s, "\"\nMaster Copy\n");
> else if (res->res_nodeid == -1)
> - rv = seq_printf(s, "\"\nLooking up master (lkid %x)\n",
> - res->res_first_lkid);
> + seq_printf(s, "\"\nLooking up master (lkid %x)\n",
> + res->res_first_lkid);
> else
> - rv = seq_printf(s, "\"\nInvalid master %d\n",
> - res->res_nodeid);
> - if (rv)
> + seq_printf(s, "\"\nInvalid master %d\n", res->res_nodeid);
> + if (seq_is_full(s))
> goto out;
I would check for seq_overflow()
Etc. There are needed many more changes if we agree on introducing
seq_is_full() and seq_overflow().
Well, we will need to touch most of these locations once again when
Steven removes return value from all the seq_* functions.
Best Regards,
Petr
> /* Print the LVB: */
> @@ -116,8 +111,8 @@ static int print_format1(struct dlm_rsb *res, struct seq_file *s)
> }
> if (rsb_flag(res, RSB_VALNOTVALID))
> seq_puts(s, " (INVALID)");
> - rv = seq_puts(s, "\n");
> - if (rv)
> + seq_puts(s, "\n");
> + if (seq_is_full(s))
> goto out;
> }
>
> @@ -125,32 +120,30 @@ static int print_format1(struct dlm_rsb *res, struct seq_file *s)
> recover_list = !list_empty(&res->res_recover_list);
>
> if (root_list || recover_list) {
> - rv = seq_printf(s, "Recovery: root %d recover %d flags %lx "
> - "count %d\n", root_list, recover_list,
> - res->res_flags, res->res_recover_locks_count);
> - if (rv)
> - goto out;
> + seq_printf(s, "Recovery: root %d recover %d flags %lx count %d\n",
> + root_list, recover_list,
> + res->res_flags, res->res_recover_locks_count);
> }
>
> /* Print the locks attached to this resource */
> seq_puts(s, "Granted Queue\n");
> list_for_each_entry(lkb, &res->res_grantqueue, lkb_statequeue) {
> - rv = print_format1_lock(s, lkb, res);
> - if (rv)
> + print_format1_lock(s, lkb, res);
> + if (seq_is_full(s))
> goto out;
> }
>
> seq_puts(s, "Conversion Queue\n");
> list_for_each_entry(lkb, &res->res_convertqueue, lkb_statequeue) {
> - rv = print_format1_lock(s, lkb, res);
> - if (rv)
> + print_format1_lock(s, lkb, res);
> + if (seq_is_full(s))
> goto out;
> }
>
> seq_puts(s, "Waiting Queue\n");
> list_for_each_entry(lkb, &res->res_waitqueue, lkb_statequeue) {
> - rv = print_format1_lock(s, lkb, res);
> - if (rv)
> + print_format1_lock(s, lkb, res);
> + if (seq_is_full(s))
> goto out;
> }
>
> @@ -159,23 +152,23 @@ static int print_format1(struct dlm_rsb *res, struct seq_file *s)
>
> seq_puts(s, "Lookup Queue\n");
> list_for_each_entry(lkb, &res->res_lookup, lkb_rsb_lookup) {
> - rv = seq_printf(s, "%08x %s", lkb->lkb_id,
> - print_lockmode(lkb->lkb_rqmode));
> + seq_printf(s, "%08x %s",
> + lkb->lkb_id, print_lockmode(lkb->lkb_rqmode));
> if (lkb->lkb_wait_type)
> seq_printf(s, " wait_type: %d", lkb->lkb_wait_type);
> - rv = seq_puts(s, "\n");
> + seq_puts(s, "\n");
> + if (seq_is_full(s))
> + goto out;
> }
> out:
> unlock_rsb(res);
> - return rv;
> }
>
> -static int print_format2_lock(struct seq_file *s, struct dlm_lkb *lkb,
> - struct dlm_rsb *r)
> +static void print_format2_lock(struct seq_file *s, struct dlm_lkb *lkb,
> + struct dlm_rsb *r)
> {
> u64 xid = 0;
> u64 us;
> - int rv;
>
> if (lkb->lkb_flags & DLM_IFL_USER) {
> if (lkb->lkb_ua)
> @@ -188,103 +181,97 @@ static int print_format2_lock(struct seq_file *s, struct dlm_lkb *lkb,
> /* id nodeid remid pid xid exflags flags sts grmode rqmode time_us
> r_nodeid r_len r_name */
>
> - rv = seq_printf(s, "%x %d %x %u %llu %x %x %d %d %d %llu %u %d \"%s\"\n",
> - lkb->lkb_id,
> - lkb->lkb_nodeid,
> - lkb->lkb_remid,
> - lkb->lkb_ownpid,
> - (unsigned long long)xid,
> - lkb->lkb_exflags,
> - lkb->lkb_flags,
> - lkb->lkb_status,
> - lkb->lkb_grmode,
> - lkb->lkb_rqmode,
> - (unsigned long long)us,
> - r->res_nodeid,
> - r->res_length,
> - r->res_name);
> - return rv;
> + seq_printf(s, "%x %d %x %u %llu %x %x %d %d %d %llu %u %d \"%s\"\n",
> + lkb->lkb_id,
> + lkb->lkb_nodeid,
> + lkb->lkb_remid,
> + lkb->lkb_ownpid,
> + (unsigned long long)xid,
> + lkb->lkb_exflags,
> + lkb->lkb_flags,
> + lkb->lkb_status,
> + lkb->lkb_grmode,
> + lkb->lkb_rqmode,
> + (unsigned long long)us,
> + r->res_nodeid,
> + r->res_length,
> + r->res_name);
> }
>
> -static int print_format2(struct dlm_rsb *r, struct seq_file *s)
> +static void print_format2(struct dlm_rsb *r, struct seq_file *s)
> {
> struct dlm_lkb *lkb;
> - int rv = 0;
>
> lock_rsb(r);
>
> list_for_each_entry(lkb, &r->res_grantqueue, lkb_statequeue) {
> - rv = print_format2_lock(s, lkb, r);
> - if (rv)
> + print_format2_lock(s, lkb, r);
> + if (seq_is_full(s))
> goto out;
> }
>
> list_for_each_entry(lkb, &r->res_convertqueue, lkb_statequeue) {
> - rv = print_format2_lock(s, lkb, r);
> - if (rv)
> + print_format2_lock(s, lkb, r);
> + if (seq_is_full(s))
> goto out;
> }
>
> list_for_each_entry(lkb, &r->res_waitqueue, lkb_statequeue) {
> - rv = print_format2_lock(s, lkb, r);
> - if (rv)
> + print_format2_lock(s, lkb, r);
> + if (seq_is_full(s))
> goto out;
> }
> out:
> unlock_rsb(r);
> - return rv;
> }
>
> -static int print_format3_lock(struct seq_file *s, struct dlm_lkb *lkb,
> +static void print_format3_lock(struct seq_file *s, struct dlm_lkb *lkb,
> int rsb_lookup)
> {
> u64 xid = 0;
> - int rv;
>
> if (lkb->lkb_flags & DLM_IFL_USER) {
> if (lkb->lkb_ua)
> xid = lkb->lkb_ua->xid;
> }
>
> - rv = seq_printf(s, "lkb %x %d %x %u %llu %x %x %d %d %d %d %d %d %u %llu %llu\n",
> - lkb->lkb_id,
> - lkb->lkb_nodeid,
> - lkb->lkb_remid,
> - lkb->lkb_ownpid,
> - (unsigned long long)xid,
> - lkb->lkb_exflags,
> - lkb->lkb_flags,
> - lkb->lkb_status,
> - lkb->lkb_grmode,
> - lkb->lkb_rqmode,
> - lkb->lkb_last_bast.mode,
> - rsb_lookup,
> - lkb->lkb_wait_type,
> - lkb->lkb_lvbseq,
> - (unsigned long long)ktime_to_ns(lkb->lkb_timestamp),
> - (unsigned long long)ktime_to_ns(lkb->lkb_last_bast_time));
> - return rv;
> + seq_printf(s, "lkb %x %d %x %u %llu %x %x %d %d %d %d %d %d %u %llu %llu\n",
> + lkb->lkb_id,
> + lkb->lkb_nodeid,
> + lkb->lkb_remid,
> + lkb->lkb_ownpid,
> + (unsigned long long)xid,
> + lkb->lkb_exflags,
> + lkb->lkb_flags,
> + lkb->lkb_status,
> + lkb->lkb_grmode,
> + lkb->lkb_rqmode,
> + lkb->lkb_last_bast.mode,
> + rsb_lookup,
> + lkb->lkb_wait_type,
> + lkb->lkb_lvbseq,
> + (unsigned long long)ktime_to_ns(lkb->lkb_timestamp),
> + (unsigned long long)ktime_to_ns(lkb->lkb_last_bast_time));
> }
>
> -static int print_format3(struct dlm_rsb *r, struct seq_file *s)
> +static void print_format3(struct dlm_rsb *r, struct seq_file *s)
> {
> struct dlm_lkb *lkb;
> int i, lvblen = r->res_ls->ls_lvblen;
> int print_name = 1;
> - int rv;
>
> lock_rsb(r);
>
> - rv = seq_printf(s, "rsb %p %d %x %lx %d %d %u %d ",
> - r,
> - r->res_nodeid,
> - r->res_first_lkid,
> - r->res_flags,
> - !list_empty(&r->res_root_list),
> - !list_empty(&r->res_recover_list),
> - r->res_recover_locks_count,
> - r->res_length);
> - if (rv)
> + seq_printf(s, "rsb %p %d %x %lx %d %d %u %d ",
> + r,
> + r->res_nodeid,
> + r->res_first_lkid,
> + r->res_flags,
> + !list_empty(&r->res_root_list),
> + !list_empty(&r->res_recover_list),
> + r->res_recover_locks_count,
> + r->res_length);
> + if (seq_is_full(s))
> goto out;
>
> for (i = 0; i < r->res_length; i++) {
> @@ -300,8 +287,8 @@ static int print_format3(struct dlm_rsb *r, struct seq_file *s)
> else
> seq_printf(s, " %02x", (unsigned char)r->res_name[i]);
> }
> - rv = seq_puts(s, "\n");
> - if (rv)
> + seq_puts(s, "\n");
> + if (seq_is_full(s))
> goto out;
>
> if (!r->res_lvbptr)
> @@ -311,58 +298,55 @@ static int print_format3(struct dlm_rsb *r, struct seq_file *s)
>
> for (i = 0; i < lvblen; i++)
> seq_printf(s, " %02x", (unsigned char)r->res_lvbptr[i]);
> - rv = seq_puts(s, "\n");
> - if (rv)
> + seq_puts(s, "\n");
> + if (seq_is_full(s))
> goto out;
>
> do_locks:
> list_for_each_entry(lkb, &r->res_grantqueue, lkb_statequeue) {
> - rv = print_format3_lock(s, lkb, 0);
> - if (rv)
> + print_format3_lock(s, lkb, 0);
> + if (seq_is_full(s))
> goto out;
> }
>
> list_for_each_entry(lkb, &r->res_convertqueue, lkb_statequeue) {
> - rv = print_format3_lock(s, lkb, 0);
> - if (rv)
> + print_format3_lock(s, lkb, 0);
> + if (seq_is_full(s))
> goto out;
> }
>
> list_for_each_entry(lkb, &r->res_waitqueue, lkb_statequeue) {
> - rv = print_format3_lock(s, lkb, 0);
> - if (rv)
> + print_format3_lock(s, lkb, 0);
> + if (seq_is_full(s))
> goto out;
> }
>
> list_for_each_entry(lkb, &r->res_lookup, lkb_rsb_lookup) {
> - rv = print_format3_lock(s, lkb, 1);
> - if (rv)
> + print_format3_lock(s, lkb, 1);
> + if (seq_is_full(s))
> goto out;
> }
> out:
> unlock_rsb(r);
> - return rv;
> }
>
> -static int print_format4(struct dlm_rsb *r, struct seq_file *s)
> +static void print_format4(struct dlm_rsb *r, struct seq_file *s)
> {
> int our_nodeid = dlm_our_nodeid();
> int print_name = 1;
> - int i, rv;
> + int i;
>
> lock_rsb(r);
>
> - rv = seq_printf(s, "rsb %p %d %d %d %d %lu %lx %d ",
> - r,
> - r->res_nodeid,
> - r->res_master_nodeid,
> - r->res_dir_nodeid,
> - our_nodeid,
> - r->res_toss_time,
> - r->res_flags,
> - r->res_length);
> - if (rv)
> - goto out;
> + seq_printf(s, "rsb %p %d %d %d %d %lu %lx %d ",
> + r,
> + r->res_nodeid,
> + r->res_master_nodeid,
> + r->res_dir_nodeid,
> + our_nodeid,
> + r->res_toss_time,
> + r->res_flags,
> + r->res_length);
>
> for (i = 0; i < r->res_length; i++) {
> if (!isascii(r->res_name[i]) || !isprint(r->res_name[i]))
> @@ -377,10 +361,9 @@ static int print_format4(struct dlm_rsb *r, struct seq_file *s)
> else
> seq_printf(s, " %02x", (unsigned char)r->res_name[i]);
> }
> - rv = seq_puts(s, "\n");
> - out:
> + seq_puts(s, "\n");
> +
> unlock_rsb(r);
> - return rv;
> }
>
> struct rsbtbl_iter {
> @@ -390,11 +373,12 @@ struct rsbtbl_iter {
> int header;
> };
>
> -/* seq_printf returns -1 if the buffer is full, and 0 otherwise.
> - If the buffer is full, seq_printf can be called again, but it
> - does nothing and just returns -1. So, the these printing routines
> - periodically check the return value to avoid wasting too much time
> - trying to print to a full buffer. */
> +/*
> + * If the buffer is full, seq_printf can be called again, but it
> + * does nothing. So, the these printing routines periodically check
> + * seq_is_full to avoid wasting too much time trying to print to
> + * a full buffer.
> + */
>
> static int table_seq_show(struct seq_file *seq, void *iter_ptr)
> {
> @@ -403,7 +387,7 @@ static int table_seq_show(struct seq_file *seq, void *iter_ptr)
>
> switch (ri->format) {
> case 1:
> - rv = print_format1(ri->rsb, seq);
> + print_format1(ri->rsb, seq);
> break;
> case 2:
> if (ri->header) {
> @@ -412,21 +396,21 @@ static int table_seq_show(struct seq_file *seq, void *iter_ptr)
> "r_nodeid r_len r_name\n");
> ri->header = 0;
> }
> - rv = print_format2(ri->rsb, seq);
> + print_format2(ri->rsb, seq);
> break;
> case 3:
> if (ri->header) {
> seq_printf(seq, "version rsb 1.1 lvb 1.1 lkb 1.1\n");
> ri->header = 0;
> }
> - rv = print_format3(ri->rsb, seq);
> + print_format3(ri->rsb, seq);
> break;
> case 4:
> if (ri->header) {
> seq_printf(seq, "version 4 rsb 2\n");
> ri->header = 0;
> }
> - rv = print_format4(ri->rsb, seq);
> + print_format4(ri->rsb, seq);
> break;
> }
>
> --
> 1.8.1.2.459.gbcd45b4.dirty
>
next prev parent reply other threads:[~2014-09-30 10:34 UTC|newest]
Thread overview: 53+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-09-26 18:37 [PATCH] seq_file: Fix seq_putc() to be consistent with seq_puts() Steven Rostedt
2014-09-29 14:41 ` Petr Mladek
2014-09-29 15:25 ` Steven Rostedt
2014-09-29 15:46 ` Joe Perches
2014-09-29 16:11 ` Steven Rostedt
2014-09-29 16:23 ` Petr Mladek
2014-09-29 16:40 ` Steven Rostedt
2014-09-29 15:47 ` Al Viro
2014-09-29 16:08 ` Joe Perches
2014-09-29 16:15 ` Steven Rostedt
2014-09-29 16:30 ` Joe Perches
2014-09-29 16:42 ` Steven Rostedt
2014-09-29 16:55 ` Joe Perches
2014-09-29 23:08 ` [PATCH 0/7] seq_printf cleanups Joe Perches
2014-09-29 23:08 ` [PATCH 1/7] seq_file: Rename static bool seq_overflow to public bool seq_is_full Joe Perches
2014-09-29 23:44 ` Steven Rostedt
2014-09-29 23:48 ` Joe Perches
2014-09-30 10:06 ` Petr Mladek
2014-09-29 23:08 ` [PATCH 2/7] netfilter: Convert print_tuple functions to return void Joe Perches
2014-09-30 10:22 ` Petr Mladek
2014-09-30 13:04 ` Joe Perches
2014-09-29 23:08 ` [PATCH 3/7] dlm: Use seq_is_full - remove seq_printf returns Joe Perches
2014-09-30 10:34 ` Petr Mladek [this message]
2014-10-27 20:17 ` Steven Rostedt
2014-10-27 20:25 ` Joe Perches
2014-10-29 12:21 ` Petr Mladek
2014-09-29 23:08 ` [PATCH 4/7] dlm: Use seq_puts, remove unnecessary trailing spaces Joe Perches
2014-09-29 23:08 ` [PATCH 5/7] fs: Convert show_fdinfo functions to void Joe Perches
2014-10-28 14:11 ` Steven Rostedt
2014-10-28 14:31 ` Joe Perches
2014-10-28 14:43 ` Steven Rostedt
2014-09-29 23:08 ` [PATCH 6/7] debugfs: Fix misuse of seq_printf return value Joe Perches
2014-10-28 14:58 ` Steven Rostedt
2014-10-28 15:25 ` Joe Perches
2014-10-28 15:42 ` Steven Rostedt
2014-10-28 15:59 ` Joe Perches
2014-11-07 19:03 ` Greg Kroah-Hartman
2014-09-29 23:08 ` [PATCH 7/7] docg3: Fix miuse " Joe Perches
2014-10-22 8:29 ` Brian Norris
2014-10-28 15:13 ` Steven Rostedt
2014-10-28 15:57 ` Joe Perches
2014-10-28 16:05 ` Steven Rostedt
2014-10-28 17:14 ` Brian Norris
2014-10-28 17:26 ` Steven Rostedt
2014-10-28 17:27 ` Joe Perches
2014-10-28 17:32 ` Steven Rostedt
2014-10-28 17:36 ` Brian Norris
2014-10-28 17:38 ` Joe Perches
2014-10-28 15:32 ` [PATCH 0/7] seq_printf cleanups Steven Rostedt
2014-10-28 15:51 ` Joe Perches
2014-10-06 3:33 ` [PATCH] seq_file: Fix seq_putc() to be consistent with seq_puts() Joe Perches
2014-09-29 16:09 ` Steven Rostedt
2014-09-29 16:41 ` Al Viro
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20140930103435.GC2707@dhcp128.suse.cz \
--to=pmladek@suse.cz \
--cc=akpm@linux-foundation.org \
--cc=ccaulfie@redhat.com \
--cc=cluster-devel@redhat.com \
--cc=joe@perches.com \
--cc=linux-kernel@vger.kernel.org \
--cc=rostedt@goodmis.org \
--cc=teigland@redhat.com \
--cc=torvalds@linux-foundation.org \
--cc=viro@ZenIV.linux.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Powered by JetHome