[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 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 9 February 2015 at 13:10, Daniel P. Berrange <berrange@redhat.com> wrote:
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.
It is required, because gnulib itself explicitly defines an extern char *program_name. See gnulib/lib/error.c:118 -- M.
On Mon, Feb 09, 2015 at 02:55:58PM +0000, Margaret Lewicka wrote:
On 9 February 2015 at 13:10, Daniel P. Berrange <berrange@redhat.com> wrote:
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.
It is required, because gnulib itself explicitly defines an extern char *program_name. See gnulib/lib/error.c:118
What actually happens in gnulib's error.c is: #ifdef _LIBC /* In the GNU C library, there is a predefined variable for this. */ # define program_name program_invocation_name #endif which on glibc causes all uses of `program_name' in the error.c source to refer to refer to a string that glibc defines called `program_invocation_name': extern char *program_invocation_name; /* defined in glibc, same as argv[0]*/ On non-glibc, error.c says: /* The calling program should define program_name and set it to the name of the executing program. */ extern char *program_name; This indicates to me that we shouldn't define `program_name' as a symbol in libguestfs. Instead every libguestfs-using program should define it. For example, virt-df (df/main.c) would have: #if /* this is Mac OS X */ char *program_name = "virt-df"; #endif That sucks a lot. I think the alternative is that you find a similar symbol defined by Mac OS X that gives you the value of argv[0], and then send a patch to gnulib which does: #if /* this is Mac OS X */ #define program_name whatever_mac_os_x_defines_as_program_name #endif My reading is that something like this might work, untested of course: #if /* this is Mac OS X */ #define program_name (((char **)*_NSGetArgv())[0]) #endif 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 11 February 2015 at 13:46, Richard W.M. Jones <rjones@redhat.com> wrote: [...]
On non-glibc, error.c says:
/* The calling program should define program_name and set it to the name of the executing program. */ extern char *program_name;
This indicates to me that we shouldn't define `program_name' as a symbol in libguestfs. Instead every libguestfs-using program should define it. For example, virt-df (df/main.c) would have:
#if /* this is Mac OS X */ char *program_name = "virt-df"; #endif
That sucks a lot.
That was my first approach, but adding a Darwin-specific ifdef to every executable shipped with libguestfs seemed much more intrusive than sneaking it in as a symbol in the library. I do see your point, though. Proposing a patch to gnulib, even should they accept it, does not fix the underlying issue, which is, essentially, that gnulib requires that symbol and libguestfs binaries don't supply it. There might be another OS that trips over this, after all. So, ugly as it is, I think I will attempt to add the program_name definition where necessary, in the form of: #if !_LIBC /* feels right to use the same condition as gnulib does */ char *program_name = name-of-the-binary #endif Unless of course the ugliness of the solution is not acceptable to you as the owner. (I do wish that you haven't gone away from using the progname module in 2013; the problem would be nonexistent.)
I think the alternative is that you find a similar symbol defined by Mac OS X that gives you the value of argv[0], and then send a patch to gnulib which does:
#if /* this is Mac OS X */ #define program_name whatever_mac_os_x_defines_as_program_name #endif
My reading is that something like this might work, untested of course:
#if /* this is Mac OS X */ #define program_name (((char **)*_NSGetArgv())[0]) #endif
Unfortunately I lack the knowledge to verify whether this is applicable, so I'd rather use the blunt-but-guaranteed approach. Unless there is someone else more capable and willing to fix it in a prettier way, of course! -- M.
On Wed, Feb 11, 2015 at 07:20:33PM +0000, Margaret Lewicka wrote:
On 11 February 2015 at 13:46, Richard W.M. Jones <rjones@redhat.com> wrote: [...]
On non-glibc, error.c says:
/* The calling program should define program_name and set it to the name of the executing program. */ extern char *program_name;
This indicates to me that we shouldn't define `program_name' as a symbol in libguestfs. Instead every libguestfs-using program should define it. For example, virt-df (df/main.c) would have:
#if /* this is Mac OS X */ char *program_name = "virt-df"; #endif
That sucks a lot.
That was my first approach, but adding a Darwin-specific ifdef to every executable shipped with libguestfs seemed much more intrusive than sneaking it in as a symbol in the library. I do see your point, though.
Proposing a patch to gnulib, even should they accept it, does not fix the underlying issue, which is, essentially, that gnulib requires that symbol and libguestfs binaries don't supply it. There might be another OS that trips over this, after all.
But it would fix it for Mac OS X, which is better than nothing. As a test, can you see if adding some variation of: #if /* this is Mac OS X */ #define program_name (((char **)*_NSGetArgv())[0]) #endif to libguestfs's gnulib/lib/error.h fixes the problem? It'll save you a lot of time if it does work. 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
On 11 February 2015 at 19:23, Richard W.M. Jones <rjones@redhat.com> wrote:
On Wed, Feb 11, 2015 at 07:20:33PM +0000, Margaret Lewicka wrote: [...]
Proposing a patch to gnulib, even should they accept it, does not fix the underlying issue, which is, essentially, that gnulib requires that symbol and libguestfs binaries don't supply it. There might be another OS that trips over this, after all.
But it would fix it for Mac OS X, which is better than nothing.
As a test, can you see if adding some variation of:
#if /* this is Mac OS X */ #define program_name (((char **)*_NSGetArgv())[0]) #endif
to libguestfs's gnulib/lib/error.h fixes the problem?
It'll save you a lot of time if it does work.
It compiles and libguestfs-test-tool doesn't bomb afterwards, if that is a sufficient definition of "working". -- M.
On Wed, Feb 11, 2015 at 09:52:59PM +0000, Margaret Lewicka wrote:
On 11 February 2015 at 19:23, Richard W.M. Jones <rjones@redhat.com> wrote:
On Wed, Feb 11, 2015 at 07:20:33PM +0000, Margaret Lewicka wrote: [...]
Proposing a patch to gnulib, even should they accept it, does not fix the underlying issue, which is, essentially, that gnulib requires that symbol and libguestfs binaries don't supply it. There might be another OS that trips over this, after all.
But it would fix it for Mac OS X, which is better than nothing.
As a test, can you see if adding some variation of:
#if /* this is Mac OS X */ #define program_name (((char **)*_NSGetArgv())[0]) #endif
to libguestfs's gnulib/lib/error.h fixes the problem?
It'll save you a lot of time if it does work.
It compiles and libguestfs-test-tool doesn't bomb afterwards, if that is a sufficient definition of "working".
Sounds pretty good to me. Please send a final patch to gnulib/lib/error.h and I'll forward it to the gnulib people for you (or you can send it yourself if you want). [In any case, the gnulib error() function is only used in a very few places in the libguestfs virt-* tools. I count just 14 uses, ignoring tests. The main reason we need all this seems to be because the gnulib error module is used by other gnulib modules that we actually care about.] 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
--- lib/error.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/lib/error.c b/lib/error.c index 6683197..36a3db7 100644 --- a/lib/error.c +++ b/lib/error.c @@ -113,9 +113,13 @@ int strerror_r (); # endif # endif +#if defined __APPLE__ && defined __MACH__ +#define program_name (((char **)*_NSGetArgv())[0]) +#else /* The calling program should define program_name and set it to the name of the executing program. */ extern char *program_name; +#endif # if HAVE_STRERROR_R || defined strerror_r # define __strerror_r strerror_r -- 1.9.3
On Thursday 12 February 2015 17:39:45 Margaret Lewicka wrote:
--- lib/error.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/lib/error.c b/lib/error.c index 6683197..36a3db7 100644 --- a/lib/error.c +++ b/lib/error.c @@ -113,9 +113,13 @@ int strerror_r (); # endif # endif
+#if defined __APPLE__ && defined __MACH__ +#define program_name (((char **)*_NSGetArgv())[0]) +#else /* The calling program should define program_name and set it to the name of the executing program. */ extern char *program_name; +#endif
# if HAVE_STRERROR_R || defined strerror_r # define __strerror_r strerror_r
We import gnulib as git submodule from the upstream gnulib.git, so this should be sent to its mailing list; see also https://www.gnu.org/software/gnulib/ (Personally, I'm not sure that the above patch would help, but I'll leave the review to gnulib people on the right ml.) Thanks, -- Pino Toscano
On 12 February 2015 at 18:50, Pino Toscano <ptoscano@redhat.com> wrote: [...]
We import gnulib as git submodule from the upstream gnulib.git, so this should be sent to its mailing list; see also https://www.gnu.org/software/gnulib/
(Personally, I'm not sure that the above patch would help, but I'll leave the review to gnulib people on the right ml.)
This was requested by Richard in <20150211220126.GV11603@redhat.com>; the hack^Wsolution is his, I am merely providing the patch. (Would not send it myself because a) I believe libguestfs's executables should properly define the variable, rather than trying to circumvent the requirement, b) I don't have the knowledge to verify the solution past "it compiles and runs".) -- M.
On Thursday 12 February 2015 18:58:17 Margaret Lewicka wrote:
On 12 February 2015 at 18:50, Pino Toscano <ptoscano@redhat.com> wrote: [...]
We import gnulib as git submodule from the upstream gnulib.git, so this should be sent to its mailing list; see also https://www.gnu.org/software/gnulib/
(Personally, I'm not sure that the above patch would help, but I'll leave the review to gnulib people on the right ml.)
This was requested by Richard in <20150211220126.GV11603@redhat.com>; the hack^Wsolution is his, I am merely providing the patch.
(Would not send it myself because a) I believe libguestfs's executables should properly define the variable, rather than trying to circumvent the requirement, b) I don't have the knowledge to verify the solution past "it compiles and runs".)
In any case, we cannot accept this patch, since it patches a file coming from an external repository. We just integrate gnulib in our workflow, using the modules of it which are needed for us (either to support quirks on different OSes, or to get some extra features). -- Pino Toscano
On 12 February 2015 at 19:01, Pino Toscano <ptoscano@redhat.com> wrote:
On Thursday 12 February 2015 18:58:17 Margaret Lewicka wrote: [...]
This was requested by Richard in <20150211220126.GV11603@redhat.com>; the hack^Wsolution is his, I am merely providing the patch.
(Would not send it myself because a) I believe libguestfs's executables should properly define the variable, rather than trying to circumvent the requirement, b) I don't have the knowledge to verify the solution past "it compiles and runs".)
In any case, we cannot accept this patch, since it patches a file coming from an external repository. We just integrate gnulib in our workflow, using the modules of it which are needed for us (either to support quirks on different OSes, or to get some extra features).
Yes, I understand. Again, I sent this out only on Richard's request: "Please send a final patch to gnulib/lib/error.h and I'll forward it to the gnulib people for you (or you can send it yourself if you want)." -- M.
On Thu, Feb 12, 2015 at 07:09:41PM +0000, Margaret Lewicka wrote:
On 12 February 2015 at 19:01, Pino Toscano <ptoscano@redhat.com> wrote:
On Thursday 12 February 2015 18:58:17 Margaret Lewicka wrote: [...]
This was requested by Richard in <20150211220126.GV11603@redhat.com>; the hack^Wsolution is his, I am merely providing the patch.
(Would not send it myself because a) I believe libguestfs's executables should properly define the variable, rather than trying to circumvent the requirement, b) I don't have the knowledge to verify the solution past "it compiles and runs".)
In any case, we cannot accept this patch, since it patches a file coming from an external repository. We just integrate gnulib in our workflow, using the modules of it which are needed for us (either to support quirks on different OSes, or to get some extra features).
Yes, I understand. Again, I sent this out only on Richard's request: "Please send a final patch to gnulib/lib/error.h and I'll forward it to the gnulib people for you (or you can send it yourself if you want)."
Yup this is fine, don't worry! 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
I posted the patch with an explanatory note on bug-gnulib. It should eventually appear in the archives here: https://lists.gnu.org/archive/html/bug-gnulib/2015-02/threads.html Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org
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
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 9 February 2015 at 13:27, Pino Toscano <ptoscano@redhat.com> wrote:
On Monday 09 February 2015 11:06:17 Margaret Lewicka wrote: [...]
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.
By all means, but in the meantime this is preventing compilation with java bindings enabled. -- M.
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
--- 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
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
* 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 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 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 :|
participants (4)
-
Daniel P. Berrange -
Margaret Lewicka -
Pino Toscano -
Richard W.M. Jones