do not set errno after failing malloc [Re: getline() behaviour change
Bruno Haible [EMAIL PROTECTED] wrote: Jim Meyering wrote: Are you advocating support for non-POSIX malloc/realloc? Sure. gnulib supports - to a large extent - mingw. It uses a malloc implementation from msvcrt.dll. This malloc does not set errno. You're dumbing down gnulib to accommodate _mingw_?!? Please, don't do that. Add a wrapper, rather than polluting all application code. This is a fundamental tenet of gnulib: If there's some portability problem, gnulib allows all of its client code to assume the desired behavior. Nonconforming hosts like mingw can endure the run-time overhead of wrappers or replacement functions. That's far cleaner than polluting malloc-using applications with obscure code to set errno upon malloc failure.
Re: getline() behaviour change
Ben Pfaff [EMAIL PROTECTED] wrote: Bruno Haible [EMAIL PROTECTED] writes: Jim Meyering wrote: A *lot* of code expects malloc and realloc to set errno when they fail. Such code is not portable to plain ISO C 99 systems. We could extend gnulib's existing malloc/realloc wrappers to ensure that malloc/realloc set errno when they fail, if it is worthwhile to do so. It is definitely worthwhile to do so.
Re: getline.h
Eric Blake [EMAIL PROTECTED] wrote:
According to Eric Blake on 8/22/2007 6:04 PM:
According to Bruno Haible on 8/22/2007 3:02 PM:
Hi Eric,
Thanks for working on this, and for the unit tests. The patch - excluding
the one of getdelim.c - is nearly perfect. But I see three problems:
Thanks for catching the nits. Please feel free to apply patches if you
come up with them before me.
Here's what I'm committing; I think it addresses all your points.
2007-08-22 Eric Blake [EMAIL PROTECTED]
Getline touchups.
* lib/getdelim.c (getdelim): Revert regression that required *n to
be 0 when *lineptr is NULL. Preserve errno across funlockfile.
* m4/getdelim.m4 (gl_FUNC_GETDELIM): Check for declaration of
getdelim, rather than whether implementation is missing.
* m4/getline.m4 (gl_FUNC_GETLINE): Likewise for getline.
* lib/stdio_.h (getline): Also declare if replacement is
required.
* doc/functions/getdelim.texi: New file.
* doc/functions/getline.texi: Likewise.
* doc/gnulib.texi (Function Substitutes): Add new files.
Reported by Bruno Haible.
I almost wrote this:
Even with that patch, errno may still end up being changed inappropriately
when getdelim succeeds yet funlockfile fails. And what about flockfile?
But then I looked it up and saw that those functions are guaranteed
not to set errno. So that's a red herring. Sorry about that.
Here's a patch to remove that unnecessary code, including the
errno = ENOMEM business, assuming that we'll eventually have
malloc/realloc/calloc wrappers that make mingw do the right thing:
[is this ok with you, Simon? ]
Getdelim touchup.
* lib/getdelim.c (getdelim): Don't bother to save/restore errno
around the funlockfile call, since funlockfile never sets errno.
Don't set errno upon failed realloc.
Index: lib/getdelim.c
===
RCS file: /cvsroot/gnulib/gnulib/lib/getdelim.c,v
retrieving revision 1.9
diff -u -p -r1.9 getdelim.c
--- lib/getdelim.c 23 Aug 2007 02:00:19 - 1.9
+++ lib/getdelim.c 23 Aug 2007 06:56:55 -
@@ -58,7 +58,6 @@ getdelim (char **lineptr, size_t *n, int
{
ssize_t result;
size_t cur_len = 0;
- int e; /* Preserve errno across funlockfile. */
if (lineptr == NULL || n == NULL || fp == NULL)
{
@@ -75,7 +74,6 @@ getdelim (char **lineptr, size_t *n, int
if (*lineptr == NULL)
{
result = -1;
- e = ENOMEM;
goto unlock_return;
}
}
@@ -88,7 +86,6 @@ getdelim (char **lineptr, size_t *n, int
if (i == EOF)
{
result = -1;
- e = errno;
break;
}
@@ -105,7 +102,7 @@ getdelim (char **lineptr, size_t *n, int
if (cur_len + 1 = needed)
{
result = -1;
- e = EOVERFLOW;
+ errno = EOVERFLOW;
goto unlock_return;
}
@@ -113,7 +110,6 @@ getdelim (char **lineptr, size_t *n, int
if (new_lineptr == NULL)
{
result = -1;
- e = ENOMEM;
goto unlock_return;
}
@@ -131,8 +127,7 @@ getdelim (char **lineptr, size_t *n, int
result = cur_len ? cur_len : result;
unlock_return:
- funlockfile (fp);
- if (result == -1)
-errno = e;
+ funlockfile (fp); /* doesn't set errno */
+
return result;
}
Re: getline.h
Jim Meyering [EMAIL PROTECTED] writes: [is this ok with you, Simon? ] Yes, please add it. /Simon
Re: Multiply exported Gnulib symbols
Bruno Haible [EMAIL PROTECTED] writes: 3) Two different libraries, say, libguile and libgettextpo, using auxiliary functions from gnulib. It may happen that libguile and libgettextpo both define rpl_strcasecmp, and that this leads to link-time issues. For this we have nothing prepackaged in gnulib-tool, but there is a Makefile snippet that does the necessary #defines, i.e. #define rpl_strcasecmp scm_strcasecmp in guile's config.h, and #define rpl_strcasecmp libgettextpo_strcasecmp in libgettextpo's config.h. You find this code in the 'config.h' rule in http://cvs.savannah.gnu.org/viewvc/gettext/gettext-tools/libgettextpo/Makefile.am?revision=1.13root=gettextview=text Nice, I wasn't aware of that. Would it be possible to merge this into gnulib somehow? I'm running into a similar problem when linking gsasl and gnutls (which both uses gnulib symbols) statically on uClinux. If you know what you are doing, and the gnulib file versions in both libraries are in sync, you could avoid the renaming step, and have both libraries (gsasl+gnutls) use only one set of gnulib symbols. This would reduce the size, which can be important for uClinux systems. This seems more complicated and error-prone, though. /Simon
Re: getline.h
Simon Josefsson [EMAIL PROTECTED] wrote: Jim Meyering [EMAIL PROTECTED] writes: [is this ok with you, Simon? ] Yes, please add it. Committed.
xreadlink: Rewrite as a simple mreadlink wrapper.
I did most of this weeks ago and just noticed I never checked it in.
Ok with you, Bruno?
Rewrite xreadlink as a trivial mreadlink wrapper.
* lib/xreadlink.c (xreadlink): Rewrite as a simple mreadlink wrapper.
* modules/xreadlink: Adjust dependencies.
Index: lib/xreadlink.c
===
RCS file: /cvsroot/gnulib/gnulib/lib/xreadlink.c,v
retrieving revision 1.26
diff -u -p -r1.26 xreadlink.c
--- lib/xreadlink.c 3 Mar 2007 19:20:41 - 1.26
+++ lib/xreadlink.c 23 Aug 2007 11:35:05 -
@@ -25,7 +25,6 @@
/* Specification. */
#include xreadlink.h
-#include stdio.h
#include string.h
#include errno.h
#include limits.h
@@ -40,92 +39,18 @@
# define SSIZE_MAX ((ssize_t) (SIZE_MAX / 2))
#endif
-#ifdef NO_XMALLOC
-# define xmalloc malloc
-#else
-# include xalloc.h
-#endif
+#include mreadlink.h
+#include xalloc.h
-/* Call readlink to get the symbolic link value of FILENAME.
- Return a pointer to that NUL-terminated string in malloc'd storage.
- If readlink fails, return NULL (caller may use errno to diagnose).
- If realloc fails, or if the link value is longer than SIZE_MAX :-),
- give a diagnostic and exit. */
+/* Call mreadlink to get the symbolic link value of FILENAME in malloc'd
+ storage. If mreadlink fails, call xalloc_die. Otherwise, return
+ the malloc'd string. */
char *
xreadlink (char const *filename)
{
- /* The initial buffer size for the link value. A power of 2
- detects arithmetic overflow earlier, but is not required. */
-#define INITIAL_BUF_SIZE 1024
-
- /* Allocate the initial buffer on the stack. This way, in the common
- case of a symlink of small size, we get away with a single small malloc()
- instead of a big malloc() followed by a shrinking realloc(). */
- char initial_buf[INITIAL_BUF_SIZE];
-
- char *buffer = initial_buf;
- size_t buf_size = sizeof (initial_buf);
-
- while (1)
-{
- /* Attempt to read the link into the current buffer. */
- ssize_t link_length = readlink (filename, buffer, buf_size);
-
- /* On AIX 5L v5.3 and HP-UX 11i v2 04/09, readlink returns -1
-with errno == ERANGE if the buffer is too small. */
- if (link_length 0 errno != ERANGE)
- {
- if (buffer != initial_buf)
- {
- int saved_errno = errno;
- free (buffer);
- errno = saved_errno;
- }
- return NULL;
- }
-
- if ((size_t) link_length buf_size)
- {
- buffer[link_length++] = '\0';
-
- /* Return it in a chunk of memory as small as possible. */
- if (buffer == initial_buf)
- {
- buffer = (char *) xmalloc (link_length);
-#ifdef NO_XMALLOC
- if (buffer == NULL)
- return NULL;
-#endif
- memcpy (buffer, initial_buf, link_length);
- }
- else
- {
- /* Shrink buffer before returning it. */
- if ((size_t) link_length buf_size)
- {
- char *smaller_buffer = (char *) realloc (buffer, link_length);
-
- if (smaller_buffer != NULL)
- buffer = smaller_buffer;
- }
- }
- return buffer;
- }
-
- if (buffer != initial_buf)
- free (buffer);
- buf_size *= 2;
- if (SSIZE_MAX buf_size || (SIZE_MAX / 2 SSIZE_MAX buf_size == 0))
-#ifdef NO_XMALLOC
- return NULL;
-#else
- xalloc_die ();
-#endif
- buffer = (char *) xmalloc (buf_size);
-#ifdef NO_XMALLOC
- if (buffer == NULL)
- return NULL;
-#endif
-}
+ char *p = mreadlink (filename);
+ if (p == NULL)
+xalloc_die ();
+ return p;
}
Index: modules/xreadlink
===
RCS file: /cvsroot/gnulib/gnulib/modules/xreadlink,v
retrieving revision 1.15
diff -u -p -r1.15 xreadlink
--- modules/xreadlink 3 Mar 2007 19:20:41 - 1.15
+++ modules/xreadlink 23 Aug 2007 11:35:05 -
@@ -1,15 +1,13 @@
Description:
-Reading symbolic links without size limitation.
+Read symbolic links without size limitation.
Files:
lib/xreadlink.h
lib/xreadlink.c
Depends-on:
-readlink
-ssize_t
-unistd
-xalloc
+mreadlink
+xalloc-die
configure.ac:
getline.h removal fixes
Committed. /Simon Index: ChangeLog === RCS file: /sources/gnulib/gnulib/ChangeLog,v retrieving revision 1.1900 diff -u -p -r1.1900 ChangeLog --- ChangeLog 23 Aug 2007 08:33:16 - 1.1900 +++ ChangeLog 23 Aug 2007 15:44:00 - @@ -1,3 +1,8 @@ +2007-08-23 Simon Josefsson [EMAIL PROTECTED] + + * lib/readline.c: Don't include getline.h, the prototype is now + found in stdio.h. + 2007-08-23 Jim Meyering [EMAIL PROTECTED] Getdelim touchup. Index: lib/readline.c === RCS file: /sources/gnulib/gnulib/lib/readline.c,v retrieving revision 1.6 diff -u -p -r1.6 readline.c --- lib/readline.c 29 Oct 2006 21:52:55 - 1.6 +++ lib/readline.c 23 Aug 2007 15:44:00 - @@ -1,5 +1,5 @@ /* readline.c --- Simple implementation of readline. - Copyright (C) 2005, 2006 Free Software Foundation, Inc. + Copyright (C) 2005, 2006, 2007 Free Software Foundation, Inc. Written by Simon Josefsson This program is free software; you can redistribute it and/or modify @@ -32,7 +32,6 @@ #include stdio.h #include string.h -#include getline.h char * readline (const char *prompt)
adding vasprintf pulls in vasnprintf
Hi! GnuTLS uses vasprintf (in just one place...) so I depend on the vasprintf module. On glibc systems, vasprintf exists and is properly detected: checking for vasprintf... yes However, the vasprintf module depends unconditionally on vasnprintf, see gnulib/modules/vasprintf: Depends-on: vasnprintf On glibc systems, vasnprintf does not exists: checking for vasnprintf... no Since there is an unconditional dependency on vasnprintf in the vasprintf module, even though I do not need vasnprintf, the gnulib-comp.m4 that gnulib-tool generate contains: gl_FUNC_VASNPRINTF This causes vasnprintf.lo to be built, even though I don't need it! [EMAIL PROTECTED]:~/src/gnutls/lgl$ ls *.lo asnprintf.lo gc-libgcrypt.lomd2.lo printf-parse.lo vasnprintf.lo dummy.lo gc-pbkdf2-sha1.lo printf-args.lo read-file.lo [EMAIL PROTECTED]:~/src/gnutls/lgl$ Note that vasprintf.lo is not built. This situation seems sub-optimal. Any ideas on how to solve this? One solution would be for the vasprintf module to not depend on the vasnprintf moulde explicitly, but list all the required files in the Files: section. This is the approach I took for the gc-* modules, where it is simple to handle. However, the vasnprintf module is quite large: Files: lib/float+.h lib/printf-args.h lib/printf-args.c lib/printf-parse.h lib/printf-parse.c lib/vasnprintf.h lib/vasnprintf.c lib/asnprintf.c m4/wchar_t.m4 m4/wint_t.m4 m4/longlong.m4 m4/intmax_t.m4 m4/stdint_h.m4 m4/inttypes_h.m4 m4/eoverflow.m4 m4/vasnprintf.m4 Depends-on: alloca-opt float stdint xsize The files recursively needed by the vasnprintf depends-on modules (i.e., alloca-opt, float, stdint, xsize, and their dependencies recursively) would need to be listed as well: lib/alloca_.h m4/alloca.m4 lib/float_.h m4/float_h.m4 lib/stdint_.h m4/stdint.m4 m4/longlong.m4 m4/ulonglong.m4 lib/xsize.h m4/xsize.m4 m4/size_max.m4 lib/size_max.h m4/include_next.m4 lib/wchar_.h m4/wchar.m4 build-aux/link-warning.h This becomes a maintenance nightmare: every time one of the recursive dependencies of vasnprintf changes, the vasprintf module has to be updated. I'm thus reluctant to propose something like this. Another solution would be for us to add a new statement in modules/* files: 'Depends-on-optionally'. Then the vasprintf module could have this patch: -Depends-on: +Depends-on-optionally: vasnprintf + +Depends-on: stdio Gnulib-tool would do exactly the same as it does today for 'Depends-on', that is pull in all the required modules and their dependencies recursively, however, it would _not_ add the configure.ac statements from the required modules into gnulib-comp.m4. What do you think? I'm not sure this is the best approach to solve it. Still another alternative may be for the modules/vasnprintf file to contain a more complex configure.ac statement, that wouldn't call the vasnprintf M4 function all the time. However, it seems the generated gnulib-comp.m4 is sorted alphanumerically rather than in module-dependency order: gl_FUNC_VASNPRINTF gl_FUNC_VASPRINTF Thus, the vasnprintf module can't know if the vasprintf module will be needed or not, since the latter M4 functions haven't been invoked yet. /Simon
