lkml.org 
[lkml]   [2015]   [Oct]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2 2/3] lib/string_helpers.c: don't lose precision in string_get_size()
From
Date
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



\
 
 \ /
  Last update: 2015-10-27 17:41    [W:0.474 / U:0.448 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site