Date: Tue, 04 Dec 2001 00:14:00 -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: <3C0C77B8.26C050B0@softweyr.com> References: <20011202011045.F5026-100000@gamplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
This is a multi-part message in MIME format.
--------------4F6730CC3992A54C281FAB7D
Content-Type: text/plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Bruce Evans wrote:
>
> On Sat, 1 Dec 2001, Wes Peters wrote:
>
> > 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.
>
> I've tested it a bit and have only found one cosmetic bug, an old one:
> the prefix for strerror(0) is "Undefined error: " (from sys_errlist[0]),
> but the prefix for other unusual errors is "Unknown error: " (from the
> function).
You BROKE strerror_r. You took an auto temporary character buffer and
converted it into a static. Please report to us what will happen when
the currently executing thread is interrupted in the middle of the loop
that writes into this static buffer by another call to strerror_r. I
am certainly not impressed by this incredibly basic error.
> One more cosmetic bug turned up in this thread, a not so old one: adding
> the length of the prefix to the size of the temporary buffer is bogus.
>
> > > - 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.
>
> When it's badly written. uerr is used in strerror() because it was
> copied from strerror_r().
uerr is used in strerror because it remains from the prior implementation.
> strerror() doesn't do any integer to string
> conversions, so using uerr in it is just an obfuscation.
But it does do range checking; I suppose 0 < errnum < sys_nerr would be
more clear.
> > > - 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.
>
> Either it or its size needs to be more global so that knowledge of its
> size doesn't need to be hard-coded into strerror().
>
> > > > 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
>
> It is not silent, but not very verbose. All of the examples of returning
> a value in style(9) use "return (foo)". There is a whole one such example.
> It apparently didn't occur to the origianal author of style(9) that this
> needed an explicit rule.
>
> > 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.
>
> libc/string might not be a good example. The best examples are:
> sys/kern (CSRG version), libc/stdio (CSRG version), and contrib/vi
> for a slightly more modern version, or even the CSRG version of
> libc/string.
The opposite of my statement above applies as well. Should you update
style(9) to more explicity require this, I will examine all of my code
in the system tree and apply parenthesis around return values. This is
an opportunity to make style(9) much more succint. I find it unfortunate
that developers new to FreeBSD are so often clubbed with this vague
document; it is truly impossible to find general agreement on what "KNF"
really is. So I'll let you update the document to truly reflect what
you think it means, take the inevitable flames and bickering, and turn
it into a working document, since it seems to be your passion. As long
as the definition of what "KNF" really means is "whatever BDE says it
means" everyone will continue to commit whatever slop they like and then
wait for your judgement. This is a rather silly bottleneck to impose
on the entire development team.
> > > (2) comments not English sentences
> >
> > Mine were. Existing ones often were not.
>
> Except for:
>
> /* strerror can't fail so handle truncation semi-elegantly */
That *is* a complete English sentence if you add a period at the end.
I did. See attached.
> > > (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.
>
> You have to look at some KNF code. The old version of strerror.c is a
> good enough example here. It uses doesn't use any blank lines except
> before the block of code begun by a comment. This was intentional.
The "indent -sob" was all the hint needed. So why don't we put this in
the style(9) man page instead of having me learn about it after a non-
conforming commit and negative review?
You, Bruce, are the accepted authority on this, let's take your accumulated
knowlege and document it rather than relying in you reviewing every commit
to /src. If nothing else, it might at least cut down on the amount of
negative feedback you have to give.
> > > (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.
>
> It might be simpler to fix indent(1) and filter commits through it.
Documenting the options to indent(1) that most closely produces style(9)
compliant code would certainly be a starting point. I think you could
easily make the point that FreeBSD's indent should default to style(9)
settings as well, or at least support a flag that does so.
> > > %%%
> > > Index: strerror.c
> > > ...
> > > This has not been tested.
> >
> > Please clarify this comment?
>
> "I rebuilt libc to check that this compiles, but have not installed libc
> or done any runtime tests".
But *I* have. I grepped the system sources for something that uses
strerror and contrived a simple test to verify libc.so worked correctly.
The test is quite simple: strip a non-existant file, as in strip /foo/bar.
I would like to have a place to document this, but neither the source nor
the commit message seems appropriate. This seems appropriate for the
"standards" group to discuss; where should we document regression tests
and/or standards compliance tests? In or near the source tree would be
an ideal location, I think. Test cases developers can use to prove
their work would be a great addition.
> I tested it a bit more and found a bug in the test code: the test of a
> short buffer returns an unterminated string (as expected), and printing
> this non-string using %s gives garbage.
The latest iteration of this pre-fills the buffer with a known character
to demonstrate that the string does not get terminated. More tests are
always welcome.
Please note that the lack of an end-to-end and system integration test
does not invalidate a unit test.
--
"Where am I, and what am I doing in this handbasket?"
Wes Peters Softweyr LLC
wes@softweyr.com http://softweyr.com/
--------------4F6730CC3992A54C281FAB7D
Content-Type: text/plain; charset=us-ascii;
name="strerror.c"
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
filename="strerror.c"
/*
* Copyright (c) 1988, 1993 The Regents of the University of California. All
* rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions are
* met: 1. Redistributions of source code must retain the above copyright
* notice, this list of conditions and the following disclaimer. 2.
* Redistributions in binary form must reproduce the above copyright notice,
* this list of conditions and the following disclaimer in the documentation
* and/or other materials provided with the distribution. 3. All advertising
* materials mentioning features or use of this software must display the
* following acknowledgement: This product includes software developed by the
* University of California, Berkeley and its contributors. 4. Neither the
* name of the University nor the names of its contributors may be used to
* endorse or promote products derived from this software without specific
* prior written permission.
*
* THIS SOFTWARE IS PROVIDED BY THE REGENTS AND CONTRIBUTORS ``AS IS'' AND ANY
* EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
* WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
* DISCLAIMED. IN NO EVENT SHALL THE REGENTS OR CONTRIBUTORS BE LIABLE FOR
* ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
* DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR
* SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER
* CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
* LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
* OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
* SUCH DAMAGE.
*/
#if defined(LIBC_SCCS) && !defined(lint)
static char sccsid[] = "@(#)strerror.c 8.1 (Berkeley) 6/4/93";
#endif /* LIBC_SCCS and not lint */
#ifndef lint
static const char rcsid[] =
"$FreeBSD: src/lib/libc/string/strerror.c,v 1.2.14.1 2001/07/09 23:30:06 obrien Exp $";
#endif
#include <stdio.h>
#include <string.h>
#include <errno.h>
static char const * unknown_prefix = "Unknown error: ";
/*
* Define a buffer size big enough to describe a 64-bit number in ASCII
* decimal, with optional leading sign and trailing NUL.
*/
#define NUMSTRINGLEN 22
int
strerror_r(int errnum, char *strerrbuf, size_t buflen)
{
unsigned int uerr;
char *p, *t;
char tmp[NUMSTRINGLEN];
int len;
uerr = errnum;
if (uerr < sys_nerr) {
len = strlcpy(strerrbuf, (char *)sys_errlist[uerr], buflen);
return (len <= buflen) ? 0 : ERANGE;
}
/*
* Print unknown errno by hand so we don't link to stdio(3). This
* collects the ASCII digits in reverse order.
*/
t = tmp;
if (errnum < 0)
uerr = -uerr;
do {
*t++ = "0123456789"[uerr % 10];
} while (uerr /= 10);
if (errnum < 0)
*t++ = '-';
/*
* Copy the "unkown" message and the number into the caller supplied
* buffer, inverting the number string.
*/
strlcpy(strerrbuf, unknown_prefix, buflen);
for (p = strerrbuf + sizeof(unknown_prefix) - 1;
p < strerrbuf + buflen; ) {
*p++ = *--t;
if (t <= tmp)
break;
}
if (p < strerrbuf + buflen) {
*p = '\0';
return 0;
}
/*
* We ran out of space in strerrbuf.
*/
return ERANGE;
}
char *
strerror(num)
int num;
{
static char ebuf[sizeof unknown_prefix + NUMSTRINGLEN];
if ((num > 0) && (num < sys_nerr))
return (char *)sys_errlist[num];
/*
* strerror must not fail so handle truncation semi-elegantly.
*/
if (strerror_r(num, ebuf, sizeof ebuf) != 0)
ebuf[sizeof ebuf - 1] = '\0';
return ebuf;
}
#ifdef STANDALONE_TEST
main()
{
char mybuf[64];
int ret;
printf("strerror(47) yeilds: %s\n", strerror(47));
strerror_r(11, mybuf, 64);
printf("strerror_r(11) yeilds: %s\n", mybuf);
strerror_r(1234, mybuf, 64);
printf("strerror_r(1234) yeilds: %s\n", mybuf);
memset(mybuf, '*', 63);
ret = strerror_r(4321, mybuf, 16);
printf("strerror_r on short buffer returns %d (%s)\n", ret, mybuf);
}
#endif
--------------4F6730CC3992A54C281FAB7D--
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?3C0C77B8.26C050B0>