[PATCH 0/4] staging: lustre: fixed some signedness warns from sparse

[PATCH 0/4] staging: lustre: fixed some signedness warns from sparse

[Stefano Manni]

Fixed some signedness warnings from sparse on lustre.

Stefano Manni (4):
  staging: lustre: fixed signedness of some socklnd params
  staging: lustre: fixed signedness of llite
  staging: lustre: fixed signedness of lov
  staging: lustre: fixed signedness of obdclass

 drivers/staging/lustre/lnet/klnds/socklnd/socklnd.h        |  4 ++--
 .../staging/lustre/lnet/klnds/socklnd/socklnd_modparams.c  |  2 +-
 drivers/staging/lustre/lustre/llite/dir.c                  |  3 ++-
 drivers/staging/lustre/lustre/llite/llite_lib.c            |  9 ++++++---
 drivers/staging/lustre/lustre/llite/lproc_llite.c          | 14 ++++++--------
 drivers/staging/lustre/lustre/lov/lov_obd.c                |  2 +-
 drivers/staging/lustre/lustre/lov/lov_offset.c             | 11 +++++++----
 drivers/staging/lustre/lustre/obdclass/obd_mount.c         |  2 +-
 8 files changed, 26 insertions(+), 21 deletions(-)

-- 
2.5.5

[PATCH 1/4] staging: lustre: fixed signedness of some socklnd params

[Stefano Manni]

sparse warnings:
warning: incorrect type in assignment (different signedness)
expected int *[addressable] [toplevel] [assigned] ksnd_nscheds
got unsigned int static [toplevel] *<noident>
warning: incorrect type in assignment (different signedness)
expected int *[addressable] [toplevel] [assigned] ksnd_zc_recv
got unsigned int static [toplevel] *<noident>
warning: incorrect type in assignment (different signedness)
expected int *[addressable] [toplevel] [assigned] ksnd_zc_recv_min_nfrags
got unsigned int static [toplevel] *<noident>

Signed-off-by: Stefano Manni <stefano.manni@gmail.com>
---
 drivers/staging/lustre/lnet/klnds/socklnd/socklnd.h           | 4 ++--
 drivers/staging/lustre/lnet/klnds/socklnd/socklnd_modparams.c | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/lustre/lnet/klnds/socklnd/socklnd.h b/drivers/staging/lustre/lnet/klnds/socklnd/socklnd.h
index d50ebdf..474f93a 100644
--- a/drivers/staging/lustre/lnet/klnds/socklnd/socklnd.h
+++ b/drivers/staging/lustre/lnet/klnds/socklnd/socklnd.h
@@ -158,10 +158,10 @@ struct ksock_tunables {
 	unsigned int *ksnd_zc_min_payload;     /* minimum zero copy payload
 						* size
 						*/
-	int          *ksnd_zc_recv;            /* enable ZC receive (for
+	unsigned int *ksnd_zc_recv;            /* enable ZC receive (for
 						* Chelsio TOE)
 						*/
-	int          *ksnd_zc_recv_min_nfrags; /* minimum # of fragments to
+	unsigned int *ksnd_zc_recv_min_nfrags; /* minimum # of fragments to
 						* enable ZC receive
 						*/
 };
diff --git a/drivers/staging/lustre/lnet/klnds/socklnd/socklnd_modparams.c b/drivers/staging/lustre/lnet/klnds/socklnd/socklnd_modparams.c
index 5663a4c..2ad89ca 100644
--- a/drivers/staging/lustre/lnet/klnds/socklnd/socklnd_modparams.c
+++ b/drivers/staging/lustre/lnet/klnds/socklnd/socklnd_modparams.c
@@ -43,7 +43,7 @@ MODULE_PARM_DESC(peer_timeout, "Seconds without aliveness news to declare peer d
  * Number of daemons in each thread pool which is percpt,
  * we will estimate reasonable value based on CPUs if it's not set.
  */
-static unsigned int nscheds;
+static int nscheds;
 module_param(nscheds, int, 0444);
 MODULE_PARM_DESC(nscheds, "# scheduler daemons in each pool while starting");
 
-- 
2.5.5

[PATCH 2/4] staging: lustre: fixed signedness of llite

[Stefano Manni]

sparse warnings on dir.c:
warning: incorrect type in argument 5 (different signedness)
expected unsigned int [usertype] *vallen
got int *<noident>

sparse warnings on llite_lib.c:
warning: incorrect type in argument 5 (different signedness)
expected unsigned int [usertype] *vallen
got int *<noident>

sparse warnings on lproc_llite.c:
warning: incorrect type in argument 2 (different signedness)
expected int *max_mdsize
got unsigned int *<noident>
warning: incorrect type in argument 2 (different signedness)
expected int *default_mdsize
got unsigned int *<noident>
warning: incorrect type in initializer (different signedness)
expected int *off_count
got unsigned int *<noident>
warning: incorrect type in initializer (different signedness)
expected int *process_count
got unsigned int *<noident>

Signed-off-by: Stefano Manni <stefano.manni@gmail.com>
---
 drivers/staging/lustre/lustre/llite/dir.c         |  3 ++-
 drivers/staging/lustre/lustre/llite/llite_lib.c   |  9 ++++++---
 drivers/staging/lustre/lustre/llite/lproc_llite.c | 14 ++++++--------
 3 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/drivers/staging/lustre/lustre/llite/dir.c b/drivers/staging/lustre/lustre/llite/dir.c
index 5b2e47c..ee6e8ef 100644
--- a/drivers/staging/lustre/lustre/llite/dir.c
+++ b/drivers/staging/lustre/lustre/llite/dir.c
@@ -1422,7 +1422,8 @@ static long ll_dir_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	case LL_IOC_FLUSHCTX:
 		return ll_flush_ctx(inode);
 	case LL_IOC_GETOBDCOUNT: {
-		int count, vallen;
+		int count;
+		unsigned int vallen;
 		struct obd_export *exp;
 
 		if (copy_from_user(&count, (int __user *)arg, sizeof(int)))
diff --git a/drivers/staging/lustre/lustre/llite/llite_lib.c b/drivers/staging/lustre/lustre/llite/llite_lib.c
index 8666f1e..6f82bff 100644
--- a/drivers/staging/lustre/lustre/llite/llite_lib.c
+++ b/drivers/staging/lustre/lustre/llite/llite_lib.c
@@ -159,7 +159,8 @@ static int client_common_fill_super(struct super_block *sb, char *md, char *dt,
 	struct md_op_data *op_data;
 	struct lustre_md lmd;
 	u64 valid;
-	int size, err, checksum;
+	int err, checksum;
+	unsigned int size;
 
 	obd = class_name2obd(md);
 	if (!obd) {
@@ -571,7 +572,8 @@ static int client_common_fill_super(struct super_block *sb, char *md, char *dt,
 
 int ll_get_max_mdsize(struct ll_sb_info *sbi, int *lmmsize)
 {
-	int size, rc;
+	int rc;
+	unsigned int size;
 
 	size = sizeof(*lmmsize);
 	rc = obd_get_info(NULL, sbi->ll_dt_exp, sizeof(KEY_MAX_EASIZE),
@@ -604,7 +606,8 @@ int ll_get_max_mdsize(struct ll_sb_info *sbi, int *lmmsize)
  */
 int ll_get_default_mdsize(struct ll_sb_info *sbi, int *lmmsize)
 {
-	int size, rc;
+	int rc;
+	unsigned int size;
 
 	size = sizeof(int);
 	rc = obd_get_info(NULL, sbi->ll_md_exp, sizeof(KEY_DEFAULT_EASIZE),
diff --git a/drivers/staging/lustre/lustre/llite/lproc_llite.c b/drivers/staging/lustre/lustre/llite/lproc_llite.c
index 644bea2..52dbd56 100644
--- a/drivers/staging/lustre/lustre/llite/lproc_llite.c
+++ b/drivers/staging/lustre/lustre/llite/lproc_llite.c
@@ -734,14 +734,13 @@ static ssize_t max_easize_show(struct kobject *kobj,
 {
 	struct ll_sb_info *sbi = container_of(kobj, struct ll_sb_info,
 					      ll_kobj);
-	unsigned int ealen;
-	int rc;
+	int rc, ealen;
 
 	rc = ll_get_max_mdsize(sbi, &ealen);
 	if (rc)
 		return rc;
 
-	return sprintf(buf, "%u\n", ealen);
+	return sprintf(buf, "%d\n", ealen);
 }
 LUSTRE_RO_ATTR(max_easize);
 
@@ -763,14 +762,13 @@ static ssize_t default_easize_show(struct kobject *kobj,
 {
 	struct ll_sb_info *sbi = container_of(kobj, struct ll_sb_info,
 					      ll_kobj);
-	unsigned int ealen;
-	int rc;
+	int rc, ealen;
 
 	rc = ll_get_default_mdsize(sbi, &ealen);
 	if (rc)
 		return rc;
 
-	return sprintf(buf, "%u\n", ealen);
+	return sprintf(buf, "%d\n", ealen);
 }
 
 /**
@@ -1479,8 +1477,8 @@ void ll_rw_stats_tally(struct ll_sb_info *sbi, pid_t pid,
 	int i, cur = -1;
 	struct ll_rw_process_info *process;
 	struct ll_rw_process_info *offset;
-	int *off_count = &sbi->ll_rw_offset_entry_count;
-	int *process_count = &sbi->ll_offset_process_count;
+	unsigned int *off_count = &sbi->ll_rw_offset_entry_count;
+	unsigned int *process_count = &sbi->ll_offset_process_count;
 	struct ll_rw_extents_info *io_extents = &sbi->ll_rw_extents_info;
 
 	if (!sbi->ll_rw_stats_on)
-- 
2.5.5

[PATCH 3/4] staging: lustre: fixed signedness of lov

[Stefano Manni]

sparse warning on lov_obd.c:
warning: incorrect type in argument 3 (different signedness)
expected int *res
got unsigned int [usertype] *i

sparse warning on lov_offset.c:
warning: incorrect type in argument 4 (different signedness)
expected long long [usertype] *<noident>
got unsigned long long *<noident>

Signed-off-by: Stefano Manni <stefano.manni@gmail.com>
---
 drivers/staging/lustre/lustre/lov/lov_obd.c    |  2 +-
 drivers/staging/lustre/lustre/lov/lov_offset.c | 11 +++++++----
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/lustre/lustre/lov/lov_obd.c b/drivers/staging/lustre/lustre/lov/lov_obd.c
index 7ce0102..3cccd79 100644
--- a/drivers/staging/lustre/lustre/lov/lov_obd.c
+++ b/drivers/staging/lustre/lustre/lov/lov_obd.c
@@ -899,7 +899,7 @@ int lov_process_config_base(struct obd_device *obd, struct lustre_cfg *lcfg,
 
 		obd_str2uuid(&obd_uuid,  lustre_cfg_buf(lcfg, 1));
 
-		rc = kstrtoint(lustre_cfg_buf(lcfg, 2), 10, indexp);
+		rc = kstrtouint(lustre_cfg_buf(lcfg, 2), 10, indexp);
 		if (rc < 0)
 			goto out;
 		rc = kstrtoint(lustre_cfg_buf(lcfg, 3), 10, genp);
diff --git a/drivers/staging/lustre/lustre/lov/lov_offset.c b/drivers/staging/lustre/lustre/lov/lov_offset.c
index 3e16e64..954a01b 100644
--- a/drivers/staging/lustre/lustre/lov/lov_offset.c
+++ b/drivers/staging/lustre/lustre/lov/lov_offset.c
@@ -44,7 +44,7 @@ u64 lov_stripe_size(struct lov_stripe_md *lsm, u64 ost_size, int stripeno)
 {
 	unsigned long ssize = lsm->lsm_stripe_size;
 	unsigned long stripe_size;
-	u64 swidth;
+	long long swidth;
 	u64 lov_size;
 	int magic = lsm->lsm_magic;
 
@@ -128,7 +128,8 @@ int lov_stripe_offset(struct lov_stripe_md *lsm, u64 lov_off,
 		      int stripeno, u64 *obdoff)
 {
 	unsigned long ssize  = lsm->lsm_stripe_size;
-	u64 stripe_off, this_stripe, swidth;
+	long long swidth;
+	u64 stripe_off, this_stripe;
 	int magic = lsm->lsm_magic;
 	int ret = 0;
 
@@ -183,7 +184,8 @@ u64 lov_size_to_stripe(struct lov_stripe_md *lsm, u64 file_size,
 		       int stripeno)
 {
 	unsigned long ssize  = lsm->lsm_stripe_size;
-	u64 stripe_off, this_stripe, swidth;
+	long long swidth;
+	u64 stripe_off, this_stripe;
 	int magic = lsm->lsm_magic;
 
 	if (file_size == OBD_OBJECT_EOF)
@@ -257,7 +259,8 @@ int lov_stripe_intersects(struct lov_stripe_md *lsm, int stripeno,
 int lov_stripe_number(struct lov_stripe_md *lsm, u64 lov_off)
 {
 	unsigned long ssize  = lsm->lsm_stripe_size;
-	u64 stripe_off, swidth;
+	long long swidth;
+	u64 stripe_off;
 	int magic = lsm->lsm_magic;
 
 	lsm_op_find(magic)->lsm_stripe_by_offset(lsm, NULL, &lov_off, &swidth);
-- 
2.5.5

[PATCH 4/4] staging: lustre: fixed signedness of obdclass

[Stefano Manni]

sparse warning on obd_mount.c:
warning: incorrect type in argument 5 (different signedness)
expected unsigned int [usertype] *vallen
got int *<noident>

Signed-off-by: Stefano Manni <stefano.manni@gmail.com>
---
 drivers/staging/lustre/lustre/obdclass/obd_mount.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/lustre/lustre/obdclass/obd_mount.c b/drivers/staging/lustre/lustre/obdclass/obd_mount.c
index 2a79a22..b0af897 100644
--- a/drivers/staging/lustre/lustre/obdclass/obd_mount.c
+++ b/drivers/staging/lustre/lustre/obdclass/obd_mount.c
@@ -273,7 +273,7 @@ int lustre_start_mgc(struct super_block *sb)
 		/* IR compatibility check, only for clients */
 		if (lmd_is_client(lsi->lsi_lmd)) {
 			int has_ir;
-			int vallen = sizeof(*data);
+			unsigned int vallen = sizeof(*data);
 			__u32 *flags = &lsi->lsi_lmd->lmd_flags;
 
 			rc = obd_get_info(NULL, obd->obd_self_export,
-- 
2.5.5

Re: [PATCH 0/4] staging: lustre: fixed some signedness warns from sparse

[Tobin C. Harding]

On Wed, Nov 22, 2017 at 08:38:27PM +0100, Stefano Manni wrote:
> Fixed some signedness warnings from sparse on lustre.
> 
> Stefano Manni (4):
>   staging: lustre: fixed signedness of some socklnd params
>   staging: lustre: fixed signedness of llite
>   staging: lustre: fixed signedness of lov
>   staging: lustre: fixed signedness of obdclass

You may like to use imperative mood for your git log brief descriptions
Stefano.  

s/fixed/fix/

For justification see Documentation/process/submitting-patches.rst.

Specifically section 2 of that document.

Hope this helps,
Tobin.


>  drivers/staging/lustre/lnet/klnds/socklnd/socklnd.h        |  4 ++--
>  .../staging/lustre/lnet/klnds/socklnd/socklnd_modparams.c  |  2 +-
>  drivers/staging/lustre/lustre/llite/dir.c                  |  3 ++-
>  drivers/staging/lustre/lustre/llite/llite_lib.c            |  9 ++++++---
>  drivers/staging/lustre/lustre/llite/lproc_llite.c          | 14 ++++++--------
>  drivers/staging/lustre/lustre/lov/lov_obd.c                |  2 +-
>  drivers/staging/lustre/lustre/lov/lov_offset.c             | 11 +++++++----
>  drivers/staging/lustre/lustre/obdclass/obd_mount.c         |  2 +-
>  8 files changed, 26 insertions(+), 21 deletions(-)
> 
> -- 
> 2.5.5
> 
> _______________________________________________
> devel mailing list
> devel@linuxdriverproject.org
> http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[lustre-devel] [PATCH 0/4] staging: lustre: fixed some signedness warns from sparse

[Tobin C. Harding]

On Wed, Nov 22, 2017 at 08:38:27PM +0100, Stefano Manni wrote:
> Fixed some signedness warnings from sparse on lustre.
> 
> Stefano Manni (4):
>   staging: lustre: fixed signedness of some socklnd params
>   staging: lustre: fixed signedness of llite
>   staging: lustre: fixed signedness of lov
>   staging: lustre: fixed signedness of obdclass

You may like to use imperative mood for your git log brief descriptions
Stefano.  

s/fixed/fix/

For justification see Documentation/process/submitting-patches.rst.

Specifically section 2 of that document.

Hope this helps,
Tobin.


>  drivers/staging/lustre/lnet/klnds/socklnd/socklnd.h        |  4 ++--
>  .../staging/lustre/lnet/klnds/socklnd/socklnd_modparams.c  |  2 +-
>  drivers/staging/lustre/lustre/llite/dir.c                  |  3 ++-
>  drivers/staging/lustre/lustre/llite/llite_lib.c            |  9 ++++++---
>  drivers/staging/lustre/lustre/llite/lproc_llite.c          | 14 ++++++--------
>  drivers/staging/lustre/lustre/lov/lov_obd.c                |  2 +-
>  drivers/staging/lustre/lustre/lov/lov_offset.c             | 11 +++++++----
>  drivers/staging/lustre/lustre/obdclass/obd_mount.c         |  2 +-
>  8 files changed, 26 insertions(+), 21 deletions(-)
> 
> -- 
> 2.5.5
> 
> _______________________________________________
> devel mailing list
> devel at linuxdriverproject.org
> http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 0/4] staging: lustre: fixed some signedness warns from sparse

[Dan Carpenter]

On Thu, Nov 23, 2017 at 03:59:26PM +1100, Tobin C. Harding wrote:
> On Wed, Nov 22, 2017 at 08:38:27PM +0100, Stefano Manni wrote:
> > Fixed some signedness warnings from sparse on lustre.
> > 
> > Stefano Manni (4):
> >   staging: lustre: fixed signedness of some socklnd params
> >   staging: lustre: fixed signedness of llite
> >   staging: lustre: fixed signedness of lov
> >   staging: lustre: fixed signedness of obdclass
> 
> You may like to use imperative mood for your git log brief descriptions
> Stefano.  
> 
> s/fixed/fix/
> 

Someone once chewed me a second butt hole for not using the imperative
mood so I know some people care intensely about this but I think so long
as you can understand the description it's fine.  I will never send a
patch for that maintainer's subsystem again, btw, which is probably
grateful for and now I can poop twice as fast so we're both winners.

Especially in the 0/4 patch which is going to be discarded.  Who cares?

regards,
dan carpenter

[lustre-devel] [PATCH 0/4] staging: lustre: fixed some signedness warns from sparse

[Dan Carpenter]

On Thu, Nov 23, 2017 at 03:59:26PM +1100, Tobin C. Harding wrote:
> On Wed, Nov 22, 2017 at 08:38:27PM +0100, Stefano Manni wrote:
> > Fixed some signedness warnings from sparse on lustre.
> > 
> > Stefano Manni (4):
> >   staging: lustre: fixed signedness of some socklnd params
> >   staging: lustre: fixed signedness of llite
> >   staging: lustre: fixed signedness of lov
> >   staging: lustre: fixed signedness of obdclass
> 
> You may like to use imperative mood for your git log brief descriptions
> Stefano.  
> 
> s/fixed/fix/
> 

Someone once chewed me a second butt hole for not using the imperative
mood so I know some people care intensely about this but I think so long
as you can understand the description it's fine.  I will never send a
patch for that maintainer's subsystem again, btw, which is probably
grateful for and now I can poop twice as fast so we're both winners.

Especially in the 0/4 patch which is going to be discarded.  Who cares?

regards,
dan carpenter

Re: [PATCH 1/4] staging: lustre: fixed signedness of some socklnd params

[Dan Carpenter]

On Wed, Nov 22, 2017 at 08:38:28PM +0100, Stefano Manni wrote:
> diff --git a/drivers/staging/lustre/lnet/klnds/socklnd/socklnd_modparams.c b/drivers/staging/lustre/lnet/klnds/socklnd/socklnd_modparams.c
> index 5663a4c..2ad89ca 100644
> --- a/drivers/staging/lustre/lnet/klnds/socklnd/socklnd_modparams.c
> +++ b/drivers/staging/lustre/lnet/klnds/socklnd/socklnd_modparams.c
> @@ -43,7 +43,7 @@ MODULE_PARM_DESC(peer_timeout, "Seconds without aliveness news to declare peer d
>   * Number of daemons in each thread pool which is percpt,
>   * we will estimate reasonable value based on CPUs if it's not set.
>   */
> -static unsigned int nscheds;
> +static int nscheds;

I've looked through this series and I feel like none of these are real
bugs.  It's just about type safety and being consistent.  Which are good
things.  I'm not sure that I like the parts where we make the variables
signed.

Here "nscheds" is the number of threads.  How can we have a negative
number?  I think it should be unsigned.  It's way more tricky to change
the rest of the code, and leave nscheds unsigned int but I think it's
probably the right thing.

regards,
dan carpenter

[lustre-devel] [PATCH 1/4] staging: lustre: fixed signedness of some socklnd params

[Dan Carpenter]

On Wed, Nov 22, 2017 at 08:38:28PM +0100, Stefano Manni wrote:
> diff --git a/drivers/staging/lustre/lnet/klnds/socklnd/socklnd_modparams.c b/drivers/staging/lustre/lnet/klnds/socklnd/socklnd_modparams.c
> index 5663a4c..2ad89ca 100644
> --- a/drivers/staging/lustre/lnet/klnds/socklnd/socklnd_modparams.c
> +++ b/drivers/staging/lustre/lnet/klnds/socklnd/socklnd_modparams.c
> @@ -43,7 +43,7 @@ MODULE_PARM_DESC(peer_timeout, "Seconds without aliveness news to declare peer d
>   * Number of daemons in each thread pool which is percpt,
>   * we will estimate reasonable value based on CPUs if it's not set.
>   */
> -static unsigned int nscheds;
> +static int nscheds;

I've looked through this series and I feel like none of these are real
bugs.  It's just about type safety and being consistent.  Which are good
things.  I'm not sure that I like the parts where we make the variables
signed.

Here "nscheds" is the number of threads.  How can we have a negative
number?  I think it should be unsigned.  It's way more tricky to change
the rest of the code, and leave nscheds unsigned int but I think it's
probably the right thing.

regards,
dan carpenter

Re: [PATCH 0/4] staging: lustre: fixed some signedness warns from sparse

[Tobin C. Harding]

On Thu, Nov 23, 2017 at 02:51:38PM +0300, Dan Carpenter wrote:
> On Thu, Nov 23, 2017 at 03:59:26PM +1100, Tobin C. Harding wrote:
> > On Wed, Nov 22, 2017 at 08:38:27PM +0100, Stefano Manni wrote:
> > > Fixed some signedness warnings from sparse on lustre.
> > > 
> > > Stefano Manni (4):
> > >   staging: lustre: fixed signedness of some socklnd params
> > >   staging: lustre: fixed signedness of llite
> > >   staging: lustre: fixed signedness of lov
> > >   staging: lustre: fixed signedness of obdclass
> > 
> > You may like to use imperative mood for your git log brief descriptions
> > Stefano.  
> > 
> > s/fixed/fix/
> > 
> 
> Someone once chewed me a second butt hole for not using the imperative
> mood so I know some people care intensely about this but I think so long
> as you can understand the description it's fine.  I will never send a
> patch for that maintainer's subsystem again, btw, which is probably
> grateful for and now I can poop twice as fast so we're both winners.

I try to only make these suggestions to people doing clean up patches to
staging, with the reasoning that if we are learning we might as well
learn the correct method from the start.

I try to be polite and helpful, I am not very long past learning these
things myself. No one likes being told they are wrong, better to learn
how to take it while in staging, that's what it's there for right.

> Especially in the 0/4 patch which is going to be discarded.  Who cares?

Definitely agree for the 0/4 patch, doesn't _need_ imperative mood.

For the record, I love your short snappy reviews Dan. My current
favourite review of all time was done by you on a what was at the time a
pretty hard patch for me. It was

	Nope

I burst out laughing when I got that one. It spurred me on to greater
things (though I'm not sure everyone would be equally encouraged :)

All the best,
Tobin.

[lustre-devel] [PATCH 0/4] staging: lustre: fixed some signedness warns from sparse

[Tobin C. Harding]

On Thu, Nov 23, 2017 at 02:51:38PM +0300, Dan Carpenter wrote:
> On Thu, Nov 23, 2017 at 03:59:26PM +1100, Tobin C. Harding wrote:
> > On Wed, Nov 22, 2017 at 08:38:27PM +0100, Stefano Manni wrote:
> > > Fixed some signedness warnings from sparse on lustre.
> > > 
> > > Stefano Manni (4):
> > >   staging: lustre: fixed signedness of some socklnd params
> > >   staging: lustre: fixed signedness of llite
> > >   staging: lustre: fixed signedness of lov
> > >   staging: lustre: fixed signedness of obdclass
> > 
> > You may like to use imperative mood for your git log brief descriptions
> > Stefano.  
> > 
> > s/fixed/fix/
> > 
> 
> Someone once chewed me a second butt hole for not using the imperative
> mood so I know some people care intensely about this but I think so long
> as you can understand the description it's fine.  I will never send a
> patch for that maintainer's subsystem again, btw, which is probably
> grateful for and now I can poop twice as fast so we're both winners.

I try to only make these suggestions to people doing clean up patches to
staging, with the reasoning that if we are learning we might as well
learn the correct method from the start.

I try to be polite and helpful, I am not very long past learning these
things myself. No one likes being told they are wrong, better to learn
how to take it while in staging, that's what it's there for right.

> Especially in the 0/4 patch which is going to be discarded.  Who cares?

Definitely agree for the 0/4 patch, doesn't _need_ imperative mood.

For the record, I love your short snappy reviews Dan. My current
favourite review of all time was done by you on a what was at the time a
pretty hard patch for me. It was

	Nope

I burst out laughing when I got that one. It spurred me on to greater
things (though I'm not sure everyone would be equally encouraged :)

All the best,
Tobin.

Re: [PATCH 0/4] staging: lustre: fixed some signedness warns from sparse

[Stefano Manni]

On Fri, 2017-11-24 at 09:20 +1100, Tobin C. Harding wrote:
> On Thu, Nov 23, 2017 at 02:51:38PM +0300, Dan Carpenter wrote:
> > On Thu, Nov 23, 2017 at 03:59:26PM +1100, Tobin C. Harding wrote:
> > > On Wed, Nov 22, 2017 at 08:38:27PM +0100, Stefano Manni wrote:
> > > > Fixed some signedness warnings from sparse on lustre.
> > > > 
> > > > Stefano Manni (4):
> > > >   staging: lustre: fixed signedness of some socklnd params
> > > >   staging: lustre: fixed signedness of llite
> > > >   staging: lustre: fixed signedness of lov
> > > >   staging: lustre: fixed signedness of obdclass
> > > 
> > > You may like to use imperative mood for your git log brief
> > > descriptions
> > > Stefano.  
> > > 
> > > s/fixed/fix/
> > > 
> > 
> > Someone once chewed me a second butt hole for not using the
> > imperative
> > mood so I know some people care intensely about this but I think so
> > long
> > as you can understand the description it's fine.  I will never send
> > a
> > patch for that maintainer's subsystem again, btw, which is probably
> > grateful for and now I can poop twice as fast so we're both
> > winners.
> 
> I try to only make these suggestions to people doing clean up patches
> to
> staging, with the reasoning that if we are learning we might as well
> learn the correct method from the start.

It's ok with your suggestion. Not a hard work to rebase 4 commit and
change the commit message.

Thanks,
Stefano

Re: [PATCH 1/4] staging: lustre: fixed signedness of some socklnd params

[Stefano Manni]

On Thu, 2017-11-23 at 15:13 +0300, Dan Carpenter wrote:
> I've looked through this series and I feel like none of these are
> real
> bugs.  It's just about type safety and being consistent.  Which are
> good
> things.  I'm not sure that I like the parts where we make the
> variables
> signed.
> 
> Here "nscheds" is the number of threads.  How can we have a negative
> number?  I think it should be unsigned.  It's way more tricky to
> change
> the rest of the code, and leave nscheds unsigned int but I think it's
> probably the right thing.
> 
> regards,
> dan carpenter
> 

I've made the module param nsched signed because the
ksock_tunables.ksnd_nscheds (the real container) is signed too.

I definitely agree with you that it does not make sense to have a
negative number of threads.
In my opinion it's better to fix this inconsistency between the param
and the container and then try submit another patch to harmonize
signedness around the code.

Thanks,
Stefano

Re: [PATCH 1/4] staging: lustre: fixed signedness of some socklnd params

[Dan Carpenter]

On Fri, Nov 24, 2017 at 12:47:21AM +0100, Stefano Manni wrote:
> I've made the module param nsched signed because the
> ksock_tunables.ksnd_nscheds (the real container) is signed too.

Yeah.  I know.  And it's way more involved and controversial to change
ksock_tunables.

> 
> I definitely agree with you that it does not make sense to have a
> negative number of threads.
> In my opinion it's better to fix this inconsistency between the param
> and the container and then try submit another patch to harmonize
> signedness around the code.

No one will ever think about types again if Sparse stops complaining.
It's better to keep the error message instead of silencing it in a less
than ideal way because it means we have to think about types.
Eventually someone will get sick of the warning and write a huger patch
that rewrites a lot of code.

regards,
dan carpenter

[lustre-devel] [PATCH 1/4] staging: lustre: fixed signedness of some socklnd params

[Dan Carpenter]

On Fri, Nov 24, 2017 at 12:47:21AM +0100, Stefano Manni wrote:
> I've made the module param nsched signed because the
> ksock_tunables.ksnd_nscheds (the real container) is signed too.

Yeah.  I know.  And it's way more involved and controversial to change
ksock_tunables.

> 
> I definitely agree with you that it does not make sense to have a
> negative number of threads.
> In my opinion it's better to fix this inconsistency between the param
> and the container and then try submit another patch to harmonize
> signedness around the code.

No one will ever think about types again if Sparse stops complaining.
It's better to keep the error message instead of silencing it in a less
than ideal way because it means we have to think about types.
Eventually someone will get sick of the warning and write a huger patch
that rewrites a lot of code.

regards,
dan carpenter

Re: [PATCH 0/4] staging: lustre: fixed some signedness warns from sparse

[Dan Carpenter]

On Fri, Nov 24, 2017 at 09:20:20AM +1100, Tobin C. Harding wrote:
> My current favourite review of all time was done by you on a what was
> at the time a pretty hard patch for me. It was
> 
> 	Nope
> 

Wow...  I know I've said that to other people but I can't believe I sent
that to *you*...  Sorry.  You write good patches.

regards,
dan carpenter

[lustre-devel] [PATCH 0/4] staging: lustre: fixed some signedness warns from sparse

[Dan Carpenter]

On Fri, Nov 24, 2017 at 09:20:20AM +1100, Tobin C. Harding wrote:
> My current favourite review of all time was done by you on a what was
> at the time a pretty hard patch for me. It was
> 
> 	Nope
> 

Wow...  I know I've said that to other people but I can't believe I sent
that to *you*...  Sorry.  You write good patches.

regards,
dan carpenter

Re: [PATCH 3/4] staging: lustre: fixed signedness of lov

[Greg Kroah-Hartman]

On Wed, Nov 22, 2017 at 08:38:30PM +0100, Stefano Manni wrote:
> sparse warning on lov_obd.c:
> warning: incorrect type in argument 3 (different signedness)
> expected int *res
> got unsigned int [usertype] *i
> 
> sparse warning on lov_offset.c:
> warning: incorrect type in argument 4 (different signedness)
> expected long long [usertype] *<noident>
> got unsigned long long *<noident>

Can you not line-wrap these sparse warning lines, it makes them hard to
read.  It's ok to leave them as very long lines in the changelog.

> 
> Signed-off-by: Stefano Manni <stefano.manni@gmail.com>
> ---
>  drivers/staging/lustre/lustre/lov/lov_obd.c    |  2 +-
>  drivers/staging/lustre/lustre/lov/lov_offset.c | 11 +++++++----
>  2 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/lustre/lustre/lov/lov_obd.c b/drivers/staging/lustre/lustre/lov/lov_obd.c
> index 7ce0102..3cccd79 100644
> --- a/drivers/staging/lustre/lustre/lov/lov_obd.c
> +++ b/drivers/staging/lustre/lustre/lov/lov_obd.c
> @@ -899,7 +899,7 @@ int lov_process_config_base(struct obd_device *obd, struct lustre_cfg *lcfg,
>  
>  		obd_str2uuid(&obd_uuid,  lustre_cfg_buf(lcfg, 1));
>  
> -		rc = kstrtoint(lustre_cfg_buf(lcfg, 2), 10, indexp);
> +		rc = kstrtouint(lustre_cfg_buf(lcfg, 2), 10, indexp);
>  		if (rc < 0)
>  			goto out;
>  		rc = kstrtoint(lustre_cfg_buf(lcfg, 3), 10, genp);
> diff --git a/drivers/staging/lustre/lustre/lov/lov_offset.c b/drivers/staging/lustre/lustre/lov/lov_offset.c
> index 3e16e64..954a01b 100644
> --- a/drivers/staging/lustre/lustre/lov/lov_offset.c
> +++ b/drivers/staging/lustre/lustre/lov/lov_offset.c
> @@ -44,7 +44,7 @@ u64 lov_stripe_size(struct lov_stripe_md *lsm, u64 ost_size, int stripeno)
>  {
>  	unsigned long ssize = lsm->lsm_stripe_size;
>  	unsigned long stripe_size;
> -	u64 swidth;
> +	long long swidth;

This feels wrong, u64 should be used instead, can the code be fixed up
to use that type properly?  Don't rely on long long anywhere, it's hard
to always know what it should be on all arches...

Please fix up and resend this series based on the reviews you've gotten
so far.

thanks,

greg k-h

[lustre-devel] [PATCH 3/4] staging: lustre: fixed signedness of lov

[Greg Kroah-Hartman]

On Wed, Nov 22, 2017 at 08:38:30PM +0100, Stefano Manni wrote:
> sparse warning on lov_obd.c:
> warning: incorrect type in argument 3 (different signedness)
> expected int *res
> got unsigned int [usertype] *i
> 
> sparse warning on lov_offset.c:
> warning: incorrect type in argument 4 (different signedness)
> expected long long [usertype] *<noident>
> got unsigned long long *<noident>

Can you not line-wrap these sparse warning lines, it makes them hard to
read.  It's ok to leave them as very long lines in the changelog.

> 
> Signed-off-by: Stefano Manni <stefano.manni@gmail.com>
> ---
>  drivers/staging/lustre/lustre/lov/lov_obd.c    |  2 +-
>  drivers/staging/lustre/lustre/lov/lov_offset.c | 11 +++++++----
>  2 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/lustre/lustre/lov/lov_obd.c b/drivers/staging/lustre/lustre/lov/lov_obd.c
> index 7ce0102..3cccd79 100644
> --- a/drivers/staging/lustre/lustre/lov/lov_obd.c
> +++ b/drivers/staging/lustre/lustre/lov/lov_obd.c
> @@ -899,7 +899,7 @@ int lov_process_config_base(struct obd_device *obd, struct lustre_cfg *lcfg,
>  
>  		obd_str2uuid(&obd_uuid,  lustre_cfg_buf(lcfg, 1));
>  
> -		rc = kstrtoint(lustre_cfg_buf(lcfg, 2), 10, indexp);
> +		rc = kstrtouint(lustre_cfg_buf(lcfg, 2), 10, indexp);
>  		if (rc < 0)
>  			goto out;
>  		rc = kstrtoint(lustre_cfg_buf(lcfg, 3), 10, genp);
> diff --git a/drivers/staging/lustre/lustre/lov/lov_offset.c b/drivers/staging/lustre/lustre/lov/lov_offset.c
> index 3e16e64..954a01b 100644
> --- a/drivers/staging/lustre/lustre/lov/lov_offset.c
> +++ b/drivers/staging/lustre/lustre/lov/lov_offset.c
> @@ -44,7 +44,7 @@ u64 lov_stripe_size(struct lov_stripe_md *lsm, u64 ost_size, int stripeno)
>  {
>  	unsigned long ssize = lsm->lsm_stripe_size;
>  	unsigned long stripe_size;
> -	u64 swidth;
> +	long long swidth;

This feels wrong, u64 should be used instead, can the code be fixed up
to use that type properly?  Don't rely on long long anywhere, it's hard
to always know what it should be on all arches...

Please fix up and resend this series based on the reviews you've gotten
so far.

thanks,

greg k-h

Re: [PATCH 0/4] staging: lustre: fixed some signedness warns from sparse

[Tobin C. Harding]

On Fri, Nov 24, 2017 at 04:02:09PM +0300, Dan Carpenter wrote:
> On Fri, Nov 24, 2017 at 09:20:20AM +1100, Tobin C. Harding wrote:
> > My current favourite review of all time was done by you on a what was
> > at the time a pretty hard patch for me. It was
> > 
> > 	Nope
> > 
> 
> Wow...  I know I've said that to other people but I can't believe I sent
> that to *you*...  Sorry.  You write good patches.

Not always :) No apology needed, I was genuinely encouraged to tighten
my game.

thanks,
Tobin

[lustre-devel] [PATCH 0/4] staging: lustre: fixed some signedness warns from sparse

[Tobin C. Harding]

On Fri, Nov 24, 2017 at 04:02:09PM +0300, Dan Carpenter wrote:
> On Fri, Nov 24, 2017 at 09:20:20AM +1100, Tobin C. Harding wrote:
> > My current favourite review of all time was done by you on a what was
> > at the time a pretty hard patch for me. It was
> > 
> > 	Nope
> > 
> 
> Wow...  I know I've said that to other people but I can't believe I sent
> that to *you*...  Sorry.  You write good patches.

Not always :) No apology needed, I was genuinely encouraged to tighten
my game.

thanks,
Tobin