From owner-freebsd-hackers@FreeBSD.ORG Tue Mar 29 21:51:14 2011 Return-Path: Delivered-To: freebsd-hackers@freebsd.org Received: by hub.freebsd.org (Postfix, from userid 1233) id 857601065670; Tue, 29 Mar 2011 21:51:14 +0000 (UTC) Date: Tue, 29 Mar 2011 21:51:14 +0000 From: Alexander Best To: Xin LI Message-ID: <20110329215114.GA36220@freebsd.org> References: <20110325002115.GA323@freebsd.org> <20110325015508.GA14565@freebsd.org> <20110325024658.GA19544@freebsd.org> <336A9ACD-29BF-41C9-BC25-917CC1E4587D@bsdimp.com> <20110325195325.GA69264@freebsd.org> <20110325223203.GA95976@freebsd.org> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-15 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: Cc: freebsd-hackers@freebsd.org Subject: Re: Switching to [KMGTPE]i prefixes? X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 29 Mar 2011 21:51:14 -0000 On Fri Mar 25 11, Xin LI wrote: > On Fri, Mar 25, 2011 at 3:32 PM, Alexander Best wrote: > > On Fri Mar 25 11, Xin LI wrote: > >> FYI I have a patch and I have incorporated some of Alexander's idea. > >> > >> Difference: > >> > >>  - Use of both HN_DIVISOR_1000 and HN_IEC_PREFIXES triggers an > >> assertion.  I think it doesn't make sense to return since this is an > >> API violation and we should just tell the caller explicitly; > > > > actually i vote for removing all asserts in humanize_number.c and return -1 > > based upon the later checks. > > > > the existing > > > > assert(buf != NULL); > > assert(suffix != NULL); > > > > checks aren't really needed, since buf and suffix are also checked later on. so > > just having: > > Well, one of my believes is that a program should crash as early as > possible, and with clear statement about "Why I crashed", when it's > compiled with debugging aids, like assertions. To test or not to test > these cases in a release binary on the other hand really depends on > whether there is security or other bad implications. This generally > makes developers' life easier, as they don't have to pursue into the > code to find out why the program crashed, etc. > > Unlike system calls, humanize_number(3) does not indicate what's wrong > via errno, e.g. it tells you "No I can't" rather than a reason of "Why > I can't do that". Assertions here gives it an opportunity to say it > loudly. > > If however the program is compiled with -DNDEBUG, these assertions > would became no-op. At this stage, in my opinion, only basic tests > should be done and we fall back to returning -1, or at least, not > crash the program in a mysterious way. > > For this reasons, I think the assertion here against flags is right, > it does not hurt if we proceed with both flags set, as we do not > access undefined memory nor overwrite undefined memory. Furthermore, > these values are more likely to be hard-wired at caller, where the > assertion should catch the case. > > >        if (scale <= 0 || (scale >= maxscale && > >            (scale & (HN_AUTOSCALE | HN_GETSCALE)) == 0)) > >                return (-1); > > I think this one is good to have for both assertion and tests. Note > that I think it should be scale < 0 here, scale == 0 seems to be a > valid value. > > >        if (buf == NULL || suffix == NULL) > >                return (-1); > > This duplication is necessary in my opinion. It's a protection > against NULL pointer deference at runtime. > > >        if ((flags & (HN_DIVISOR_1000 | HN_IEC_PREFIXES)) == 0) > >                return (-1); > > I'd vote no for this one for the reason above. > > >>  - DIVISOR_1000 and !1000 cases use just same prefixes, so merge them > >> while keeping divisor intact; > > > > good idea. however i think you should add a comment to point out that the > > default behavior is !DIVISOR_1000 && !HN_IEC_PREFIXES. one has to look very > > closely to find out. > > Will do. > > > #define HN_DECIMAL              0x01 > > #define HN_NOSPACE              0x02 > > #define HN_B                    0x04 > > #define HN_DIVISOR_1000         0x08 > > #define HN_IEC_PREFIXES         0x40 > > > > #define HN_GETSCALE             0x10 > > #define HN_AUTOSCALE            0x20 > > Thinking again and I think we are just fine to use HN_IEC_PREFIXES == > 0x10 here. I don't think there is a reason why we can't use 0x10 for > flags. > > Here is what in my mind. I have stolen some comments from your > version of patch to explain the meaning of the HN_IEC_PREFIXES option > as well. any plans to commit this patch? > > -- > Xin LI http://www.delphij.net -- a13x