Libguestfs - Feb 2015 - Re: [PATCH 1/5] macosx: Add required third parameter for xdrproc_t callbacks

>From Apple's xdr.h:
"If your code invokes an xdrproc_t callback, it must be modified to pass
a third parameter, which may simply be zero."
---
 src/proto.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/src/proto.c b/src/proto.c
index 92ae84d..57f4882 100644
--- a/src/proto.c
+++ b/src/proto.c
@@ -252,7 +252,12 @@ guestfs___send (guestfs_h *g, int proc_nr,
    * have no parameters.
    */
   if (xdrp) {
+#if !(defined __APPLE__ && defined __MACH__)
     if (!(*xdrp) (&xdr, args)) {
+#else
+    /* Mac OS X's implementation of xdrproc_t requires a third parameter */
+    if (!(*xdrp) (&xdr, args, 0)) {
+#endif
       error (g, _("dispatch failed to marshal args"));
       return -1;
     }
@@ -681,7 +686,12 @@ guestfs___recv (guestfs_h *g, const char *fn,
       return -1;
     }
   } else {
+#if !(defined __APPLE__ && defined __MACH__)
     if (xdrp && ret && !xdrp (&xdr, ret)) {
+#else
+    /* Mac OS X's implementation of xdrproc_t requires a third parameter */
+    if (xdrp && ret && !xdrp (&xdr, ret, 0)) {
+#endif
       error (g, "%s: failed to parse reply", fn);
       xdr_destroy (&xdr);
       return -1;
-- 
1.9.3

gnulib's error.c requires program_name to be externally defined
for !_LIBC systems. This defines program_name for Darwin only.
---
 configure.ac       | 3 +++
 src/Makefile.am    | 6 ++++++
 src/program_name.c | 4 ++++
 3 files changed, 13 insertions(+)
 create mode 100644 src/program_name.c

diff --git a/configure.ac b/configure.ac
index 37850a3..a2fb99e 100644
--- a/configure.ac
+++ b/configure.ac
@@ -581,6 +581,9 @@ fi
 AC_MSG_RESULT([$DISTRO])
 AC_SUBST([DISTRO])
 
+dnl Check if we're building on MacOS X.
+AM_CONDITIONAL([DARWIN], [test "x$(uname)" = "xDarwin"])
+
 dnl Add extra packages to the appliance.
 AC_ARG_WITH([extra-packages],
     [AS_HELP_STRING([--with-extra-packages="pkg1 pkg2 ..."],
diff --git a/src/Makefile.am b/src/Makefile.am
index 2496887..1c36035 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -131,6 +131,12 @@ libguestfs_la_SOURCES = \
 	tmpdirs.c \
 	libguestfs.syms
 
+if DARWIN
+# gnulib requires program_name to be defined on non-glibc systems, in
+# particular MacOS X.
+libguestfs_la_SOURCES += program_name.c
+endif
+
 libguestfs_la_CPPFLAGS = \
 	-DGUESTFS_DEFAULT_PATH='"$(libdir)/guestfs"' \
 	-DGUESTFS_WARN_DEPRECATED=1 \
diff --git a/src/program_name.c b/src/program_name.c
new file mode 100644
index 0000000..e1b2e19
--- /dev/null
+++ b/src/program_name.c
@@ -0,0 +1,4 @@
+/* gnulib requires char *program_name to be defined for non-glibc systems
+ * (like MacOS X).
+ */
+const char *program_name = "libguestfs";
-- 
1.9.3

On JDK 8, doclint is enabled by default with strict validation of HTML
tags, which causes the build to fail. See
http://blog.joda.org/2014/02/turning-off-doclint-in-jdk-8-javadoc.html
---
 java/Makefile.am | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/java/Makefile.am b/java/Makefile.am
index fa5b45c..a76a9d1 100644
--- a/java/Makefile.am
+++ b/java/Makefile.am
@@ -68,7 +68,7 @@ clean-local:
 if HAVE_JAVA
 
 JAVAC_FLAGS = $(EXTRA_JAVAC_FLAGS) -encoding utf-8
-JAVADOC_FLAGS = -encoding utf-8
+JAVADOC_FLAGS = -encoding utf-8 -Xdoclint:none
 
 # Java source.
 
-- 
1.9.3

---
 configure.ac                    | 1 +
 src/guestfs-internal-frontend.h | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/configure.ac b/configure.ac
index a2fb99e..0f7f97e 100644
--- a/configure.ac
+++ b/configure.ac
@@ -315,6 +315,7 @@ AC_CHECK_FUNCS([\
     be32toh \
     fsync \
     futimens \
+    getprogname \
     getxattr \
     htonl \
     htons \
diff --git a/src/guestfs-internal-frontend.h b/src/guestfs-internal-frontend.h
index 2b9070b..d496655 100644
--- a/src/guestfs-internal-frontend.h
+++ b/src/guestfs-internal-frontend.h
@@ -133,6 +133,8 @@ extern void guestfs___cleanup_pclose (void *ptr);
  */
 #if HAVE_DECL_PROGRAM_INVOCATION_SHORT_NAME == 1
 #  define guestfs___program_name program_invocation_short_name
+#elif HAVE_GETPROGNAME
+#  define guestfs___program_name getprogname()
 #else
 #  define guestfs___program_name "libguestfs"
 #endif
-- 
1.9.3

* Workaround for linux-specific fuser -v
* Workaround for linux-specific fusermount
---
 fuse/guestunmount.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/fuse/guestunmount.c b/fuse/guestunmount.c
index 3df481b..2190ba0 100644
--- a/fuse/guestunmount.c
+++ b/fuse/guestunmount.c
@@ -257,7 +257,12 @@ do_fusermount (const char *mountpoint, char **error_rtn)
     /* We have to parse error messages from fusermount, so ... */
     setenv ("LC_ALL", "C", 1);
 
+#ifdef __linux__
     execlp ("fusermount", "fusermount", "-u",
mountpoint, NULL);
+#else
+    /* use umount where fusermount is not available */
+    execlp ("umount", "umount", mountpoint, NULL);
+#endif
     perror ("exec");
     _exit (EXIT_FAILURE);
   }
@@ -334,7 +339,19 @@ do_fuser (const char *mountpoint)
   }
 
   if (pid == 0) {               /* Child - run /sbin/fuser. */
+#ifdef __linux__
     execlp ("/sbin/fuser", "fuser", "-v",
"-m", mountpoint, NULL);
+#else
+    /* BSD and Mac OS X versions of fuser do not have the -v option */
+    const char *cmd_prefix = "/bin/ps -p \"$(fuser -c ";
+    const char *cmd_suffix = " 2>/dev/null)\" -o user,pid,comm
2>/dev/null";
+    char *cmd = malloc (strlen(cmd_prefix) + strlen(mountpoint) +
strlen(cmd_suffix) + 1);
+    if (cmd) {
+      sprintf (cmd, "%s%s%s", cmd_prefix, mountpoint, cmd_suffix);
+      execlp ("/bin/sh", "sh", "-c", cmd, NULL);
+      free (cmd);
+    }
+#endif
     _exit (EXIT_FAILURE);
   }
 
-- 
1.9.3

On Monday 09 February 2015 11:06:15 Margaret Lewicka
wrote:
> >From Apple's xdr.h:
> "If your code invokes an xdrproc_t callback, it must be modified to
pass
> a third parameter, which may simply be zero."
> ---
>  src/proto.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/src/proto.c b/src/proto.c
> index 92ae84d..57f4882 100644
> --- a/src/proto.c
> +++ b/src/proto.c
> @@ -252,7 +252,12 @@ guestfs___send (guestfs_h *g, int proc_nr,
>     * have no parameters.
>     */
>    if (xdrp) {
> +#if !(defined __APPLE__ && defined __MACH__)
>      if (!(*xdrp) (&xdr, args)) {
> +#else
> +    /* Mac OS X's implementation of xdrproc_t requires a third
parameter */
> +    if (!(*xdrp) (&xdr, args, 0)) {
> +#endif
>        error (g, _("dispatch failed to marshal args"));
>        return -1;
>      }
> @@ -681,7 +686,12 @@ guestfs___recv (guestfs_h *g, const char *fn,
>        return -1;
>      }
>    } else {
> +#if !(defined __APPLE__ && defined __MACH__)
>      if (xdrp && ret && !xdrp (&xdr, ret)) {
> +#else
> +    /* Mac OS X's implementation of xdrproc_t requires a third
parameter */
> +    if (xdrp && ret && !xdrp (&xdr, ret, 0)) {
> +#endif
>        error (g, "%s: failed to parse reply", fn);
>        xdr_destroy (&xdr);
>        return -1;
This should rather be a configure check, instead of hardcoding how a
libc behaves.

Also, what does the third parameter represent?

-- 
Pino Toscano

On Mon, Feb 09, 2015 at 01:39:55PM +0100, Pino Toscano
wrote:
> On Monday 09 February 2015 11:06:15 Margaret Lewicka wrote:
> > >From Apple's xdr.h:
> > "If your code invokes an xdrproc_t callback, it must be modified
to pass
> > a third parameter, which may simply be zero."
> > ---
> >  src/proto.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/src/proto.c b/src/proto.c
> > index 92ae84d..57f4882 100644
> > --- a/src/proto.c
> > +++ b/src/proto.c
> > @@ -252,7 +252,12 @@ guestfs___send (guestfs_h *g, int proc_nr,
> >     * have no parameters.
> >     */
> >    if (xdrp) {
> > +#if !(defined __APPLE__ && defined __MACH__)
> >      if (!(*xdrp) (&xdr, args)) {
> > +#else
> > +    /* Mac OS X's implementation of xdrproc_t requires a third
parameter */
> > +    if (!(*xdrp) (&xdr, args, 0)) {
> > +#endif
> >        error (g, _("dispatch failed to marshal args"));
> >        return -1;
> >      }
> > @@ -681,7 +686,12 @@ guestfs___recv (guestfs_h *g, const char *fn,
> >        return -1;
> >      }
> >    } else {
> > +#if !(defined __APPLE__ && defined __MACH__)
> >      if (xdrp && ret && !xdrp (&xdr, ret)) {
> > +#else
> > +    /* Mac OS X's implementation of xdrproc_t requires a third
parameter */
> > +    if (xdrp && ret && !xdrp (&xdr, ret, 0)) {
> > +#endif
> >        error (g, "%s: failed to parse reply", fn);
> >        xdr_destroy (&xdr);
> >        return -1;
> 
> This should rather be a configure check, instead of hardcoding how a
> libc behaves.
Actually you can just unconditionally pass the 3rd parameter on all
platforms. See this commit in libvirt for rationale:

  commit 9fa3a8ab6fd82ad2f5a14b490696085061418718
  Author: Doug Goldstein <cardoe@cardoe.com>
  Date:   Wed Oct 30 11:22:58 2013 -0500

    MacOS: Handle changes to xdrproc_t definition
    
    With Mac OS X 10.9, xdrproc_t is no longer defined as:
    
    typedef bool_t (*xdrproc_t)(XDR *, ...);
    
    but instead as:
    
    typdef bool_t (*xdrproc_t)(XDR *, void *, unsigned int);
    
    For reference, Linux systems typically define it as:
    
    typedef bool_t (*xdrproc_t)(XDR *, void *, ...);
    
    The rationale explained in the header is that using a vararg is
    incorrect and has a potential to change the ABI slightly do to compiler
    optimizations taken and the undefined behavior. They decided
    to specify the exact number of parameters and for compatibility with old
    code decided to make the signature require 3 arguments. The third
    argument is ignored for cases that its not used and its recommended to
    supply a 0.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

On Mon, Feb 09, 2015 at 11:06:19AM +0000, Margaret Lewicka
wrote:
> * Workaround for linux-specific fuser -v
> * Workaround for linux-specific fusermount
> ---
>  fuse/guestunmount.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/fuse/guestunmount.c b/fuse/guestunmount.c
> index 3df481b..2190ba0 100644
> --- a/fuse/guestunmount.c
> +++ b/fuse/guestunmount.c
> @@ -257,7 +257,12 @@ do_fusermount (const char *mountpoint, char
**error_rtn)
>      /* We have to parse error messages from fusermount, so ... */
>      setenv ("LC_ALL", "C", 1);
>  
> +#ifdef __linux__
>      execlp ("fusermount", "fusermount",
"-u", mountpoint, NULL);
> +#else
> +    /* use umount where fusermount is not available */
> +    execlp ("umount", "umount", mountpoint, NULL);
> +#endif
>      perror ("exec");
>      _exit (EXIT_FAILURE);
>    }
> @@ -334,7 +339,19 @@ do_fuser (const char *mountpoint)
>    }
>  
>    if (pid == 0) {               /* Child - run /sbin/fuser. */
> +#ifdef __linux__
>      execlp ("/sbin/fuser", "fuser", "-v",
"-m", mountpoint, NULL);
> +#else
> +    /* BSD and Mac OS X versions of fuser do not have the -v option */
> +    const char *cmd_prefix = "/bin/ps -p \"$(fuser -c ";
> +    const char *cmd_suffix = " 2>/dev/null)\" -o
user,pid,comm 2>/dev/null";
> +    char *cmd = malloc (strlen(cmd_prefix) + strlen(mountpoint) +
strlen(cmd_suffix) + 1);
> +    if (cmd) {
> +      sprintf (cmd, "%s%s%s", cmd_prefix, mountpoint,
cmd_suffix);
> +      execlp ("/bin/sh", "sh", "-c", cmd,
NULL);
This is vulnerable to shell meta characters in the user supplied
"mountpoint" string. I dont know if it is a security exploit in
the context of libguestfs alone, but it could cause problems when
considered apps using libguestfs. Any use of shell should really
be avoided when dealing with user supplied input.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

On Mon, Feb 09, 2015 at 11:06:16AM +0000, Margaret Lewicka
wrote:
> gnulib's error.c requires program_name to be externally defined
> for !_LIBC systems. This defines program_name for Darwin only.
> ---
>  configure.ac       | 3 +++
>  src/Makefile.am    | 6 ++++++
>  src/program_name.c | 4 ++++
>  3 files changed, 13 insertions(+)
>  create mode 100644 src/program_name.c
src/guestfs-internal-frontend.h has a #define to replace use
of 'program_name' with something that is defined, so this
should not be required unless the header includes are not
right somewhere.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

On Monday 09 February 2015 11:06:17 Margaret Lewicka
wrote:
> On JDK 8, doclint is enabled by default with strict validation of HTML
> tags, which causes the build to fail. See
> http://blog.joda.org/2014/02/turning-off-doclint-in-jdk-8-javadoc.html
> ---
>  java/Makefile.am | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/java/Makefile.am b/java/Makefile.am
> index fa5b45c..a76a9d1 100644
> --- a/java/Makefile.am
> +++ b/java/Makefile.am
> @@ -68,7 +68,7 @@ clean-local:
>  if HAVE_JAVA
>  
>  JAVAC_FLAGS = $(EXTRA_JAVAC_FLAGS) -encoding utf-8
> -JAVADOC_FLAGS = -encoding utf-8
> +JAVADOC_FLAGS = -encoding utf-8 -Xdoclint:none
I'd say it would be much better to fix the issues doclint issues,
if possible, rather than disabling all of them.

-- 
Pino Toscano

On Mon, Feb 09, 2015 at 11:06:17AM +0000, Margaret Lewicka
wrote:
> On JDK 8, doclint is enabled by default with strict validation of HTML
> tags, which causes the build to fail. See
> http://blog.joda.org/2014/02/turning-off-doclint-in-jdk-8-javadoc.html
> ---
>  java/Makefile.am | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/java/Makefile.am b/java/Makefile.am
> index fa5b45c..a76a9d1 100644
> --- a/java/Makefile.am
> +++ b/java/Makefile.am
> @@ -68,7 +68,7 @@ clean-local:
>  if HAVE_JAVA
>  
>  JAVAC_FLAGS = $(EXTRA_JAVAC_FLAGS) -encoding utf-8
> -JAVADOC_FLAGS = -encoding utf-8
> +JAVADOC_FLAGS = -encoding utf-8 -Xdoclint:none
>  
>  # Java source.
>  
As Pino says it's probably better to fix these.  However I accepted
this patch and have pushed it upstream.

Thanks,

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

On Mon, Feb 09, 2015 at 11:06:18AM +0000, Margaret Lewicka
wrote:
> ---
>  configure.ac                    | 1 +
>  src/guestfs-internal-frontend.h | 2 ++
>  2 files changed, 3 insertions(+)
> 
> diff --git a/configure.ac b/configure.ac
> index a2fb99e..0f7f97e 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -315,6 +315,7 @@ AC_CHECK_FUNCS([\
>      be32toh \
>      fsync \
>      futimens \
> +    getprogname \
>      getxattr \
>      htonl \
>      htons \
> diff --git a/src/guestfs-internal-frontend.h
b/src/guestfs-internal-frontend.h
> index 2b9070b..d496655 100644
> --- a/src/guestfs-internal-frontend.h
> +++ b/src/guestfs-internal-frontend.h
> @@ -133,6 +133,8 @@ extern void guestfs___cleanup_pclose (void *ptr);
>   */
>  #if HAVE_DECL_PROGRAM_INVOCATION_SHORT_NAME == 1
>  #  define guestfs___program_name program_invocation_short_name
> +#elif HAVE_GETPROGNAME
> +#  define guestfs___program_name getprogname()
>  #else
>  #  define guestfs___program_name "libguestfs"
>  #endif
> -- 
Thanks, ACKed and pushed.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html