Re: [PATCH v2 2/3] lib/string_helpers.c: don't lose precision in string_get_size()

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Tue, 2015-10-27 at 17:16 +0100, Vitaly Kuznetsov wrote:
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:

[]

> > +	if (!blk_size) {
> > > +		WARN_ON(1);
> > 
> > Hmm... Isn't it too strong? WARN_ONCE() might reduce a noise. Or
> > even
> > pr_warn_once/ratelimited().
> 
> Nobody is supposed to call string_get_size() with blk_size = 0, if
> someone does that - it is a bug and that's what WARN_ON is supposed
> to
> report. I'm OK with changing it to WARN_ONCE() but I don't see a big
> difference - nobody's calling string_get_size() in a loop, one/two
> calls
> per one storage device is expected.

I'm fine with WARN_ONCE() if there is no objection.

> 
> > 
> > > +		size = 0;
> > > +		goto out;
> > >  	}
> > 
> > What about doing it before if (!size) ?
> > 
> > Like 
> > 
> > if (!blk_size) {
> >  pr_warn_once(); /* or WARN_ONCE() ? */
> >  /* Override size to follow error path */
> >  size = 0;
> > }
> >  
> > if (!size)
> 
> To be honest I don't see a big difference but I'm fine with the
> change :-)

Maybe Rasmus can judge me.

> > > -	remainder %= divisor[units];
> > > +	remainder -= (remainder / divisor[units]) *
> > > divisor[units];
> > 
> > I'm sorry I didn't get what the purpose of change here.
> > 
> > (Yes, I was thinking about u64 on 32-bit architecture, but % and /
> > are
> > working in the similar way aren't they?)
> 
> Thanks for noticing, there is no functional change here, it just made
> the code easier to understand (for me only?). I'm OK with reverting
> it
> to '%='.

For me is the opposite. remainder = remainder % divisor[units] will
work as well, but why change the code at all.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy