Libguestfs - Feb 2015 - Re: [PATCH 1/3] macosx: Includes/defines for byteswap operations

---
 src/inspect-apps.c       | 13 ++++++++++++-
 src/inspect-fs-windows.c |  6 ++++++
 src/journal.c            |  5 +++++
 3 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/src/inspect-apps.c b/src/inspect-apps.c
index 20cf00a..8fbae9c 100644
--- a/src/inspect-apps.c
+++ b/src/inspect-apps.c
@@ -35,11 +35,22 @@
 #include <sys/endian.h>
 #endif
 
-/* be32toh is usually a macro definend in <endian.h>, but it might be
+/* be32toh is usually a macro defined in <endian.h>, but it might be
  * a function in some system so check both, and if neither is defined
  * then define be32toh for RHEL 5.
  */
 #if !defined(HAVE_BE32TOH) && !defined(be32toh)
+
+#if defined __APPLE__ && defined __MACH__
+/* Define/include necessary items on MacOS X */
+#include <machine/endian.h>
+#define __BIG_ENDIAN    BIG_ENDIAN
+#define __LITTLE_ENDIAN   LITTLE_ENDIAN
+#define __BYTE_ORDER    BYTE_ORDER
+#include <libkern/OSByteOrder.h>
+#define __bswap_32      OSSwapConstInt32
+#endif /* __APPLE__ */
+
 #if __BYTE_ORDER == __LITTLE_ENDIAN
 #define be32toh(x) __bswap_32 (x)
 #else
diff --git a/src/inspect-fs-windows.c b/src/inspect-fs-windows.c
index 682157a..421a5b1 100644
--- a/src/inspect-fs-windows.c
+++ b/src/inspect-fs-windows.c
@@ -36,6 +36,12 @@
 #include <sys/endian.h>
 #endif
 
+#if defined __APPLE__ && defined __MACH__
+#include <libkern/OSByteOrder.h>
+#define le32toh(x) OSSwapLittleToHostInt32(x)
+#define le64toh(x) OSSwapLittleToHostInt64(x)
+#endif
+
 #include <pcre.h>
 
 #include "c-ctype.h"
diff --git a/src/journal.c b/src/journal.c
index 1070067..c563b7f 100644
--- a/src/journal.c
+++ b/src/journal.c
@@ -35,6 +35,11 @@
 #include <sys/endian.h>
 #endif
 
+#if defined __APPLE__ && defined __MACH__
+#include <libkern/OSByteOrder.h>
+#define be64toh(x) OSSwapBigToHostInt64(x)
+#endif
+
 #include "full-read.h"
 
 #include "guestfs.h"
-- 
1.9.3

---
 builder/pxzcat-c.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/builder/pxzcat-c.c b/builder/pxzcat-c.c
index 0bbd296..dec9cc2 100644
--- a/builder/pxzcat-c.c
+++ b/builder/pxzcat-c.c
@@ -214,8 +214,10 @@ pxzcat (value filenamev, value outputfilev, unsigned
nr_threads)
     unix_error (err, (char *) "ftruncate", outputfilev);
   }
 
+#if defined HAVE_POSIX_FADVISE
   /* Tell the kernel we won't read the output file. */
   ignore_value (posix_fadvise (fd, 0, 0,
POSIX_FADV_RANDOM|POSIX_FADV_DONTNEED));
+#endif
 
   /* Iterate over blocks. */
   iter_blocks (idx, nr_threads, filenamev, fd, outputfilev, ofd);
-- 
1.9.3

As advised by Daniel P. Berrange, the third parameter can be passed on all
platforms rather than specifically Mac.

Quoting a libvirt commit rationale after Daniel:

 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:

   typedef 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.
---
 src/proto.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/proto.c b/src/proto.c
index 92ae84d..e229abb 100644
--- a/src/proto.c
+++ b/src/proto.c
@@ -252,7 +252,7 @@ guestfs___send (guestfs_h *g, int proc_nr,
    * have no parameters.
    */
   if (xdrp) {
-    if (!(*xdrp) (&xdr, args)) {
+    if (!(*xdrp) (&xdr, args, 0)) {
       error (g, _("dispatch failed to marshal args"));
       return -1;
     }
@@ -681,7 +681,7 @@ guestfs___recv (guestfs_h *g, const char *fn,
       return -1;
     }
   } else {
-    if (xdrp && ret && !xdrp (&xdr, ret)) {
+    if (xdrp && ret && !xdrp (&xdr, ret, 0)) {
       error (g, "%s: failed to parse reply", fn);
       xdr_destroy (&xdr);
       return -1;
-- 
1.9.3

On Thursday 12 February 2015 17:28:46 Margaret Lewicka
wrote:
> ---
>  src/inspect-apps.c       | 13 ++++++++++++-
>  src/inspect-fs-windows.c |  6 ++++++
>  src/journal.c            |  5 +++++
>  3 files changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/src/inspect-apps.c b/src/inspect-apps.c
> index 20cf00a..8fbae9c 100644
> --- a/src/inspect-apps.c
> +++ b/src/inspect-apps.c
> @@ -35,11 +35,22 @@
>  #include <sys/endian.h>
>  #endif
>  
> -/* be32toh is usually a macro definend in <endian.h>, but it might
be
> +/* be32toh is usually a macro defined in <endian.h>, but it might be
>   * a function in some system so check both, and if neither is defined
>   * then define be32toh for RHEL 5.
>   */
>  #if !defined(HAVE_BE32TOH) && !defined(be32toh)
> +
> +#if defined __APPLE__ && defined __MACH__
> +/* Define/include necessary items on MacOS X */
> +#include <machine/endian.h>
> +#define __BIG_ENDIAN    BIG_ENDIAN
> +#define __LITTLE_ENDIAN   LITTLE_ENDIAN
> +#define __BYTE_ORDER    BYTE_ORDER
> +#include <libkern/OSByteOrder.h>
> +#define __bswap_32      OSSwapConstInt32
> +#endif /* __APPLE__ */
> +
>  #if __BYTE_ORDER == __LITTLE_ENDIAN
>  #define be32toh(x) __bswap_32 (x)
>  #else
> diff --git a/src/inspect-fs-windows.c b/src/inspect-fs-windows.c
> index 682157a..421a5b1 100644
> --- a/src/inspect-fs-windows.c
> +++ b/src/inspect-fs-windows.c
> @@ -36,6 +36,12 @@
>  #include <sys/endian.h>
>  #endif
>  
> +#if defined __APPLE__ && defined __MACH__
> +#include <libkern/OSByteOrder.h>
> +#define le32toh(x) OSSwapLittleToHostInt32(x)
> +#define le64toh(x) OSSwapLittleToHostInt64(x)
> +#endif
> +
>  #include <pcre.h>
>  
>  #include "c-ctype.h"
> diff --git a/src/journal.c b/src/journal.c
> index 1070067..c563b7f 100644
> --- a/src/journal.c
> +++ b/src/journal.c
> @@ -35,6 +35,11 @@
>  #include <sys/endian.h>
>  #endif
>  
> +#if defined __APPLE__ && defined __MACH__
> +#include <libkern/OSByteOrder.h>
> +#define be64toh(x) OSSwapBigToHostInt64(x)
> +#endif
> +
>  #include "full-read.h"
>  
>  #include "guestfs.h"
This code clearly need some refactoring, before adding more copy/pasted
parts to it.  I'd say to do this in two steps:
a) moving the definitions of the current be32toh/etc to a new
   src/guestfs-byteswap.h (internal), making use of it
b) add the proper definitions needed on Mac OS X

Thanks,
-- 
Pino Toscano

On Thursday 12 February 2015 17:28:47 Margaret Lewicka
wrote:
> ---
>  builder/pxzcat-c.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/builder/pxzcat-c.c b/builder/pxzcat-c.c
> index 0bbd296..dec9cc2 100644
> --- a/builder/pxzcat-c.c
> +++ b/builder/pxzcat-c.c
> @@ -214,8 +214,10 @@ pxzcat (value filenamev, value outputfilev, unsigned
nr_threads)
>      unix_error (err, (char *) "ftruncate", outputfilev);
>    }
>  
> +#if defined HAVE_POSIX_FADVISE
>    /* Tell the kernel we won't read the output file. */
>    ignore_value (posix_fadvise (fd, 0, 0,
POSIX_FADV_RANDOM|POSIX_FADV_DONTNEED));
> +#endif
>  
>    /* Iterate over blocks. */
>    iter_blocks (idx, nr_threads, filenamev, fd, outputfilev, ofd);
> 
LGTM.

-- 
Pino Toscano

On 12 February 2015 at 18:28, Pino Toscano <ptoscano@redhat.com> wrote:
[much byteswapping]
> This code clearly need some refactoring, before adding more copy/pasted
> parts to it.  I'd say to do this in two steps:
> a) moving the definitions of the current be32toh/etc to a new
>    src/guestfs-byteswap.h (internal), making use of it
> b) add the proper definitions needed on Mac OS X
Heh. Yet again, I should have gone with the first hunch, but
refactoring existing code seemed, um, preposterous for a newbie.
Thanks for the suggestion.

Is it generally preferred to post updated versions of the same patch
(or rather new approaches to patching the same code) with an
In-Reply-To: to previous patch, or in a new thread?

-- 
M.

On Thu, Feb 12, 2015 at 05:28:47PM +0000, Margaret Lewicka
wrote:
> ---
>  builder/pxzcat-c.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/builder/pxzcat-c.c b/builder/pxzcat-c.c
> index 0bbd296..dec9cc2 100644
> --- a/builder/pxzcat-c.c
> +++ b/builder/pxzcat-c.c
> @@ -214,8 +214,10 @@ pxzcat (value filenamev, value outputfilev, unsigned
nr_threads)
>      unix_error (err, (char *) "ftruncate", outputfilev);
>    }
>  
> +#if defined HAVE_POSIX_FADVISE
>    /* Tell the kernel we won't read the output file. */
>    ignore_value (posix_fadvise (fd, 0, 0,
POSIX_FADV_RANDOM|POSIX_FADV_DONTNEED));
> +#endif
>  
>    /* Iterate over blocks. */
>    iter_blocks (idx, nr_threads, filenamev, fd, outputfilev, ofd);
> -- 
> 1.9.3
Pushed.

Thanks,

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

On Thu, Feb 12, 2015 at 05:28:48PM +0000, Margaret Lewicka
wrote:
> As advised by Daniel P. Berrange, the third parameter can be passed on all
> platforms rather than specifically Mac.
> 
> Quoting a libvirt commit rationale after Daniel:
> 
>  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:
> 
>    typedef 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.
> ---
>  src/proto.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/proto.c b/src/proto.c
> index 92ae84d..e229abb 100644
> --- a/src/proto.c
> +++ b/src/proto.c
> @@ -252,7 +252,7 @@ guestfs___send (guestfs_h *g, int proc_nr,
>     * have no parameters.
>     */
>    if (xdrp) {
> -    if (!(*xdrp) (&xdr, args)) {
> +    if (!(*xdrp) (&xdr, args, 0)) {
>        error (g, _("dispatch failed to marshal args"));
>        return -1;
>      }
> @@ -681,7 +681,7 @@ guestfs___recv (guestfs_h *g, const char *fn,
>        return -1;
>      }
>    } else {
> -    if (xdrp && ret && !xdrp (&xdr, ret)) {
> +    if (xdrp && ret && !xdrp (&xdr, ret, 0)) {
>        error (g, "%s: failed to parse reply", fn);
>        xdr_destroy (&xdr);
>        return -1;
> -- 
> 1.9.3
This works fine for me, so pushed.

Thanks,

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

As Pino said this could probably do with a bit of refactoring to clean
it up, but I've committed it anyway because it's a step towards fixing
OS X builds.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/