Move strerror_ts() to error.h/c
PATCH 1/1 Move strerror_ts() to error.h/c
Move strerror_ts() to error.c, which is a more suitable location than the random collection of unrelated stuff called misc.c. While doing so, simplify the implementation.
(I think we should find a better place for everything currently in misc.c, and get rid of it all together. This patch is part of that effort.)
--- src/openvpn/error.c | 8 ++++++++ src/openvpn/error.h | 5 +++++ src/openvpn/misc.c | 15 --------------- src/openvpn/misc.h | 6 ------ 4 files changed, 13 insertions(+), 21 deletions(-)
src/openvpn/error.c +8 -0
@@ -815,6 +815,14 @@ msg_flags_string(const unsigned int flags, struct gc_arena *gc)return BSTR (&out); } +#ifdef HAVE_STRERROR +const char * +strerror_ts (int errnum,struct gc_arena *gc) +{ +return string_alloc (openvpn_strerror (errnum, gc), gc); +} +#endif +#ifdef ENABLE_DEBUG void crash (void )
src/openvpn/error.h +5 -0
@@ -398,6 +398,11 @@ nonfatal(const unsigned int err) return err & M_FATAL ? (err ^ M_FATAL) | M_NONFATAL : err; } +#ifdef HAVE_STRERROR +/** A thread-safe version of strerror */ +const char *strerror_ts (int errnum,struct gc_arena *gc); +#endif +#include "errlevel.h" #endif /* ifndef ERROR_H */
src/openvpn/misc.c +0 -15
@@ -444,21 +444,6 @@ init_random_seed(void) } } -/* thread-safe strerror */ - -const char * -strerror_ts (int errnum,struct gc_arena *gc) -{ -#ifdef HAVE_STRERROR -struct buffer out =alloc_buf_gc (256 , gc); - -buf_printf (&out,"%s" ,openvpn_strerror (errnum, gc)); -return BSTR (&out); -#else -return "[error string unavailable]" ; -#endif -} -/* * Set environmental variable (int or string). *
src/openvpn/misc.h +0 -6
@@ -95,12 +95,6 @@ openvpn_run_script(const struct argv *a, const struct env_set *es, const unsigne } -#ifdef HAVE_STRERROR -/* a thread-safe version of strerror */ -const char *strerror_ts (int errnum,struct gc_arena *gc); - -#endif -/* Set standard file descriptors to /dev/null */ void set_std_files_to_null (bool stdin_only);
Signature · 1 line
2.11.0
Hi,
Quoted 6 lines · Steffan Karger
Move
strerror_ts()toerror.c, which is a more suitable location than the random collection of unrelated stuff calledmisc.c. While doing so, simplify the implementation.(I think we should find a better place for everything currently in
misc.c, and get rid of it all together. This patch is part of that effort.)
While I do like the patch, parts of it do not make sense :-)
src/openvpn/error.c msg_flags_string(const unsigned int flags, struct gc_arena *gc) · line 825
Quoted 8 lines
src/openvpn/error.c +8
-0+#ifdef HAVE_STRERROR +const char * +strerror_ts (int errnum,struct gc_arena *gc) +{ +return string_alloc (openvpn_strerror (errnum, gc), gc); +} +#endif +
... so where's the "#else" here?
OTOH, the old code was a bit weird - #ifndef HAVE_STRERROR, the function itself would return a static string, but the *prototype* would disappear from the header file.
I think we can safely assume that strerror() is available on all the platforms (so, kick out the test from configure.ac as well :) ) - but it might be worthwile to have a look at strerror_r() or strerror_s(), if we really are concerned about eventual threading OpenVPN and making this properly reentrant.
As a side note, why anyone would implement strerror() in a non-thread- safe way ("it's only returning a pointer to a static string table, isn't it?") - reading through
https://stackoverflow.com/questions/900338/why-cant-i-use-strerror
has the answer: NLS, and "string tables dynamically loaded at run-time", so the string isn't actually static inside strerror().
gert
Signature · 4 lines
USENET is *not* the non-clickable part of WWW!
//www.muc.de/~gert/
Gert Doering - Munich, Germany [email protected]...
fax: +49-89-35655025 [email protected]...PATCH 1/1 Move strerror_ts() to error.h/c
Move strerror_ts() to error.c, which is a more suitable location than the random collection of unrelated stuff called misc.c.
While doing so, simplify the implementation and remove the defines. strerror() is required by C89 and C99, and we require C99, so no need to check.
We might want to switch to strerror_r() and get rid of this function all together (it isn't as thread-safe as it claims), but while strerror() is part of the C standard, strerror_r() is 'just' posix. So I'd prefer not to pull that into this patch.
(I think we should find a better place for everything currently in misc.c, and get rid of it all together. This patch is part of that effort.)
---
v2 - don't check for strerror presence, it's in C99.
configure.ac | 2 +- src/openvpn/error.c | 6 ++++++ src/openvpn/error.h | 3 +++ src/openvpn/misc.c | 15 --------------- src/openvpn/misc.h | 6 ------ 5 files changed, 10 insertions(+), 22 deletions(-)
configure.ac +1 -1
@@ -662,7 +662,7 @@ AC_FUNC_FORK AC_CHECK_FUNCS([ \ daemon chroot getpwnam setuid nice system getpid dup dup2 \ - getpass strerror syslog openlog mlockall getgrnam setgid \ + getpass syslog openlog mlockall getgrnam setgid \ setgroups stat flock readv writev time gettimeofday \ ctime memset vsnprintf strdup \ setsid chdir putenv getpeername unlink \
src/openvpn/error.c +6 -0
@@ -815,6 +815,12 @@ msg_flags_string(const unsigned int flags, struct gc_arena *gc)return BSTR (&out); } +const char * +strerror_ts (int errnum,struct gc_arena *gc) +{ +return string_alloc (openvpn_strerror (errnum, gc), gc); +} +#ifdef ENABLE_DEBUG void crash (void )
src/openvpn/error.h +3 -0
@@ -398,6 +398,9 @@ nonfatal(const unsigned int err) return err & M_FATAL ? (err ^ M_FATAL) | M_NONFATAL : err; } +/** A thread-safe version of strerror */ +const char *strerror_ts (int errnum,struct gc_arena *gc); +#include "errlevel.h" #endif /* ifndef ERROR_H */
src/openvpn/misc.c +0 -15
@@ -444,21 +444,6 @@ init_random_seed(void) } } -/* thread-safe strerror */ - -const char * -strerror_ts (int errnum,struct gc_arena *gc) -{ -#ifdef HAVE_STRERROR -struct buffer out =alloc_buf_gc (256 , gc); - -buf_printf (&out,"%s" ,openvpn_strerror (errnum, gc)); -return BSTR (&out); -#else -return "[error string unavailable]" ; -#endif -} -/* * Set environmental variable (int or string). *
src/openvpn/misc.h +0 -6
@@ -95,12 +95,6 @@ openvpn_run_script(const struct argv *a, const struct env_set *es, const unsigne } -#ifdef HAVE_STRERROR -/* a thread-safe version of strerror */ -const char *strerror_ts (int errnum,struct gc_arena *gc); - -#endif -/* Set standard file descriptors to /dev/null */ void set_std_files_to_null (bool stdin_only);
Signature · 1 line
2.7.4
PATCH v3 1/1 Remove strerror_ts()
This function was only called in string format functions, which already copy the contents, so all this ever did was adding redundant malloc() and free() calls.
Also, this wasn't as thread-safe as it claims: another thread could still change the string value between the strerror() and buf_printf() calls. So, instead of a not needed false sense of thread-safeness, just be honest and use strerror() directly. If we ever need thread-safeness, we should do a proper solution based on e.g. strerror_r()/strerror_s().
(I think we should find a better place for everything currently in misc.c, and get rid of the file all together. In this case, the better place is /dev/null. This patch is part of that effort.)
---
v2 - don't check for strerror presence, it's in C99. v3 - just remove the function, it serves no real purpose.
configure.ac | 2 +- src/openvpn/error.c | 15 +++++---------- src/openvpn/manage.c | 5 ++--- src/openvpn/misc.c | 15 --------------- src/openvpn/misc.h | 6 ------ src/openvpn/socket.c | 6 ++---- 6 files changed, 10 insertions(+), 39 deletions(-)
configure.ac +1 -1
@@ -662,7 +662,7 @@ AC_FUNC_FORK AC_CHECK_FUNCS([ \ daemon chroot getpwnam setuid nice system getpid dup dup2 \ - getpass strerror syslog openlog mlockall getgrnam setgid \ + getpass syslog openlog mlockall getgrnam setgid \ setgroups stat flock readv writev time gettimeofday \ ctime memset vsnprintf strdup \ setsid chdir putenv getpeername unlink \
src/openvpn/error.c +5 -10
@@ -267,7 +267,7 @@ x_msg_va(const unsigned int flags, const char *format, va_list arglist)if ((flags &M_ERRNO ) && e) {openvpn_snprintf (m2,ERR_BUF_SIZE ,"%s: %s (errno=%d)" , - m1,strerror_ts (e, &gc), e); + m1,strerror (e, &gc), e);SWAP ; }
@@ -693,20 +693,15 @@ x_check_status(int status, {if (extended_msg) { -msg (x_cs_info_level,"%s %s [%s]: %s (code=%d)" , - description, +msg (x_cs_info_level,"%s %s [%s]: %s (code=%d)" , description, sock ?proto2ascii (sock->info.proto, sock->info.af, true) :"" , - extended_msg, -strerror_ts (my_errno, &gc), - my_errno); + extended_msg,strerror (my_errno, &gc), my_errno); }else { -msg (x_cs_info_level,"%s %s: %s (code=%d)" , - description, +msg (x_cs_info_level,"%s %s: %s (code=%d)" , description, sock ?proto2ascii (sock->info.proto, sock->info.af, true) :"" , -strerror_ts (my_errno, &gc), - my_errno); +strerror (my_errno, &gc), my_errno); }if (x_cs_err_delay_ms)
src/openvpn/manage.c +2 -3
@@ -2006,9 +2006,8 @@ man_io_error(struct management *man, const char *prefix)if (!ignore_sys_error (err)) {struct gc_arena gc =gc_new (); -msg (D_MANAGEMENT ,"MANAGEMENT: TCP %s error: %s" , - prefix, -strerror_ts (err, &gc)); +msg (D_MANAGEMENT ,"MANAGEMENT: TCP %s error: %s" , prefix, +strerror (err, &gc));gc_free (&gc);return true; }
src/openvpn/misc.c +0 -15
@@ -444,21 +444,6 @@ init_random_seed(void) } } -/* thread-safe strerror */ - -const char * -strerror_ts (int errnum,struct gc_arena *gc) -{ -#ifdef HAVE_STRERROR -struct buffer out =alloc_buf_gc (256 , gc); - -buf_printf (&out,"%s" ,openvpn_strerror (errnum, gc)); -return BSTR (&out); -#else -return "[error string unavailable]" ; -#endif -} -/* * Set environmental variable (int or string). *
src/openvpn/misc.h +0 -6
@@ -95,12 +95,6 @@ openvpn_run_script(const struct argv *a, const struct env_set *es, const unsigne } -#ifdef HAVE_STRERROR -/* a thread-safe version of strerror */ -const char *strerror_ts (int errnum,struct gc_arena *gc); - -#endif -/* Set standard file descriptors to /dev/null */ void set_std_files_to_null (bool stdin_only);
src/openvpn/socket.c +2 -4
@@ -1473,10 +1473,8 @@ socket_connect(socket_descriptor_t *sd,if (status) { -msg (D_LINK_ERRORS , -"TCP: connect to %s failed: %s" , -print_sockaddr (dest, &gc), -strerror_ts (status, &gc)); +msg (D_LINK_ERRORS ,"TCP: connect to %s failed: %s" , +print_sockaddr (dest, &gc),strerror (status, &gc));openvpn_close_socket (*sd); *sd =SOCKET_UNDEFINED ;
Signature · 1 line
2.7.4
Re: [Openvpn-devel] [PATCH v3] Remove strerror_ts()
Hi,
src/openvpn/error.c x_msg_va(const unsigned int flags, const char *format, va_list arglist) · line 270
Quoted 5 lines · Steffan Karger
if ((flags & M_ERRNO) && e) { openvpn_snprintf(m2, ERR_BUF_SIZE, "%s: %s (errno=%d)",src/openvpn/error.c +1
-1- m1, strerror_ts(e, &gc), e); + m1, strerror(e, &gc), e);
Can I have a v4, please...?
src/openvpn/error.c x_check_status(int status, · line 698
Quoted 3 lines
src/openvpn/error.c +1
-2-strerror_ts (my_errno, &gc), - my_errno); + extended_msg, strerror(my_errno, &gc), my_errno);
... with strerror() calls that do not carry a &gc...
(Why is that not exploding in your compile tests?)
thanks,
gert
Signature · 4 lines
USENET is *not* the non-clickable part of WWW!
//www.muc.de/~gert/
Gert Doering - Munich, Germany [email protected]...
fax: +49-89-35655025 [email protected]...PATCH v4 1/1 Remove strerror_ts()
This function was only called in string format functions, which already copy the contents, so all this ever did was adding redundant malloc() and free() calls.
Also, this wasn't as thread-safe as it claims: another thread could still change the string value between the strerror() and buf_printf() calls. So, instead of a not needed false sense of thread-safeness, just be honest and use strerror() directly.
(I think we should find a better place for everything currently in misc.c, and get rid of it all together. In this case, the better place is /dev/null. This patch is part of that effort.)
---
v2 - don't check for strerror presence, it's in C99. v3 - just remove the function, it serves no real purpose. v4 - strerror() does not want a gc...
configure.ac | 2 +-
src/openvpn/error.c | 15 +++++---------- src/openvpn/manage.c | 5 ++--- src/openvpn/misc.c | 15 --------------- src/openvpn/misc.h | 6 ------ src/openvpn/socket.c | 6 ++---- 6 files changed, 10 insertions(+), 39 deletions(-)
configure.ac +1 -1
@@ -662,7 +662,7 @@ AC_FUNC_FORK AC_CHECK_FUNCS([ \ daemon chroot getpwnam setuid nice system getpid dup dup2 \ - getpass strerror syslog openlog mlockall getgrnam setgid \ + getpass syslog openlog mlockall getgrnam setgid \ setgroups stat flock readv writev time gettimeofday \ ctime memset vsnprintf strdup \ setsid chdir putenv getpeername unlink \
src/openvpn/error.c +5 -10
@@ -267,7 +267,7 @@ x_msg_va(const unsigned int flags, const char *format, va_list arglist)if ((flags &M_ERRNO ) && e) {openvpn_snprintf (m2,ERR_BUF_SIZE ,"%s: %s (errno=%d)" , - m1,strerror_ts (e, &gc), e); + m1,strerror (e), e);SWAP ; }
@@ -693,20 +693,15 @@ x_check_status(int status, {if (extended_msg) { -msg (x_cs_info_level,"%s %s [%s]: %s (code=%d)" , - description, +msg (x_cs_info_level,"%s %s [%s]: %s (code=%d)" , description, sock ?proto2ascii (sock->info.proto, sock->info.af, true) :"" , - extended_msg, -strerror_ts (my_errno, &gc), - my_errno); + extended_msg,strerror (my_errno), my_errno); }else { -msg (x_cs_info_level,"%s %s: %s (code=%d)" , - description, +msg (x_cs_info_level,"%s %s: %s (code=%d)" , description, sock ?proto2ascii (sock->info.proto, sock->info.af, true) :"" , -strerror_ts (my_errno, &gc), - my_errno); +strerror (my_errno), my_errno); }if (x_cs_err_delay_ms)
src/openvpn/manage.c +2 -3
@@ -2006,9 +2006,8 @@ man_io_error(struct management *man, const char *prefix)if (!ignore_sys_error (err)) {struct gc_arena gc =gc_new (); -msg (D_MANAGEMENT ,"MANAGEMENT: TCP %s error: %s" , - prefix, -strerror_ts (err, &gc)); +msg (D_MANAGEMENT ,"MANAGEMENT: TCP %s error: %s" , prefix, +strerror (err));gc_free (&gc);return true; }
src/openvpn/misc.c +0 -15
@@ -444,21 +444,6 @@ init_random_seed(void) } } -/* thread-safe strerror */ - -const char * -strerror_ts (int errnum,struct gc_arena *gc) -{ -#ifdef HAVE_STRERROR -struct buffer out =alloc_buf_gc (256 , gc); - -buf_printf (&out,"%s" ,openvpn_strerror (errnum, gc)); -return BSTR (&out); -#else -return "[error string unavailable]" ; -#endif -} -/* * Set environmental variable (int or string). *
src/openvpn/misc.h +0 -6
@@ -95,12 +95,6 @@ openvpn_run_script(const struct argv *a, const struct env_set *es, const unsigne } -#ifdef HAVE_STRERROR -/* a thread-safe version of strerror */ -const char *strerror_ts (int errnum,struct gc_arena *gc); - -#endif -/* Set standard file descriptors to /dev/null */ void set_std_files_to_null (bool stdin_only);
src/openvpn/socket.c +2 -4
@@ -1473,10 +1473,8 @@ socket_connect(socket_descriptor_t *sd,if (status) { -msg (D_LINK_ERRORS , -"TCP: connect to %s failed: %s" , -print_sockaddr (dest, &gc), -strerror_ts (status, &gc)); +msg (D_LINK_ERRORS ,"TCP: connect to %s failed: %s" , +print_sockaddr (dest, &gc),strerror (status));openvpn_close_socket (*sd); *sd =SOCKET_UNDEFINED ;
Signature · 1 line
2.7.4
[Openvpn-devel] [PATCH applied] Re: Remove strerror_ts()
ACK.
Your patch has been applied to the master and release/2.4 branch.
commit fd2a29ab2668fea9c0ac972d5ec69f00232c88b6 (master) commit 5315193bb9e83980ca24d4628c4e1adfb8f012a9 (release/2.4)
Author: Steffan Karger Date: Thu Jul 20 13:39:00 2017 +0200
Remove strerror_ts()
Signed-off-by: Steffan Karger <[email protected]...>
Acked-by: Gert Doering <[email protected]...>
Message-Id: <[email protected]...>
URL: https://www.mail-archive.com/[email protected]/msg15105.html
Signed-off-by: Gert Doering <[email protected]...>--
kind regards,
Gert Doering