Re: [perl #46681] [TODO] [C] Use strerror_r instead of strerror
chromatic via RT wrote: On Monday 15 September 2008 23:21:26 Christoph Otto wrote: --- src/pmc/os.pmc (revision 31173) +++ src/pmc/os.pmc (working copy) @@ -31,9 +31,6 @@ #include parrot/parrot.h -/* XXX Check if we need to deallocate strerror strings */ -/* XXX apparently, strerror_r is thread-safe and should be used instead.*/ - static PMC *OS_PMC; pmclass OS singleton { @@ -92,9 +89,8 @@ RETURN(STRING *scwd); } else { -const char * const errmsg = strerror(errno); Parrot_ex_throw_from_c_args(interp, NULL, EXCEPTION_EXTERNAL_ERROR, -errmsg); +%Ss, Parrot_strerror(interp, errno)); } } This pattern's almost common enough to tempt me to write Parrot_ex_throw_strerror(). -- c It was also almost enough to make me write Parrot_strerror. This patch contains a file that I neglected to include with the previous one. Thanks to Andy Dougherty for catching that. Index: src/ops/sys.ops === --- src/ops/sys.ops (revision 31174) +++ src/ops/sys.ops (working copy) @@ -66,13 +66,11 @@ } op err(out STR) { - const char * const tmp = strerror(errno); - $1 = string_make(interp, tmp, strlen(tmp), ascii, 0); + $1 = Parrot_strerror(interp, errno); } op err(out STR, in INT) { - const char * const tmp = strerror($2); - $1 = string_make(interp, tmp, strlen(tmp), ascii, 0); + $1 = Parrot_strerror(interp, $2); } Index: src/pmc/os.pmc === --- src/pmc/os.pmc (revision 31174) +++ src/pmc/os.pmc (working copy) @@ -31,9 +31,6 @@ #include parrot/parrot.h -/* XXX Check if we need to deallocate strerror strings */ -/* XXX apparently, strerror_r is thread-safe and should be used instead.*/ - static PMC *OS_PMC; pmclass OS singleton { @@ -92,9 +89,8 @@ RETURN(STRING *scwd); } else { -const char * const errmsg = strerror(errno); Parrot_ex_throw_from_c_args(interp, NULL, EXCEPTION_EXTERNAL_ERROR, -errmsg); +%Ss, Parrot_strerror(interp, errno)); } } @@ -118,9 +114,8 @@ #endif string_cstring_free(cpath); if (error) { -char *errmsg = strerror(errno); Parrot_ex_throw_from_c_args(interp, NULL, EXCEPTION_EXTERNAL_ERROR, -errmsg); +%Ss, Parrot_strerror(interp, errno)); } } @@ -141,10 +136,9 @@ int error = stat(cpath, info); if (error) { -const char * const errmsg = strerror(errno); string_cstring_free(cpath); Parrot_ex_throw_from_c_args(interp, NULL, EXCEPTION_EXTERNAL_ERROR, -errmsg); +%Ss, Parrot_strerror(interp, errno)); } if (S_ISDIR(info.st_mode)) { @@ -155,18 +149,16 @@ #endif string_cstring_free(cpath); if (error) { -const char * const errmsg = strerror(errno); Parrot_ex_throw_from_c_args(interp, NULL, EXCEPTION_EXTERNAL_ERROR, -errmsg); +%Ss, Parrot_strerror(interp, errno)); } } else { error = remove(cpath); string_cstring_free(cpath); if (error) { -const char * const errmsg = strerror(errno); Parrot_ex_throw_from_c_args(interp, NULL, EXCEPTION_EXTERNAL_ERROR, -errmsg); +%Ss, Parrot_strerror(interp, errno)); } } } @@ -192,9 +184,8 @@ #endif string_cstring_free(cpath); if (error) { -const char * const errmsg = strerror(errno); Parrot_ex_throw_from_c_args(interp, NULL, EXCEPTION_EXTERNAL_ERROR, -errmsg); +%Ss, Parrot_strerror(interp, errno)); } } @@ -231,9 +222,8 @@ string_cstring_free(cpath); if (error) { -const char * const errmsg = strerror(errno); Parrot_ex_throw_from_c_args(interp, NULL, EXCEPTION_EXTERNAL_ERROR, -errmsg); +%Ss, Parrot_strerror(interp, errno)); } else { PMC * const array = pmc_new(interp, enum_class_FixedPMCArray); @@ -296,9 +286,8 @@ string_cstring_free(cpath); if (error) { -const char * const errmsg = strerror(errno); Parrot_ex_throw_from_c_args(interp, NULL, EXCEPTION_EXTERNAL_ERROR, -errmsg); +%Ss, Parrot_strerror(interp, errno)); } else { PMC * const array = pmc_new(interp, enum_class_FixedPMCArray); @@ -343,9 +332,8 @@ string_cstring_free(cto); if (error) { -const char * const errmsg = strerror(errno);
Re: [perl #46681] [TODO] [C] Use strerror_r instead of strerror
I think you've gone about this in the right way, but need to handle the various cases a bit more broadly. Specifically, you can't assume that everyone has strerror() at all (Solaris 8 doesn't, for example) nor that everyone who uses int strerror_r() will also define either _XOPEN_SOURCE or _POSIX_C_SOURCE (NetBSD doesn't, for example). I think perhaps a structure like the following might work: #if defined(HAS_STRERROR_R) # if defined(STRERROR_R_RETURNS_INT) /* stuff using int strerror_r(); */ # else /* stuff using char *strerror_r() */ #else /* stuff using plain strerror(). */ #endif Currently, you are trying to guess which branch based on various #defines, but non-Linux systems will fall through to the wrong branch, and systems without strerror_r will fail completely. Ideally, Configure.pl ought to figure which branch you need. Alas, it doesn't. However, perl5's Configure does (at least for Unix systems), so you could just use it's results. -- Andy Dougherty [EMAIL PROTECTED]
Re: [perl #46681] [TODO] [C] Use strerror_r instead of strerror
After looking around Parrot and realizing that there were several other places where strerror is used, I'm trying a different approach. The attached patch adds config/gen/platform/generic/strerror.c, which contains a new function Parrot_strerror. This is just a wrapper to abstract away the non-reentrancy and platform-specific semantics of strerror. After a reconfigure, Parrot builds successfully and all tests pass on Ubuntu/x86. I would like comments on whether the patch puts its code in the correct places and if it's a generally good idea. If both these are true, I'll go ahead testing on other platforms and converting other instances of strerror. Christoph Index: src/pmc/file.pmc === --- src/pmc/file.pmc (revision 31046) +++ src/pmc/file.pmc (working copy) @@ -22,9 +22,15 @@ # include direct.h #endif +#define ERRBUF_SIZE 128 + #include parrot/parrot.h -/* RT#46681 apparently, strerror_r is thread-safe and should be used instead.*/ +# define THROW_FILE_EXCEPTION(interp) \ +{ \ +STRING *errstr = Parrot_strerror((interp), errno); \ +Parrot_ex_throw_from_c_args((interp), NULL, EXCEPTION_LIBRARY_ERROR, %Ss, errstr); \ +} static PMC *File_PMC; pmclass File singleton { @@ -73,9 +79,14 @@ #endif string_cstring_free(cpath); -if (error) +if (error errno == ENOENT) { RETURN(INTVAL 0); +} +else if (error) { +THROW_FILE_EXCEPTION(INTERP); +} + RETURN(INTVAL 1); } @@ -89,6 +100,7 @@ */ + METHOD is_dir(STRING *path) { struct stat info; char *cpath = string_to_cstring(interp, path); @@ -100,9 +112,7 @@ string_cstring_free(cpath); if (error) { -char *errmsg = strerror(errno); -Parrot_ex_throw_from_c_args(interp, NULL, EXCEPTION_EXTERNAL_ERROR, -errmsg); +THROW_FILE_EXCEPTION(INTERP); } if (S_ISDIR(info.st_mode)) @@ -132,9 +142,7 @@ string_cstring_free(cpath); if (error) { -char *errmsg = strerror(errno); -Parrot_ex_throw_from_c_args(interp, NULL, EXCEPTION_EXTERNAL_ERROR, -errmsg); +THROW_FILE_EXCEPTION(INTERP); } if (S_ISREG(info.st_mode)) @@ -155,20 +163,20 @@ METHOD is_link(STRING *path) { #ifdef WIN32 -/* I love win32 implementations */ +/* I love win32. */ RETURN(INTVAL 0); #else struct stat info; +char *cpath; +int error; -char *cpath = string_to_cstring(interp, path); -int error = lstat(cpath, info); +cpath = string_to_cstring(interp, path); +error = lstat(cpath, info); string_cstring_free(cpath); if (error) { -char *errmsg = strerror(errno); -Parrot_ex_throw_from_c_args(interp, NULL, EXCEPTION_EXTERNAL_ERROR, -errmsg); +THROW_FILE_EXCEPTION(INTERP); } if (S_ISLNK(info.st_mode)) @@ -197,11 +205,13 @@ METHOD copy(STRING *from, STRING *to) { #define CHUNK_SIZE 1024 +char errmsg[ERRBUF_SIZE]; char *cfrom = string_to_cstring(interp, from); FILE *source = fopen(cfrom, rb); string_cstring_free(cfrom); + if (source) { char *cto= string_to_cstring(interp, to); FILE *target = fopen(cto, w+b); @@ -226,16 +236,12 @@ fclose(target); } else { -char *errmsg = strerror(errno); -Parrot_ex_throw_from_c_args(interp, NULL, EXCEPTION_EXTERNAL_ERROR, -errmsg); +THROW_FILE_EXCEPTION(INTERP); } fclose(source); } else { -char *errmsg = strerror(errno); -Parrot_ex_throw_from_c_args(interp, NULL, EXCEPTION_EXTERNAL_ERROR, -errmsg); +THROW_FILE_EXCEPTION(INTERP); } #undef CHUNK_SIZE } @@ -259,9 +265,7 @@ string_cstring_free(cto); if (error) { -char *errmsg = strerror(errno); -Parrot_ex_throw_from_c_args(interp, NULL, EXCEPTION_EXTERNAL_ERROR, -errmsg); +THROW_FILE_EXCEPTION(INTERP); } } } Index: config/gen/platform/win32/string.h === --- config/gen/platform/win32/string.h (revision 31046) +++ config/gen/platform/win32/string.h (working copy) @@ -12,6 +12,8 @@ # if _MSC_VER = 1400 #define strdup _strdup # endif +/* This is only useful with msvc. gcc/mingw can use strerror_r. */ +# define strerror_r(errnum, buf, buflen) strerror_s((buf), (buflen), (errnum)) #endif #endif /* PARROT_PLATFORM_WIN32_STRING_H_GUARD */ Index: config/gen/platform/platform_interface.h
Re: [perl #46681] [TODO] [C] Use strerror_r instead of strerror
Will Coleda via RT wrote: On Tue Jul 22 23:34:13 2008, [EMAIL PROTECTED] wrote: Christoph Otto via RT wrote: This version of the patch should dtrt with all versions of strerror_r. It works on my Debian/x86 box and I'll be testing it on any *nix I can get my hands on Tuesday. If it works fine there, if someone can test it on windows and if the patch looks OK, I'll commit it and close this ticket. Christoph This patch contains a fix and a simplification. It should now be cross-platform and thread-safe. I'll test on some other *nixes and go on from there. If nothing else it works fine on Ubuntu/x86. Based on Christoph's instructions in #parrot, I tested this using an MSVC compiler, with the following results: snip explodey brokenness Here's v8 of the patch. After poking around on gcc/win, msvc and gcc/linux I'm pretty sure this will at least pass all tests on all those platforms and anything else that has a sufficiently POSIXy strerror_r. Unfortunately it seems that strerror_s (MS' strerror_r) isn't available to gcc on windows, so that platform may be stuck with a non-thread-safe file PMC. An updated patch proving me wrong would be welcome. It'd be good to get this patch reviewed before it's applied, but the main goal is to get all tests passing on Linux/gcc, msvc and win/gcc. Index: src/pmc/file.pmc === --- src/pmc/file.pmc (revision 30979) +++ src/pmc/file.pmc (working copy) @@ -24,8 +24,56 @@ #include parrot/parrot.h -/* RT#46681 apparently, strerror_r is thread-safe and should be used instead.*/ +/* strerror_r should truncate the message if it's too long for the supplied + * buffer. It's probably best to just specify a sane default buffer size than + * to worry about retrying calls. */ +#define ERRBUF_SIZE 128 +/* use POSIXy strerror_r (*nix) or strerror_s macro (msvc)*/ +#if (!defined(WIN32) !defined(_GNU_SOURCE)) || \ +(defined(_MSC_VER) defined(WIN32)) +# define THROW_FILE_EXCEPTION(interp, func) \ +{ \ +char errmsg[ERRBUF_SIZE]; \ +int err_status; \ +err_status = strerror_r(errno, errmsg, ERRBUF_SIZE); \ +/* Linux's POSIXy strerror_r returns -1 on error, others return an error code */ \ +if (err_status == -1) \ +err_status = errno; \ +\ +if (err_status == 0 || err_status == ERANGE) { \ +Parrot_ex_throw_from_c_args(\ +(interp), NULL, EXCEPTION_LIBRARY_ERROR, %s, errmsg); \ +} \ +else if (err_status == EINVAL){ \ +Parrot_ex_throw_from_c_args((interp), NULL, EXCEPTION_LIBRARY_ERROR, \ +%s returned an invalid error code (%d), #func, errno); \ +} \ +else { \ +Parrot_ex_throw_from_c_args((interp), NULL, EXCEPTION_LIBRARY_ERROR, \ +strerror_r() returned an unknown error code: %d, err_status); \ +} \ +} + +/* use GNU-specific strerror_r */ +#elif defined(_GNU_SOURCE) +# define THROW_FILE_EXCEPTION(interp, func) \ +{ \ +/* GNU strerror_r DTRT for unknown error codes */ \ +char errmsg[ERRBUF_SIZE]; \ +char *errstr = strerror_r(errno, errmsg, ERRBUF_SIZE); \ +Parrot_ex_throw_from_c_args((interp), NULL, EXCEPTION_LIBRARY_ERROR, %s, errstr); \ +} + +/* using gcc on windows, so we're stuck with plain strerror */ +#else +# define THROW_FILE_EXCEPTION(interp, func) \ +{ \ +char *errmsg = strerror(errno); \ +Parrot_ex_throw_from_c_args((interp), NULL, EXCEPTION_LIBRARY_ERROR, %s,errmsg); \ +} +#endif + static PMC *File_PMC; pmclass File singleton { @@ -73,9 +121,18 @@ #endif string_cstring_free(cpath); -if (error) +if (error errno == ENOENT) { RETURN(INTVAL 0); +} +else if (error) { +#ifdef WIN32 +THROW_FILE_EXCEPTION(INTERP, stat); +#else +THROW_FILE_EXCEPTION(INTERP, lstat); +#endif +} + RETURN(INTVAL 1); } @@ -89,6 +146,7 @@ */ + METHOD is_dir(STRING *path) { struct stat info; char *cpath = string_to_cstring(interp, path); @@ -100,9 +158,11 @@ string_cstring_free(cpath); if (error) { -char *errmsg = strerror(errno); -Parrot_ex_throw_from_c_args(interp, NULL, EXCEPTION_EXTERNAL_ERROR, -errmsg); +#ifdef WIN32 +THROW_FILE_EXCEPTION(INTERP, stat); +#else +THROW_FILE_EXCEPTION(INTERP, lstat); +#endif } if (S_ISDIR(info.st_mode)) @@ -132,9 +192,11 @@ string_cstring_free(cpath); if (error) { -char *errmsg = strerror(errno); -Parrot_ex_throw_from_c_args(interp, NULL, EXCEPTION_EXTERNAL_ERROR, -errmsg); +#ifdef WIN32 +THROW_FILE_EXCEPTION(INTERP, stat); +#else +THROW_FILE_EXCEPTION(INTERP,
Re: [perl #46681] [TODO] [C] Use strerror_r instead of strerror
Christoph Otto via RT wrote: This version of the patch should dtrt with all versions of strerror_r. It works on my Debian/x86 box and I'll be testing it on any *nix I can get my hands on Tuesday. If it works fine there, if someone can test it on windows and if the patch looks OK, I'll commit it and close this ticket. Christoph This patch contains a fix and a simplification. It should now be cross-platform and thread-safe. I'll test on some other *nixes and go on from there. If nothing else it works fine on Ubuntu/x86. Index: src/pmc/file.pmc === --- src/pmc/file.pmc (revision 29691) +++ src/pmc/file.pmc (working copy) @@ -24,8 +24,50 @@ #include parrot/parrot.h -/* RT#46681 apparently, strerror_r is thread-safe and should be used instead.*/ +/* strerror_r should truncate the message if it's too long for the supplied + * buffer. It's probably best to just specify a sane default buffer size than + * to worry about retrying calls. */ +#define ERRBUF_SIZE 128 +#ifndef _GNU_SOURCE +/* use POSIXy strerror_r */ +# define STRERROR_R_EXCEPTION(interp, func) \ +{ \ +char errmsg[ERRBUF_SIZE]; \ +int err_status; \ +err_status = strerror_r(errno, errmsg, ERRBUF_SIZE); \ +/* Linux's POSIXy strerror_r returns -1 on error, others return an error code */ \ +if (err_status == -1) \ +err_status = errno; \ +\ +if (err_status == 0 || err_status == ERANGE) { \ +STRING *errmsg_pstring = string_make((interp), errmsg, strlen(errmsg), NULL, 0); \ +real_exception((interp), NULL, E_SystemError, %Ss, errmsg_pstring); \ +} \ +else if (err_status == EINVAL){ \ +real_exception((interp), NULL, E_SystemError, \ +%s returned an invalid error code (%d), #func, errno); \ +} \ +else { \ +real_exception((interp), NULL, E_SystemError, \ +strerror_r() returned an unknown error code: %d, err_status); \ +} \ +} +#else +/* use GNU-specific strerror_r */ +# define STRERROR_R_EXCEPTION(interp, func) \ +{ \ +/* GNU strerror_r DTRT for unknown error codes */ \ +char errmsg[ERRBUF_SIZE]; \ +char *errstr = strerror_r(errno, errmsg, ERRBUF_SIZE); \ +STRING *errmsg_pstring = string_make((interp), errstr, strlen(errstr), NULL, 0); \ +real_exception((interp), NULL, E_SystemError, %Ss, errmsg_pstring); \ +} +#endif + + + + static PMC *File_PMC; pmclass File singleton { @@ -89,6 +131,7 @@ */ + METHOD is_dir(STRING *path) { struct stat info; char *cpath = string_to_cstring(interp, path); @@ -100,8 +143,11 @@ string_cstring_free(cpath); if (error) { -char *errmsg = strerror(errno); -real_exception(interp, NULL, E_SystemError, errmsg); +#ifdef WIN32 +STRERROR_R_EXCEPTION(INTERP, stat); +#else +STRERROR_R_EXCEPTION(INTERP, lstat); +#endif } if (S_ISDIR(info.st_mode)) @@ -131,8 +177,11 @@ string_cstring_free(cpath); if (error) { -char *errmsg = strerror(errno); -real_exception(interp, NULL, E_SystemError, errmsg); +#ifdef WIN32 +STRERROR_R_EXCEPTION(INTERP, stat); +#else +STRERROR_R_EXCEPTION(INTERP, lstat); +#endif } if (S_ISREG(info.st_mode)) @@ -164,8 +213,7 @@ string_cstring_free(cpath); if (error) { -char *errmsg = strerror(errno); -real_exception(interp, NULL, E_SystemError, errmsg); +STRERROR_R_EXCEPTION(INTERP, lstat); } if (S_ISLNK(info.st_mode)) @@ -199,6 +247,8 @@ string_cstring_free(cfrom); +char errmsg[ERRBUF_SIZE]; + if (source) { char *cto= string_to_cstring(interp, to); FILE *target = fopen(cto, w+b); @@ -223,14 +273,12 @@ fclose(target); } else { -char *errmsg = strerror(errno); -real_exception(interp, NULL, E_SystemError, errmsg); +STRERROR_R_EXCEPTION(INTERP, fopen); } fclose(source); } else { -char *errmsg = strerror(errno); -real_exception(interp, NULL, E_SystemError, errmsg); +STRERROR_R_EXCEPTION(INTERP, fopen); } #undef CHUNK_SIZE } @@ -254,8 +302,7 @@ string_cstring_free(cto); if (error) { -char *errmsg = strerror(errno); -real_exception(interp, NULL, E_SystemError, errmsg); +STRERROR_R_EXCEPTION(INTERP, rename); } } } Index: config/gen/platform/win32/string.h === --- config/gen/platform/win32/string.h (revision 29691) +++
[perl #46681] [TODO] [C] Use strerror_r instead of strerror
On Tue Jul 22 23:34:13 2008, [EMAIL PROTECTED] wrote: Christoph Otto via RT wrote: This version of the patch should dtrt with all versions of strerror_r. It works on my Debian/x86 box and I'll be testing it on any *nix I can get my hands on Tuesday. If it works fine there, if someone can test it on windows and if the patch looks OK, I'll commit it and close this ticket. Christoph This patch contains a fix and a simplification. It should now be cross-platform and thread-safe. I'll test on some other *nixes and go on from there. If nothing else it works fine on Ubuntu/x86. Based on Christoph's instructions in #parrot, I tested this using an MSVC compiler, with the following results: C:\research\parrotnmake Microsoft (R) Program Maintenance Utility Version 8.00.50727.42 Copyright (C) Microsoft Corporation. All rights reserved. Compiling with: xx.c cl -I.\include -nologo -GF -W4 -MD -Zi -DNDEBUG -DWIN32 -D_CONSOLE -DNO_STRICT - DUSE_SITECUSTOMIZE -DPRIVLIB_LAST_IN_INC -D_CRT_SECURE_NO_DEPRECATE - DHASATTRIBU TE_DEPRECATED -wd4101 -DHASATTRIBUTE_NORETURN -wd4101 -Zi -wd4127 -wd4054 - wd431 0 -DHAS_JIT -DI386 -I. -Fo xx.obj -c xx.c C:\Perl\bin\perl.exe tools\build\pmc2c.pl --dump src\pmc\file.pmc C:\Perl\bin\perl.exe tools\build\pmc2c.pl --c src\pmc\file.pmc C:\Perl\bin\perl.exe tools\build\c2str.pl src\pmc\file.c src\pmc\file. str C:\Perl\bin\perl.exe tools\build\c2str.pl --all src\string.c src\pmc\file.c .\src\pmc\file.pmc(169) : warning C4189: 'pmc' : local variable is initialized b ut not referenced .\src\pmc\file.pmc(147) : warning C4013: 'strerror_r' undefined; assuming extern returning int .\src\pmc\file.pmc(318) : warning C4189: 'pmc' : local variable is initialized b ut not referenced .\src\pmc\file.pmc(475) : warning C4189: 'pmc' : local variable is initialized b ut not referenced .\src\pmc\file.pmc(632) : warning C4189: 'pmc' : local variable is initialized b ut not referenced .\src\pmc\file.pmc(633) : warning C4189: 'path' : local variable is initialized but not referenced .\src\pmc\file.pmc(250) : error C2143: syntax error : missing ';' before 'type' NMAKE : fatal error U1077: 'C:\Perl\bin\perl.exe' : return code '0x2' Stop. -- Will Coke Coleda
[perl #46681] [TODO] [C] Use strerror_r instead of strerror
On Tue Jul 22 23:34:13 2008, [EMAIL PROTECTED] wrote: This patch contains a fix and a simplification. It should now be cross-platform and thread-safe. I'll test on some other *nixes and go on from there. If nothing else it works fine on Ubuntu/x86. It also works in FreeBSD 7.0 and OpenBSD 4.3. If someone can confirm that it's OK on windows (msvc or cygwin) and that the patch otherwise looks good, it can be committed and this ticket resolved.
Re: [perl #46681] [TODO] [C] Use strerror_r instead of strerror
Andy Dougherty via RT wrote: On Thu, 17 Jul 2008, Christoph Otto via RT wrote: On Thu Jul 17 15:53:12 2008, julianalbo wrote: On Thu, Jul 17, 2008 at 9:59 PM, Christoph Otto via RT [EMAIL PROTECTED] wrote: trick. The attached patch (v5) properly fixes the problem on my system. There shouldn't be any remaining issues, but the patch ought to be tested on a another *nix and Windows. [ . . . ] char *errstr = strerror_r(errno, errmsg, ERRBUF_SIZE); Unfortunately, that's not portable. The POSIXy version of strerror_r returns an integer, specifically 0 if successful, and something else otherwise. (What that something else actually is seems to be defined differently by different vendors, alas. OpenBSD and Solaris return errno. Linux/glibc returns -1.) Thanks for pointing that out. After what it's taking to get this to work, I'm looking forward to my next patch being silently ignored. I suppose there's a reason my spell checker wanted to turn strerror to terrorist. This version of the patch should dtrt with all versions of strerror_r. It works on my Debian/x86 box and I'll be testing it on any *nix I can get my hands on Tuesday. If it works fine there, if someone can test it on windows and if the patch looks OK, I'll commit it and close this ticket. Christoph Index: src/pmc/file.pmc === --- src/pmc/file.pmc (revision 29667) +++ src/pmc/file.pmc (working copy) @@ -24,8 +24,50 @@ #include parrot/parrot.h -/* RT#46681 apparently, strerror_r is thread-safe and should be used instead.*/ +/* strerror_r should truncate the message if it's too long for the supplied + * buffer. It's probably best to just specify a sane default buffer size than + * to worry about retrying calls. */ +#define ERRBUF_SIZE 128 +/* feature test macro taken from Ubuntu's strerror man page */ +#if (_POSIX_C_SOURCE = 200112L || _XOPEN_SOURCE = 600) ! _GNU_SOURCE +/* use POSIXy strerror_r */ +# define STRERROR_R_EXCEPTION(interp, func) \ +{ \ +char errmsg[ERRBUF_SIZE]; \ +int err_status; \ +err_status = strerror_r(errno, errmsg, ERRBUF_SIZE); \ +/* Linux's POSIXy strerror_r returns -1 on error, others return an error code */ \ +if (err_status == -1) \ +err_status = errno; \ +\ +if (err_status == 0 || err_status == ERANGE) { \ +STRING *errmsg_pstring = string_make((interp), errmsg, strlen(errmsg), NULL, 0); \ +real_exception((interp), NULL, E_SystemError, %Ss, errmsg_pstring); \ +} \ +else if (err_status == EINVAL){ \ +real_exception((interp), NULL, E_SystemError, \ +%s returned an invalid error code (%d), #func, errno); \ +else { \ +real_exception((interp), NULL, E_SystemError, \ +strerror_r() returned an unknown error code: %d, err_status); \ +} \ +} +#else +/* use GNU-specific strerror_r */ +# define STRERROR_R_EXCEPTION(interp, func) \ +{ \ +/* GNU strerror_r DTRT for unknown error codes */ \ +char errmsg[ERRBUF_SIZE]; \ +char *errstr = strerror_r(errno, errmsg, ERRBUF_SIZE); \ +STRING *errmsg_pstring = string_make((interp), errstr, strlen(errstr), NULL, 0); \ +real_exception((interp), NULL, E_SystemError, %Ss, errmsg_pstring); \ +} +#endif + + + + static PMC *File_PMC; pmclass File singleton { @@ -89,6 +131,7 @@ */ + METHOD is_dir(STRING *path) { struct stat info; char *cpath = string_to_cstring(interp, path); @@ -100,8 +143,11 @@ string_cstring_free(cpath); if (error) { -char *errmsg = strerror(errno); -real_exception(interp, NULL, E_SystemError, errmsg); +#ifdef WIN32 +STRERROR_R_EXCEPTION(INTERP, stat); +#else +STRERROR_R_EXCEPTION(INTERP, lstat); +#endif } if (S_ISDIR(info.st_mode)) @@ -131,8 +177,11 @@ string_cstring_free(cpath); if (error) { -char *errmsg = strerror(errno); -real_exception(interp, NULL, E_SystemError, errmsg); +#ifdef WIN32 +STRERROR_R_EXCEPTION(INTERP, stat); +#else +STRERROR_R_EXCEPTION(INTERP, lstat); +#endif } if (S_ISREG(info.st_mode)) @@ -164,8 +213,7 @@ string_cstring_free(cpath); if (error) { -char *errmsg = strerror(errno); -real_exception(interp, NULL, E_SystemError, errmsg); +STRERROR_R_EXCEPTION(INTERP, lstat); } if (S_ISLNK(info.st_mode)) @@ -199,6 +247,8 @@ string_cstring_free(cfrom); +char errmsg[ERRBUF_SIZE]; + if (source) { char *cto= string_to_cstring(interp, to); FILE *target = fopen(cto, w+b); @@ -223,14 +273,12 @@ fclose(target); } else { -
Re: [perl #46681] [TODO] [C] Use strerror_r instead of strerror
On Thursday 17 July 2008 15:44:36 NotFound wrote: [EMAIL PROTECTED] wrote: With this patch, the new tests still pass on Linux/x86. The patch uses STRING-strstart to avoid leaking a malloc'd buffer when throwing an exception, which may or may not be considered kosher in this situation. Can't you use real_exception(interp, NULL, E_SystemError, %Ss, errmsg_pstring); ? Using internal details of parrot strings must be avoided. NotFound's solution is correct; that's the way to go here. -- c
[perl #46681] [TODO] [C] Use strerror_r instead of strerror
On Thu Jul 17 15:53:12 2008, julianalbo wrote: On Thu, Jul 17, 2008 at 9:59 PM, Christoph Otto via RT [EMAIL PROTECTED] wrote: With this patch, the new tests still pass on Linux/x86. The patch uses STRING-strstart to avoid leaking a malloc'd buffer when throwing an exception, which may or may not be considered kosher in this situation. Can't you use real_exception(interp, NULL, E_SystemError, %Ss, errmsg_pstring); ? Using internal details of parrot strings must be avoided. I agree. I just didn't know real_exception was smart enough to do that trick. The attached patch (v5) properly fixes the problem on my system. There shouldn't be any remaining issues, but the patch ought to be tested on a another *nix and Windows. Index: src/pmc/file.pmc === --- src/pmc/file.pmc (revision 29551) +++ src/pmc/file.pmc (working copy) @@ -26,6 +26,21 @@ /* RT#46681 apparently, strerror_r is thread-safe and should be used instead.*/ +/* strerror_r should truncate the message if its too long for the supplied buffer + * it's probably best to just specify a sane default buffer size than to worry + * about retrying calls + */ +#define ERRBUF_SIZE 128 + +#define STRERROR_R_EXCEPTION() \ +{ \ +char errmsg[ERRBUF_SIZE]; \ +char *errstr = strerror_r(errno, errmsg, ERRBUF_SIZE); \ +STRING *errmsg_pstring = string_make(interp, errstr, strlen(errstr), NULL, 0); \ +real_exception(interp, NULL, E_SystemError, %Ss, errmsg_pstring); \ +} + + static PMC *File_PMC; pmclass File singleton { @@ -89,6 +104,7 @@ */ + METHOD is_dir(STRING *path) { struct stat info; char *cpath = string_to_cstring(interp, path); @@ -100,8 +116,7 @@ string_cstring_free(cpath); if (error) { -char *errmsg = strerror(errno); -real_exception(interp, NULL, E_SystemError, errmsg); +STRERROR_R_EXCEPTION(); } if (S_ISDIR(info.st_mode)) @@ -131,8 +146,7 @@ string_cstring_free(cpath); if (error) { -char *errmsg = strerror(errno); -real_exception(interp, NULL, E_SystemError, errmsg); +STRERROR_R_EXCEPTION(); } if (S_ISREG(info.st_mode)) @@ -164,8 +178,7 @@ string_cstring_free(cpath); if (error) { -char *errmsg = strerror(errno); -real_exception(interp, NULL, E_SystemError, errmsg); +STRERROR_R_EXCEPTION(); } if (S_ISLNK(info.st_mode)) @@ -199,6 +212,8 @@ string_cstring_free(cfrom); +char errmsg[ERRBUF_SIZE]; + if (source) { char *cto= string_to_cstring(interp, to); FILE *target = fopen(cto, w+b); @@ -223,14 +238,12 @@ fclose(target); } else { -char *errmsg = strerror(errno); -real_exception(interp, NULL, E_SystemError, errmsg); +STRERROR_R_EXCEPTION(); } fclose(source); } else { -char *errmsg = strerror(errno); -real_exception(interp, NULL, E_SystemError, errmsg); +STRERROR_R_EXCEPTION(); } #undef CHUNK_SIZE } @@ -254,8 +267,7 @@ string_cstring_free(cto); if (error) { -char *errmsg = strerror(errno); -real_exception(interp, NULL, E_SystemError, errmsg); +STRERROR_R_EXCEPTION(); } } } Index: config/gen/platform/win32/string.h === --- config/gen/platform/win32/string.h (revision 29551) +++ config/gen/platform/win32/string.h (working copy) @@ -14,6 +14,8 @@ # endif #endif +#define strerror_r(errnum, buf, buflen) strerror_s((buf), (buflen), (errnum)) + #endif /* PARROT_PLATFORM_WIN32_STRING_H_GUARD */ /*
Re: [perl #46681] [TODO] [C] Use strerror_r instead of strerror
On Thu, Jul 17, 2008 at 7:44 PM, Christoph Otto via RT [EMAIL PROTECTED] wrote: On Thu Jul 17 15:53:12 2008, julianalbo wrote: On Thu, Jul 17, 2008 at 9:59 PM, Christoph Otto via RT [EMAIL PROTECTED] wrote: With this patch, the new tests still pass on Linux/x86. The patch uses STRING-strstart to avoid leaking a malloc'd buffer when throwing an exception, which may or may not be considered kosher in this situation. Can't you use real_exception(interp, NULL, E_SystemError, %Ss, errmsg_pstring); ? Using internal details of parrot strings must be avoided. I agree. I just didn't know real_exception was smart enough to do that trick. The attached patch (v5) properly fixes the problem on my system. There shouldn't be any remaining issues, but the patch ought to be tested on a another *nix and Windows. iirc we shouldn't be assuming interp in macros (it should always be passed). i could be wrong, so i've copied the man with the feathered broom for an official ruling. ~jerry
Re: [perl #46681] [TODO] [C] Use strerror_r instead of strerror
On Thu, 17 Jul 2008, Christoph Otto via RT wrote: On Thu Jul 17 15:53:12 2008, julianalbo wrote: On Thu, Jul 17, 2008 at 9:59 PM, Christoph Otto via RT [EMAIL PROTECTED] wrote: trick. The attached patch (v5) properly fixes the problem on my system. There shouldn't be any remaining issues, but the patch ought to be tested on a another *nix and Windows. [ . . . ] char *errstr = strerror_r(errno, errmsg, ERRBUF_SIZE); Unfortunately, that's not portable. The POSIXy version of strerror_r returns an integer, specifically 0 if successful, and something else otherwise. (What that something else actually is seems to be defined differently by different vendors, alas. OpenBSD and Solaris return errno. Linux/glibc returns -1.) -- Andy Dougherty [EMAIL PROTECTED]
[perl #46681] [TODO] [C] Use strerror_r instead of strerror
On Wed Apr 23 18:18:00 2008, [EMAIL PROTECTED] wrote: This thread trailed off about 4 months ago. Could we get an update on its status, i.e., whether it should be applied, what OSes it's passing on, etc. Thank you very much. kid51 The tests passed because the strerror/strerror_r code doesn't have any coverage. I added some tests that make sure the error message consists only of alphanumerics and whitespace in r29551. The tests pass with the current src/pmc/file.pmc, but fail with the attached patch. I don't have the tuits to fix the patch, but at least now it's obvious that it was broken.
[perl #46681] [TODO] [C] Use strerror_r instead of strerror
On Thu Jul 17 01:17:51 2008, cotto wrote: On Wed Apr 23 18:18:00 2008, [EMAIL PROTECTED] wrote: This thread trailed off about 4 months ago. Could we get an update on its status, i.e., whether it should be applied, what OSes it's passing on, etc. Thank you very much. kid51 The tests passed because the strerror/strerror_r code doesn't have any coverage. I added some tests that make sure the error message consists only of alphanumerics and whitespace in r29551. The tests pass with the current src/pmc/file.pmc, but fail with the attached patch. I don't have the tuits to fix the patch, but at least now it's obvious that it was broken. With this patch, the new tests still pass on Linux/x86. The patch uses STRING-strstart to avoid leaking a malloc'd buffer when throwing an exception, which may or may not be considered kosher in this situation. If this is judged appropriate, I'd appreciate test confirmation from at least OSX/x86 and Windows since strerror_r seems to be a special function. Index: src/pmc/file.pmc === --- src/pmc/file.pmc (revision 29551) +++ src/pmc/file.pmc (working copy) @@ -26,6 +26,21 @@ /* RT#46681 apparently, strerror_r is thread-safe and should be used instead.*/ +/* strerror_r should truncate the message if its too long for the supplied buffer + * it's probably best to just specify a sane default buffer size than to worry + * about retrying calls + */ +#define ERRBUF_SIZE 128 + +#define STRERROR_R_EXCEPTION() \ +{ \ +char errmsg[ERRBUF_SIZE]; \ +char *errstr = strerror_r(errno, errmsg, ERRBUF_SIZE); \ +STRING *errmsg_pstring = string_make(interp, errstr, strlen(errstr), NULL, 0); \ +real_exception(interp, NULL, E_SystemError, errmsg_pstring-strstart); \ +} + + static PMC *File_PMC; pmclass File singleton { @@ -89,6 +104,7 @@ */ + METHOD is_dir(STRING *path) { struct stat info; char *cpath = string_to_cstring(interp, path); @@ -100,8 +116,7 @@ string_cstring_free(cpath); if (error) { -char *errmsg = strerror(errno); -real_exception(interp, NULL, E_SystemError, errmsg); +STRERROR_R_EXCEPTION(); } if (S_ISDIR(info.st_mode)) @@ -131,8 +146,7 @@ string_cstring_free(cpath); if (error) { -char *errmsg = strerror(errno); -real_exception(interp, NULL, E_SystemError, errmsg); +STRERROR_R_EXCEPTION(); } if (S_ISREG(info.st_mode)) @@ -164,8 +178,7 @@ string_cstring_free(cpath); if (error) { -char *errmsg = strerror(errno); -real_exception(interp, NULL, E_SystemError, errmsg); +STRERROR_R_EXCEPTION(); } if (S_ISLNK(info.st_mode)) @@ -199,6 +212,8 @@ string_cstring_free(cfrom); +char errmsg[ERRBUF_SIZE]; + if (source) { char *cto= string_to_cstring(interp, to); FILE *target = fopen(cto, w+b); @@ -223,14 +238,12 @@ fclose(target); } else { -char *errmsg = strerror(errno); -real_exception(interp, NULL, E_SystemError, errmsg); +STRERROR_R_EXCEPTION(); } fclose(source); } else { -char *errmsg = strerror(errno); -real_exception(interp, NULL, E_SystemError, errmsg); +STRERROR_R_EXCEPTION(); } #undef CHUNK_SIZE } @@ -254,8 +267,7 @@ string_cstring_free(cto); if (error) { -char *errmsg = strerror(errno); -real_exception(interp, NULL, E_SystemError, errmsg); +STRERROR_R_EXCEPTION(); } } } Index: config/gen/platform/win32/string.h === --- config/gen/platform/win32/string.h (revision 29551) +++ config/gen/platform/win32/string.h (working copy) @@ -14,6 +14,8 @@ # endif #endif +#define strerror_r(errnum, buf, buflen) strerror_s(buf, buflen, errnum) + #endif /* PARROT_PLATFORM_WIN32_STRING_H_GUARD */ /*
Re: [perl #46681] [TODO] [C] Use strerror_r instead of strerror
On Thu, Jul 17, 2008 at 9:59 PM, Christoph Otto via RT [EMAIL PROTECTED] wrote: With this patch, the new tests still pass on Linux/x86. The patch uses STRING-strstart to avoid leaking a malloc'd buffer when throwing an exception, which may or may not be considered kosher in this situation. Can't you use real_exception(interp, NULL, E_SystemError, %Ss, errmsg_pstring); ? Using internal details of parrot strings must be avoided. -- Salu2
[perl #46681] [TODO] [C] Use strerror_r instead of strerror
This thread trailed off about 4 months ago. Could we get an update on its status, i.e., whether it should be applied, what OSes it's passing on, etc. Thank you very much. kid51
Re: [perl #46681] [TODO] [C] Use strerror_r instead of strerror
On Nov 28, 2007, at 1:29 AM, Joe Sadusk via RT wrote: Heh, ok, grammar fixed and resubmitted. And yes, I've tested it on Linux (Fedora 7) and Windows with MSVC 2005. I don't have a non-glibc unix to try it out with however. Joe On Mon Nov 26 09:50:28 2007, [EMAIL PROTECTED] wrote: On Sunday 25 November 2007 21:48:01 Joe Sadusk via RT wrote: Ok, here's try two. Platform specific stuff has been moved into config/gen/platform, and non-win32 platforms use strerror_r directly. Change its to it's in the comment before #define ERRBUF_SIZE and this patch looks reasonable to me, if it passes on Windows and Unixy platforms. -- c Tested on osx/x86; all tests pass. -- Will Coke Coleda [EMAIL PROTECTED]
[perl #46681] [TODO] [C] Use strerror_r instead of strerror
Heh, ok, grammar fixed and resubmitted. And yes, I've tested it on Linux (Fedora 7) and Windows with MSVC 2005. I don't have a non-glibc unix to try it out with however. Joe On Mon Nov 26 09:50:28 2007, [EMAIL PROTECTED] wrote: On Sunday 25 November 2007 21:48:01 Joe Sadusk via RT wrote: Ok, here's try two. Platform specific stuff has been moved into config/gen/platform, and non-win32 platforms use strerror_r directly. Change its to it's in the comment before #define ERRBUF_SIZE and this patch looks reasonable to me, if it passes on Windows and Unixy platforms. -- c strerror_3.patch Description: Binary data
[perl #46681] [TODO] [C] Use strerror_r instead of strerror
Ok, here's try two. Platform specific stuff has been moved into config/gen/platform, and non-win32 platforms use strerror_r directly. On Fri Nov 23 01:29:35 2007, kjs wrote: On Nov 22, 2007 9:03 PM, Joe Sadusk via RT [EMAIL PROTECTED] wrote: Reposting this because this is my first patch, and it didn't occur to me that if I don't CC perl6-internals, no one would notice it. It turns out that strerror_r is POSIX only, and windows has the slightly different strerror_s (exactly the same except the arguments are in a different order). I defined a macro that abstracts this, but I don't know if there's a standard header file cross platform macros like this are put in, so I put it inline in the pmc. If anyone has a suggestion of where this belongs, I'll gladly move it. I'm no expert on this, but in config/gen/platform, there are subdirectories that contain files that do platform dependent stuff. (I only know this because I once added config/gen/platform/win32/string.h, which defines a macro for strdup for MSVC users.) I suspect that your macro should go in a similar file, but this should be confirmed by someone with more knowledge on this. kjs diff --git a/config/gen/platform/generic/string.h b/config/gen/platform/generic/string.h new file mode 100644 index 000..8de1ca1 --- /dev/null +++ b/config/gen/platform/generic/string.h @@ -0,0 +1,22 @@ +/* + * $Id$ + * Copyright (C) 2004-2007, The Perl Foundation. + */ + +#ifndef PARROT_PLATFORM_GENERIC_STRING_H_GUARD +#define PARROT_PLATFORM_GENERIC_STRING_H_GUARD + +#if defined __GLIBC__ +# define _XOPEN_SOURCE 600 +#endif + +#include string.h + +#endif /* PARROT_PLATFORM_GENERIC_STRING_H_GUARD */ + +/* + * Local variables: + * c-file-style: parrot + * End: + * vim: expandtab shiftwidth=4: + */ diff --git a/config/gen/platform/win32/string.h b/config/gen/platform/win32/string.h index 607745a..bd47db3 100644 --- a/config/gen/platform/win32/string.h +++ b/config/gen/platform/win32/string.h @@ -14,6 +14,8 @@ # endif #endif +#define strerror_r(errnum, buf, buflen) strerror_s(buf, buflen, errnum) + #endif /* PARROT_PLATFORM_WIN32_STRING_H_GUARD */ /* diff --git a/src/pmc/file.pmc b/src/pmc/file.pmc index 3c8d801..56be62d 100644 --- a/src/pmc/file.pmc +++ b/src/pmc/file.pmc @@ -27,6 +27,13 @@ CFile is a singleton class which provides access to File functions. /* RT#46679 Check if we need to deallocate strerror strings */ /* RT#46681 apparently, strerror_r is thread-safe and should be used instead.*/ +/* strerror_r should truncate the message if its too long for the supplied buffer + * its probably best to just specify a sane default buffer size than to worry + * about retrying calls + */ +#define ERRBUF_SIZE 128 + + static PMC *File_PMC; pmclass File singleton { @@ -98,7 +105,8 @@ Returns a true value (1) if the supplied path is a directory. string_cstring_free(cpath); if (error) { -char *errmsg = strerror(errno); +char errmsg[ERRBUF_SIZE]; +strerror_r(errno, errmsg, ERRBUF_SIZE); real_exception(interp, NULL, E_SystemError, errmsg); } @@ -129,7 +137,8 @@ Returns a true value (1) if the supplied path is a plain file. string_cstring_free(cpath); if (error) { -char *errmsg = strerror(errno); +char errmsg[ERRBUF_SIZE]; +strerror_r(errno, errmsg, ERRBUF_SIZE); real_exception(interp, NULL, E_SystemError, errmsg); } @@ -162,7 +171,8 @@ Returns a true value (1) if the supplied path is a link. string_cstring_free(cpath); if (error) { -char *errmsg = strerror(errno); +char errmsg[ERRBUF_SIZE]; +strerror_r(errno, errmsg, ERRBUF_SIZE); real_exception(interp, NULL, E_SystemError, errmsg); } @@ -197,6 +207,8 @@ free to change or give me hints on how to change it. -- ambs FILE *source = fopen(cfrom, rb); +char errmsg[ERRBUF_SIZE]; + if (source) { FILE *target = fopen(cto, w+b); @@ -218,13 +230,13 @@ free to change or give me hints on how to change it. -- ambs fclose(target); } else { -char *errmsg = strerror(errno); +strerror_r(errno, errmsg, ERRBUF_SIZE); real_exception(interp, NULL, E_SystemError, errmsg); } fclose(source); } else { -char *errmsg = strerror(errno); +strerror_r(errno, errmsg, ERRBUF_SIZE); real_exception(interp, NULL, E_SystemError, errmsg); } string_cstring_free(cfrom); @@ -251,7 +263,8 @@ Rename a file Cfrom to the path Cto. string_cstring_free(cto); if (error) { -char *errmsg = strerror(errno); +char errmsg[ERRBUF_SIZE]; +strerror_r(errno, errmsg, ERRBUF_SIZE);
Re: [perl #46681] [TODO] [C] Use strerror_r instead of strerror
On Sunday 25 November 2007 21:48:01 Joe Sadusk via RT wrote: Ok, here's try two. Platform specific stuff has been moved into config/gen/platform, and non-win32 platforms use strerror_r directly. Change its to it's in the comment before #define ERRBUF_SIZE and this patch looks reasonable to me, if it passes on Windows and Unixy platforms. -- c
Re: [perl #46681] [TODO] [C] Use strerror_r instead of strerror
On Nov 22, 2007 9:03 PM, Joe Sadusk via RT [EMAIL PROTECTED] wrote: Reposting this because this is my first patch, and it didn't occur to me that if I don't CC perl6-internals, no one would notice it. It turns out that strerror_r is POSIX only, and windows has the slightly different strerror_s (exactly the same except the arguments are in a different order). I defined a macro that abstracts this, but I don't know if there's a standard header file cross platform macros like this are put in, so I put it inline in the pmc. If anyone has a suggestion of where this belongs, I'll gladly move it. I'm no expert on this, but in config/gen/platform, there are subdirectories that contain files that do platform dependent stuff. (I only know this because I once added config/gen/platform/win32/string.h, which defines a macro for strdup for MSVC users.) I suspect that your macro should go in a similar file, but this should be confirmed by someone with more knowledge on this. kjs
[perl #46681] [TODO] [C] Use strerror_r instead of strerror
Reposting this because this is my first patch, and it didn't occur to me that if I don't CC perl6-internals, no one would notice it. It turns out that strerror_r is POSIX only, and windows has the slightly different strerror_s (exactly the same except the arguments are in a different order). I defined a macro that abstracts this, but I don't know if there's a standard header file cross platform macros like this are put in, so I put it inline in the pmc. If anyone has a suggestion of where this belongs, I'll gladly move it. There is also the case of older UNIX platforms such as Solaris 7, which decided to take standard strerror and make it thread safe by using thread local storage. How much of a priority are platforms like this?
[perl #46681] [TODO] [C] Use strerror_r instead of strerror
# New Ticket Created by Paul Cochrane # Please include the string: [perl #46681] # in the subject line of all future correspondence about this issue. # URL: http://rt.perl.org/rt3/Ticket/Display.html?id=46681 In src/pmc/file.pmc there is the todo item: /* XXX apparently, strerror_r is thread-safe and should be used instead.*/ So, convert all instances of strerror to strerror_r and test.