From owner-freebsd-hackers@FreeBSD.ORG Wed Mar 30 00:15:47 2011 Return-Path: Delivered-To: freebsd-hackers@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id ED7C6106566C; Wed, 30 Mar 2011 00:15:47 +0000 (UTC) (envelope-from delphij@gmail.com) Received: from mail-iy0-f182.google.com (mail-iy0-f182.google.com [209.85.210.182]) by mx1.freebsd.org (Postfix) with ESMTP id A1F608FC1B; Wed, 30 Mar 2011 00:15:47 +0000 (UTC) Received: by iyj12 with SMTP id 12so879608iyj.13 for ; Tue, 29 Mar 2011 17:15:47 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-type :content-transfer-encoding; bh=FN7VV5Pob/ZkifVIjDjv9OWbvkg7TR920J/BciX7g7s=; b=Ai1KFcth5nc0i+sL62Ysmj4CijpzHdbKVmMIjK7/bm81jNcZIBoeTF2HiKkNZb8gzT j71YW+6umbL6Szq5Jo3HZ8kPtNfwJEoJ+Z3/AUzxbNCNsVl1udhH3cJfqo86fqin7nqw SIbt7qk0FOjCAsL3DfqFxgF3C72L2xUqZ1X84= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type:content-transfer-encoding; b=k4kv7eV+gWcyr5jN60Yjrg0s7XQ6YhKbABen4Zp9VzBZ+kLFvVyXGviAZrdP0IqZak sFKOXK5CSchgjwD+LxSNZTem24SefA7xWpVNq9ltU1upfs3o5CAVT5AgtXIyYyfN3MR0 z/0WsjdpBwveJSCgGa4MQkh9tazSvjak1Zb+E= MIME-Version: 1.0 Received: by 10.43.47.134 with SMTP id us6mr238291icb.152.1301444146661; Tue, 29 Mar 2011 17:15:46 -0700 (PDT) Received: by 10.231.143.209 with HTTP; Tue, 29 Mar 2011 17:15:46 -0700 (PDT) In-Reply-To: <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> <20110329215114.GA36220@freebsd.org> Date: Tue, 29 Mar 2011 17:15:46 -0700 Message-ID: From: Xin LI To: Alexander Best Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable 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: Wed, 30 Mar 2011 00:15:48 -0000 On Tue, Mar 29, 2011 at 2:51 PM, Alexander Best wrote= : > On Fri Mar 25 11, Xin LI wrote: >> On Fri, Mar 25, 2011 at 3:32 PM, Alexander Best wr= ote: >> > On Fri Mar 25 11, Xin LI wrote: >> >> FYI I have a patch and I have incorporated some of Alexander's idea. >> >> >> >> Difference: >> >> >> >> =C2=A0- Use of both HN_DIVISOR_1000 and HN_IEC_PREFIXES triggers an >> >> assertion. =C2=A0I think it doesn't make sense to return since this i= s an >> >> API violation and we should just tell the caller explicitly; >> > >> > actually i vote for removing all asserts in humanize_number.c and retu= rn -1 >> > based upon the later checks. >> > >> > the existing >> > >> > assert(buf !=3D NULL); >> > assert(suffix !=3D NULL); >> > >> > checks aren't really needed, since buf and suffix are also checked lat= er 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. =C2=A0To test or not to t= est >> these cases in a release binary on the other hand really depends on >> whether there is security or other bad implications. =C2=A0This generall= y >> 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". =C2=A0Assertions here gives it an opportunity to say i= t >> loudly. >> >> If however the program is compiled with -DNDEBUG, these assertions >> would became no-op. =C2=A0At 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. =C2=A0Furthermor= e, >> these values are more likely to be hard-wired at caller, where the >> assertion should catch the case. >> >> > =C2=A0 =C2=A0 =C2=A0 =C2=A0if (scale <=3D 0 || (scale >=3D maxscale && >> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(scale & (HN_AUTOSCALE | HN_G= ETSCALE)) =3D=3D 0)) >> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return (-1); >> >> I think this one is good to have for both assertion and tests. =C2=A0Not= e >> that I think it should be scale < 0 here, scale =3D=3D 0 seems to be a >> valid value. >> >> > =C2=A0 =C2=A0 =C2=A0 =C2=A0if (buf =3D=3D NULL || suffix =3D=3D NULL) >> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return (-1); >> >> This duplication is necessary in my opinion. =C2=A0It's a protection >> against NULL pointer deference at runtime. >> >> > =C2=A0 =C2=A0 =C2=A0 =C2=A0if ((flags & (HN_DIVISOR_1000 | HN_IEC_PREF= IXES)) =3D=3D 0) >> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return (-1); >> >> I'd vote no for this one for the reason above. >> >> >> =C2=A0- 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 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A00x0= 1 >> > #define HN_NOSPACE =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A00x0= 2 >> > #define HN_B =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A00x04 >> > #define HN_DIVISOR_1000 =C2=A0 =C2=A0 =C2=A0 =C2=A0 0x08 >> > #define HN_IEC_PREFIXES =C2=A0 =C2=A0 =C2=A0 =C2=A0 0x40 >> > >> > #define HN_GETSCALE =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 0x10 >> > #define HN_AUTOSCALE =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A00x20 >> >> Thinking again and I think we are just fine to use HN_IEC_PREFIXES =3D= =3D >> 0x10 here. =C2=A0I don't think there is a reason why we can't use 0x10 f= or >> flags. >> >> Here is what in my mind. =C2=A0I 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? I think I should be able to commit a final version by Friday, still need some polishing... Cheers, --=20 Xin LI http://www.delphij.net