Bug 94570

Summary: Use BYTE_ORDER to determine byte order
Product: Spice Reporter: Jasper Lievisse Adriaanse <jasper>
Component: protocolAssignee: Spice Bug List <spice-bugs>
Status: RESOLVED MOVED QA Contact:
Severity: normal    
Priority: medium    
Version: unspecified   
Hardware: Other   
OS: All   
Whiteboard:
i915 platform: i915 features:
Attachments: Use BYTE_ORDER to determine byte order
Use _BYTE_ORDER to determine byte order
Check for machine/endian.h
Use _BYTE_ORDER to determine byte order
Check for endian.h in spice-gtk

Description Jasper Lievisse Adriaanse 2016-03-16 13:31:56 UTC
Created attachment 122347 [details]
Use BYTE_ORDER to determine byte order

Currently macros.h goes through various hoops to determine the byte order of the target. This broke the build on OpenBSD/sparc64 as it wasn't listed in hardcoded tests.

So it would be better to use BYTE_ORDER (http://austingroupbugs.net/view.php?id=162) instead and if that's not available try to manually figure out the byte order.

Attached patch implements that and is currently shipped in OpenBSD.
Comment 1 Eduardo Lima (Etrunko) 2016-03-16 15:38:47 UTC
--- spice/macros.h.orig	Thu Mar 10 15:14:49 2016
+++ spice/macros.h	Wed Mar 16 14:18:03 2016
@@ -381,6 +381,17 @@
 #define SPICE_ENDIAN_BIG    1234
 #define SPICE_ENDIAN_PDP    2143
 
+/* Lets see if we can use a standard header first... */
+#if defined(BYTE_ORDER) && defined(LITTLE_ENDIAN) \
+    && defined(BIG_ENDIAN)
+#  include <machine/endian.h>
+#  if BYTE_ORDER == LITTLE_ENDIAN
+#    define SPICE_ENDIAN SPICE_ENDIAN_LITTLE
+#  elif BYTE_ORDER == BIG_ENDIAN
+#    define SPICE_ENDIAN SPICE_ENDIAN_BIG
+#  endif
+#endif
+

So, instead of relying on those defined macros to include that header, why don't you check for it in configure.ac with AC_CHECK_HEADERS macro?

You would end with something like:

#if HAVE_MACHINE_ENDIAN_H
   #include <machine/endian.h>
...
#endif
Comment 2 Frediano Ziglio 2016-03-16 16:00:11 UTC
Actually the code looks weird. First you verify that the three macros are all defined that you include an header to define again these macros.
Also the "standard header" comment is not true, at least if you consider C as the standard.
Comment 3 Jasper Lievisse Adriaanse 2016-03-21 12:57:35 UTC
(In reply to Eduardo Lima (Etrunko) from comment #1)
> --- spice/macros.h.orig	Thu Mar 10 15:14:49 2016
> +++ spice/macros.h	Wed Mar 16 14:18:03 2016
> @@ -381,6 +381,17 @@
>  #define SPICE_ENDIAN_BIG    1234
>  #define SPICE_ENDIAN_PDP    2143
>  
> +/* Lets see if we can use a standard header first... */
> +#if defined(BYTE_ORDER) && defined(LITTLE_ENDIAN) \
> +    && defined(BIG_ENDIAN)
> +#  include <machine/endian.h>
> +#  if BYTE_ORDER == LITTLE_ENDIAN
> +#    define SPICE_ENDIAN SPICE_ENDIAN_LITTLE
> +#  elif BYTE_ORDER == BIG_ENDIAN
> +#    define SPICE_ENDIAN SPICE_ENDIAN_BIG
> +#  endif
> +#endif
> +
> 
> So, instead of relying on those defined macros to include that header, why
> don't you check for it in configure.ac with AC_CHECK_HEADERS macro?
> 
> You would end with something like:
> 
> #if HAVE_MACHINE_ENDIAN_H
>    #include <machine/endian.h>
> ...
> #endif

That won't work out of the box though, as macros.h is a public header. So all projects that use macros.h would have to explicitly add the AC_CHECK_HEADERS check for machine/endian.h, no?

Taking your latter comment into account, how about something along these lines?

+#if defined(BYTE_ORDER)
+#  if BYTE_ORDER == LITTLE_ENDIAN
+#    define SPICE_ENDIAN SPICE_ENDIAN_LITTLE
+#  elif BYTE_ORDER == BIG_ENDIAN
+#    define SPICE_ENDIAN SPICE_ENDIAN_BIG
+#  endif
+#endif

This would guard the code in the sense that it won't be evaluated if endian.h isn't already included through another header.
Comment 4 Frediano Ziglio 2016-04-12 08:03:48 UTC
> 
> Taking your latter comment into account, how about something along these
> lines?
> 
> +#if defined(BYTE_ORDER)
> +#  if BYTE_ORDER == LITTLE_ENDIAN
> +#    define SPICE_ENDIAN SPICE_ENDIAN_LITTLE
> +#  elif BYTE_ORDER == BIG_ENDIAN
> +#    define SPICE_ENDIAN SPICE_ENDIAN_BIG
> +#  endif
> +#endif
> 
> This would guard the code in the sense that it won't be evaluated if
> endian.h isn't already included through another header.

Does this work on *BSD ?
Comment 5 Jasper Lievisse Adriaanse 2016-04-12 08:08:11 UTC
Works fine on OpenBSD at least.
Comment 6 Jasper Lievisse Adriaanse 2016-04-13 07:53:31 UTC
I was mistaken, it does require endian.h to work properly. I'll rework this diff to add a configure check so the inclusion is guarded.
Comment 7 Jasper Lievisse Adriaanse 2016-04-27 10:43:43 UTC
Created attachment 123301 [details] [review]
Use _BYTE_ORDER to determine byte order
Comment 8 Jasper Lievisse Adriaanse 2016-04-27 10:44:56 UTC
Created attachment 123302 [details] [review]
Check for machine/endian.h

Patch for spice-gtk to check for machine/endian.h
Comment 9 Jasper Lievisse Adriaanse 2016-05-02 09:24:48 UTC
Created attachment 123405 [details] [review]
Use _BYTE_ORDER to determine byte order

Updated to actually use the originally intended header, endian.h.
Comment 10 Jasper Lievisse Adriaanse 2016-05-02 09:25:42 UTC
Created attachment 123406 [details] [review]
Check for endian.h in spice-gtk
Comment 11 GitLab Migration User 2018-06-03 10:18:29 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/spice/spice-protocol/issues/1.

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.