Date: Fri, 25 Mar 2011 16:27:06 -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: <BANLkTinzLOB6xOMsn4aT6LJzQ6GOyhYpwg@mail.gmail.com> In-Reply-To: <20110325223203.GA95976@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>
next in thread | previous in thread | raw e-mail | index | archive | help
--00504502cc8f9fc715049f56ee36 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable 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: >> >> =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 is a= n >> 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 !=3D NULL); > assert(suffix !=3D 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. > =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_GETS= CALE)) =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. Note 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. It's a protection against NULL pointer deference at runtime. > =C2=A0 =C2=A0 =C2=A0 =C2=A0if ((flags & (HN_DIVISOR_1000 | HN_IEC_PREFIXE= S)) =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 th= em >> 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 ve= ry > closely to find out. Will do. > #define HN_DECIMAL =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A00x01 > #define HN_NOSPACE =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A00x02 > #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. 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. --=20 Xin LI <delphij@delphij.net> http://www.delphij.net --00504502cc8f9fc715049f56ee36 Content-Type: text/x-patch; charset=US-ASCII; name="humanize_number.c.diff" Content-Disposition: attachment; filename="humanize_number.c.diff" Content-Transfer-Encoding: base64 X-Attachment-Id: f_glpqpog20 SW5kZXg6IGh1bWFuaXplX251bWJlci5jCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIGh1bWFuaXplX251bWJlci5j CShyZXZpc2lvbiAyMjAwMDkpCisrKyBodW1hbml6ZV9udW1iZXIuYwkod29ya2luZyBjb3B5KQpA QCAtNDIsNDUgKzQyLDU5IEBACiAjaW5jbHVkZSA8bG9jYWxlLmg+CiAjaW5jbHVkZSA8bGlidXRp bC5oPgogCitzdGF0aWMgY29uc3QgaW50IG1heHNjYWxlID0gNzsKKwogaW50CiBodW1hbml6ZV9u dW1iZXIoY2hhciAqYnVmLCBzaXplX3QgbGVuLCBpbnQ2NF90IHF1b3RpZW50LAogICAgIGNvbnN0 IGNoYXIgKnN1ZmZpeCwgaW50IHNjYWxlLCBpbnQgZmxhZ3MpCiB7CiAJY29uc3QgY2hhciAqcHJl Zml4ZXMsICpzZXA7Ci0JaW50CWksIHIsIHJlbWFpbmRlciwgbWF4c2NhbGUsIHMxLCBzMiwgc2ln bjsKKwlpbnQJaSwgciwgcmVtYWluZGVyLCBzMSwgczIsIHNpZ247CiAJaW50NjRfdAlkaXZpc29y LCBtYXg7CiAJc2l6ZV90CWJhc2VsZW47CiAKIAlhc3NlcnQoYnVmICE9IE5VTEwpOwogCWFzc2Vy dChzdWZmaXggIT0gTlVMTCk7CiAJYXNzZXJ0KHNjYWxlID49IDApOworCWFzc2VydChzY2FsZSA8 IG1heHNjYWxlIHx8ICgoKHNjYWxlICYgKEhOX0FVVE9TQ0FMRSB8IEhOX0dFVFNDQUxFKSkgIT0g MCkpKTsKKwlhc3NlcnQoISgoZmxhZ3MgJiBITl9ESVZJU09SXzEwMDApICYmIChmbGFncyAmIEhO X0lFQ19QUkVGSVhFUykpKTsKIAogCXJlbWFpbmRlciA9IDA7CiAKLQlpZiAoZmxhZ3MgJiBITl9E SVZJU09SXzEwMDApIHsKLQkJLyogU0kgZm9yIGRlY2ltYWwgbXVsdGlwbGllcyAqLwotCQlkaXZp c29yID0gMTAwMDsKLQkJaWYgKGZsYWdzICYgSE5fQikKLQkJCXByZWZpeGVzID0gIkJcMGtcME1c MEdcMFRcMFBcMEUiOwotCQllbHNlCi0JCQlwcmVmaXhlcyA9ICJcMFwwa1wwTVwwR1wwVFwwUFww RSI7Ci0JfSBlbHNlIHsKKwlpZiAoZmxhZ3MgJiBITl9JRUNfUFJFRklYRVMpIHsKKwkJYmFzZWxl biA9IDI7CiAJCS8qCi0JCSAqIGJpbmFyeSBtdWx0aXBsaWVzCi0JCSAqIFhYWCBJRUMgNjAwMjct MiByZWNvbW1lbmRzIEtpLCBNaSwgR2kuLi4KKwkJICogVXNlIHRoZSBwcmVmaXhlcyBmb3IgcG93 ZXIgb2YgdHdvIHJlY29tbWVuZGVkIGJ5CisJCSAqIHRoZSBJbnRlcm5hdGlvbmFsIEVsZWN0cm90 ZWNobmljYWwgQ29tbWlzc2lvbgorCQkgKiAoSUVDKSBpbiBJRUMgODAwMDAtMyAoc3VwZXJzZWVk aW5nIElFQyA2MDAyNy0yKQorCQkgKiAoaS5lLiBLaSwgTWksIEdpLi4uKS4KKwkJICoKKwkJICog SE5fSUVDX1BSRUZJWEVTIGltcGxpZXMgYSBkaXZpc29yIG9mIDEwMjQgaGVyZQorCQkgKiAodXNl IG9mIEhOX0RJVklTT1JfMTAwMCB3b3VsZCBoYXZlIHRyaWdnZXJlZAorCQkgKiBhbiBhc3NlcnRp b24gZWFybGllcikuCiAJCSAqLwogCQlkaXZpc29yID0gMTAyNDsKIAkJaWYgKGZsYWdzICYgSE5f QikKLQkJCXByZWZpeGVzID0gIkJcMEtcME1cMEdcMFRcMFBcMEUiOworCQkJcHJlZml4ZXMgPSAi QlwwXDBLaVwwTWlcMEdpXDBUaVwwUGlcMEVpIjsKIAkJZWxzZQotCQkJcHJlZml4ZXMgPSAiXDBc MEtcME1cMEdcMFRcMFBcMEUiOworCQkJcHJlZml4ZXMgPSAiXDBcMEtpXDBNaVwwR2lcMFRpXDBQ aVwwRWkiOworCX0gZWxzZSB7CisJCWJhc2VsZW4gPSAxOworCQlpZiAoZmxhZ3MgJiBITl9ESVZJ U09SXzEwMDApCisJCQlkaXZpc29yID0gMTAwMDsKKwkJZWxzZQorCQkJZGl2aXNvciA9IDEwMjQ7 CisKKwkJaWYgKGZsYWdzICYgSE5fQikKKwkJCXByZWZpeGVzID0gIkJcMFwwa1wwXDBNXDBcMEdc MFwwVFwwXDBQXDBcMEUiOworCQllbHNlCisJCQlwcmVmaXhlcyA9ICJcMFwwXDBrXDBcME1cMFww R1wwXDBUXDBcMFBcMFwwRSI7CiAJfQogCi0jZGVmaW5lCVNDQUxFMlBSRUZJWChzY2FsZSkJKCZw cmVmaXhlc1soc2NhbGUpIDw8IDFdKQotCW1heHNjYWxlID0gNzsKKyNkZWZpbmUJU0NBTEUyUFJF RklYKHNjYWxlKQkoJnByZWZpeGVzWyhzY2FsZSkgKiAzXSkKIAotCWlmIChzY2FsZSA+PSBtYXhz Y2FsZSAmJgotCSAgICAoc2NhbGUgJiAoSE5fQVVUT1NDQUxFIHwgSE5fR0VUU0NBTEUpKSA9PSAw KQorCWlmIChzY2FsZSA8IDAgfHwgKHNjYWxlID49IG1heHNjYWxlICYmCisJICAgIChzY2FsZSAm IChITl9BVVRPU0NBTEUgfCBITl9HRVRTQ0FMRSkpID09IDApKQogCQlyZXR1cm4gKC0xKTsKIAog CWlmIChidWYgPT0gTlVMTCB8fCBzdWZmaXggPT0gTlVMTCkKQEAgLTkxLDEwICsxMDUsMTAgQEAK IAlpZiAocXVvdGllbnQgPCAwKSB7CiAJCXNpZ24gPSAtMTsKIAkJcXVvdGllbnQgPSAtcXVvdGll bnQ7Ci0JCWJhc2VsZW4gPSAzOwkJLyogc2lnbiwgZGlnaXQsIHByZWZpeCAqLworCQliYXNlbGVu ICs9IDI7CQkvKiBzaWduLCBkaWdpdCAqLwogCX0gZWxzZSB7CiAJCXNpZ24gPSAxOwotCQliYXNl bGVuID0gMjsJCS8qIGRpZ2l0LCBwcmVmaXggKi8KKwkJYmFzZWxlbiArPSAxOwkJLyogZGlnaXQg Ki8KIAl9CiAJaWYgKGZsYWdzICYgSE5fTk9TUEFDRSkKIAkJc2VwID0gIiI7CkluZGV4OiBsaWJ1 dGlsLmgKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PQotLS0gbGlidXRpbC5oCShyZXZpc2lvbiAyMjAwMDkpCisrKyBsaWJ1 dGlsLmgJKHdvcmtpbmcgY29weSkKQEAgLTIyMCw3ICsyMjAsOSBAQAogI2RlZmluZSBITl9OT1NQ QUNFCQkweDAyCiAjZGVmaW5lIEhOX0IJCQkweDA0CiAjZGVmaW5lIEhOX0RJVklTT1JfMTAwMAkJ MHgwOAorI2RlZmluZSBITl9JRUNfUFJFRklYRVMJCTB4MTAKIAorLyogbWF4c2NhbGUgPSAweDA3 ICovCiAjZGVmaW5lIEhOX0dFVFNDQUxFCQkweDEwCiAjZGVmaW5lIEhOX0FVVE9TQ0FMRQkJMHgy MAogCg== --00504502cc8f9fc715049f56ee36--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?BANLkTinzLOB6xOMsn4aT6LJzQ6GOyhYpwg>