Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1665937 > unrolled thread

[PATCH 0/6] staging: lustre: lustre: assortment of checkpatch cleanups

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/6] staging: lustre: lustre: assortment of checkpatch cleanups James Simmons <jsimmons@infradead.org> - 2017-06-14 17:10 +0200
    [PATCH 6/6] staging: lustre: lustre: fix all braces issues reported by checkpatch James Simmons <jsimmons@infradead.org> - 2017-06-14 17:10 +0200
      Re: [PATCH 6/6] staging: lustre: lustre: fix all braces issues  reported by checkpatch Joe Perches <joe@perches.com> - 2017-06-14 20:30 +0200
        Re: [PATCH 6/6] staging: lustre: lustre: fix all braces issues  reported by checkpatch James Simmons <jsimmons@infradead.org> - 2017-06-15 18:40 +0200
          Re: [PATCH 6/6] staging: lustre: lustre: fix all braces issues  reported by checkpatch Joe Perches <joe@perches.com> - 2017-06-15 19:00 +0200
    Re: [PATCH 1/6] staging: lustre: lustre: resolve "use spaces  between elements" checkpatch errors Joe Perches <joe@perches.com> - 2017-06-14 20:20 +0200
      Re: [PATCH 1/6] staging: lustre: lustre: resolve "use spaces between  elements" checkpatch errors James Simmons <jsimmons@infradead.org> - 2017-06-15 18:10 +0200
        Re: [PATCH 1/6] staging: lustre: lustre: resolve "use spaces  between elements" checkpatch errors Joe Perches <joe@perches.com> - 2017-06-15 18:30 +0200
          Re: [PATCH 1/6] staging: lustre: lustre: resolve "use spaces between  elements" checkpatch errors James Simmons <jsimmons@infradead.org> - 2017-06-15 19:00 +0200
            Re: [PATCH 1/6] staging: lustre: lustre: resolve "use spaces  between elements" checkpatch errors Joe Perches <joe@perches.com> - 2017-06-15 19:10 +0200
              Re: [PATCH 1/6] staging: lustre: lustre: resolve "use spaces between  elements" checkpatch errors James Simmons <jsimmons@infradead.org> - 2017-06-15 19:20 +0200
    Re: [PATCH 5/6] staging: lustre: lustre: several over 80 characters  cleanups Joe Perches <joe@perches.com> - 2017-06-14 20:20 +0200
      Re: [PATCH 5/6] staging: lustre: lustre: several over 80 characters  cleanups James Simmons <jsimmons@infradead.org> - 2017-06-15 18:10 +0200
        Re: [PATCH 5/6] staging: lustre: lustre: several over 80 characters  cleanups Joe Perches <joe@perches.com> - 2017-06-15 18:20 +0200

#1665937 — [PATCH 0/6] staging: lustre: lustre: assortment of checkpatch cleanups

This patch series covers a good chunk of the checkpatch issues in the
current lustre client code.

James Simmons (6):
  staging: lustre: lustre: resolve "use spaces between elements" checkpatch errors
  staging: lustre: lustre: make all struct file_operations constant
  staging: lustre: lustre: fix all bare unsigned usage
  staging: lustre: lustre: add all missing indentifier names
  staging: lustre: lustre: several over 80 characters cleanups
  staging: lustre: lustre: fix all braces issues reported by checkpatch

 drivers/staging/lustre/lustre/fid/fid_request.c    |  2 +-
 drivers/staging/lustre/lustre/fld/fld_cache.c      |  9 ++--
 drivers/staging/lustre/lustre/fld/lproc_fld.c      |  2 +-
 drivers/staging/lustre/lustre/include/cl_object.h  |  6 +--
 .../staging/lustre/lustre/include/lprocfs_status.h | 15 ++++---
 drivers/staging/lustre/lustre/include/lu_object.h  |  4 +-
 .../lustre/lustre/include/lustre/lustre_idl.h      |  6 +--
 .../lustre/lustre/include/lustre/lustre_user.h     |  2 +-
 drivers/staging/lustre/lustre/include/lustre_fid.h |  2 +-
 drivers/staging/lustre/lustre/ldlm/ldlm_extent.c   |  5 ++-
 drivers/staging/lustre/lustre/ldlm/ldlm_internal.h | 40 +++++++++--------
 drivers/staging/lustre/lustre/ldlm/ldlm_lock.c     | 42 +++++++++++-------
 drivers/staging/lustre/lustre/ldlm/ldlm_pool.c     |  3 +-
 drivers/staging/lustre/lustre/ldlm/ldlm_request.c  | 20 +++++----
 drivers/staging/lustre/lustre/ldlm/ldlm_resource.c |  4 +-
 drivers/staging/lustre/lustre/llite/dcache.c       |  4 +-
 drivers/staging/lustre/lustre/llite/dir.c          | 12 ++---
 drivers/staging/lustre/lustre/llite/file.c         | 51 +++++++++++-----------
 .../staging/lustre/lustre/llite/llite_internal.h   | 36 ++++++++-------
 drivers/staging/lustre/lustre/llite/llite_lib.c    | 39 +++++++++--------
 drivers/staging/lustre/lustre/llite/llite_mmap.c   |  2 +-
 drivers/staging/lustre/lustre/llite/llite_nfs.c    | 13 +++---
 drivers/staging/lustre/lustre/llite/lproc_llite.c  | 12 ++---
 drivers/staging/lustre/lustre/llite/namei.c        | 46 +++++++++----------
 drivers/staging/lustre/lustre/llite/rw.c           |  6 ++-
 drivers/staging/lustre/lustre/llite/rw26.c         |  4 +-
 drivers/staging/lustre/lustre/llite/statahead.c    | 23 +++++-----
 drivers/staging/lustre/lustre/llite/super25.c      |  3 +-
 drivers/staging/lustre/lustre/llite/symlink.c      |  4 +-
 drivers/staging/lustre/lustre/llite/vvp_dev.c      | 11 ++---
 drivers/staging/lustre/lustre/llite/vvp_io.c       |  2 +-
 drivers/staging/lustre/lustre/llite/vvp_object.c   |  2 +-
 drivers/staging/lustre/lustre/llite/xattr.c        |  6 +--
 drivers/staging/lustre/lustre/llite/xattr_cache.c  |  4 +-
 drivers/staging/lustre/lustre/lmv/lmv_fld.c        |  2 +-
 drivers/staging/lustre/lustre/lmv/lmv_intent.c     |  9 ++--
 drivers/staging/lustre/lustre/lmv/lmv_obd.c        | 50 +++++++++++----------
 drivers/staging/lustre/lustre/lov/lov_io.c         |  2 +-
 drivers/staging/lustre/lustre/lov/lov_merge.c      |  4 +-
 drivers/staging/lustre/lustre/lov/lov_object.c     |  7 ++-
 drivers/staging/lustre/lustre/lov/lov_pack.c       |  9 ++--
 drivers/staging/lustre/lustre/lov/lov_pool.c       | 19 +++++---
 drivers/staging/lustre/lustre/lov/lproc_lov.c      | 12 +++--
 drivers/staging/lustre/lustre/mdc/mdc_lib.c        | 15 ++++---
 drivers/staging/lustre/lustre/mdc/mdc_locks.c      | 11 ++---
 drivers/staging/lustre/lustre/mdc/mdc_reint.c      |  2 +-
 drivers/staging/lustre/lustre/mdc/mdc_request.c    | 16 +++----
 drivers/staging/lustre/lustre/mgc/mgc_request.c    |  5 ++-
 drivers/staging/lustre/lustre/obdclass/cl_lock.c   |  2 +-
 drivers/staging/lustre/lustre/obdclass/cl_page.c   |  2 +-
 drivers/staging/lustre/lustre/obdclass/genops.c    |  6 ++-
 drivers/staging/lustre/lustre/obdclass/llog_cat.c  | 12 ++---
 drivers/staging/lustre/lustre/obdclass/llog_swab.c |  2 +-
 .../lustre/lustre/obdclass/lprocfs_status.c        |  2 +-
 drivers/staging/lustre/lustre/obdclass/lu_object.c |  2 +-
 .../staging/lustre/lustre/obdecho/echo_client.c    |  7 +--
 drivers/staging/lustre/lustre/osc/lproc_osc.c      |  6 ++-
 drivers/staging/lustre/lustre/osc/osc_cache.c      | 24 +++++-----
 .../staging/lustre/lustre/osc/osc_cl_internal.h    |  4 +-
 drivers/staging/lustre/lustre/osc/osc_internal.h   |  2 +-
 drivers/staging/lustre/lustre/osc/osc_request.c    | 17 +++++---
 drivers/staging/lustre/lustre/ptlrpc/client.c      | 25 ++++++-----
 drivers/staging/lustre/lustre/ptlrpc/import.c      |  8 ++--
 drivers/staging/lustre/lustre/ptlrpc/layout.c      |  7 +--
 drivers/staging/lustre/lustre/ptlrpc/llog_client.c |  3 +-
 .../staging/lustre/lustre/ptlrpc/lproc_ptlrpc.c    | 15 ++++---
 .../staging/lustre/lustre/ptlrpc/pack_generic.c    |  2 +-
 .../staging/lustre/lustre/ptlrpc/ptlrpc_internal.h |  4 +-
 drivers/staging/lustre/lustre/ptlrpc/sec.c         |  3 +-
 drivers/staging/lustre/lustre/ptlrpc/service.c     |  2 +-
 70 files changed, 425 insertions(+), 337 deletions(-)

-- 
1.8.3.1

[toc] | [next] | [standalone]

#1665938 — [PATCH 6/6] staging: lustre: lustre: fix all braces issues reported by checkpatch

Cleanup all braces that was reported by checkpatch. The only
issue not fixed up is in mdc_lock.c. Removing the braces in
the case of mdc_lock.c will break the build.

Signed-off-by: James Simmons <jsimmons@infradead.org>
---
 drivers/staging/lustre/lustre/fld/fld_cache.c  | 3 ++-
 drivers/staging/lustre/lustre/ldlm/ldlm_lock.c | 9 +++++----
 drivers/staging/lustre/lustre/llite/vvp_dev.c  | 5 +++--
 3 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/lustre/lustre/fld/fld_cache.c b/drivers/staging/lustre/lustre/fld/fld_cache.c
index b852fed..adaa094 100644
--- a/drivers/staging/lustre/lustre/fld/fld_cache.c
+++ b/drivers/staging/lustre/lustre/fld/fld_cache.c
@@ -348,9 +348,10 @@ static void fld_cache_overlap_handle(struct fld_cache *cache,
 
 		f_curr->fce_range.lsr_end = new_start;
 		fld_cache_entry_add(cache, f_new, &f_curr->fce_list);
-	} else
+	} else {
 		CERROR("NEW range =" DRANGE " curr = " DRANGE "\n",
 		       PRANGE(range), PRANGE(&f_curr->fce_range));
+	}
 }
 
 struct fld_cache_entry
diff --git a/drivers/staging/lustre/lustre/ldlm/ldlm_lock.c b/drivers/staging/lustre/lustre/ldlm/ldlm_lock.c
index 8599134..f47343a 100644
--- a/drivers/staging/lustre/lustre/ldlm/ldlm_lock.c
+++ b/drivers/staging/lustre/lustre/ldlm/ldlm_lock.c
@@ -1029,11 +1029,11 @@ void ldlm_grant_lock(struct ldlm_lock *lock, struct list_head *work_list)
 	if (work_list && lock->l_completion_ast)
 		ldlm_add_ast_work_item(lock, NULL, work_list);
 
-	if (res->lr_type == LDLM_PLAIN || res->lr_type == LDLM_IBITS)
+	if (res->lr_type == LDLM_PLAIN || res->lr_type == LDLM_IBITS) {
 		ldlm_grant_lock_with_skiplist(lock);
-	else if (res->lr_type == LDLM_EXTENT)
+	} else if (res->lr_type == LDLM_EXTENT) {
 		ldlm_extent_add_lock(res, lock);
-	else if (res->lr_type == LDLM_FLOCK) {
+	} else if (res->lr_type == LDLM_FLOCK) {
 		/*
 		 * We should not add locks to granted list in the following
 		 * cases:
@@ -1046,8 +1046,9 @@ void ldlm_grant_lock(struct ldlm_lock *lock, struct list_head *work_list)
 		    ldlm_is_test_lock(lock) || ldlm_is_flock_deadlock(lock))
 			return;
 		ldlm_resource_add_lock(res, &res->lr_granted, lock);
-	} else
+	} else {
 		LBUG();
+	}
 
 	ldlm_pool_add(&ldlm_res_to_ns(res)->ns_pool, lock);
 }
diff --git a/drivers/staging/lustre/lustre/llite/vvp_dev.c b/drivers/staging/lustre/lustre/llite/vvp_dev.c
index 8e45672..2b60699 100644
--- a/drivers/staging/lustre/lustre/llite/vvp_dev.c
+++ b/drivers/staging/lustre/lustre/llite/vvp_dev.c
@@ -591,9 +591,10 @@ static void *vvp_pgcache_start(struct seq_file *f, loff_t *pos)
 	env = cl_env_get(&refcheck);
 	if (!IS_ERR(env)) {
 		sbi = f->private;
-		if (sbi->ll_site->ls_obj_hash->hs_cur_bits > 64 - PGC_OBJ_SHIFT)
+		if (sbi->ll_site->ls_obj_hash->hs_cur_bits >
+		    64 - PGC_OBJ_SHIFT) {
 			pos = ERR_PTR(-EFBIG);
-		else {
+		} else {
 			*pos = vvp_pgcache_find(env, &sbi->ll_cl->cd_lu_dev,
 						*pos);
 			if (*pos == ~0ULL)
-- 
1.8.3.1

[toc] | [prev] | [next] | [standalone]

#1666134 — Re: [PATCH 6/6] staging: lustre: lustre: fix all braces issues reported by checkpatch

On Wed, 2017-06-14 at 11:01 -0400, James Simmons wrote:
> Cleanup all braces that was reported by checkpatch. The only
> issue not fixed up is in mdc_lock.c. Removing the braces in
> the case of mdc_lock.c will break the build.

[]

> diff --git a/drivers/staging/lustre/lustre/llite/vvp_dev.c b/drivers/staging/lustre/lustre/llite/vvp_dev.c
[]
> @@ -591,9 +591,10 @@ static void *vvp_pgcache_start(struct seq_file *f, loff_t *pos)
>  	env = cl_env_get(&refcheck);
>  	if (!IS_ERR(env)) {
>  		sbi = f->private;
> -		if (sbi->ll_site->ls_obj_hash->hs_cur_bits > 64 - PGC_OBJ_SHIFT)
> +		if (sbi->ll_site->ls_obj_hash->hs_cur_bits >
> +		    64 - PGC_OBJ_SHIFT) {
>  			pos = ERR_PTR(-EFBIG);
> -		else {
> +		} else {
>  			*pos = vvp_pgcache_find(env, &sbi->ll_cl->cd_lu_dev,
>  						*pos);
>  			if (*pos == ~0ULL)

Sometimes is nicer to rearrange the code with smaller
indentation by using early returns and/or goto <labels>.

Something like:

static void *vvp_pgcache_start(struct seq_file *f, loff_t *pos)
{
	struct ll_sb_info *sbi;
	struct lu_env     *env;
	u16 refcheck;

	sbi = f->private;

	env = cl_env_get(&refcheck);
	if (IS_ERR(env))
		return pos;

	sbi = f->private;
	if (sbi->ll_site->ls_obj_hash->hs_cur_bits > 64 - PGC_OBJ_SHIFT) {
		pos = ERR_PTR(-EFBIG);
		goto out;
	}

	*pos = vvp_pgcache_find(env, &sbi->ll_cl->cd_lu_dev, *pos);
	if (*pos == ~0ULL)
		pos = NULL;

out:
	cl_env_put(env, &refcheck);

	return pos;
}

[toc] | [prev] | [next] | [standalone]

#1666866 — Re: [PATCH 6/6] staging: lustre: lustre: fix all braces issues reported by checkpatch

[Multipart message — attachments visible in raw view] — view raw

> On Wed, 2017-06-14 at 11:01 -0400, James Simmons wrote:
> > Cleanup all braces that was reported by checkpatch. The only
> > issue not fixed up is in mdc_lock.c. Removing the braces in
> > the case of mdc_lock.c will break the build.
> 
> []
> 
> > diff --git a/drivers/staging/lustre/lustre/llite/vvp_dev.c b/drivers/staging/lustre/lustre/llite/vvp_dev.c
> []
> > @@ -591,9 +591,10 @@ static void *vvp_pgcache_start(struct seq_file *f, loff_t *pos)
> >  	env = cl_env_get(&refcheck);
> >  	if (!IS_ERR(env)) {
> >  		sbi = f->private;
> > -		if (sbi->ll_site->ls_obj_hash->hs_cur_bits > 64 - PGC_OBJ_SHIFT)
> > +		if (sbi->ll_site->ls_obj_hash->hs_cur_bits >
> > +		    64 - PGC_OBJ_SHIFT) {
> >  			pos = ERR_PTR(-EFBIG);
> > -		else {
> > +		} else {
> >  			*pos = vvp_pgcache_find(env, &sbi->ll_cl->cd_lu_dev,
> >  						*pos);
> >  			if (*pos == ~0ULL)
> 
> Sometimes is nicer to rearrange the code with smaller
> indentation by using early returns and/or goto <labels>.

Do you mind if I submit a separate patch for this? It would be nice to
land the current cleanups as is. Looking at the code I see where more
simplication along this line can be done. I submitted a patch for our
test harness:

https://review.whamcloud.com/#/c/27664

In the near future it will be pushed here.

> Something like:
> 
> static void *vvp_pgcache_start(struct seq_file *f, loff_t *pos)
> {
> 	struct ll_sb_info *sbi;
> 	struct lu_env     *env;
> 	u16 refcheck;
> 
> 	sbi = f->private;
> 
> 	env = cl_env_get(&refcheck);
> 	if (IS_ERR(env))
> 		return pos;
> 
> 	sbi = f->private;
> 	if (sbi->ll_site->ls_obj_hash->hs_cur_bits > 64 - PGC_OBJ_SHIFT) {
> 		pos = ERR_PTR(-EFBIG);
> 		goto out;
> 	}
> 
> 	*pos = vvp_pgcache_find(env, &sbi->ll_cl->cd_lu_dev, *pos);
> 	if (*pos == ~0ULL)
> 		pos = NULL;
> 
> out:
> 	cl_env_put(env, &refcheck);
> 
> 	return pos;
> }
> 
> 

[toc] | [prev] | [next] | [standalone]

#1666900 — Re: [PATCH 6/6] staging: lustre: lustre: fix all braces issues reported by checkpatch

On Thu, 2017-06-15 at 17:38 +0100, James Simmons wrote:
> > On Wed, 2017-06-14 at 11:01 -0400, James Simmons wrote:
> > > Cleanup all braces that was reported by checkpatch. The only
> > > issue not fixed up is in mdc_lock.c. Removing the braces in
> > > the case of mdc_lock.c will break the build.
> > 
> > []
> > 
> > > diff --git a/drivers/staging/lustre/lustre/llite/vvp_dev.c b/drivers/staging/lustre/lustre/llite/vvp_dev.c
> > 
> > []
> > > @@ -591,9 +591,10 @@ static void *vvp_pgcache_start(struct seq_file *f, loff_t *pos)
> > >  	env = cl_env_get(&refcheck);
> > >  	if (!IS_ERR(env)) {
> > >  		sbi = f->private;
> > > -		if (sbi->ll_site->ls_obj_hash->hs_cur_bits > 64 - PGC_OBJ_SHIFT)
> > > +		if (sbi->ll_site->ls_obj_hash->hs_cur_bits >
> > > +		    64 - PGC_OBJ_SHIFT) {
> > >  			pos = ERR_PTR(-EFBIG);
> > > -		else {
> > > +		} else {
> > >  			*pos = vvp_pgcache_find(env, &sbi->ll_cl->cd_lu_dev,
> > >  						*pos);
> > >  			if (*pos == ~0ULL)
> > 
> > Sometimes is nicer to rearrange the code with smaller
> > indentation by using early returns and/or goto <labels>.
> 
> Do you mind if I submit a separate patch for this?
> It would be nice to land the current cleanups as is.

Of course not.

> Looking at the code I see where more
> simplication along this line can be done. I submitted a patch for our
> test harness:

Oh good.

> In the near future it will be pushed here.

Swell.

One of the blocks in vvp_pgcache_show looks like it
has asymmetric braces though.

	 } else

Perhaps you should be using the linux-next version
of checkpatch which warns about this.

And don't take any of what I wrote as a requirement.

All were merely suggestions and could be implemented
an appropriate time or ignored completely.

[toc] | [prev] | [next] | [standalone]

#1666130 — Re: [PATCH 1/6] staging: lustre: lustre: resolve "use spaces between elements" checkpatch errors

On Wed, 2017-06-14 at 11:01 -0400, James Simmons wrote:
> Due to the way the DFID was embedded in our debug strings checkpatch
> would report the following error:

unrelated trivia

> diff --git a/drivers/staging/lustre/lustre/include/lustre/lustre_user.h b/drivers/staging/lustre/lustre/include/lustre/lustre_user.h
[]
> @@ -532,7 +532,7 @@ static inline void obd_uuid2fsname(char *buf, char *uuid, int buflen)
>  #define FID_NOBRACE_LEN 40
>  #define FID_LEN (FID_NOBRACE_LEN + 2)
>  #define DFID_NOBRACE "%#llx:0x%x:0x%x"

It's odd to use a mixture of %#x and 0x%x.

Using
	#define DFID_NOBRACE "%#llx:%#x:%#x"
would also save a couple bytes per use.

Does there need to be a difference between an SFID
and a DFID_NOBRACE?

[toc] | [prev] | [next] | [standalone]

#1666823 — Re: [PATCH 1/6] staging: lustre: lustre: resolve "use spaces between elements" checkpatch errors

> On Wed, 2017-06-14 at 11:01 -0400, James Simmons wrote:
> > Due to the way the DFID was embedded in our debug strings checkpatch
> > would report the following error:
> 
> unrelated trivia
> 
> > diff --git a/drivers/staging/lustre/lustre/include/lustre/lustre_user.h b/drivers/staging/lustre/lustre/include/lustre/lustre_user.h
> []
> > @@ -532,7 +532,7 @@ static inline void obd_uuid2fsname(char *buf, char *uuid, int buflen)
> >  #define FID_NOBRACE_LEN 40
> >  #define FID_LEN (FID_NOBRACE_LEN + 2)
> >  #define DFID_NOBRACE "%#llx:0x%x:0x%x"
> 
> It's odd to use a mixture of %#x and 0x%x.
> 
> Using
> 	#define DFID_NOBRACE "%#llx:%#x:%#x"
> would also save a couple bytes per use.

Changing that format would break things very badly. This is used in user 
land utilities and the kernel code. 
 
> Does there need to be a difference between an SFID
> and a DFID_NOBRACE?

 

[toc] | [prev] | [next] | [standalone]

#1666853 — Re: [PATCH 1/6] staging: lustre: lustre: resolve "use spaces between elements" checkpatch errors

On Thu, 2017-06-15 at 17:03 +0100, James Simmons wrote:
> > On Wed, 2017-06-14 at 11:01 -0400, James Simmons wrote:
> > > Due to the way the DFID was embedded in our debug strings checkpatch
> > > would report the following error:
> > 
> > unrelated trivia
> > 
> > > diff --git a/drivers/staging/lustre/lustre/include/lustre/lustre_user.h b/drivers/staging/lustre/lustre/include/lustre/lustre_user.h
> > 
> > []
> > > @@ -532,7 +532,7 @@ static inline void obd_uuid2fsname(char *buf, char *uuid, int buflen)
> > >  #define FID_NOBRACE_LEN 40
> > >  #define FID_LEN (FID_NOBRACE_LEN + 2)
> > >  #define DFID_NOBRACE "%#llx:0x%x:0x%x"
> > 
> > It's odd to use a mixture of %#x and 0x%x.
> > 
> > Using
> > 	#define DFID_NOBRACE "%#llx:%#x:%#x"
> > would also save a couple bytes per use.
> 
> Changing that format would break things very badly. This is used in user 
> land utilities and the kernel code. 

Really?  Why would anything break?

$ cat fmt.c
#include <stdio.h>
#include <stdlib.h>

int main(int argc, char **argv)
{
	printf("%#llx:0x%x:0x%x\n", (unsigned long long)1, 2, 3);
	printf("%#llx:%#x:%#x\n", (unsigned long long)1, 2, 3);
	return 0;
}

$ gcc fmt.c
$ ./a.out
0x1:0x2:0x3
0x1:0x2:0x3

[toc] | [prev] | [next] | [standalone]

#1666903 — Re: [PATCH 1/6] staging: lustre: lustre: resolve "use spaces between elements" checkpatch errors

> On Thu, 2017-06-15 at 17:03 +0100, James Simmons wrote:
> > > On Wed, 2017-06-14 at 11:01 -0400, James Simmons wrote:
> > > > Due to the way the DFID was embedded in our debug strings checkpatch
> > > > would report the following error:
> > > 
> > > unrelated trivia
> > > 
> > > > diff --git a/drivers/staging/lustre/lustre/include/lustre/lustre_user.h b/drivers/staging/lustre/lustre/include/lustre/lustre_user.h
> > > 
> > > []
> > > > @@ -532,7 +532,7 @@ static inline void obd_uuid2fsname(char *buf, char *uuid, int buflen)
> > > >  #define FID_NOBRACE_LEN 40
> > > >  #define FID_LEN (FID_NOBRACE_LEN + 2)
> > > >  #define DFID_NOBRACE "%#llx:0x%x:0x%x"
> > > 
> > > It's odd to use a mixture of %#x and 0x%x.
> > > 
> > > Using
> > > 	#define DFID_NOBRACE "%#llx:%#x:%#x"
> > > would also save a couple bytes per use.
> > 
> > Changing that format would break things very badly. This is used in user 
> > land utilities and the kernel code. 
> 
> Really?  Why would anything break?

It shouldn't break anything but I'm paranoid. In the past I have change 
"simple" things to have it blow up. I pushed the change to our test 
harness just to make sure. 
 
> $ cat fmt.c
> #include <stdio.h>
> #include <stdlib.h>
> 
> int main(int argc, char **argv)
> {
> 	printf("%#llx:0x%x:0x%x\n", (unsigned long long)1, 2, 3);
> 	printf("%#llx:%#x:%#x\n", (unsigned long long)1, 2, 3);
> 	return 0;
> }
> 
> $ gcc fmt.c
> $ ./a.out
> 0x1:0x2:0x3
> 0x1:0x2:0x3
> 
> 

[toc] | [prev] | [next] | [standalone]

#1666908 — Re: [PATCH 1/6] staging: lustre: lustre: resolve "use spaces between elements" checkpatch errors

On Thu, 2017-06-15 at 17:57 +0100, James Simmons wrote:
> > On Thu, 2017-06-15 at 17:03 +0100, James Simmons wrote:
> > > > On Wed, 2017-06-14 at 11:01 -0400, James Simmons wrote:
> > > > > Due to the way the DFID was embedded in our debug strings checkpatch
> > > > > would report the following error:
> > > > 
> > > > unrelated trivia
> > > > 
> > > > > diff --git a/drivers/staging/lustre/lustre/include/lustre/lustre_user.h b/drivers/staging/lustre/lustre/include/lustre/lustre_user.h
> > > > 
> > > > []
> > > > > @@ -532,7 +532,7 @@ static inline void obd_uuid2fsname(char *buf, char *uuid, int buflen)
> > > > >  #define FID_NOBRACE_LEN 40
> > > > >  #define FID_LEN (FID_NOBRACE_LEN + 2)
> > > > >  #define DFID_NOBRACE "%#llx:0x%x:0x%x"
> > > > 
> > > > It's odd to use a mixture of %#x and 0x%x.
> > > > 
> > > > Using
> > > > 	#define DFID_NOBRACE "%#llx:%#x:%#x"
> > > > would also save a couple bytes per use.
> > > 
> > > Changing that format would break things very badly. This is used in user 
> > > land utilities and the kernel code. 
> > 
> > Really?  Why would anything break?
> 
> It shouldn't break anything but I'm paranoid.

Which is an entirely different thing than
writing "would break things very badly".

Paranoia is fine, incorrect statements of fact
like that less so.

> In the past I have change 
> "simple" things to have it blow up. I pushed the change to our test 
> harness just to make sure. 

Understandable.

cheers, Joe

[toc] | [prev] | [next] | [standalone]

#1666914 — Re: [PATCH 1/6] staging: lustre: lustre: resolve "use spaces between elements" checkpatch errors

> On Thu, 2017-06-15 at 17:57 +0100, James Simmons wrote:
> > > On Thu, 2017-06-15 at 17:03 +0100, James Simmons wrote:
> > > > > On Wed, 2017-06-14 at 11:01 -0400, James Simmons wrote:
> > > > > > Due to the way the DFID was embedded in our debug strings checkpatch
> > > > > > would report the following error:
> > > > > 
> > > > > unrelated trivia
> > > > > 
> > > > > > diff --git a/drivers/staging/lustre/lustre/include/lustre/lustre_user.h b/drivers/staging/lustre/lustre/include/lustre/lustre_user.h
> > > > > 
> > > > > []
> > > > > > @@ -532,7 +532,7 @@ static inline void obd_uuid2fsname(char *buf, char *uuid, int buflen)
> > > > > >  #define FID_NOBRACE_LEN 40
> > > > > >  #define FID_LEN (FID_NOBRACE_LEN + 2)
> > > > > >  #define DFID_NOBRACE "%#llx:0x%x:0x%x"
> > > > > 
> > > > > It's odd to use a mixture of %#x and 0x%x.
> > > > > 
> > > > > Using
> > > > > 	#define DFID_NOBRACE "%#llx:%#x:%#x"
> > > > > would also save a couple bytes per use.
> > > > 
> > > > Changing that format would break things very badly. This is used in user 
> > > > land utilities and the kernel code. 
> > > 
> > > Really?  Why would anything break?
> > 
> > It shouldn't break anything but I'm paranoid.
> 
> Which is an entirely different thing than
> writing "would break things very badly".
> 
> Paranoia is fine, incorrect statements of fact
> like that less so.

Meant no offense. When you said "would also save a couple bytes per use"
I took that as meaning it would change the data format. Should of tried
it myself to see if that was the case. 
 
> > In the past I have change 
> > "simple" things to have it blow up. I pushed the change to our test 
> > harness just to make sure. 
> 
> Understandable.
> 
> cheers, Joe
> 
> 

[toc] | [prev] | [next] | [standalone]

#1666131 — Re: [PATCH 5/6] staging: lustre: lustre: several over 80 characters cleanups

On Wed, 2017-06-14 at 11:01 -0400, James Simmons wrote:
> Cleanup many of the over 80 characters reported by checkpatch

Please don't let checkpatch get in the way of lustre
readability.

lustre commonly uses very long identifiers.
Long identifiers and 80 columns don't mix well.

It might be simpler to declare in some document that
lustre uses lines of up to whatever length and require
that checkpatch should be used with the --max-line-length
option when run on lustre code.

[toc] | [prev] | [next] | [standalone]

#1666828 — Re: [PATCH 5/6] staging: lustre: lustre: several over 80 characters cleanups

> On Wed, 2017-06-14 at 11:01 -0400, James Simmons wrote:
> > Cleanup many of the over 80 characters reported by checkpatch
> 
> Please don't let checkpatch get in the way of lustre
> readability.
> 
> lustre commonly uses very long identifiers.
> Long identifiers and 80 columns don't mix well.
> 
> It might be simpler to declare in some document that
> lustre uses lines of up to whatever length and require
> that checkpatch should be used with the --max-line-length
> option when run on lustre code.

Greg would you be okay with this? If we changed to a max-line-length to 
say 128 thay would mean very few checkpatch issues would remain.

[toc] | [prev] | [next] | [standalone]

#1666839 — Re: [PATCH 5/6] staging: lustre: lustre: several over 80 characters cleanups

On Thu, 2017-06-15 at 17:06 +0100, James Simmons wrote:
> > On Wed, 2017-06-14 at 11:01 -0400, James Simmons wrote:
> > > Cleanup many of the over 80 characters reported by checkpatch
> > 
> > Please don't let checkpatch get in the way of lustre
> > readability.
> > 
> > lustre commonly uses very long identifiers.
> > Long identifiers and 80 columns don't mix well.
> > 
> > It might be simpler to declare in some document that
> > lustre uses lines of up to whatever length and require
> > that checkpatch should be used with the --max-line-length
> > option when run on lustre code.
> 
> Greg would you be okay with this?

I trust Greg isn't a zealot.

Linus Torvalds has said he prefers a longer line length
(up to 100 cols)

https://lkml.org/lkml/2016/12/15/749

> If we changed to a max-line-length to 
> say 128 thay would mean very few checkpatch issues would remain.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web