Libguestfs - Jan 2018 - Re: [PATCH] lua, perl: Use thread-safe strerror_r instead of strerror (RHBZ#1536763).

---
 generator/lua.ml  | 22 +++++++++++++++-------
 generator/perl.ml |  2 +-
 lua/Makefile.am   |  1 +
 3 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/generator/lua.ml b/generator/lua.ml
index dd6aedbe9..7cfceb152 100644
--- a/generator/lua.ml
+++ b/generator/lua.ml
@@ -63,6 +63,8 @@ let generate_lua_c ()  #endif
 #endif
 
+#include \"ignore-value.h\"
+
 #include <guestfs.h>
 #include \"guestfs-utils.h\"
 
@@ -142,6 +144,7 @@ guestfs_int_lua_create (lua_State *L)
   guestfs_h *g;
   struct userdata *u;
   unsigned flags = 0;
+  char err[128];
 
   if (lua_gettop (L) == 1) {
     OPTARG_IF_SET (1, \"environment\",
@@ -157,9 +160,10 @@ guestfs_int_lua_create (lua_State *L)
     return luaL_error (L, \"Guestfs.create: too many arguments\");
 
   g = guestfs_create_flags (flags);
-  if (!g)
-    return luaL_error (L, \"Guestfs.create: cannot create handle:
%%s\",
-                       strerror (errno));
+  if (!g) {
+    ignore_value (strerror_r (errno, err, sizeof err));
+    return luaL_error (L, \"Guestfs.create: cannot create handle:
%%s\", err);
+  }
 
   guestfs_set_error_handler (g, NULL, NULL);
 
@@ -226,6 +230,7 @@ error__tostring (lua_State *L)
 {
   int code;
   const char *msg;
+  char err[128];
 
   lua_pushliteral (L, \"code\");
   lua_gettable (L, 1);
@@ -234,8 +239,10 @@ error__tostring (lua_State *L)
   lua_gettable (L, 1);
   msg = luaL_checkstring (L, -1);
 
-  if (code)
-    lua_pushfstring (L, \"%%s: %%s\", msg, strerror (code));
+  if (code) {
+    ignore_value (strerror_r (code, err, sizeof err));
+    lua_pushfstring (L, \"%%s: %%s\", msg, err);
+  }
   else
     lua_pushstring (L, msg);
 
@@ -640,11 +647,12 @@ get_string_list (lua_State *L, int index)
   const size_t len = lua_objlen (L, index);
   size_t i;
   char **strs;
+  char err[128];
 
   strs = malloc ((len+1) * sizeof (char *));
   if (strs == NULL) {
-    luaL_error (L, \"get_string_list: malloc failed: %%s\",
-                strerror (errno));
+    ignore_value (strerror_r (errno, err, sizeof err));
+    luaL_error (L, \"get_string_list: malloc failed: %%s\", err);
     /*NOTREACHED*/
     return NULL;
   }
diff --git a/generator/perl.ml b/generator/perl.ml
index bd7e328cd..240bf3b54 100644
--- a/generator/perl.ml
+++ b/generator/perl.ml
@@ -308,7 +308,7 @@ PREINIT:
    CODE:
       str = guestfs_event_to_string (event_bitmask);
       if (str == NULL)
-        croak (\"%%s\", strerror (errno));
+        croak (\"%%m\");
       RETVAL = newSVpv (str, 0);
       free (str);
  OUTPUT:
diff --git a/lua/Makefile.am b/lua/Makefile.am
index f90c1d7cb..a26c0baee 100644
--- a/lua/Makefile.am
+++ b/lua/Makefile.am
@@ -41,6 +41,7 @@ libluaguestfs_la_SOURCES = lua-guestfs.c
 
 libluaguestfs_la_CPPFLAGS = \
 	-DGUESTFS_PRIVATE=1 \
+	-I$(srcdir)/../gnulib/lib -I../gnulib/lib \
 	-I$(top_srcdir)/common/utils -I$(top_builddir)/common/utils \
 	-I$(top_srcdir)/lib -I$(top_builddir)/lib
 
-- 
2.13.2

On Monday, 22 January 2018 18:02:08 CET Richard W.M. Jones
wrote:
> ---
>  generator/lua.ml  | 22 +++++++++++++++-------
>  generator/perl.ml |  2 +-
>  lua/Makefile.am   |  1 +
>  3 files changed, 17 insertions(+), 8 deletions(-)
Ah, you just beat me to it... OTOH you missed the ruby binding, see
ruby/ext/guestfs/handle.c.
> diff --git a/generator/lua.ml b/generator/lua.ml
> index dd6aedbe9..7cfceb152 100644
> --- a/generator/lua.ml
> +++ b/generator/lua.ml
> @@ -63,6 +63,8 @@ let generate_lua_c () >  #endif
>  #endif
>  
> +#include \"ignore-value.h\"
In my local version, ignore_value() was not needed in the lua bindings
Did you get a build failure without it?
>  #include <guestfs.h>
>  #include \"guestfs-utils.h\"
>  
> @@ -142,6 +144,7 @@ guestfs_int_lua_create (lua_State *L)
>    guestfs_h *g;
>    struct userdata *u;
>    unsigned flags = 0;
> +  char err[128];
In other places were strerror_r is used, the size of the buffer is 256.
Can you please adapt it? (Mostly so we have the same code everywhere.)

Thanks,
-- 
Pino Toscano

On Mon, Jan 22, 2018 at 06:19:33PM +0100, Pino Toscano
wrote:
> > On Monday, 22 January 2018 18:02:08 CET Richard W.M. Jones wrote:
> > ---
> >  generator/lua.ml  | 22 +++++++++++++++-------
> >  generator/perl.ml |  2 +-
> >  lua/Makefile.am   |  1 +
> >  3 files changed, 17 insertions(+), 8 deletions(-)
> 
> Ah, you just beat me to it... OTOH you missed the ruby binding, see
> ruby/ext/guestfs/handle.c.
This one is too hard for me to fix.  I need to include the gnulib
directory (to get the replacement strerror_r) otherwise I see:

handle.c: In function 'guestfs_int_ruby_event_to_string':
handle.c:285:5: error: ignoring return value of 'strerror_r', declared
with attribute warn_unused_result [-Werror=unused-result]
     strerror_r (errno, err, sizeof err);
     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

but I couldn't work out how to add that without breaking Ruby's
conftests.

In any case Ruby is irretrievably broken with pthread, see recent
nbdkit commits ...
> > diff --git a/generator/lua.ml b/generator/lua.ml
> > index dd6aedbe9..7cfceb152 100644
> > --- a/generator/lua.ml
> > +++ b/generator/lua.ml
> > @@ -63,6 +63,8 @@ let generate_lua_c () > >  #endif
> >  #endif
> >  
> > +#include \"ignore-value.h\"
> 
> In my local version, ignore_value() was not needed in the lua bindings
> Did you get a build failure without it?
In fact no I don't need it.

BUT I do need to add the gnulib directory so we get the replacement
strerror_r, because the original strerror_r from glibc is declared
with warn_unused_result:

lua-guestfs.c: In function 'guestfs_int_lua_create':
lua-guestfs.c:178:5: error: ignoring return value of 'strerror_r',
declared with attribute warn_unused_result [-Werror=unused-result]
     strerror_r (errno, err, sizeof err);
     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Anyway, I dropped ignore_value but still added gnulib/lib to the
include path.
> >  #include <guestfs.h>
> >  #include \"guestfs-utils.h\"
> >  
> > @@ -142,6 +144,7 @@ guestfs_int_lua_create (lua_State *L)
> >    guestfs_h *g;
> >    struct userdata *u;
> >    unsigned flags = 0;
> > +  char err[128];
> 
> In other places were strerror_r is used, the size of the buffer is 256.
> Can you please adapt it? (Mostly so we have the same code everywhere.)
I pushed this with the changes above, plus err[256].

Thanks,

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW