From owner-svn-src-all@FreeBSD.ORG Thu Aug 29 11:14:59 2013 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTP id 924A435E; Thu, 29 Aug 2013 11:14:59 +0000 (UTC) (envelope-from pluknet@gmail.com) Received: from mail-we0-x22a.google.com (mail-we0-x22a.google.com [IPv6:2a00:1450:400c:c03::22a]) (using TLSv1 with cipher ECDHE-RSA-RC4-SHA (128/128 bits)) (No client certificate requested) by mx1.freebsd.org (Postfix) with ESMTPS id 937492CEB; Thu, 29 Aug 2013 11:14:58 +0000 (UTC) Received: by mail-we0-f170.google.com with SMTP id w62so279336wes.1 for ; Thu, 29 Aug 2013 04:14:56 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:sender:in-reply-to:references:date:message-id:subject :from:to:cc:content-type; bh=COwH5GpqpZ/cimN4GLvEWpIXdVHh9yujlVV3ZvvEeEA=; b=p7Zfs3R5Tem6K6kNQ2XreEc+hI03IMthPWd2qqhdlibx0433yehww1f19y82cjek1S /f5I55tZ7i3TtIzcKdhCdgb+cu7Zut7QyybDpARujTTnCLHOfSbh1zR7ihjeXeWqeEJN 7GxP1x4IBGkgy5uvpgsetNfrVDmUlqMwf8WupVOgrfLSc8eaCIwTDubHI2Unh9x6+uG5 BosYkBCviAgt+7IOImW4a0YavNwEFHHpg0Sm2C9SDrGefZTSF1gr5Mswm2L5gnQLlnlG i8UYFwQm3lsoUoOkM52r+OMmfDcIC9SvS8qwyrhpcMuDtO+t0Abm21G9CtPP4qZxHtfH 0sDw== MIME-Version: 1.0 X-Received: by 10.194.222.2 with SMTP id qi2mr5361924wjc.14.1377774896827; Thu, 29 Aug 2013 04:14:56 -0700 (PDT) Sender: pluknet@gmail.com Received: by 10.216.62.5 with HTTP; Thu, 29 Aug 2013 04:14:56 -0700 (PDT) In-Reply-To: <20130822185756.Y1297@besplex.bde.org> References: <201308211646.r7LGk6eV051215@svn.freebsd.org> <5214F72B.7070006@freebsd.org> <20130821190309.GB52908@omg> <20130821202725.GA4991@stack.nl> <20130821212413.GC52908@omg> <20130821213755.GA8052@stack.nl> <20130822185756.Y1297@besplex.bde.org> Date: Thu, 29 Aug 2013 15:14:56 +0400 X-Google-Sender-Auth: VtJL-ijyxSNeQ5VAgaOnB38CIyk Message-ID: Subject: Re: svn commit: r254600 - head/lib/libutil From: Sergey Kandaurov To: Bruce Evans Content-Type: text/plain; charset=ISO-8859-1 Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Andrey Chernov , Jilles Tjoelker X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 29 Aug 2013 11:14:59 -0000 On 22 August 2013 14:51, Bruce Evans wrote: > On Wed, 21 Aug 2013, Jilles Tjoelker wrote: > >> On Thu, Aug 22, 2013 at 01:24:13AM +0400, Sergey Kandaurov wrote: >>> >>> On Wed, Aug 21, 2013 at 10:27:25PM +0200, Jilles Tjoelker wrote: >>>> >>>> On Wed, Aug 21, 2013 at 11:03:10PM +0400, Sergey Kandaurov wrote: >>>>> >>>>> On Wed, Aug 21, 2013 at 09:21:47PM +0400, Andrey Chernov wrote: >>>>>> >>>>>> On 21.08.2013 20:46, Sergey Kandaurov wrote: >>>>>>> >>>>>>> number = strtoumax(buf, &endptr, 0); >>>>>>> >>>>>>> + if (number == UINTMAX_MAX && errno == ERANGE) { >>>>>>> + return (-1); >>>>>>> + } >> >> >>>>>> You need to reset errno before strtoumax() call (errno = 0), because >>>>>> any >>>>>> of previous functions may left it as ERANGE. > > > This also has a bogus test for UINTMAX_MAX wheich prevents input of > the number UINTMAX_MAX on systems where the limit for the function > (UINT64_T_MAX) is the same as the limit for strtoumax(). This test is > a wrong way of doing the errno thing. > > This also has a high density of style bugs: > - excessive braces > - extra blank line > > expand_number() remains a very badly designed and implemented function. > Its design errors start with its name. It doesn't expand numbers. It > converts strings to numbers. The strtoumax() family is missing this > bug and others. Its design errors continue with it taking a uint64_t > arg and not a uint64_t arg. > > Its implementation errors begin with broken range checking. The above > is an incomplete fix for this. Unless uintmax_t is the same as uint64_t, > there are still the following bugs: > - the return value of strotumax() is assigned to a uint64_t. This may > overflow. After overflow, the bogus (number == UINTMAX_MAX) check > has no effect, since when overflow is possible 'number' cannot equal > UINTMAX_MAX. > - values that exceed the maximum supported (UINT64_MAX) are not detected. > They are silently truncated to uint64_t. Ack. [...] > > Some of the other bugs in the old version: > > % /* > % * Convert an expression of the following forms to a uint64_t. > % * 1) A positive decimal number. > % * 2) A positive decimal number followed by a 'b' or 'B' (mult by 1). > % * 3) A positive decimal number followed by a 'k' or 'K' (mult by 1 << > 10). > % * 4) A positive decimal number followed by a 'm' or 'M' (mult by 1 << > 20). > % * 5) A positive decimal number followed by a 'g' or 'G' (mult by 1 << > 30). > % * 6) A positive decimal number followed by a 't' or 'T' (mult by 1 << > 40). > % * 7) A positive decimal number followed by a 'p' or 'P' (mult by 1 << > 50). > % * 8) A positive decimal number followed by a 'e' or 'E' (mult by 1 << > 60). > > This garbage was copied from a similar comment in dd/args.c. The number is > actually neither positive nor decimal, especially in dd where there is a > variant of this function that returns negative numbers. Another design > error > in this function is that it can't return negative numbers. Well, I see no useful contents in this comment. It is obvious and verbose. Probably it should be trimmed. > > % */ > % > > This bogus blank line detaches the comment from the code that it describes, > resulting in the comment describing null code. Sorry, I didn't see such blank line in head. > > % int > % expand_number(const char *buf, uint64_t *num) > % { > % uint64_t number; > % unsigned shift; > % char *endptr; > % % number = strtoumax(buf, &endptr, 0); > > This supports non-positive non-decimal numbers. The support for non-decimal > numbers is more intentional (it is given by the base 0 in the call). > Negative > numbers are only supported by strtoumax() allowing a minus sign and > returning a nonnegative number equal to 1 larger than UINTMAX_MAX less the > number without the minus sign. > Fixing the verbose comment above would result in adding more verboseness citing from man strtoumax(3) wrt. signedness and base details. Yet another point to trim it. [ dd dd details here ] > > To start fixing expand_number(): > > /* Fix comment here. */ > int > > expand_number(const char *buf, uint64_t *num) > { > char *endptr; > uintmax_t num; > uint64_t number; > unsigned shift; > int serrno; > > serrno = errno; > errno = 0; > num = strtoumax(buf, &endptr, 0); > if (num > UINT64_MAX) > errno = ERANGE; > if (errno != 0) > return (-1); > errno = serrno; > number = num; > ... > } > > This depends on the POSIX bug for detecting the no-digits case. > Fortunately, the early error handling of this function is simple and > compatible with that of strtoumax(), so we don't need to translate > strtoumax()'s errors except for adjusting its range error. > Thanks for your valuable input. What about this change (on top of head)? Index: expand_number.c =================================================================== --- expand_number.c (revision 255017) +++ expand_number.c (working copy) @@ -35,42 +35,29 @@ #include #include -/* - * Convert an expression of the following forms to a uint64_t. - * 1) A positive decimal number. - * 2) A positive decimal number followed by a 'b' or 'B' (mult by 1). - * 3) A positive decimal number followed by a 'k' or 'K' (mult by 1 << 10). - * 4) A positive decimal number followed by a 'm' or 'M' (mult by 1 << 20). - * 5) A positive decimal number followed by a 'g' or 'G' (mult by 1 << 30). - * 6) A positive decimal number followed by a 't' or 'T' (mult by 1 << 40). - * 7) A positive decimal number followed by a 'p' or 'P' (mult by 1 << 50). - * 8) A positive decimal number followed by a 'e' or 'E' (mult by 1 << 60). - */ int expand_number(const char *buf, uint64_t *num) { + char *endptr; + uintmax_t umaxval; uint64_t number; - int saved_errno; unsigned shift; - char *endptr; + int serrno; - saved_errno = errno; + serrno = errno; errno = 0; - - number = strtoumax(buf, &endptr, 0); - - if (number == UINTMAX_MAX && errno == ERANGE) { - return (-1); - } - - if (errno == 0) - errno = saved_errno; - + umaxval = strtoumax(buf, &endptr, 0); if (endptr == buf) { /* No valid digits. */ errno = EINVAL; return (-1); } + if (umaxval > UINT64_MAX) + errno = ERANGE; + if (errno != 0) + return (-1); + errno = serrno; + number = umaxval; switch (tolower((unsigned char)*endptr)) { case 'e': -- wbr, pluknet