[libvirt] [PATCH] MacOS: Handle changes to args in xdrproc_t
With Mac OS X 10.9, xdrproc_t is no longer defined as: typedef bool_t (*xdrproc_t) (XDR *, void *, ...); but instead as typedef bool-t (*xdrproc_t) (XDR *, void *, unsigned int); The rationale explained in the header is that using a vararg is incorrect and has a potential to change the ABI slightly. 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. --- configure.ac | 41 +++++++++++++++++++++++++++++++++++++++++ src/rpc/virnetmessage.c | 10 ++++++++-- 2 files changed, 49 insertions(+), 2 deletions(-) diff --git a/configure.ac b/configure.ac index 1c5b168..f2bae88 100644 --- a/configure.ac +++ b/configure.ac @@ -697,6 +697,47 @@ if test x"$with_remote" = x"yes" || test x"$with_libvirtd" = x"yes"; then *) XDR_CFLAGS=$lv_cv_xdr_cflags ;; esac AC_SUBST([XDR_CFLAGS]) + + dnl Mac OS X Mavericks (10.9) dropped the varargs definition that + dnl allowed 2 or 3 parameters to xdrproc_t callbacks and decided on + dnl 3 arguments being a must. + old_CFLAGS=$CFLAGS + AC_CACHE_CHECK([whether xdrproc_t callbacks take 2 or 3 args], + [lv_cv_xdrproc_t_args], [ + CFLAGS="$old_CFLAGS $XDR_CFLAGS" + AC_COMPILE_IFELSE([AC_LANG_PROGRAM( + [[ + #include <rpc/rpc.h> + ]], + [[ + XDR xdr; + xdrproc_t filter = NULL; + + return (filter)(&xdr, NULL); + ]])], + [lv_cv_xdrproc_t_args=2], [ + AC_COMPILE_IFELSE([AC_LANG_PROGRAM( + [[ + #include <rpc/rpc.h> + ]], + [[ + XDR xdr; + xdrproc_t filter = NULL; + + return (filter)(&xdr, NULL, 0); + ]])], + [lv_cv_xdrproc_t_args=3], + [lv_cv_xdrproc_t_args=unknown]) + ]) + CFLAGS="$old_CFLAGS" + ]) + case $lv_cv_xdrproc_t_args in + unknown) AC_MSG_ERROR([cannot determine arg count for xdrproc_t]) ;; + *) + AC_DEFINE_UNQUOTED([XDRPROC_T_ARG_COUNT], [$lv_cv_xdrproc_t_args], + [number of arguments that xdrproc_t func ptr takes]) + ;; + esac fi diff --git a/src/rpc/virnetmessage.c b/src/rpc/virnetmessage.c index d60366b..79e496f 100644 --- a/src/rpc/virnetmessage.c +++ b/src/rpc/virnetmessage.c @@ -33,6 +33,12 @@ #define VIR_FROM_THIS VIR_FROM_RPC +#if XDRPROC_T_ARG_COUNT == 3 +# define VIR_XDRPROC(proc, xdr, data) ((proc)((xdr), (data), 0)) +#else +# define VIR_XDRPROC(proc, xdr, data) ((proc)((xdr), (data))) +#endif + virNetMessagePtr virNetMessageNew(bool tracked) { virNetMessagePtr msg; @@ -345,7 +351,7 @@ int virNetMessageEncodePayload(virNetMessagePtr msg, msg->bufferLength - msg->bufferOffset, XDR_ENCODE); /* Try to encode the payload. If the buffer is too small increase it. */ - while (!(*filter)(&xdr, data)) { + while (!VIR_XDRPROC(*filter, &xdr, data)) { unsigned int newlen = (msg->bufferLength - VIR_NET_MESSAGE_LEN_MAX) * 4; if (newlen > VIR_NET_MESSAGE_MAX) { @@ -402,7 +408,7 @@ int virNetMessageDecodePayload(virNetMessagePtr msg, xdrmem_create(&xdr, msg->buffer + msg->bufferOffset, msg->bufferLength - msg->bufferOffset, XDR_DECODE); - if (!(*filter)(&xdr, data)) { + if (!VIR_XDRPROC(*filter, &xdr, data)) { virReportError(VIR_ERR_RPC, "%s", _("Unable to decode message payload")); goto error; } -- 1.8.1.5
On Mon, Oct 28, 2013 at 1:05 PM, Doug Goldstein <cardoe@cardoe.com> wrote:
With Mac OS X 10.9, xdrproc_t is no longer defined as:
typedef bool_t (*xdrproc_t) (XDR *, void *, ...);
This was the typedef from Linux accidentally so I've updated that as well.
but instead as
typedef bool-t (*xdrproc_t) (XDR *, void *, unsigned int);
I fixed the typo above in "bool_t". For some more information, I was originally thinking about just passing the third argument as 0 always within our code but unfortunately that would break at least on uclibc where they had the same concerns as Apple but instead changed the prototype to just 2 parameters. If people would prefer that I can submit that change instead. -- Doug Goldstein
On 10/28/2013 12:05 PM, Doug Goldstein wrote:
With Mac OS X 10.9, xdrproc_t is no longer defined as:
typedef bool_t (*xdrproc_t) (XDR *, void *, ...);
but instead as
typedef bool-t (*xdrproc_t) (XDR *, void *, unsigned int);
The rationale explained in the header is that using a vararg is incorrect and has a potential to change the ABI slightly. 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. --- configure.ac | 41 +++++++++++++++++++++++++++++++++++++++++ src/rpc/virnetmessage.c | 10 ++++++++-- 2 files changed, 49 insertions(+), 2 deletions(-)
I'd like some feedback from someone else who can actually test this on MacOS, as well as FreeBSD, but it does seem reasonable to get in 1.1.4.
diff --git a/configure.ac b/configure.ac index 1c5b168..f2bae88 100644 --- a/configure.ac +++ b/configure.ac @@ -697,6 +697,47 @@ if test x"$with_remote" = x"yes" || test x"$with_libvirtd" = x"yes"; then *) XDR_CFLAGS=$lv_cv_xdr_cflags ;; esac AC_SUBST([XDR_CFLAGS])
Not your fault, but we probably ought to move xdr stuff into m4/virt-xdr.m4 - but that can wait till post-release.
+ AC_DEFINE_UNQUOTED([XDRPROC_T_ARG_COUNT], [$lv_cv_xdrproc_t_args], + [number of arguments that xdrproc_t func ptr takes])
Seems reasonable; but I'm a bit worried about accepting args=2 in the cases where we actually needed the varargs to pass 3. It may be safer to pass 3 always, unless we have empirical evidence that uclibc will fail to compile if we don't limit to exactly 2 (and not just a thread archives where they were contemplating forcing just 2, but where I don't know if the thread was actually applied as a patch).
+#if XDRPROC_T_ARG_COUNT == 3 +# define VIR_XDRPROC(proc, xdr, data) ((proc)((xdr), (data), 0)) +#else +# define VIR_XDRPROC(proc, xdr, data) ((proc)((xdr), (data))) +#endif
This seems like a nice abstraction for the problem at hand. Here's hoping we get someone to provide test results in a timely manner. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Tue, Oct 29, 2013 at 6:28 PM, Eric Blake <eblake@redhat.com> wrote:
On 10/28/2013 12:05 PM, Doug Goldstein wrote:
With Mac OS X 10.9, xdrproc_t is no longer defined as:
typedef bool_t (*xdrproc_t) (XDR *, void *, ...);
but instead as
typedef bool-t (*xdrproc_t) (XDR *, void *, unsigned int);
The rationale explained in the header is that using a vararg is incorrect and has a potential to change the ABI slightly. 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. --- configure.ac | 41 +++++++++++++++++++++++++++++++++++++++++ src/rpc/virnetmessage.c | 10 ++++++++-- 2 files changed, 49 insertions(+), 2 deletions(-)
I'd like some feedback from someone else who can actually test this on MacOS, as well as FreeBSD, but it does seem reasonable to get in 1.1.4.
Gave this a change a whirl on FreeBSD 9.2 and it was fine. But obviously more testing is better.
+ AC_DEFINE_UNQUOTED([XDRPROC_T_ARG_COUNT], [$lv_cv_xdrproc_t_args], + [number of arguments that xdrproc_t func ptr takes])
Seems reasonable; but I'm a bit worried about accepting args=2 in the cases where we actually needed the varargs to pass 3. It may be safer to pass 3 always, unless we have empirical evidence that uclibc will fail to compile if we don't limit to exactly 2 (and not just a thread archives where they were contemplating forcing just 2, but where I don't know if the thread was actually applied as a patch).
fwiw, it appears that uclibc master [1] has not gone that route so I'm not sure what became of that thread. Hard coding our implementation to always pass 3 arguments was my other approach that I had mentioned on IRC but I wasn't sure about any negative repercussions on other platforms. [1] http://git.uclibc.org/uClibc/tree/include/rpc/xdr.h#n149 -- Doug Goldstein
On Wed, Oct 30, 2013 at 1:30 PM, Doug Goldstein <cardoe@cardoe.com> wrote:
On Tue, Oct 29, 2013 at 6:28 PM, Eric Blake <eblake@redhat.com> wrote:
On 10/28/2013 12:05 PM, Doug Goldstein wrote:
With Mac OS X 10.9, xdrproc_t is no longer defined as:
typedef bool_t (*xdrproc_t) (XDR *, void *, ...);
but instead as
typedef bool-t (*xdrproc_t) (XDR *, void *, unsigned int);
The rationale explained in the header is that using a vararg is incorrect and has a potential to change the ABI slightly. 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. --- configure.ac | 41 +++++++++++++++++++++++++++++++++++++++++ src/rpc/virnetmessage.c | 10 ++++++++-- 2 files changed, 49 insertions(+), 2 deletions(-)
I'd like some feedback from someone else who can actually test this on MacOS, as well as FreeBSD, but it does seem reasonable to get in 1.1.4.
Gave this a change a whirl on FreeBSD 9.2 and it was fine. But obviously more testing is better.
I tested this patch on FreeBSD 9.2, Mac OS X 0.7.4 and 10.8.5. It worked on all of them. ozaki-r
+ AC_DEFINE_UNQUOTED([XDRPROC_T_ARG_COUNT], [$lv_cv_xdrproc_t_args], + [number of arguments that xdrproc_t func ptr takes])
Seems reasonable; but I'm a bit worried about accepting args=2 in the cases where we actually needed the varargs to pass 3. It may be safer to pass 3 always, unless we have empirical evidence that uclibc will fail to compile if we don't limit to exactly 2 (and not just a thread archives where they were contemplating forcing just 2, but where I don't know if the thread was actually applied as a patch).
fwiw, it appears that uclibc master [1] has not gone that route so I'm not sure what became of that thread. Hard coding our implementation to always pass 3 arguments was my other approach that I had mentioned on IRC but I wasn't sure about any negative repercussions on other platforms.
[1] http://git.uclibc.org/uClibc/tree/include/rpc/xdr.h#n149
-- Doug Goldstein
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Nov 2, 2013, at 11:01, Ryota Ozaki wrote:
I tested this patch on FreeBSD 9.2, Mac OS X 0.7.4 and 10.8.5. It worked on all of them.
Further data points: I successfully built libvirt 1.1.3 with this patch (in MacPorts) on Mac OS X 10.5.8 (on both an Intel Mac and a PowerPC Mac). libvirt doesn’t build on Mac OS X 10.4.11 (on a PowerPC Mac), but not because of this patch.
On Sat, Nov 2, 2013 at 4:06 PM, Ryan Schmidt <libvirt-2013d@ryandesign.com> wrote:
On Nov 2, 2013, at 11:01, Ryota Ozaki wrote:
I tested this patch on FreeBSD 9.2, Mac OS X 0.7.4 and 10.8.5. It worked on all of them.
Further data points: I successfully built libvirt 1.1.3 with this patch (in MacPorts) on Mac OS X 10.5.8 (on both an Intel Mac and a PowerPC Mac).
libvirt doesn’t build on Mac OS X 10.4.11 (on a PowerPC Mac), but not because of this patch.
Thanks for the feedback guys. I pushed the other patch (the same that went into Homebrew and MacPorts) to master. I'll keep this one around just in case the other breaks a platform we didn't anticipate and need to change course. -- Doug Goldstein
participants (4)
-
Doug Goldstein -
Eric Blake -
Ryan Schmidt -
Ryota Ozaki