Date: Tue, 29 Mar 2011 17:15:46 -0700 From: Xin LI <delphij@gmail.com> To: Alexander Best <arundel@freebsd.org> Cc: freebsd-hackers@freebsd.org Subject: Re: Switching to [KMGTPE]i prefixes? Message-ID: <BANLkTin%2Bka2e=SMu0ev4zO6P8U0wsebFvA@mail.gmail.com> 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> <AANLkTinEcT__Wtc6LkSyqqMnQwuKVUbZC4dPZvZH_dSX@mail.gmail.com> <20110325223203.GA95976@freebsd.org> <BANLkTinzLOB6xOMsn4aT6LJzQ6GOyhYpwg@mail.gmail.com> <20110329215114.GA36220@freebsd.org>
index | next in thread | previous in thread | raw e-mail
On Tue, Mar 29, 2011 at 2:51 PM, Alexander Best <arundel@freebsd.org> wrote: > On Fri Mar 25 11, Xin LI wrote: >> On Fri, Mar 25, 2011 at 3:32 PM, Alexander Best <arundel@freebsd.org> 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? I think I should be able to commit a final version by Friday, still need some polishing... Cheers, -- Xin LI <delphij@delphij.net> http://www.delphij.nethelp
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?BANLkTin%2Bka2e=SMu0ev4zO6P8U0wsebFvA>