From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965089AbbJ0QYW (ORCPT ); Tue, 27 Oct 2015 12:24:22 -0400 Received: from mga14.intel.com ([192.55.52.115]:28315 "EHLO mga14.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S964879AbbJ0QYU (ORCPT ); Tue, 27 Oct 2015 12:24:20 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.20,205,1444719600"; d="scan'208";a="804495440" Message-ID: <1445963056.6332.41.camel@linux.intel.com> Subject: Re: [PATCH v2 2/3] lib/string_helpers.c: don't lose precision in string_get_size() From: Andy Shevchenko To: Vitaly Kuznetsov Cc: Andrew Morton , Rasmus Villemoes , Ulf Hansson , James Bottomley , Kees Cook , linux-kernel@vger.kernel.org Date: Tue, 27 Oct 2015 18:24:16 +0200 In-Reply-To: <87pp00walr.fsf@vitty.brq.redhat.com> References: <1445954787-18104-1-git-send-email-vkuznets@redhat.com> <1445954787-18104-3-git-send-email-vkuznets@redhat.com> <1445959501.6332.33.camel@linux.intel.com> <87pp00walr.fsf@vitty.brq.redhat.com> Organization: Intel Finland Oy Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.18.1-1 Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, 2015-10-27 at 17:16 +0100, Vitaly Kuznetsov wrote: > Andy Shevchenko 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 Intel Finland Oy