Re: [dmidecode] [PATCH] dmidecode: SMBIOS data is little-endian

[Neil Horman]

On Mon, Aug 06, 2018 at 04:29:37PM +0200, Jean Delvare wrote:
> I originally assumed that the SMBIOS data would use the byte ordering
> of the host, however since v2.8.0 the SMBIOS specification clarified
> that the SMBIOS data is always little-endian. So change the access
> helpers accordingly.
> 
> This is good news as far as portability is concerned: we can dump the
> table of one host and decode it on another.
> 
This all looks correct, but couldn't we simplify it by simply aliasing
WORD/DWORD/QWORD to ntohs/ntohl?  Those are posix compliant, and would avoid you
having to re-invent the wheel here.  They could be used directly of course, but
if you already have these macros in place, it seems like a good opportunity to
do some code elimination

Neil

> Signed-off-by: Jean Delvare <address@hidden>
> ---
>  types.h |   27 ++++++++++++---------------
>  1 file changed, 12 insertions(+), 15 deletions(-)
> 
> --- dmidecode.orig/types.h    2018-08-06 16:09:54.272472516 +0200
> +++ dmidecode/types.h 2018-08-06 16:14:01.015885077 +0200
> @@ -11,8 +11,7 @@ typedef unsigned int u32;
>  /*
>   * You may use the following defines to adjust the type definitions
>   * depending on the architecture:
> - * - Define BIGENDIAN on big-endian systems. Untested, as all target
> - *   systems to date are little-endian.
> + * - Define BIGENDIAN on big-endian systems.
>   * - Define ALIGNMENT_WORKAROUND if your system doesn't support
>   *   non-aligned memory access. In this case, we use a slower, but safer,
>   *   memory access method. This should be done automatically in config.h
> @@ -31,7 +30,7 @@ typedef struct {
>  } u64;
>  #endif
>  
> -#ifdef ALIGNMENT_WORKAROUND
> +#if defined(ALIGNMENT_WORKAROUND) || defined(BIGENDIAN)
>  static inline u64 U64(u32 low, u32 high)
>  {
>       u64 self;
> @@ -43,20 +42,18 @@ static inline u64 U64(u32 low, u32 high)
>  }
>  #endif
>  
> -#ifdef ALIGNMENT_WORKAROUND
> -#    ifdef BIGENDIAN
> -#    define WORD(x) (u16)((x)[1] + ((x)[0] << 8))
> -#    define DWORD(x) (u32)((x)[3] + ((x)[2] << 8) + ((x)[1] << 16) + ((x)[0] 
> << 24))
> -#    define QWORD(x) (U64(DWORD(x + 4), DWORD(x)))
> -#    else /* BIGENDIAN */
> -#    define WORD(x) (u16)((x)[0] + ((x)[1] << 8))
> -#    define DWORD(x) (u32)((x)[0] + ((x)[1] << 8) + ((x)[2] << 16) + ((x)[3] 
> << 24))
> -#    define QWORD(x) (U64(DWORD(x), DWORD(x + 4)))
> -#    endif /* BIGENDIAN */
> -#else /* ALIGNMENT_WORKAROUND */
> +/*
> + * Per SMBIOS v2.8.0 and later, all structures assume a little-endian
> + * ordering convention.
> + */
> +#if defined(ALIGNMENT_WORKAROUND) || defined(BIGENDIAN)
> +#define WORD(x) (u16)((x)[0] + ((x)[1] << 8))
> +#define DWORD(x) (u32)((x)[0] + ((x)[1] << 8) + ((x)[2] << 16) + ((x)[3] << 
> 24))
> +#define QWORD(x) (U64(DWORD(x), DWORD(x + 4)))
> +#else /* ALIGNMENT_WORKAROUND || BIGENDIAN */
>  #define WORD(x) (u16)(*(const u16 *)(x))
>  #define DWORD(x) (u32)(*(const u32 *)(x))
>  #define QWORD(x) (*(const u64 *)(x))
> -#endif /* ALIGNMENT_WORKAROUND */
> +#endif /* ALIGNMENT_WORKAROUND || BIGENDIAN */
>  
>  #endif
> 
> 
> -- 
> Jean Delvare
> SUSE L3 Support
> 
> _______________________________________________
> https://lists.nongnu.org/mailman/listinfo/dmidecode-devel
>