Messages in this thread | | | Subject | Re: [PATCH v2 2/3] lib/string_helpers.c: don't lose precision in string_get_size() | From | Andy Shevchenko <> | Date | Tue, 27 Oct 2015 18:24:16 +0200 |
| |
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
| |