Re: [Xen-devel] [PATCH 1 of 5] blktap: remove HAVE_BYTESWAP_H checking, since it's defined by qemu

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Mon, 2011-12-19 at 09:58 +0000, Roger Pau Monnà wrote:
> 2011/12/19 Ian Campbell <Ian.Campbell@xxxxxxxxxx>:
> > On Sun, 2011-12-18 at 12:48 +0000, Roger Pau Monne wrote:
> >> # HG changeset patch
> >> # User Roger Pau Monne <roger.pau@xxxxxxxxxxxxx>
> >> # Date 1324171782 -3600
> >> # Node ID eed78eb655c40b0ac9af1b14c1cc03204f696b0b
> >> # Parent  b783e76e63a99c9d87fca4974492f60af99a2e7a
> >> blktap: remove HAVE_BYTESWAP_H checking, since it's defined by qemu
> >>
> >> blktap was using defines set by qemu, even when the qemu config file
> >> is not included. Remove this checkings, and instead check if the file
> >> has been included before defining the necessary macros.
> >>
> >> The output error is:
> >>
> >> In file included from block-qcow.c:37:0:
> >> bswap.h:23:0: error: "bswap_16" redefined [-Werror]
> >> /usr/include/byteswap.h:30:0: note: this is the location of the
> >> previous definition
> >> bswap.h:31:0: error: "bswap_32" redefined [-Werror]
> >> /usr/include/byteswap.h:33:0: note: this is the location of the
> >> previous definition
> >> bswap.h:41:0: error: "bswap_64" redefined [-Werror]
> >> /usr/include/byteswap.h:37:0: note: this is the location of the
> >> previous definition
> >> cc1: all warnings being treated as errors
> >>
> >> Signed-off-by: Roger Pau Monne <roger.pau@xxxxxxxxxxxxx>
> >>
> >> diff -r b783e76e63a9 -r eed78eb655c4 tools/blktap/drivers/bswap.h
> >> --- a/tools/blktap/drivers/bswap.h    Sun Dec 18 02:29:42 2011 +0100
> >> +++ b/tools/blktap/drivers/bswap.h    Sun Dec 18 02:29:42 2011 +0100
> >> @@ -15,9 +15,7 @@
> >>  #define bswap_64(x) swap64(x)
> >>  #else
> >>
> >> -#ifdef HAVE_BYTESWAP_H
> >> -#include <byteswap.h>
> >> -#else
> >> +#ifndef _BYTESWAP_H
> >
> > This is basically saying "if the user hasn't already include byteswap.h"
> > but it seems to rely on that header using the precise guard _BYTESWAP_H
> > which I presume can differ across platforms. I don't think this is a
> > viable approach.
> 
> From what I saw _BYTESWAP_H is defined in uclibc and glibc (Solaris
> and NetBSD have their own preprocessor logic, so this doesn't apply to
> them). It's just a safeguard that avoids redefining the macros if
> someone has included byteswap.h behind our backs.

It's not an effective safeguard though.

> > Given that we have our own definitions of these things any why not just
> > remove the ifdef and single the other #include of byteswap.h and always
> > use the ones we defined?
> >
> > The other alternative is to remove our own version and always include
> > byteswap.h.
> >
> > The right answer depends on how standardised byteswap.h is. It seems to
> > be in glibc but is it in uclibc and the BSD libcs? I can't find a
> > definitive reference but it seems like it is specified by POSIX?
> 
> The solutions I see are:
> 
>  * Check if byteswap.h has been included (this patch).
>  * undef bswap_* functions and define our own.
>  * Include byteswap.h.

   * Never include byteswap.h and use our own bswap_* functions which we
     already define.

I think this is the correct option. No need to undef stuff. There is
only one other include of byteswap.h in blktap.

Ian.



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel