Date: Sat, 01 Dec 2001 00:08:46 -0700 From: Wes Peters <wes@softweyr.com> To: Bruce Evans <bde@zeta.org.au> Cc: Bill Fenner <fenner@research.att.com>, mike@FreeBSD.org, freebsd-standards@bostonradio.org Subject: Re: strerror_r() implementation Message-ID: <3C0881FE.9CA0DD90@softweyr.com> References: <20011130210640.X752-100000@gamplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Bruce Evans wrote:
>
> I prefer the version enclosed at the end. It has the following additional
> non-formatting changes:
> - parametrize the buffer size according to the sizes in strerror_r()
...but do it in a way that doesn't break the function.
> - unobfuscate uerr (don't use an unsigned variable just to hand-optimize
> for 20-year-old compilers).
When did "don't change code that is known to work" get thrown out?
uerr is used because the simplistic ASCII conversion doesn't really
grok signedness. It does do quite a nice job in very little code.
> - remove redundant code (the strerror_r() can't fail unless we used the
> wrong buffer size, and we should get this right by not hard-coding the
> size of tmp[])
> `tmp' should have a better name now that it is more global. I didn't
> change it because that would require large changes in strerror_r().
It's a throw-away character buffer and making it more global is an error.
> > The last change is a whitespace error. Mustn't have any style(9) nits,
> > right? ;^)
>
> I agree, so about 10 more of them in strerror() alone ;^). They were in
> the following classes:
> (1) missing parens around return values
style(9) remains silent on this subject. Until required to do so by
style(9) I will not submit to this barbarism. A quick check of other
routines in libc/string showed about 50% usage of return (foo) vs.
return foo.
> (2) comments not English sentences
Mine were. Existing ones often were not.
> (3) excessive vertical whitespace (KNF uses indent -sob)
This would be much more understandable if style(9) spelled out what is
"acceptable" or "required" vertical whitespace. As it currently stands,
there is no mention of the word "vertical" anywhere, and no mention of
"blank line" after the description of include files. Again, existing
usage in the libc/string directory is all over the map, and style(9)
is completely opaque on the subject.
> (4) excessive indentation for comments at the right of code (32 is normal).
The only mention of right-comments in style(9) simply says "Try to align
the comments".
style(9) would be much easier to adhere to if it were actually documented
in style(9), rather than being the unagreed-upon figment of the imagination
of several dozen different people.
> The comment fixes might not be obvious because the comments just went away.
> All of these bugs except (4) were not in rev.1.4.
> %%%
> Index: strerror.c
> ===================================================================
> RCS file: /home/ncvs/src/lib/libc/string/strerror.c,v
> retrieving revision 1.5
> diff -u -2 -r1.5 strerror.c
> --- strerror.c 27 Nov 2001 07:39:46 -0000 1.5
> +++ strerror.c 30 Nov 2001 10:08:58 -0000
> @@ -42,12 +42,13 @@
> #include <errno.h>
>
> +#define UPREFIX "Unknown error: "
> +
> +static char tmp[40]; /* 64-bit number + slop */
Uh, no, you can't promote this from auto to file-static without
breaking the function. Making the function style(9) compliant
but non-functional isn't really going to help. Recall that the
_r means "reentrant" and this function no longer is.
>
> int
> strerror_r(int errnum, char *strerrbuf, size_t buflen)
> {
> -#define UPREFIX "Unknown error: "
> unsigned int uerr;
> char *p, *t;
> - char tmp[40]; /* 64-bit number + slop */
> int len;
>
> @@ -84,30 +85,15 @@
> }
>
> -
> -/*
> - * NOTE: the following length should be enough to hold the longest defined
> - * error message in sys_errlist, defined in ../gen/errlst.c. This is a WAG
> - * that is better than the previous value.
> - */
> -#define ERR_LEN 64
> -
> char *
> strerror(num)
> int num;
> {
> - unsigned int uerr;
> - static char ebuf[ERR_LEN];
> + static char ebuf[sizeof(UPREFIX) - 1 + sizeof(tmp)];
Again, this will not work, since (tmp) cannot be file-static. Obviously
a #define size is warranted here.
> - uerr = num; /* convert to unsigned */
> - if (uerr < sys_nerr)
> - return (char *)sys_errlist[uerr];
> -
> - /* strerror can't fail so handle truncation semi-elegantly */
> - if (strerror_r(num, ebuf, (size_t) ERR_LEN) != 0)
> - ebuf[ERR_LEN - 1] = '\0';
> -
> - return ebuf;
> + if (num >= 0 && num < sys_nerr)
> + return ((char *)sys_errlist[num]);
> + (void)strerror_r(num, ebuf, sizeof(ebuf));
> + return (ebuf);
> }
> -
>
> #ifdef STANDALONE_TEST
> %%%
>
> This has not been tested.
Please clarify this comment?
--
"Where am I, and what am I doing in this handbasket?"
Wes Peters Softweyr LLC
wes@softweyr.com http://softweyr.com/
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-standards" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?3C0881FE.9CA0DD90>