[PATCH v2 0/5] General Build Fixes

[PATCH v2 0/5] General Build Fixes

[Alistair Francis]

This patch series is a list of build issues that appeared when
buildling Xen 4.8.0 in buildroot. Hopefully some of them can be
accepted upstream to help others who are trying to build Xen in
the future.

V2:
 - Remove the #include path changes
    - It turns out this only applies to musl (although it shouldn't
      break glibc though). Instead of renaming the #include files I
      have just removed the -Werror flags.
 - Allow people building Xen to pass in EXTRA_CFLAGS.

Alistair Francis (5):
  Remove hardcoded strict -Werror checking
  config/StdGNU.mk: Allows users to pass in EXTRA_CFLAGS
  tools/blktap2/vhd: Remove unused struct stat stats
  tools/blktap2: Fix missing header file
  tools/blktap2/drivers: Remove non-existent sys/sysctl.h include

 Config.mk                              | 2 +-
 config/StdGNU.mk                       | 3 +++
 tools/blktap2/drivers/Makefile         | 1 -
 tools/blktap2/drivers/block-remus.c    | 1 -
 tools/blktap2/include/atomicio.h       | 2 ++
 tools/blktap2/vhd/lib/libvhd-journal.c | 1 -
 tools/libxl/Makefile                   | 2 +-
 tools/xentrace/Makefile                | 2 --
 8 files changed, 7 insertions(+), 7 deletions(-)

-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v2 1/5] Remove hardcoded strict -Werror checking

[Alistair Francis]

Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---
 Config.mk                      | 2 +-
 tools/blktap2/drivers/Makefile | 1 -
 tools/libxl/Makefile           | 2 +-
 tools/xentrace/Makefile        | 2 --
 4 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/Config.mk b/Config.mk
index 3ec7367..e3cda81 100644
--- a/Config.mk
+++ b/Config.mk
@@ -34,7 +34,7 @@ CONFIG_$(XEN_OS) := y
 SHELL     ?= /bin/sh
 
 # Tools to run on system hosting the build
-HOSTCFLAGS  = -Wall -Werror -Wstrict-prototypes -O2 -fomit-frame-pointer
+HOSTCFLAGS  = -Wall -Wstrict-prototypes -O2 -fomit-frame-pointer
 HOSTCFLAGS += -fno-strict-aliasing
 
 DISTDIR     ?= $(XEN_ROOT)/dist
diff --git a/tools/blktap2/drivers/Makefile b/tools/blktap2/drivers/Makefile
index 5328c40..7a62a3f 100644
--- a/tools/blktap2/drivers/Makefile
+++ b/tools/blktap2/drivers/Makefile
@@ -9,7 +9,6 @@ QCOW_UTIL  = img2qcow qcow-create qcow2raw
 LOCK_UTIL  = lock-util
 INST_DIR   = $(sbindir)
 
-CFLAGS    += -Werror
 CFLAGS    += -Wno-unused
 CFLAGS    += -fno-strict-aliasing
 CFLAGS    += -I$(BLKTAP_ROOT)/include -I$(BLKTAP_ROOT)/drivers
diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
index 91e2f97..e8a37ef 100644
--- a/tools/libxl/Makefile
+++ b/tools/libxl/Makefile
@@ -11,7 +11,7 @@ MINOR = 0
 XLUMAJOR = 4.9
 XLUMINOR = 0
 
-CFLAGS += -Werror -Wno-format-zero-length -Wmissing-declarations \
+CFLAGS += -Wno-format-zero-length -Wmissing-declarations \
 	-Wno-declaration-after-statement -Wformat-nonliteral
 CFLAGS += -I. -fPIC
 
diff --git a/tools/xentrace/Makefile b/tools/xentrace/Makefile
index c8c36a8..ac5c534 100644
--- a/tools/xentrace/Makefile
+++ b/tools/xentrace/Makefile
@@ -1,8 +1,6 @@
 XEN_ROOT=$(CURDIR)/../..
 include $(XEN_ROOT)/tools/Rules.mk
 
-CFLAGS += -Werror
-
 CFLAGS += $(CFLAGS_libxenevtchn)
 CFLAGS += $(CFLAGS_libxenctrl)
 LDLIBS += $(LDLIBS_libxenevtchn)
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 1/5] Remove hardcoded strict -Werror checking

[Doug Goldstein]

[-- Attachment #1.1.1: Type: text/plain, Size: 2263 bytes --]

On 12/20/16 1:46 PM, Alistair Francis wrote:
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> ---
>  Config.mk                      | 2 +-
>  tools/blktap2/drivers/Makefile | 1 -
>  tools/libxl/Makefile           | 2 +-
>  tools/xentrace/Makefile        | 2 --
>  4 files changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/Config.mk b/Config.mk
> index 3ec7367..e3cda81 100644
> --- a/Config.mk
> +++ b/Config.mk
> @@ -34,7 +34,7 @@ CONFIG_$(XEN_OS) := y
>  SHELL     ?= /bin/sh
>  
>  # Tools to run on system hosting the build
> -HOSTCFLAGS  = -Wall -Werror -Wstrict-prototypes -O2 -fomit-frame-pointer
> +HOSTCFLAGS  = -Wall -Wstrict-prototypes -O2 -fomit-frame-pointer
>  HOSTCFLAGS += -fno-strict-aliasing
>  
>  DISTDIR     ?= $(XEN_ROOT)/dist
> diff --git a/tools/blktap2/drivers/Makefile b/tools/blktap2/drivers/Makefile
> index 5328c40..7a62a3f 100644
> --- a/tools/blktap2/drivers/Makefile
> +++ b/tools/blktap2/drivers/Makefile
> @@ -9,7 +9,6 @@ QCOW_UTIL  = img2qcow qcow-create qcow2raw
>  LOCK_UTIL  = lock-util
>  INST_DIR   = $(sbindir)
>  
> -CFLAGS    += -Werror
>  CFLAGS    += -Wno-unused
>  CFLAGS    += -fno-strict-aliasing
>  CFLAGS    += -I$(BLKTAP_ROOT)/include -I$(BLKTAP_ROOT)/drivers
> diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
> index 91e2f97..e8a37ef 100644
> --- a/tools/libxl/Makefile
> +++ b/tools/libxl/Makefile
> @@ -11,7 +11,7 @@ MINOR = 0
>  XLUMAJOR = 4.9
>  XLUMINOR = 0
>  
> -CFLAGS += -Werror -Wno-format-zero-length -Wmissing-declarations \
> +CFLAGS += -Wno-format-zero-length -Wmissing-declarations \
>  	-Wno-declaration-after-statement -Wformat-nonliteral
>  CFLAGS += -I. -fPIC
>  
> diff --git a/tools/xentrace/Makefile b/tools/xentrace/Makefile
> index c8c36a8..ac5c534 100644
> --- a/tools/xentrace/Makefile
> +++ b/tools/xentrace/Makefile
> @@ -1,8 +1,6 @@
>  XEN_ROOT=$(CURDIR)/../..
>  include $(XEN_ROOT)/tools/Rules.mk
>  
> -CFLAGS += -Werror
> -
>  CFLAGS += $(CFLAGS_libxenevtchn)
>  CFLAGS += $(CFLAGS_libxenctrl)
>  LDLIBS += $(LDLIBS_libxenevtchn)
> 

Can you pass -Wno-error in EXTRA_CFLAGS from 2/5? Wish I could take
credit for the idea but it was all Andrew Cooper's.

-- 
Doug Goldstein


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 959 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 1/5] Remove hardcoded strict -Werror checking

[Andrew Cooper]

On 20/12/2016 20:06, Doug Goldstein wrote:
> On 12/20/16 1:46 PM, Alistair Francis wrote:
>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>> ---
>>  Config.mk                      | 2 +-
>>  tools/blktap2/drivers/Makefile | 1 -
>>  tools/libxl/Makefile           | 2 +-
>>  tools/xentrace/Makefile        | 2 --
>>  4 files changed, 2 insertions(+), 5 deletions(-)
>>
>> diff --git a/Config.mk b/Config.mk
>> index 3ec7367..e3cda81 100644
>> --- a/Config.mk
>> +++ b/Config.mk
>> @@ -34,7 +34,7 @@ CONFIG_$(XEN_OS) := y
>>  SHELL     ?= /bin/sh
>>  
>>  # Tools to run on system hosting the build
>> -HOSTCFLAGS  = -Wall -Werror -Wstrict-prototypes -O2 -fomit-frame-pointer
>> +HOSTCFLAGS  = -Wall -Wstrict-prototypes -O2 -fomit-frame-pointer
>>  HOSTCFLAGS += -fno-strict-aliasing
>>  
>>  DISTDIR     ?= $(XEN_ROOT)/dist
>> diff --git a/tools/blktap2/drivers/Makefile b/tools/blktap2/drivers/Makefile
>> index 5328c40..7a62a3f 100644
>> --- a/tools/blktap2/drivers/Makefile
>> +++ b/tools/blktap2/drivers/Makefile
>> @@ -9,7 +9,6 @@ QCOW_UTIL  = img2qcow qcow-create qcow2raw
>>  LOCK_UTIL  = lock-util
>>  INST_DIR   = $(sbindir)
>>  
>> -CFLAGS    += -Werror
>>  CFLAGS    += -Wno-unused
>>  CFLAGS    += -fno-strict-aliasing
>>  CFLAGS    += -I$(BLKTAP_ROOT)/include -I$(BLKTAP_ROOT)/drivers
>> diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
>> index 91e2f97..e8a37ef 100644
>> --- a/tools/libxl/Makefile
>> +++ b/tools/libxl/Makefile
>> @@ -11,7 +11,7 @@ MINOR = 0
>>  XLUMAJOR = 4.9
>>  XLUMINOR = 0
>>  
>> -CFLAGS += -Werror -Wno-format-zero-length -Wmissing-declarations \
>> +CFLAGS += -Wno-format-zero-length -Wmissing-declarations \
>>  	-Wno-declaration-after-statement -Wformat-nonliteral
>>  CFLAGS += -I. -fPIC
>>  
>> diff --git a/tools/xentrace/Makefile b/tools/xentrace/Makefile
>> index c8c36a8..ac5c534 100644
>> --- a/tools/xentrace/Makefile
>> +++ b/tools/xentrace/Makefile
>> @@ -1,8 +1,6 @@
>>  XEN_ROOT=$(CURDIR)/../..
>>  include $(XEN_ROOT)/tools/Rules.mk
>>  
>> -CFLAGS += -Werror
>> -
>>  CFLAGS += $(CFLAGS_libxenevtchn)
>>  CFLAGS += $(CFLAGS_libxenctrl)
>>  LDLIBS += $(LDLIBS_libxenevtchn)
>>
> Can you pass -Wno-error in EXTRA_CFLAGS from 2/5? Wish I could take
> credit for the idea but it was all Andrew Cooper's.

The point is that, especially with kernel-level development, almost all
warnings are relevant to correctness.  I have only seen 2? false
positives in 5 years, and have lost count of how many issues -Werror has
caught before the code actually got committed.

~Andrew


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 1/5] Remove hardcoded strict -Werror checking

[Alistair Francis]

On Tue, Dec 20, 2016 at 12:16 PM, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
> On 20/12/2016 20:06, Doug Goldstein wrote:
>> On 12/20/16 1:46 PM, Alistair Francis wrote:
>>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>>> ---
>>>  Config.mk                      | 2 +-
>>>  tools/blktap2/drivers/Makefile | 1 -
>>>  tools/libxl/Makefile           | 2 +-
>>>  tools/xentrace/Makefile        | 2 --
>>>  4 files changed, 2 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/Config.mk b/Config.mk
>>> index 3ec7367..e3cda81 100644
>>> --- a/Config.mk
>>> +++ b/Config.mk
>>> @@ -34,7 +34,7 @@ CONFIG_$(XEN_OS) := y
>>>  SHELL     ?= /bin/sh
>>>
>>>  # Tools to run on system hosting the build
>>> -HOSTCFLAGS  = -Wall -Werror -Wstrict-prototypes -O2 -fomit-frame-pointer
>>> +HOSTCFLAGS  = -Wall -Wstrict-prototypes -O2 -fomit-frame-pointer
>>>  HOSTCFLAGS += -fno-strict-aliasing
>>>
>>>  DISTDIR     ?= $(XEN_ROOT)/dist
>>> diff --git a/tools/blktap2/drivers/Makefile b/tools/blktap2/drivers/Makefile
>>> index 5328c40..7a62a3f 100644
>>> --- a/tools/blktap2/drivers/Makefile
>>> +++ b/tools/blktap2/drivers/Makefile
>>> @@ -9,7 +9,6 @@ QCOW_UTIL  = img2qcow qcow-create qcow2raw
>>>  LOCK_UTIL  = lock-util
>>>  INST_DIR   = $(sbindir)
>>>
>>> -CFLAGS    += -Werror
>>>  CFLAGS    += -Wno-unused
>>>  CFLAGS    += -fno-strict-aliasing
>>>  CFLAGS    += -I$(BLKTAP_ROOT)/include -I$(BLKTAP_ROOT)/drivers
>>> diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
>>> index 91e2f97..e8a37ef 100644
>>> --- a/tools/libxl/Makefile
>>> +++ b/tools/libxl/Makefile
>>> @@ -11,7 +11,7 @@ MINOR = 0
>>>  XLUMAJOR = 4.9
>>>  XLUMINOR = 0
>>>
>>> -CFLAGS += -Werror -Wno-format-zero-length -Wmissing-declarations \
>>> +CFLAGS += -Wno-format-zero-length -Wmissing-declarations \
>>>      -Wno-declaration-after-statement -Wformat-nonliteral
>>>  CFLAGS += -I. -fPIC
>>>
>>> diff --git a/tools/xentrace/Makefile b/tools/xentrace/Makefile
>>> index c8c36a8..ac5c534 100644
>>> --- a/tools/xentrace/Makefile
>>> +++ b/tools/xentrace/Makefile
>>> @@ -1,8 +1,6 @@
>>>  XEN_ROOT=$(CURDIR)/../..
>>>  include $(XEN_ROOT)/tools/Rules.mk
>>>
>>> -CFLAGS += -Werror
>>> -
>>>  CFLAGS += $(CFLAGS_libxenevtchn)
>>>  CFLAGS += $(CFLAGS_libxenctrl)
>>>  LDLIBS += $(LDLIBS_libxenevtchn)
>>>
>> Can you pass -Wno-error in EXTRA_CFLAGS from 2/5? Wish I could take
>> credit for the idea but it was all Andrew Cooper's.

I tried that, I still see errors when building lower levels like tools/xentrace.

>
> The point is that, especially with kernel-level development, almost all
> warnings are relevant to correctness.  I have only seen 2? false
> positives in 5 years, and have lost count of how many issues -Werror has
> caught before the code actually got committed.

I agree, but the problem is that as warnings change these cause all
sorts of build issues.

Thanks,

Alistair

>
> ~Andrew
>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 1/5] Remove hardcoded strict -Werror checking

[Doug Goldstein]

[-- Attachment #1.1.1: Type: text/plain, Size: 3066 bytes --]

On 12/20/16 2:16 PM, Andrew Cooper wrote:
> On 20/12/2016 20:06, Doug Goldstein wrote:
>> On 12/20/16 1:46 PM, Alistair Francis wrote:
>>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>>> ---
>>>  Config.mk                      | 2 +-
>>>  tools/blktap2/drivers/Makefile | 1 -
>>>  tools/libxl/Makefile           | 2 +-
>>>  tools/xentrace/Makefile        | 2 --
>>>  4 files changed, 2 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/Config.mk b/Config.mk
>>> index 3ec7367..e3cda81 100644
>>> --- a/Config.mk
>>> +++ b/Config.mk
>>> @@ -34,7 +34,7 @@ CONFIG_$(XEN_OS) := y
>>>  SHELL     ?= /bin/sh
>>>  
>>>  # Tools to run on system hosting the build
>>> -HOSTCFLAGS  = -Wall -Werror -Wstrict-prototypes -O2 -fomit-frame-pointer
>>> +HOSTCFLAGS  = -Wall -Wstrict-prototypes -O2 -fomit-frame-pointer
>>>  HOSTCFLAGS += -fno-strict-aliasing
>>>  
>>>  DISTDIR     ?= $(XEN_ROOT)/dist
>>> diff --git a/tools/blktap2/drivers/Makefile b/tools/blktap2/drivers/Makefile
>>> index 5328c40..7a62a3f 100644
>>> --- a/tools/blktap2/drivers/Makefile
>>> +++ b/tools/blktap2/drivers/Makefile
>>> @@ -9,7 +9,6 @@ QCOW_UTIL  = img2qcow qcow-create qcow2raw
>>>  LOCK_UTIL  = lock-util
>>>  INST_DIR   = $(sbindir)
>>>  
>>> -CFLAGS    += -Werror
>>>  CFLAGS    += -Wno-unused
>>>  CFLAGS    += -fno-strict-aliasing
>>>  CFLAGS    += -I$(BLKTAP_ROOT)/include -I$(BLKTAP_ROOT)/drivers
>>> diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
>>> index 91e2f97..e8a37ef 100644
>>> --- a/tools/libxl/Makefile
>>> +++ b/tools/libxl/Makefile
>>> @@ -11,7 +11,7 @@ MINOR = 0
>>>  XLUMAJOR = 4.9
>>>  XLUMINOR = 0
>>>  
>>> -CFLAGS += -Werror -Wno-format-zero-length -Wmissing-declarations \
>>> +CFLAGS += -Wno-format-zero-length -Wmissing-declarations \
>>>  	-Wno-declaration-after-statement -Wformat-nonliteral
>>>  CFLAGS += -I. -fPIC
>>>  
>>> diff --git a/tools/xentrace/Makefile b/tools/xentrace/Makefile
>>> index c8c36a8..ac5c534 100644
>>> --- a/tools/xentrace/Makefile
>>> +++ b/tools/xentrace/Makefile
>>> @@ -1,8 +1,6 @@
>>>  XEN_ROOT=$(CURDIR)/../..
>>>  include $(XEN_ROOT)/tools/Rules.mk
>>>  
>>> -CFLAGS += -Werror
>>> -
>>>  CFLAGS += $(CFLAGS_libxenevtchn)
>>>  CFLAGS += $(CFLAGS_libxenctrl)
>>>  LDLIBS += $(LDLIBS_libxenevtchn)
>>>
>> Can you pass -Wno-error in EXTRA_CFLAGS from 2/5? Wish I could take
>> credit for the idea but it was all Andrew Cooper's.
> 
> The point is that, especially with kernel-level development, almost all
> warnings are relevant to correctness.  I have only seen 2? false
> positives in 5 years, and have lost count of how many issues -Werror has
> caught before the code actually got committed.
> 
> ~Andrew
> 

I agree with you Andrew that its good for kernel/hypervisor development.
I also agree its good for userspace code prior to being committed or
part of a CI loop. Where I wish there was a switch to flip it off is on
user space code that pulls in headers from third party packages like tools/.

-- 
Doug Goldstein


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 959 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 1/5] Remove hardcoded strict -Werror checking

[Ian Jackson]

Doug Goldstein writes ("Re: [Xen-devel] [PATCH v2 1/5] Remove hardcoded strict -Werror checking"):
> I agree with you Andrew that its good for kernel/hypervisor development.
> I also agree its good for userspace code prior to being committed or
> part of a CI loop. Where I wish there was a switch to flip it off is on
> user space code that pulls in headers from third party packages like tools/.

APPEND_CFLAGS=-Wno-error

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 1/5] Remove hardcoded strict -Werror checking

[Jan Beulich]

>>> On 20.12.16 at 20:46, <alistair.francis@xilinx.com> wrote:
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>

Without some rationale given I don't think such changes are
acceptable at all. And then, as already pointed out others, the
use of -Werror is there not just for fun. If anything I think an
override to that default could be acceptable.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 1/5] Remove hardcoded strict -Werror checking

[Alistair Francis]

On Thu, Dec 22, 2016 at 12:41 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 20.12.16 at 20:46, <alistair.francis@xilinx.com> wrote:
>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>
> Without some rationale given I don't think such changes are
> acceptable at all. And then, as already pointed out others, the
> use of -Werror is there not just for fun. If anything I think an
> override to that default could be acceptable.

Unfortunately the APPEND_CFLAGS=-Wno-error doesn't fix all the issues
as I still see warnings/errors when building: tools/kconfig/conf.c.

I understand that you want it in by default.

Everyone seems fairly open to an override. Is a environment variable,
which if set will disable Werror acceptable? Something like NO_ERROR=Y
which will result in no -Werror being appended.

Thanks,

Alistair

>
> Jan
>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 1/5] Remove hardcoded strict -Werror checking

[Ian Jackson]

Alistair Francis writes ("Re: [Xen-devel] [PATCH v2 1/5] Remove hardcoded strict -Werror checking"):
> On Thu, Dec 22, 2016 at 12:41 AM, Jan Beulich <JBeulich@suse.com> wrote:
>> On 20.12.16 at 20:46, <alistair.francis@xilinx.com> wrote:
> >> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> >
> > Without some rationale given I don't think such changes are
> > acceptable at all. And then, as already pointed out others, the
> > use of -Werror is there not just for fun. If anything I think an
> > override to that default could be acceptable.
> 
> Unfortunately the APPEND_CFLAGS=-Wno-error doesn't fix all the issues
> as I still see warnings/errors when building: tools/kconfig/conf.c.

That sounds like a bug to me.  Do you know why it's not effective
there ?

> Everyone seems fairly open to an override. Is a environment variable,
> which if set will disable Werror acceptable? Something like NO_ERROR=Y
> which will result in no -Werror being appended.

Yes, an environment variable would be acceptable, but it should have
the right name and semantics and ideally we could reuse an existing
variable or fix it if it is broken.

How about `APPEND_CFLAGS=-Wno-error' ? :-)

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 1/5] Remove hardcoded strict -Werror checking

[Alistair Francis]

On Thu, Dec 22, 2016 at 11:22 AM, Ian Jackson <ian.jackson@eu.citrix.com> wrote:
> Alistair Francis writes ("Re: [Xen-devel] [PATCH v2 1/5] Remove hardcoded strict -Werror checking"):
>> On Thu, Dec 22, 2016 at 12:41 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>> On 20.12.16 at 20:46, <alistair.francis@xilinx.com> wrote:
>> >> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>> >
>> > Without some rationale given I don't think such changes are
>> > acceptable at all. And then, as already pointed out others, the
>> > use of -Werror is there not just for fun. If anything I think an
>> > override to that default could be acceptable.
>>
>> Unfortunately the APPEND_CFLAGS=-Wno-error doesn't fix all the issues
>> as I still see warnings/errors when building: tools/kconfig/conf.c.
>
> That sounds like a bug to me.  Do you know why it's not effective
> there ?

It actually might be an issue in the way buildroot is handling the arguments.

I'll look into it and see what I find after the holidays.

Thanks,

Alistair

>
>> Everyone seems fairly open to an override. Is a environment variable,
>> which if set will disable Werror acceptable? Something like NO_ERROR=Y
>> which will result in no -Werror being appended.
>
> Yes, an environment variable would be acceptable, but it should have
> the right name and semantics and ideally we could reuse an existing
> variable or fix it if it is broken.
>
> How about `APPEND_CFLAGS=-Wno-error' ? :-)
>
> Thanks,
> Ian.
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 1/5] Remove hardcoded strict -Werror checking

[Alistair Francis]

On Thu, Dec 22, 2016 at 1:12 PM, Alistair Francis
<alistair.francis@xilinx.com> wrote:
> On Thu, Dec 22, 2016 at 11:22 AM, Ian Jackson <ian.jackson@eu.citrix.com> wrote:
>> Alistair Francis writes ("Re: [Xen-devel] [PATCH v2 1/5] Remove hardcoded strict -Werror checking"):
>>> On Thu, Dec 22, 2016 at 12:41 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 20.12.16 at 20:46, <alistair.francis@xilinx.com> wrote:
>>> >> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>>> >
>>> > Without some rationale given I don't think such changes are
>>> > acceptable at all. And then, as already pointed out others, the
>>> > use of -Werror is there not just for fun. If anything I think an
>>> > override to that default could be acceptable.
>>>
>>> Unfortunately the APPEND_CFLAGS=-Wno-error doesn't fix all the issues
>>> as I still see warnings/errors when building: tools/kconfig/conf.c.
>>
>> That sounds like a bug to me.  Do you know why it's not effective
>> there ?
>
> It actually might be an issue in the way buildroot is handling the arguments.
>
> I'll look into it and see what I find after the holidays.

Nope, it does look like a Xen build issue. I included the full failing
log below:

PATH="/work/alistai/software/buildroot/output/host/bin:/work/alistai/software/buildroot/output/host/sbin:/work/alistai/software/buildroot/output/host/usr/bin:/work/alistai/software/buildroot/output/host/usr/sbin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games:/snap/bin"
XEN_TARGET_ARCH=arm32
CROSS_COMPILE=/work/alistai/software/buildroot/output/host/usr/bin/arm-linux-
PATH="/work/alistai/software/buildroot/output/host/bin:/work/alistai/software/buildroot/output/host/sbin:/work/alistai/software/buildroot/output/host/usr/bin:/work/alistai/software/buildroot/output/host/usr/sbin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games:/snap/bin"
AR="/work/alistai/software/buildroot/output/host/usr/bin/arm-linux-ar"
AS="/work/alistai/software/buildroot/output/host/usr/bin/arm-linux-as"
LD="/work/alistai/software/buildroot/output/host/usr/bin/arm-linux-ld"
NM="/work/alistai/software/buildroot/output/host/usr/bin/arm-linux-nm"
CC="/work/alistai/software/buildroot/output/host/usr/bin/arm-linux-gcc"
GCC="/work/alistai/software/buildroot/output/host/usr/bin/arm-linux-gcc"
CPP="/work/alistai/software/buildroot/output/host/usr/bin/arm-linux-cpp"
CXX="/work/alistai/software/buildroot/output/host/usr/bin/arm-linux-g++"
FC="/work/alistai/software/buildroot/output/host/usr/bin/arm-linux-gfortran"
F77="/work/alistai/software/buildroot/output/host/usr/bin/arm-linux-gfortran"
RANLIB="/work/alistai/software/buildroot/output/host/usr/bin/arm-linux-ranlib"
READELF="/work/alistai/software/buildroot/output/host/usr/bin/arm-linux-readelf"
STRIP="/work/alistai/software/buildroot/output/host/usr/bin/arm-linux-strip"
OBJCOPY="/work/alistai/software/buildroot/output/host/usr/bin/arm-linux-objcopy"
OBJDUMP="/work/alistai/software/buildroot/output/host/usr/bin/arm-linux-objdump"
AR_FOR_BUILD="/usr/bin/ar" AS_FOR_BUILD="/usr/bin/as"
CC_FOR_BUILD="/usr/bin/gcc" GCC_FOR_BUILD="/usr/bin/gcc"
CXX_FOR_BUILD="/usr/bin/g++" LD_FOR_BUILD="/usr/bin/ld"
CPPFLAGS_FOR_BUILD="-I/work/alistai/software/buildroot/output/host/usr/include"
CFLAGS_FOR_BUILD="-O2
-I/work/alistai/software/buildroot/output/host/usr/include"
CXXFLAGS_FOR_BUILD="-O2
-I/work/alistai/software/buildroot/output/host/usr/include"
LDFLAGS_FOR_BUILD="-L/work/alistai/software/buildroot/output/host/lib
-L/work/alistai/software/buildroot/output/host/usr/lib
-Wl,-rpath,/work/alistai/software/buildroot/output/host/usr/lib"
FCFLAGS_FOR_BUILD=""
DEFAULT_ASSEMBLER="/work/alistai/software/buildroot/output/host/usr/bin/arm-linux-as"
DEFAULT_LINKER="/work/alistai/software/buildroot/output/host/usr/bin/arm-linux-ld"
CPPFLAGS="-D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE
-D_FILE_OFFSET_BITS=64" APPEND_CFLAGS="-Wno-error -D_LARGEFILE_SOURCE
-D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64  -Os "
CXXFLAGS="-D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE
-D_FILE_OFFSET_BITS=64  -Os " LDFLAGS="" FAPPEND_CFLAGS="-Wno-error
-Os " FFLAGS=" -Os "
PKG_CONFIG="/work/alistai/software/buildroot/output/host/usr/bin/pkg-config"
STAGING_DIR="/work/alistai/software/buildroot/output/host/usr/arm-buildroot-linux-musleabihf/sysroot"
INTLTOOL_PERL=/usr/bin/perl /usr/bin/make -j11 dist-xen dist-tools -C
/work/alistai/software/buildroot/output/build/xen-4.8.0/
make[1]: Entering directory
'/work/alistai/software/buildroot/output/build/xen-4.8.0'
/usr/bin/make -C xen install
/usr/bin/make -C tools install
make[2]: Entering directory
'/work/alistai/software/buildroot/output/build/xen-4.8.0/xen'
make[2]: Entering directory
'/work/alistai/software/buildroot/output/build/xen-4.8.0/tools'
/usr/bin/make -f
/work/alistai/software/buildroot/output/build/xen-4.8.0/xen/tools/kconfig/Makefile.kconfig
ARCH=arm32 SRCARCH=arm HOSTCC="/usr/bin/gcc" HOSTCXX="/usr/bin/g++"
defconfig
make[3]: Entering directory
'/work/alistai/software/buildroot/output/build/xen-4.8.0/xen'
make[3]: Entering directory
'/work/alistai/software/buildroot/output/build/xen-4.8.0/tools'
/usr/bin/gcc -Wp,-MD,tools/kconfig/.conf.o.d -Wall -Werror
-Wstrict-prototypes -O2 -fomit-frame-pointer -fno-strict-aliasing
-Wdeclaration-after-statement     -DCURSES_LOC="<ncurses.h>"
-DNCURSES_WIDECHAR=1 -DLOCALE -DKBUILD_NO_NLS  -c -o
tools/kconfig/conf.o tools/kconfig/conf.c
/usr/bin/gcc -Wp,-MD,tools/kconfig/.zconf.tab.o.d -Wall -Werror
-Wstrict-prototypes -O2 -fomit-frame-pointer -fno-strict-aliasing
-Wdeclaration-after-statement     -DCURSES_LOC="<ncurses.h>"
-DNCURSES_WIDECHAR=1 -DLOCALE -DKBUILD_NO_NLS -Itools/kconfig -c -o
tools/kconfig/zconf.tab.o tools/kconfig/zconf.tab.c
tools/kconfig/conf.c: In function ‘check_stdin’:
tools/kconfig/conf.c:77:3: error: format not a string literal and no
format arguments [-Werror=format-security]
   printf(_("aborted!\n\n"));
   ^
tools/kconfig/conf.c:78:3: error: format not a string literal and no
format arguments [-Werror=format-security]
   printf(_("Console input/output is redirected. "));
   ^
tools/kconfig/conf.c:79:3: error: format not a string literal and no
format arguments [-Werror=format-security]
   printf(_("Run 'make oldconfig' to update configuration.\n\n"));
   ^
tools/kconfig/conf.c: In function ‘conf_askvalue’:
tools/kconfig/conf.c:89:3: error: format not a string literal and no
format arguments [-Werror=format-security]
   printf(_("(NEW) "));
   ^
tools/kconfig/conf.c: In function ‘conf_choice’:
tools/kconfig/conf.c:290:5: error: format not a string literal and no
format arguments [-Werror=format-security]
     printf(_(" (NEW)"));
     ^
tools/kconfig/conf.c: In function ‘check_conf’:
tools/kconfig/conf.c:438:6: error: format not a string literal and no
format arguments [-Werror=format-security]
      printf(_("*\n* Restart config...\n*\n"));
      ^
tools/kconfig/conf.c: In function ‘main’:
tools/kconfig/conf.c:640:6: error: format not a string literal and no
format arguments [-Werror=format-security]
      _("\n*** The configuration requires explicit update.\n\n"));
      ^
tools/kconfig/conf.c:693:4: error: format not a string literal and no
format arguments [-Werror=format-security]
    fprintf(stderr, _("\n*** Error during writing of the configuration.\n\n"));
    ^
tools/kconfig/conf.c:697:4: error: format not a string literal and no
format arguments [-Werror=format-security]
    fprintf(stderr, _("\n*** Error during update of the configuration.\n\n"));
    ^
tools/kconfig/conf.c:708:4: error: format not a string literal and no
format arguments [-Werror=format-security]
    fprintf(stderr, _("\n*** Error during writing of the configuration.\n\n"));


>
> Thanks,
>
> Alistair
>
>>
>>> Everyone seems fairly open to an override. Is a environment variable,
>>> which if set will disable Werror acceptable? Something like NO_ERROR=Y
>>> which will result in no -Werror being appended.
>>
>> Yes, an environment variable would be acceptable, but it should have
>> the right name and semantics and ideally we could reuse an existing
>> variable or fix it if it is broken.
>>
>> How about `APPEND_CFLAGS=-Wno-error' ? :-)
>>
>> Thanks,
>> Ian.
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> https://lists.xen.org/xen-devel

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 1/5] Remove hardcoded strict -Werror checking

[Alistair Francis]

On Thu, Dec 22, 2016 at 1:15 PM, Alistair Francis
<alistair.francis@xilinx.com> wrote:
> On Thu, Dec 22, 2016 at 1:12 PM, Alistair Francis
> <alistair.francis@xilinx.com> wrote:
>> On Thu, Dec 22, 2016 at 11:22 AM, Ian Jackson <ian.jackson@eu.citrix.com> wrote:
>>> Alistair Francis writes ("Re: [Xen-devel] [PATCH v2 1/5] Remove hardcoded strict -Werror checking"):
>>>> On Thu, Dec 22, 2016 at 12:41 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 20.12.16 at 20:46, <alistair.francis@xilinx.com> wrote:
>>>> >> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>>>> >
>>>> > Without some rationale given I don't think such changes are
>>>> > acceptable at all. And then, as already pointed out others, the
>>>> > use of -Werror is there not just for fun. If anything I think an
>>>> > override to that default could be acceptable.
>>>>
>>>> Unfortunately the APPEND_CFLAGS=-Wno-error doesn't fix all the issues
>>>> as I still see warnings/errors when building: tools/kconfig/conf.c.
>>>
>>> That sounds like a bug to me.  Do you know why it's not effective
>>> there ?
>>
>> It actually might be an issue in the way buildroot is handling the arguments.
>>
>> I'll look into it and see what I find after the holidays.

I dug into this a little more. Adding the APPEND_CFLAGS="-Wno-error"
fixes almost everything. The only problem I see is in the log below,
where tools/kconfig/conf.c fails to build as the -Wno-error doesn't
propagate down.

If I manage to find a fix today I'll send it, otherwise this can wait
until next year.

Thanks,

Alistair

>
> Nope, it does look like a Xen build issue. I included the full failing
> log below:
>
> PATH="/work/alistai/software/buildroot/output/host/bin:/work/alistai/software/buildroot/output/host/sbin:/work/alistai/software/buildroot/output/host/usr/bin:/work/alistai/software/buildroot/output/host/usr/sbin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games:/snap/bin"
> XEN_TARGET_ARCH=arm32
> CROSS_COMPILE=/work/alistai/software/buildroot/output/host/usr/bin/arm-linux-
> PATH="/work/alistai/software/buildroot/output/host/bin:/work/alistai/software/buildroot/output/host/sbin:/work/alistai/software/buildroot/output/host/usr/bin:/work/alistai/software/buildroot/output/host/usr/sbin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games:/snap/bin"
> AR="/work/alistai/software/buildroot/output/host/usr/bin/arm-linux-ar"
> AS="/work/alistai/software/buildroot/output/host/usr/bin/arm-linux-as"
> LD="/work/alistai/software/buildroot/output/host/usr/bin/arm-linux-ld"
> NM="/work/alistai/software/buildroot/output/host/usr/bin/arm-linux-nm"
> CC="/work/alistai/software/buildroot/output/host/usr/bin/arm-linux-gcc"
> GCC="/work/alistai/software/buildroot/output/host/usr/bin/arm-linux-gcc"
> CPP="/work/alistai/software/buildroot/output/host/usr/bin/arm-linux-cpp"
> CXX="/work/alistai/software/buildroot/output/host/usr/bin/arm-linux-g++"
> FC="/work/alistai/software/buildroot/output/host/usr/bin/arm-linux-gfortran"
> F77="/work/alistai/software/buildroot/output/host/usr/bin/arm-linux-gfortran"
> RANLIB="/work/alistai/software/buildroot/output/host/usr/bin/arm-linux-ranlib"
> READELF="/work/alistai/software/buildroot/output/host/usr/bin/arm-linux-readelf"
> STRIP="/work/alistai/software/buildroot/output/host/usr/bin/arm-linux-strip"
> OBJCOPY="/work/alistai/software/buildroot/output/host/usr/bin/arm-linux-objcopy"
> OBJDUMP="/work/alistai/software/buildroot/output/host/usr/bin/arm-linux-objdump"
> AR_FOR_BUILD="/usr/bin/ar" AS_FOR_BUILD="/usr/bin/as"
> CC_FOR_BUILD="/usr/bin/gcc" GCC_FOR_BUILD="/usr/bin/gcc"
> CXX_FOR_BUILD="/usr/bin/g++" LD_FOR_BUILD="/usr/bin/ld"
> CPPFLAGS_FOR_BUILD="-I/work/alistai/software/buildroot/output/host/usr/include"
> CFLAGS_FOR_BUILD="-O2
> -I/work/alistai/software/buildroot/output/host/usr/include"
> CXXFLAGS_FOR_BUILD="-O2
> -I/work/alistai/software/buildroot/output/host/usr/include"
> LDFLAGS_FOR_BUILD="-L/work/alistai/software/buildroot/output/host/lib
> -L/work/alistai/software/buildroot/output/host/usr/lib
> -Wl,-rpath,/work/alistai/software/buildroot/output/host/usr/lib"
> FCFLAGS_FOR_BUILD=""
> DEFAULT_ASSEMBLER="/work/alistai/software/buildroot/output/host/usr/bin/arm-linux-as"
> DEFAULT_LINKER="/work/alistai/software/buildroot/output/host/usr/bin/arm-linux-ld"
> CPPFLAGS="-D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE
> -D_FILE_OFFSET_BITS=64" APPEND_CFLAGS="-Wno-error -D_LARGEFILE_SOURCE
> -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64  -Os "
> CXXFLAGS="-D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE
> -D_FILE_OFFSET_BITS=64  -Os " LDFLAGS="" FAPPEND_CFLAGS="-Wno-error
> -Os " FFLAGS=" -Os "
> PKG_CONFIG="/work/alistai/software/buildroot/output/host/usr/bin/pkg-config"
> STAGING_DIR="/work/alistai/software/buildroot/output/host/usr/arm-buildroot-linux-musleabihf/sysroot"
> INTLTOOL_PERL=/usr/bin/perl /usr/bin/make -j11 dist-xen dist-tools -C
> /work/alistai/software/buildroot/output/build/xen-4.8.0/
> make[1]: Entering directory
> '/work/alistai/software/buildroot/output/build/xen-4.8.0'
> /usr/bin/make -C xen install
> /usr/bin/make -C tools install
> make[2]: Entering directory
> '/work/alistai/software/buildroot/output/build/xen-4.8.0/xen'
> make[2]: Entering directory
> '/work/alistai/software/buildroot/output/build/xen-4.8.0/tools'
> /usr/bin/make -f
> /work/alistai/software/buildroot/output/build/xen-4.8.0/xen/tools/kconfig/Makefile.kconfig
> ARCH=arm32 SRCARCH=arm HOSTCC="/usr/bin/gcc" HOSTCXX="/usr/bin/g++"
> defconfig
> make[3]: Entering directory
> '/work/alistai/software/buildroot/output/build/xen-4.8.0/xen'
> make[3]: Entering directory
> '/work/alistai/software/buildroot/output/build/xen-4.8.0/tools'
> /usr/bin/gcc -Wp,-MD,tools/kconfig/.conf.o.d -Wall -Werror
> -Wstrict-prototypes -O2 -fomit-frame-pointer -fno-strict-aliasing
> -Wdeclaration-after-statement     -DCURSES_LOC="<ncurses.h>"
> -DNCURSES_WIDECHAR=1 -DLOCALE -DKBUILD_NO_NLS  -c -o
> tools/kconfig/conf.o tools/kconfig/conf.c
> /usr/bin/gcc -Wp,-MD,tools/kconfig/.zconf.tab.o.d -Wall -Werror
> -Wstrict-prototypes -O2 -fomit-frame-pointer -fno-strict-aliasing
> -Wdeclaration-after-statement     -DCURSES_LOC="<ncurses.h>"
> -DNCURSES_WIDECHAR=1 -DLOCALE -DKBUILD_NO_NLS -Itools/kconfig -c -o
> tools/kconfig/zconf.tab.o tools/kconfig/zconf.tab.c
> tools/kconfig/conf.c: In function ‘check_stdin’:
> tools/kconfig/conf.c:77:3: error: format not a string literal and no
> format arguments [-Werror=format-security]
>    printf(_("aborted!\n\n"));
>    ^
> tools/kconfig/conf.c:78:3: error: format not a string literal and no
> format arguments [-Werror=format-security]
>    printf(_("Console input/output is redirected. "));
>    ^
> tools/kconfig/conf.c:79:3: error: format not a string literal and no
> format arguments [-Werror=format-security]
>    printf(_("Run 'make oldconfig' to update configuration.\n\n"));
>    ^
> tools/kconfig/conf.c: In function ‘conf_askvalue’:
> tools/kconfig/conf.c:89:3: error: format not a string literal and no
> format arguments [-Werror=format-security]
>    printf(_("(NEW) "));
>    ^
> tools/kconfig/conf.c: In function ‘conf_choice’:
> tools/kconfig/conf.c:290:5: error: format not a string literal and no
> format arguments [-Werror=format-security]
>      printf(_(" (NEW)"));
>      ^
> tools/kconfig/conf.c: In function ‘check_conf’:
> tools/kconfig/conf.c:438:6: error: format not a string literal and no
> format arguments [-Werror=format-security]
>       printf(_("*\n* Restart config...\n*\n"));
>       ^
> tools/kconfig/conf.c: In function ‘main’:
> tools/kconfig/conf.c:640:6: error: format not a string literal and no
> format arguments [-Werror=format-security]
>       _("\n*** The configuration requires explicit update.\n\n"));
>       ^
> tools/kconfig/conf.c:693:4: error: format not a string literal and no
> format arguments [-Werror=format-security]
>     fprintf(stderr, _("\n*** Error during writing of the configuration.\n\n"));
>     ^
> tools/kconfig/conf.c:697:4: error: format not a string literal and no
> format arguments [-Werror=format-security]
>     fprintf(stderr, _("\n*** Error during update of the configuration.\n\n"));
>     ^
> tools/kconfig/conf.c:708:4: error: format not a string literal and no
> format arguments [-Werror=format-security]
>     fprintf(stderr, _("\n*** Error during writing of the configuration.\n\n"));
>
>
>>
>> Thanks,
>>
>> Alistair
>>
>>>
>>>> Everyone seems fairly open to an override. Is a environment variable,
>>>> which if set will disable Werror acceptable? Something like NO_ERROR=Y
>>>> which will result in no -Werror being appended.
>>>
>>> Yes, an environment variable would be acceptable, but it should have
>>> the right name and semantics and ideally we could reuse an existing
>>> variable or fix it if it is broken.
>>>
>>> How about `APPEND_CFLAGS=-Wno-error' ? :-)
>>>
>>> Thanks,
>>> Ian.
>>>
>>> _______________________________________________
>>> Xen-devel mailing list
>>> Xen-devel@lists.xen.org
>>> https://lists.xen.org/xen-devel

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 1/5] Remove hardcoded strict -Werror checking

[Andrew Cooper]

On 22/12/16 21:41, Alistair Francis wrote:
> On Thu, Dec 22, 2016 at 1:15 PM, Alistair Francis
> <alistair.francis@xilinx.com> wrote:
>> On Thu, Dec 22, 2016 at 1:12 PM, Alistair Francis
>> <alistair.francis@xilinx.com> wrote:
>>> On Thu, Dec 22, 2016 at 11:22 AM, Ian Jackson <ian.jackson@eu.citrix.com> wrote:
>>>> Alistair Francis writes ("Re: [Xen-devel] [PATCH v2 1/5] Remove hardcoded strict -Werror checking"):
>>>>> On Thu, Dec 22, 2016 at 12:41 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>> On 20.12.16 at 20:46, <alistair.francis@xilinx.com> wrote:
>>>>>>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>>>>>> Without some rationale given I don't think such changes are
>>>>>> acceptable at all. And then, as already pointed out others, the
>>>>>> use of -Werror is there not just for fun. If anything I think an
>>>>>> override to that default could be acceptable.
>>>>> Unfortunately the APPEND_CFLAGS=-Wno-error doesn't fix all the issues
>>>>> as I still see warnings/errors when building: tools/kconfig/conf.c.
>>>> That sounds like a bug to me.  Do you know why it's not effective
>>>> there ?
>>> It actually might be an issue in the way buildroot is handling the arguments.
>>>
>>> I'll look into it and see what I find after the holidays.
> I dug into this a little more. Adding the APPEND_CFLAGS="-Wno-error"
> fixes almost everything. The only problem I see is in the log below,
> where tools/kconfig/conf.c fails to build as the -Wno-error doesn't
> propagate down.
>
> If I manage to find a fix today I'll send it, otherwise this can wait
> until next year.

Something like this?

diff --git a/xen/Makefile b/xen/Makefile
index dc6862e04d..2d7a567c9c 100644
--- a/xen/Makefile
+++ b/xen/Makefile
@@ -253,14 +253,14 @@ kconfig := silentoldconfig oldconfig config 
menuconfig defconfig \
         randconfig
  .PHONY: $(kconfig)
  $(kconfig):
-       $(MAKE) -f $(BASEDIR)/tools/kconfig/Makefile.kconfig 
ARCH=$(ARCH) SRCARCH=$(SRCARCH) HOSTCC="$(HOSTCC)" HOSTCXX="$(HOSTCXX)" $@
+       $(MAKE) -f $(BASEDIR)/tools/kconfig/Makefile.kconfig 
ARCH=$(ARCH) SRCARCH=$(SRCARCH) HOSTCC="$(HOSTCC)" HOSTCXX="$(HOSTCXX)" 
HOST_EXTRACFLAGS="$(APPEND_CFLAGS)" $@

  include/config/%.conf: include/config/auto.conf.cmd $(KCONFIG_CONFIG)
-       $(MAKE) -f $(BASEDIR)/tools/kconfig/Makefile.kconfig 
ARCH=$(ARCH) SRCARCH=$(SRCARCH) HOSTCC="$(HOSTCC)" HOSTCXX="$(HOSTCXX)" 
silentoldconfig
+       $(MAKE) -f $(BASEDIR)/tools/kconfig/Makefile.kconfig 
ARCH=$(ARCH) SRCARCH=$(SRCARCH) HOSTCC="$(HOSTCC)" HOSTCXX="$(HOSTCXX)" 
HOST_EXTRACFLAGS="$(APPEND_CFLAGS)" silentoldconfig

  # Allow people to just run `make` as before and not force them to 
configure
  $(KCONFIG_CONFIG):
-       $(MAKE) -f $(BASEDIR)/tools/kconfig/Makefile.kconfig 
ARCH=$(ARCH) SRCARCH=$(SRCARCH) HOSTCC="$(HOSTCC)" HOSTCXX="$(HOSTCXX)" 
defconfig
+       $(MAKE) -f $(BASEDIR)/tools/kconfig/Makefile.kconfig 
ARCH=$(ARCH) SRCARCH=$(SRCARCH) HOSTCC="$(HOSTCC)" HOSTCXX="$(HOSTCXX)" 
HOST_EXTRACFLAGS="$(APPEND_CFLAGS)" defconfig

  # Break the dependency chain for the first run
  include/config/auto.conf.cmd: ;


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 1/5] Remove hardcoded strict -Werror checking

[Doug Goldstein]

[-- Attachment #1.1.1: Type: text/plain, Size: 3446 bytes --]

On 12/22/16 3:47 PM, Andrew Cooper wrote:
> On 22/12/16 21:41, Alistair Francis wrote:
>> On Thu, Dec 22, 2016 at 1:15 PM, Alistair Francis
>> <alistair.francis@xilinx.com> wrote:
>>> On Thu, Dec 22, 2016 at 1:12 PM, Alistair Francis
>>> <alistair.francis@xilinx.com> wrote:
>>>> On Thu, Dec 22, 2016 at 11:22 AM, Ian Jackson
>>>> <ian.jackson@eu.citrix.com> wrote:
>>>>> Alistair Francis writes ("Re: [Xen-devel] [PATCH v2 1/5] Remove
>>>>> hardcoded strict -Werror checking"):
>>>>>> On Thu, Dec 22, 2016 at 12:41 AM, Jan Beulich <JBeulich@suse.com>
>>>>>> wrote:
>>>>>>> On 20.12.16 at 20:46, <alistair.francis@xilinx.com> wrote:
>>>>>>>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>>>>>>> Without some rationale given I don't think such changes are
>>>>>>> acceptable at all. And then, as already pointed out others, the
>>>>>>> use of -Werror is there not just for fun. If anything I think an
>>>>>>> override to that default could be acceptable.
>>>>>> Unfortunately the APPEND_CFLAGS=-Wno-error doesn't fix all the issues
>>>>>> as I still see warnings/errors when building: tools/kconfig/conf.c.
>>>>> That sounds like a bug to me.  Do you know why it's not effective
>>>>> there ?
>>>> It actually might be an issue in the way buildroot is handling the
>>>> arguments.
>>>>
>>>> I'll look into it and see what I find after the holidays.
>> I dug into this a little more. Adding the APPEND_CFLAGS="-Wno-error"
>> fixes almost everything. The only problem I see is in the log below,
>> where tools/kconfig/conf.c fails to build as the -Wno-error doesn't
>> propagate down.
>>
>> If I manage to find a fix today I'll send it, otherwise this can wait
>> until next year.
> 
> Something like this?
> 
> diff --git a/xen/Makefile b/xen/Makefile
> index dc6862e04d..2d7a567c9c 100644
> --- a/xen/Makefile
> +++ b/xen/Makefile
> @@ -253,14 +253,14 @@ kconfig := silentoldconfig oldconfig config
> menuconfig defconfig \
>         randconfig
>  .PHONY: $(kconfig)
>  $(kconfig):
> -       $(MAKE) -f $(BASEDIR)/tools/kconfig/Makefile.kconfig
> ARCH=$(ARCH) SRCARCH=$(SRCARCH) HOSTCC="$(HOSTCC)" HOSTCXX="$(HOSTCXX)" $@
> +       $(MAKE) -f $(BASEDIR)/tools/kconfig/Makefile.kconfig
> ARCH=$(ARCH) SRCARCH=$(SRCARCH) HOSTCC="$(HOSTCC)" HOSTCXX="$(HOSTCXX)"
> HOST_EXTRACFLAGS="$(APPEND_CFLAGS)" $@
> 
>  include/config/%.conf: include/config/auto.conf.cmd $(KCONFIG_CONFIG)
> -       $(MAKE) -f $(BASEDIR)/tools/kconfig/Makefile.kconfig
> ARCH=$(ARCH) SRCARCH=$(SRCARCH) HOSTCC="$(HOSTCC)" HOSTCXX="$(HOSTCXX)"
> silentoldconfig
> +       $(MAKE) -f $(BASEDIR)/tools/kconfig/Makefile.kconfig
> ARCH=$(ARCH) SRCARCH=$(SRCARCH) HOSTCC="$(HOSTCC)" HOSTCXX="$(HOSTCXX)"
> HOST_EXTRACFLAGS="$(APPEND_CFLAGS)" silentoldconfig
> 
>  # Allow people to just run `make` as before and not force them to
> configure
>  $(KCONFIG_CONFIG):
> -       $(MAKE) -f $(BASEDIR)/tools/kconfig/Makefile.kconfig
> ARCH=$(ARCH) SRCARCH=$(SRCARCH) HOSTCC="$(HOSTCC)" HOSTCXX="$(HOSTCXX)"
> defconfig
> +       $(MAKE) -f $(BASEDIR)/tools/kconfig/Makefile.kconfig
> ARCH=$(ARCH) SRCARCH=$(SRCARCH) HOSTCC="$(HOSTCC)" HOSTCXX="$(HOSTCXX)"
> HOST_EXTRACFLAGS="$(APPEND_CFLAGS)" defconfig
> 
>  # Break the dependency chain for the first run
>  include/config/auto.conf.cmd: ;
> 

That should do the trick.

Reviewed-by: Doug Goldstein <cardoe@cardoe.com>

-- 
Doug Goldstein


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 959 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 1/5] Remove hardcoded strict -Werror checking

[Alistair Francis]

On Thu, Dec 22, 2016 at 2:00 PM, Doug Goldstein <cardoe@cardoe.com> wrote:
> On 12/22/16 3:47 PM, Andrew Cooper wrote:
>> On 22/12/16 21:41, Alistair Francis wrote:
>>> On Thu, Dec 22, 2016 at 1:15 PM, Alistair Francis
>>> <alistair.francis@xilinx.com> wrote:
>>>> On Thu, Dec 22, 2016 at 1:12 PM, Alistair Francis
>>>> <alistair.francis@xilinx.com> wrote:
>>>>> On Thu, Dec 22, 2016 at 11:22 AM, Ian Jackson
>>>>> <ian.jackson@eu.citrix.com> wrote:
>>>>>> Alistair Francis writes ("Re: [Xen-devel] [PATCH v2 1/5] Remove
>>>>>> hardcoded strict -Werror checking"):
>>>>>>> On Thu, Dec 22, 2016 at 12:41 AM, Jan Beulich <JBeulich@suse.com>
>>>>>>> wrote:
>>>>>>>> On 20.12.16 at 20:46, <alistair.francis@xilinx.com> wrote:
>>>>>>>>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>>>>>>>> Without some rationale given I don't think such changes are
>>>>>>>> acceptable at all. And then, as already pointed out others, the
>>>>>>>> use of -Werror is there not just for fun. If anything I think an
>>>>>>>> override to that default could be acceptable.
>>>>>>> Unfortunately the APPEND_CFLAGS=-Wno-error doesn't fix all the issues
>>>>>>> as I still see warnings/errors when building: tools/kconfig/conf.c.
>>>>>> That sounds like a bug to me.  Do you know why it's not effective
>>>>>> there ?
>>>>> It actually might be an issue in the way buildroot is handling the
>>>>> arguments.
>>>>>
>>>>> I'll look into it and see what I find after the holidays.
>>> I dug into this a little more. Adding the APPEND_CFLAGS="-Wno-error"
>>> fixes almost everything. The only problem I see is in the log below,
>>> where tools/kconfig/conf.c fails to build as the -Wno-error doesn't
>>> propagate down.
>>>
>>> If I manage to find a fix today I'll send it, otherwise this can wait
>>> until next year.
>>
>> Something like this?
>>
>> diff --git a/xen/Makefile b/xen/Makefile
>> index dc6862e04d..2d7a567c9c 100644
>> --- a/xen/Makefile
>> +++ b/xen/Makefile
>> @@ -253,14 +253,14 @@ kconfig := silentoldconfig oldconfig config
>> menuconfig defconfig \
>>         randconfig
>>  .PHONY: $(kconfig)
>>  $(kconfig):
>> -       $(MAKE) -f $(BASEDIR)/tools/kconfig/Makefile.kconfig
>> ARCH=$(ARCH) SRCARCH=$(SRCARCH) HOSTCC="$(HOSTCC)" HOSTCXX="$(HOSTCXX)" $@
>> +       $(MAKE) -f $(BASEDIR)/tools/kconfig/Makefile.kconfig
>> ARCH=$(ARCH) SRCARCH=$(SRCARCH) HOSTCC="$(HOSTCC)" HOSTCXX="$(HOSTCXX)"
>> HOST_EXTRACFLAGS="$(APPEND_CFLAGS)" $@
>>
>>  include/config/%.conf: include/config/auto.conf.cmd $(KCONFIG_CONFIG)
>> -       $(MAKE) -f $(BASEDIR)/tools/kconfig/Makefile.kconfig
>> ARCH=$(ARCH) SRCARCH=$(SRCARCH) HOSTCC="$(HOSTCC)" HOSTCXX="$(HOSTCXX)"
>> silentoldconfig
>> +       $(MAKE) -f $(BASEDIR)/tools/kconfig/Makefile.kconfig
>> ARCH=$(ARCH) SRCARCH=$(SRCARCH) HOSTCC="$(HOSTCC)" HOSTCXX="$(HOSTCXX)"
>> HOST_EXTRACFLAGS="$(APPEND_CFLAGS)" silentoldconfig
>>
>>  # Allow people to just run `make` as before and not force them to
>> configure
>>  $(KCONFIG_CONFIG):
>> -       $(MAKE) -f $(BASEDIR)/tools/kconfig/Makefile.kconfig
>> ARCH=$(ARCH) SRCARCH=$(SRCARCH) HOSTCC="$(HOSTCC)" HOSTCXX="$(HOSTCXX)"
>> defconfig
>> +       $(MAKE) -f $(BASEDIR)/tools/kconfig/Makefile.kconfig
>> ARCH=$(ARCH) SRCARCH=$(SRCARCH) HOSTCC="$(HOSTCC)" HOSTCXX="$(HOSTCXX)"
>> HOST_EXTRACFLAGS="$(APPEND_CFLAGS)" defconfig
>>
>>  # Break the dependency chain for the first run
>>  include/config/auto.conf.cmd: ;
>>
>
> That should do the trick.
>
> Reviewed-by: Doug Goldstein <cardoe@cardoe.com>

I got this to work as well:

diff --git a/xen/tools/kconfig/Makefile b/xen/tools/kconfig/Makefile
index aceaaed..32e2359 100644
--- a/xen/tools/kconfig/Makefile
+++ b/xen/tools/kconfig/Makefile
@@ -155,7 +155,7 @@ check-lxdialog  :=
$(srctree)/$(src)/lxdialog/check-lxdialog.sh
 # Use recursively expanded variables so we do not call gcc unless
 # we really need to do so. (Do not call gcc as part of make mrproper)
 HOST_EXTRACFLAGS += $(shell $(CONFIG_SHELL) $(check-lxdialog) -ccflags) \
-                    -DLOCALE
+                    -DLOCALE $(APPEND_CFLAGS)

 # ===========================================================================
 # Shared Makefile for the various kconfig executables:

But yours looks like it should work as well.

Do you want to send a patch?

Thanks,

Alistair

>
> --
> Doug Goldstein
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel
>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 1/5] Remove hardcoded strict -Werror checking

[Alistair Francis]

On Thu, Dec 22, 2016 at 2:11 PM, Alistair Francis
<alistair.francis@xilinx.com> wrote:
> On Thu, Dec 22, 2016 at 2:00 PM, Doug Goldstein <cardoe@cardoe.com> wrote:
>> On 12/22/16 3:47 PM, Andrew Cooper wrote:
>>> On 22/12/16 21:41, Alistair Francis wrote:
>>>> On Thu, Dec 22, 2016 at 1:15 PM, Alistair Francis
>>>> <alistair.francis@xilinx.com> wrote:
>>>>> On Thu, Dec 22, 2016 at 1:12 PM, Alistair Francis
>>>>> <alistair.francis@xilinx.com> wrote:
>>>>>> On Thu, Dec 22, 2016 at 11:22 AM, Ian Jackson
>>>>>> <ian.jackson@eu.citrix.com> wrote:
>>>>>>> Alistair Francis writes ("Re: [Xen-devel] [PATCH v2 1/5] Remove
>>>>>>> hardcoded strict -Werror checking"):
>>>>>>>> On Thu, Dec 22, 2016 at 12:41 AM, Jan Beulich <JBeulich@suse.com>
>>>>>>>> wrote:
>>>>>>>>> On 20.12.16 at 20:46, <alistair.francis@xilinx.com> wrote:
>>>>>>>>>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>>>>>>>>> Without some rationale given I don't think such changes are
>>>>>>>>> acceptable at all. And then, as already pointed out others, the
>>>>>>>>> use of -Werror is there not just for fun. If anything I think an
>>>>>>>>> override to that default could be acceptable.
>>>>>>>> Unfortunately the APPEND_CFLAGS=-Wno-error doesn't fix all the issues
>>>>>>>> as I still see warnings/errors when building: tools/kconfig/conf.c.
>>>>>>> That sounds like a bug to me.  Do you know why it's not effective
>>>>>>> there ?
>>>>>> It actually might be an issue in the way buildroot is handling the
>>>>>> arguments.
>>>>>>
>>>>>> I'll look into it and see what I find after the holidays.
>>>> I dug into this a little more. Adding the APPEND_CFLAGS="-Wno-error"
>>>> fixes almost everything. The only problem I see is in the log below,
>>>> where tools/kconfig/conf.c fails to build as the -Wno-error doesn't
>>>> propagate down.
>>>>
>>>> If I manage to find a fix today I'll send it, otherwise this can wait
>>>> until next year.
>>>
>>> Something like this?
>>>
>>> diff --git a/xen/Makefile b/xen/Makefile
>>> index dc6862e04d..2d7a567c9c 100644
>>> --- a/xen/Makefile
>>> +++ b/xen/Makefile
>>> @@ -253,14 +253,14 @@ kconfig := silentoldconfig oldconfig config
>>> menuconfig defconfig \
>>>         randconfig
>>>  .PHONY: $(kconfig)
>>>  $(kconfig):
>>> -       $(MAKE) -f $(BASEDIR)/tools/kconfig/Makefile.kconfig
>>> ARCH=$(ARCH) SRCARCH=$(SRCARCH) HOSTCC="$(HOSTCC)" HOSTCXX="$(HOSTCXX)" $@
>>> +       $(MAKE) -f $(BASEDIR)/tools/kconfig/Makefile.kconfig
>>> ARCH=$(ARCH) SRCARCH=$(SRCARCH) HOSTCC="$(HOSTCC)" HOSTCXX="$(HOSTCXX)"
>>> HOST_EXTRACFLAGS="$(APPEND_CFLAGS)" $@
>>>
>>>  include/config/%.conf: include/config/auto.conf.cmd $(KCONFIG_CONFIG)
>>> -       $(MAKE) -f $(BASEDIR)/tools/kconfig/Makefile.kconfig
>>> ARCH=$(ARCH) SRCARCH=$(SRCARCH) HOSTCC="$(HOSTCC)" HOSTCXX="$(HOSTCXX)"
>>> silentoldconfig
>>> +       $(MAKE) -f $(BASEDIR)/tools/kconfig/Makefile.kconfig
>>> ARCH=$(ARCH) SRCARCH=$(SRCARCH) HOSTCC="$(HOSTCC)" HOSTCXX="$(HOSTCXX)"
>>> HOST_EXTRACFLAGS="$(APPEND_CFLAGS)" silentoldconfig
>>>
>>>  # Allow people to just run `make` as before and not force them to
>>> configure
>>>  $(KCONFIG_CONFIG):
>>> -       $(MAKE) -f $(BASEDIR)/tools/kconfig/Makefile.kconfig
>>> ARCH=$(ARCH) SRCARCH=$(SRCARCH) HOSTCC="$(HOSTCC)" HOSTCXX="$(HOSTCXX)"
>>> defconfig
>>> +       $(MAKE) -f $(BASEDIR)/tools/kconfig/Makefile.kconfig
>>> ARCH=$(ARCH) SRCARCH=$(SRCARCH) HOSTCC="$(HOSTCC)" HOSTCXX="$(HOSTCXX)"
>>> HOST_EXTRACFLAGS="$(APPEND_CFLAGS)" defconfig
>>>
>>>  # Break the dependency chain for the first run
>>>  include/config/auto.conf.cmd: ;
>>>
>>
>> That should do the trick.
>>
>> Reviewed-by: Doug Goldstein <cardoe@cardoe.com>
>
> I got this to work as well:
>
> diff --git a/xen/tools/kconfig/Makefile b/xen/tools/kconfig/Makefile
> index aceaaed..32e2359 100644
> --- a/xen/tools/kconfig/Makefile
> +++ b/xen/tools/kconfig/Makefile
> @@ -155,7 +155,7 @@ check-lxdialog  :=
> $(srctree)/$(src)/lxdialog/check-lxdialog.sh
>  # Use recursively expanded variables so we do not call gcc unless
>  # we really need to do so. (Do not call gcc as part of make mrproper)
>  HOST_EXTRACFLAGS += $(shell $(CONFIG_SHELL) $(check-lxdialog) -ccflags) \
> -                    -DLOCALE
> +                    -DLOCALE $(APPEND_CFLAGS)
>
>  # ===========================================================================
>  # Shared Makefile for the various kconfig executables:
>
> But yours looks like it should work as well.
>
> Do you want to send a patch?

You can add this to it:

Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
Tested-by: Alistair Francis <alistair.francis@xilinx.com>

Thanks,

Alistair

>
> Thanks,
>
> Alistair
>
>>
>> --
>> Doug Goldstein
>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> https://lists.xen.org/xen-devel
>>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 1/5] Remove hardcoded strict -Werror checking

[Jan Beulich]

>>> Alistair Francis <alistair.francis@xilinx.com> 12/22/16 8:14 PM >>>
>On Thu, Dec 22, 2016 at 12:41 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 20.12.16 at 20:46, <alistair.francis@xilinx.com> wrote:
>>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>>
>> Without some rationale given I don't think such changes are
>> acceptable at all. And then, as already pointed out others, the
>> use of -Werror is there not just for fun. If anything I think an
>> override to that default could be acceptable.
>
>Unfortunately the APPEND_CFLAGS=-Wno-error doesn't fix all the issues
>as I still see warnings/errors when building: tools/kconfig/conf.c.

Well, it's quite obvious that the same set of options (and hence overrides)
can't possibly fit both the build of target binaries and the build of build
helper tools (i.e. build host binaries).

>I understand that you want it in by default.
>
>Everyone seems fairly open to an override. Is a environment variable,
>which if set will disable Werror acceptable? Something like NO_ERROR=Y
>which will result in no -Werror being appended.

I dislike environment variables for such purposes, and would prefer requiring
such to be added as make options. If it was an environment variable, it
should start with XEN_. And its name should fully reflect the purpose, i.e. I
shouldn't have to guess what kinds of errors would be suppressed. Perhaps
WARN_NO_ERROR?

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 1/5] Remove hardcoded strict -Werror checking

[Jan Beulich]

>>> "Jan Beulich" <jbeulich@suse.com> 12/27/16 4:42 PM >>>
>>> Alistair Francis <alistair.francis@xilinx.com> 12/22/16 8:14 PM >>>
>>Everyone seems fairly open to an override. Is a environment variable,
>>which if set will disable Werror acceptable? Something like NO_ERROR=Y
>>which will result in no -Werror being appended.
>
>I dislike environment variables for such purposes, and would prefer requiring
>such to be added as make options. If it was an environment variable, it
>should start with XEN_. And its name should fully reflect the purpose, i.e. I
>shouldn't have to guess what kinds of errors would be suppressed. Perhaps
>WARN_NO_ERROR?

That said, I'm not sure everyone agreed on putting an override in place. I think
Andrew had made it quite clear that there is a reason for -Werror to be in use,
and we shouldn't encourage people weakening code by tolerating warnings
(even if for the purpose of upstream integration no warnings will be permitted
anyway, due to -Werror remaining the default).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 1/5] Remove hardcoded strict -Werror checking

[Doug Goldstein]

[-- Attachment #1.1.1: Type: text/plain, Size: 1516 bytes --]

On 12/27/16 9:53 AM, Jan Beulich wrote:
>>>> "Jan Beulich" <jbeulich@suse.com> 12/27/16 4:42 PM >>>
>>>> Alistair Francis <alistair.francis@xilinx.com> 12/22/16 8:14 PM >>>
>>> Everyone seems fairly open to an override. Is a environment variable,
>>> which if set will disable Werror acceptable? Something like NO_ERROR=Y
>>> which will result in no -Werror being appended.
>>
>> I dislike environment variables for such purposes, and would prefer requiring
>> such to be added as make options. If it was an environment variable, it
>> should start with XEN_. And its name should fully reflect the purpose, i.e. I
>> shouldn't have to guess what kinds of errors would be suppressed. Perhaps
>> WARN_NO_ERROR?
> 
> That said, I'm not sure everyone agreed on putting an override in place. I think
> Andrew had made it quite clear that there is a reason for -Werror to be in use,
> and we shouldn't encourage people weakening code by tolerating warnings
> (even if for the purpose of upstream integration no warnings will be permitted
> anyway, due to -Werror remaining the default).
> 
> Jan
> 

That was for the hypervisor bits. Anything that relies on user space
code and headers that will be provided by the distro should be able to
build with -Werror. That's something all distros have to patch out and
is downstream hostile to do since a different toolchain than what the
developers use can generate warnings or slight different dependencies an
generate them.

-- 
Doug Goldstein


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 959 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 1/5] Remove hardcoded strict -Werror checking

[Andrew Cooper]

On 27/12/16 15:53, Jan Beulich wrote:
>>>> "Jan Beulich" <jbeulich@suse.com> 12/27/16 4:42 PM >>>
>>>> Alistair Francis <alistair.francis@xilinx.com> 12/22/16 8:14 PM >>>
>>> Everyone seems fairly open to an override. Is a environment variable,
>>> which if set will disable Werror acceptable? Something like NO_ERROR=Y
>>> which will result in no -Werror being appended.
>> I dislike environment variables for such purposes, and would prefer requiring
>> such to be added as make options. If it was an environment variable, it
>> should start with XEN_. And its name should fully reflect the purpose, i.e. I
>> shouldn't have to guess what kinds of errors would be suppressed. Perhaps
>> WARN_NO_ERROR?
> That said, I'm not sure everyone agreed on putting an override in place. I think
> Andrew had made it quite clear that there is a reason for -Werror to be in use,
> and we shouldn't encourage people weakening code by tolerating warnings
> (even if for the purpose of upstream integration no warnings will be permitted
> anyway, due to -Werror remaining the default).

For development, -Werror should remain the default.

For downstream integration, an ability to override -Werror is useful for 
distros, especially in cases of using a newer compiler than the code was 
ever developed against.

However, it should definitely be the case that a positive choice needs 
to be taken to disable -Werror, which should hopefully make people thing 
twice about doing so.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 1/5] Remove hardcoded strict -Werror checking

[George Dunlap]

On Tue, Dec 27, 2016 at 5:07 PM, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
> On 27/12/16 15:53, Jan Beulich wrote:
>>>>>
>>>>> "Jan Beulich" <jbeulich@suse.com> 12/27/16 4:42 PM >>>
>>>>> Alistair Francis <alistair.francis@xilinx.com> 12/22/16 8:14 PM >>>
>>>>
>>>> Everyone seems fairly open to an override. Is a environment variable,
>>>> which if set will disable Werror acceptable? Something like NO_ERROR=Y
>>>> which will result in no -Werror being appended.
>>>
>>> I dislike environment variables for such purposes, and would prefer
>>> requiring
>>> such to be added as make options. If it was an environment variable, it
>>> should start with XEN_. And its name should fully reflect the purpose,
>>> i.e. I
>>> shouldn't have to guess what kinds of errors would be suppressed. Perhaps
>>> WARN_NO_ERROR?
>>
>> That said, I'm not sure everyone agreed on putting an override in place. I
>> think
>> Andrew had made it quite clear that there is a reason for -Werror to be in
>> use,
>> and we shouldn't encourage people weakening code by tolerating warnings
>> (even if for the purpose of upstream integration no warnings will be
>> permitted
>> anyway, due to -Werror remaining the default).
>
>
> For development, -Werror should remain the default.
>
> For downstream integration, an ability to override -Werror is useful for
> distros, especially in cases of using a newer compiler than the code was
> ever developed against.
>
> However, it should definitely be the case that a positive choice needs to be
> taken to disable -Werror, which should hopefully make people thing twice
> about doing so.

Wouldn't it make more sense to disable -Werror for the release?

This seems similar to ASSERT().  The idea behind having ASSERT()
enabled during development and disabled for releases is that there are
different goals for each.  During development you expect things to be
unstable and crash, and you want to fix anything that comes up, and
you definitely want to catch as many bugs as you possibly can.  Once
something is released and put into production, the priority changes --
the goal now is to keep the system up and running, not to find bugs;
so everything for which an incorrect assumption is *less bad* than a
crash should not crash.

Similarly, during development we definitely want all the help we can
get to find programmer mistakes; and so having the build fail every
time gcc detects something funny is a big help for that.

But with a release, the priorities are different.  Releases are not
for developers, but for downstreams and for users.  They're not really
in a position to fix the issue.  And in any case, whatever the issue
is, it's pretty certain that dozens of other places with older
compilers are *already* running with that issue and just don't know
about it.  So running without -Werror *just for the release* makes the
project as a whole more useful for downstreams and users, without
hurting development, it seems to me.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 1/5] Remove hardcoded strict -Werror checking

[Andrew Cooper]

On 29/12/2016 17:10, George Dunlap wrote:
> On Tue, Dec 27, 2016 at 5:07 PM, Andrew Cooper
> <andrew.cooper3@citrix.com> wrote:
>> On 27/12/16 15:53, Jan Beulich wrote:
>>>>>> "Jan Beulich" <jbeulich@suse.com> 12/27/16 4:42 PM >>>
>>>>>> Alistair Francis <alistair.francis@xilinx.com> 12/22/16 8:14 PM >>>
>>>>> Everyone seems fairly open to an override. Is a environment variable,
>>>>> which if set will disable Werror acceptable? Something like NO_ERROR=Y
>>>>> which will result in no -Werror being appended.
>>>> I dislike environment variables for such purposes, and would prefer
>>>> requiring
>>>> such to be added as make options. If it was an environment variable, it
>>>> should start with XEN_. And its name should fully reflect the purpose,
>>>> i.e. I
>>>> shouldn't have to guess what kinds of errors would be suppressed. Perhaps
>>>> WARN_NO_ERROR?
>>> That said, I'm not sure everyone agreed on putting an override in place. I
>>> think
>>> Andrew had made it quite clear that there is a reason for -Werror to be in
>>> use,
>>> and we shouldn't encourage people weakening code by tolerating warnings
>>> (even if for the purpose of upstream integration no warnings will be
>>> permitted
>>> anyway, due to -Werror remaining the default).
>>
>> For development, -Werror should remain the default.
>>
>> For downstream integration, an ability to override -Werror is useful for
>> distros, especially in cases of using a newer compiler than the code was
>> ever developed against.
>>
>> However, it should definitely be the case that a positive choice needs to be
>> taken to disable -Werror, which should hopefully make people thing twice
>> about doing so.
> Wouldn't it make more sense to disable -Werror for the release?

-1.  This is not a sensible suggestion IMO.

The reason for switching off debugging for a release is because there is
a runtime overhead of the debugging, and we don't provide security
support in case the ASSERT()s are a little too aggressive.

The reason behind using -Werror doesn't change at the point of a
release.  A warning on a release branch is just as important to fix as a
warning on master.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 1/5] Remove hardcoded strict -Werror checking

[Jan Beulich]

>>> On 29.12.16 at 18:30, <andrew.cooper3@citrix.com> wrote:
> On 29/12/2016 17:10, George Dunlap wrote:
>> On Tue, Dec 27, 2016 at 5:07 PM, Andrew Cooper
>> <andrew.cooper3@citrix.com> wrote:
>>> On 27/12/16 15:53, Jan Beulich wrote:
>>>>>>> "Jan Beulich" <jbeulich@suse.com> 12/27/16 4:42 PM >>>
>>>>>>> Alistair Francis <alistair.francis@xilinx.com> 12/22/16 8:14 PM >>>
>>>>>> Everyone seems fairly open to an override. Is a environment variable,
>>>>>> which if set will disable Werror acceptable? Something like NO_ERROR=Y
>>>>>> which will result in no -Werror being appended.
>>>>> I dislike environment variables for such purposes, and would prefer
>>>>> requiring
>>>>> such to be added as make options. If it was an environment variable, it
>>>>> should start with XEN_. And its name should fully reflect the purpose,
>>>>> i.e. I
>>>>> shouldn't have to guess what kinds of errors would be suppressed. Perhaps
>>>>> WARN_NO_ERROR?
>>>> That said, I'm not sure everyone agreed on putting an override in place. I
>>>> think
>>>> Andrew had made it quite clear that there is a reason for -Werror to be in
>>>> use,
>>>> and we shouldn't encourage people weakening code by tolerating warnings
>>>> (even if for the purpose of upstream integration no warnings will be
>>>> permitted
>>>> anyway, due to -Werror remaining the default).
>>>
>>> For development, -Werror should remain the default.
>>>
>>> For downstream integration, an ability to override -Werror is useful for
>>> distros, especially in cases of using a newer compiler than the code was
>>> ever developed against.
>>>
>>> However, it should definitely be the case that a positive choice needs to be
>>> taken to disable -Werror, which should hopefully make people thing twice
>>> about doing so.
>> Wouldn't it make more sense to disable -Werror for the release?
> 
> -1.  This is not a sensible suggestion IMO.
> 
> The reason for switching off debugging for a release is because there is
> a runtime overhead of the debugging, and we don't provide security
> support in case the ASSERT()s are a little too aggressive.
> 
> The reason behind using -Werror doesn't change at the point of a
> release.  A warning on a release branch is just as important to fix as a
> warning on master.

The more that, with changed optimization settings, warnings
occasionally change too (potentially pointing out so far overlooked
issues). If _all_ our downstreams participated in at least RC testing,
the whole situation might be a little different, but without that I
think we should rather hope for them to report issues with compiler
versions no-one of us tried to build with.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 1/5] Remove hardcoded strict -Werror checking

[Ian Jackson]

Jan Beulich writes ("Re: [Xen-devel] [PATCH v2 1/5] Remove hardcoded strict -Werror checking"):
> Alistair Francis <alistair.francis@xilinx.com> 12/22/16 8:14 PM >>>
> >Unfortunately the APPEND_CFLAGS=-Wno-error doesn't fix all the issues
> >as I still see warnings/errors when building: tools/kconfig/conf.c.
> 
> Well, it's quite obvious that the same set of options (and hence overrides)
> can't possibly fit both the build of target binaries and the build of build
> helper tools (i.e. build host binaries).

I think that the example of `-Wno-error' shows that this is, in fact,
false.  There are some overrides that, if desirable, apply equally
everywhere.

-Wno-error is a good example because it is mostly needed when a new
compiler throws up new warnings for existing code.  Admittedly,
-Wno-error may not be the best response, but it is often the most
expedient, and we shouldn't make it unnecessarily hard for
downstreams to support it.

For now I think the proposed approach, to make kconf build honour
APPEND_CFLAGS, is good.  Do you agree, Jan ?

If someone wants to introduce HYPERVISOR_APPEND_CFLAGS and
TOOLS_APPEND_CFLAGS, I don't object, but that should be a separate
project.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 1/5] Remove hardcoded strict -Werror checking

[Jan Beulich]

>>> On 03.01.17 at 15:38, <ian.jackson@eu.citrix.com> wrote:
> Jan Beulich writes ("Re: [Xen-devel] [PATCH v2 1/5] Remove hardcoded strict 
> -Werror checking"):
>> Alistair Francis <alistair.francis@xilinx.com> 12/22/16 8:14 PM >>>
>> >Unfortunately the APPEND_CFLAGS=-Wno-error doesn't fix all the issues
>> >as I still see warnings/errors when building: tools/kconfig/conf.c.
>> 
>> Well, it's quite obvious that the same set of options (and hence overrides)
>> can't possibly fit both the build of target binaries and the build of build
>> helper tools (i.e. build host binaries).
> 
> I think that the example of `-Wno-error' shows that this is, in fact,
> false.  There are some overrides that, if desirable, apply equally
> everywhere.

I agree, but I don't think this is relevant here: How does one
easily(!) know that APPEND_CFLAGS affects both host and
target binaries? Just like there is HOSTCC and HOSTCFLAGS,
there should be (e.g.) APPEND_HOSTCFLAGS. What if
$(HOSTCC) doesn't even understand -Wno-error (or any other
option put into such a shared variable)?

> -Wno-error is a good example because it is mostly needed when a new
> compiler throws up new warnings for existing code.  Admittedly,
> -Wno-error may not be the best response, but it is often the most
> expedient, and we shouldn't make it unnecessarily hard for
> downstreams to support it.
> 
> For now I think the proposed approach, to make kconf build honour
> APPEND_CFLAGS, is good.  Do you agree, Jan ?
> 
> If someone wants to introduce HYPERVISOR_APPEND_CFLAGS and
> TOOLS_APPEND_CFLAGS, I don't object, but that should be a separate
> project.

As per above, I disagree. We shouldn't help people making
mistakes by lumping together two disjoint sets of flags.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v2 2/5] config/StdGNU.mk: Allows users to pass in EXTRA_CFLAGS

[Alistair Francis]

Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---
 config/StdGNU.mk | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/config/StdGNU.mk b/config/StdGNU.mk
index 6be8233..a6cdd82 100644
--- a/config/StdGNU.mk
+++ b/config/StdGNU.mk
@@ -35,6 +35,9 @@ UTIL_LIBS = -lutil
 SONAME_LDFLAG = -soname
 SHLIB_LDFLAGS = -shared
 
+# Allow users to add extra CFLAGS
+CFLAGS += $(EXTRA_CFLAGS)
+
 ifeq ($(lto),y)
 CFLAGS += -flto
 LDFLAGS-$(clang) += -plugin LLVMgold.so
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 2/5] config/StdGNU.mk: Allows users to pass in EXTRA_CFLAGS

[Doug Goldstein]

[-- Attachment #1.1.1: Type: text/plain, Size: 662 bytes --]

On 12/20/16 1:46 PM, Alistair Francis wrote:
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> ---
>  config/StdGNU.mk | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/config/StdGNU.mk b/config/StdGNU.mk
> index 6be8233..a6cdd82 100644
> --- a/config/StdGNU.mk
> +++ b/config/StdGNU.mk
> @@ -35,6 +35,9 @@ UTIL_LIBS = -lutil
>  SONAME_LDFLAG = -soname
>  SHLIB_LDFLAGS = -shared
>  
> +# Allow users to add extra CFLAGS
> +CFLAGS += $(EXTRA_CFLAGS)
> +
>  ifeq ($(lto),y)
>  CFLAGS += -flto
>  LDFLAGS-$(clang) += -plugin LLVMgold.so
> 

Reviewed-by: Doug Goldstein <cardoe@cardoe.com>

-- 
Doug Goldstein


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 959 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 2/5] config/StdGNU.mk: Allows users to pass in EXTRA_CFLAGS

[Wei Liu]

On Tue, Dec 20, 2016 at 11:46:57AM -0800, Alistair Francis wrote:
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> ---
>  config/StdGNU.mk | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/config/StdGNU.mk b/config/StdGNU.mk
> index 6be8233..a6cdd82 100644
> --- a/config/StdGNU.mk
> +++ b/config/StdGNU.mk
> @@ -35,6 +35,9 @@ UTIL_LIBS = -lutil
>  SONAME_LDFLAG = -soname
>  SHLIB_LDFLAGS = -shared
>  
> +# Allow users to add extra CFLAGS
> +CFLAGS += $(EXTRA_CFLAGS)
> +

Assuming you mostly want to eliminate tools build flag(s) -- there are
already two things for similar tasks: APPEND_CFLAGS and
EXTRA_CFLAGS_XEN_TOOLS. Do they not fit for your purpose?

>  ifeq ($(lto),y)
>  CFLAGS += -flto
>  LDFLAGS-$(clang) += -plugin LLVMgold.so
> -- 
> 2.7.4
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 2/5] config/StdGNU.mk: Allows users to pass in EXTRA_CFLAGS

[Jan Beulich]

>>> On 20.12.16 at 20:46, <alistair.francis@xilinx.com> wrote:
> --- a/config/StdGNU.mk
> +++ b/config/StdGNU.mk
> @@ -35,6 +35,9 @@ UTIL_LIBS = -lutil
>  SONAME_LDFLAG = -soname
>  SHLIB_LDFLAGS = -shared
>  
> +# Allow users to add extra CFLAGS
> +CFLAGS += $(EXTRA_CFLAGS)

Along the lines of Wei's reply - please remember that this controls
both tools and hypervisor builds, yet it seems quite possible that
intended overrides may only be for one of the two.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v2 3/5] tools/blktap2/vhd: Remove unused struct stat stats

[Alistair Francis]

The unsued variable 'struct stat stats' causes build errors in some
situations. As it isn't used just remove it.

Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---
 tools/blktap2/vhd/lib/libvhd-journal.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tools/blktap2/vhd/lib/libvhd-journal.c b/tools/blktap2/vhd/lib/libvhd-journal.c
index 26e26e7..862890f 100644
--- a/tools/blktap2/vhd/lib/libvhd-journal.c
+++ b/tools/blktap2/vhd/lib/libvhd-journal.c
@@ -1260,7 +1260,6 @@ vhd_journal_create(vhd_journal_t *j, const char *file, const char *jfile)
 	int i, err;
 	size_t size;
 	off_t off;
-	struct stat stats;
 
 	memset(j, 0, sizeof(vhd_journal_t));
 	j->jfd = -1;
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 3/5] tools/blktap2/vhd: Remove unused struct stat stats

[Doug Goldstein]

[-- Attachment #1.1.1: Type: text/plain, Size: 872 bytes --]

On 12/20/16 1:46 PM, Alistair Francis wrote:
> The unsued variable 'struct stat stats' causes build errors in some
> situations. As it isn't used just remove it.
> 
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> ---
>  tools/blktap2/vhd/lib/libvhd-journal.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/tools/blktap2/vhd/lib/libvhd-journal.c b/tools/blktap2/vhd/lib/libvhd-journal.c
> index 26e26e7..862890f 100644
> --- a/tools/blktap2/vhd/lib/libvhd-journal.c
> +++ b/tools/blktap2/vhd/lib/libvhd-journal.c
> @@ -1260,7 +1260,6 @@ vhd_journal_create(vhd_journal_t *j, const char *file, const char *jfile)
>  	int i, err;
>  	size_t size;
>  	off_t off;
> -	struct stat stats;
>  
>  	memset(j, 0, sizeof(vhd_journal_t));
>  	j->jfd = -1;
> 

Reviewed-by: Doug Goldstein <cardoe@cardoe.com>

-- 
Doug Goldstein


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 959 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v2 4/5] tools/blktap2: Fix missing header file

[Alistair Francis]

To avoid build errors relating to missing delcarations of ssize_t add
the appripriote header file to atomic.h.

Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---
 tools/blktap2/include/atomicio.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/blktap2/include/atomicio.h b/tools/blktap2/include/atomicio.h
index 7eccf20..5a1120e 100644
--- a/tools/blktap2/include/atomicio.h
+++ b/tools/blktap2/include/atomicio.h
@@ -25,6 +25,8 @@
  * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 
+#include <sys/types.h>
+
 /*
  * Ensure all of data on socket comes through. f==read || f==vwrite
  */
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 4/5] tools/blktap2: Fix missing header file

[Konrad Rzeszutek Wilk]

On Tue, Dec 20, 2016 at 11:46:59AM -0800, Alistair Francis wrote:
> To avoid build errors relating to missing delcarations of ssize_t add
> the appripriote header file to atomic.h.

appropiate.

> 
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> ---
>  tools/blktap2/include/atomicio.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/tools/blktap2/include/atomicio.h b/tools/blktap2/include/atomicio.h
> index 7eccf20..5a1120e 100644
> --- a/tools/blktap2/include/atomicio.h
> +++ b/tools/blktap2/include/atomicio.h
> @@ -25,6 +25,8 @@
>   * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>   */
>  
> +#include <sys/types.h>
> +
>  /*
>   * Ensure all of data on socket comes through. f==read || f==vwrite
>   */
> -- 
> 2.7.4
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 4/5] tools/blktap2: Fix missing header file

[Doug Goldstein]

[-- Attachment #1.1.1: Type: text/plain, Size: 865 bytes --]

On 12/20/16 1:46 PM, Alistair Francis wrote:
> To avoid build errors relating to missing delcarations of ssize_t add

declarations

> the appripriote header file to atomic.h.

appropriate

> 
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> ---
>  tools/blktap2/include/atomicio.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/tools/blktap2/include/atomicio.h b/tools/blktap2/include/atomicio.h
> index 7eccf20..5a1120e 100644
> --- a/tools/blktap2/include/atomicio.h
> +++ b/tools/blktap2/include/atomicio.h
> @@ -25,6 +25,8 @@
>   * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>   */
>  
> +#include <sys/types.h>
> +
>  /*
>   * Ensure all of data on socket comes through. f==read || f==vwrite
>   */
> 

Reviewed-by: Doug Goldstein <cardoe@cardoe.com>

-- 
Doug Goldstein


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 959 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v2 5/5] tools/blktap2/drivers: Remove non-existent sys/sysctl.h include

[Alistair Francis]

To avoid build errors related to missing file 'sys/sysctl.h' by removing
the #include statement.

Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---
 tools/blktap2/drivers/block-remus.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tools/blktap2/drivers/block-remus.c b/tools/blktap2/drivers/block-remus.c
index 079588d..7401800 100644
--- a/tools/blktap2/drivers/block-remus.c
+++ b/tools/blktap2/drivers/block-remus.c
@@ -54,7 +54,6 @@
 #include <netinet/in.h>
 #include <arpa/inet.h>
 #include <sys/param.h>
-#include <sys/sysctl.h>
 #include <unistd.h>
 #include <sys/stat.h>
 
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 5/5] tools/blktap2/drivers: Remove non-existent sys/sysctl.h include

[Wei Liu]

On Tue, Dec 20, 2016 at 11:47:00AM -0800, Alistair Francis wrote:
> To avoid build errors related to missing file 'sys/sysctl.h' by removing
> the #include statement.
> 
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>

I can find this in Linux. Maybe this is also due to the libc you're
using?

On the flip side, if this header is unused anyway I think I'm fine with
taking this patch.

> ---
>  tools/blktap2/drivers/block-remus.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/tools/blktap2/drivers/block-remus.c b/tools/blktap2/drivers/block-remus.c
> index 079588d..7401800 100644
> --- a/tools/blktap2/drivers/block-remus.c
> +++ b/tools/blktap2/drivers/block-remus.c
> @@ -54,7 +54,6 @@
>  #include <netinet/in.h>
>  #include <arpa/inet.h>
>  #include <sys/param.h>
> -#include <sys/sysctl.h>
>  #include <unistd.h>
>  #include <sys/stat.h>
>  
> -- 
> 2.7.4
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 5/5] tools/blktap2/drivers: Remove non-existent sys/sysctl.h include

[Alistair Francis]

On Thu, Dec 22, 2016 at 2:54 AM, Wei Liu <wei.liu2@citrix.com> wrote:
> On Tue, Dec 20, 2016 at 11:47:00AM -0800, Alistair Francis wrote:
>> To avoid build errors related to missing file 'sys/sysctl.h' by removing
>> the #include statement.
>>
>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>
> I can find this in Linux. Maybe this is also due to the libc you're
> using?

Yes, I think you are right. I am using musl when I see the error.

>
> On the flip side, if this header is unused anyway I think I'm fine with
> taking this patch.

Thanks, that is what I was thinking too.

Thanks,

Alistair

>
>> ---
>>  tools/blktap2/drivers/block-remus.c | 1 -
>>  1 file changed, 1 deletion(-)
>>
>> diff --git a/tools/blktap2/drivers/block-remus.c b/tools/blktap2/drivers/block-remus.c
>> index 079588d..7401800 100644
>> --- a/tools/blktap2/drivers/block-remus.c
>> +++ b/tools/blktap2/drivers/block-remus.c
>> @@ -54,7 +54,6 @@
>>  #include <netinet/in.h>
>>  #include <arpa/inet.h>
>>  #include <sys/param.h>
>> -#include <sys/sysctl.h>
>>  #include <unistd.h>
>>  #include <sys/stat.h>
>>
>> --
>> 2.7.4
>>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 5/5] tools/blktap2/drivers: Remove non-existent sys/sysctl.h include

[Wei Liu]

On Thu, Dec 22, 2016 at 07:44:19AM -0800, Alistair Francis wrote:
> On Thu, Dec 22, 2016 at 2:54 AM, Wei Liu <wei.liu2@citrix.com> wrote:
> > On Tue, Dec 20, 2016 at 11:47:00AM -0800, Alistair Francis wrote:
> >> To avoid build errors related to missing file 'sys/sysctl.h' by removing
> >> the #include statement.
> >>
> >> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> >
> > I can find this in Linux. Maybe this is also due to the libc you're
> > using?
> 
> Yes, I think you are right. I am using musl when I see the error.
> 
> >
> > On the flip side, if this header is unused anyway I think I'm fine with
> > taking this patch.
> 
> Thanks, that is what I was thinking too.
> 

OK. I think I will need to update the commit message to:

  tools/blktap2: remove unused inclusion of sys/sysctl.h
  
  That header file is not used. Removing it would avoid build error with
  musl libc, which doesn't have that header.

If you have no objection I can make the change while committing.

Wei.

> Thanks,
> 
> Alistair
> 
> >
> >> ---
> >>  tools/blktap2/drivers/block-remus.c | 1 -
> >>  1 file changed, 1 deletion(-)
> >>
> >> diff --git a/tools/blktap2/drivers/block-remus.c b/tools/blktap2/drivers/block-remus.c
> >> index 079588d..7401800 100644
> >> --- a/tools/blktap2/drivers/block-remus.c
> >> +++ b/tools/blktap2/drivers/block-remus.c
> >> @@ -54,7 +54,6 @@
> >>  #include <netinet/in.h>
> >>  #include <arpa/inet.h>
> >>  #include <sys/param.h>
> >> -#include <sys/sysctl.h>
> >>  #include <unistd.h>
> >>  #include <sys/stat.h>
> >>
> >> --
> >> 2.7.4
> >>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 5/5] tools/blktap2/drivers: Removenon-existent sys/sysctl.h include

[alistair23]

[-- Attachment #1.1: Type: text/plain, Size: 2245 bytes --]

Hey Wei,

Sorry about the top post but I’m doing this from my phone.

That sounds good to me, you can use that commit message.

Thanks,

Alistair

From: Wei Liu
Sent: Friday, 23 December 2016 6:12 AM
To: Alistair Francis
Cc: Wei Liu; Doug Goldstein; ian.jackson@eu.citrix.com; imhy.yang@gmail.com; rshriram@cs.ubc.ca; xen-devel@lists.xenproject.org
Subject: Re: [Xen-devel] [PATCH v2 5/5] tools/blktap2/drivers: Removenon-existent sys/sysctl.h include

On Thu, Dec 22, 2016 at 07:44:19AM -0800, Alistair Francis wrote:
> On Thu, Dec 22, 2016 at 2:54 AM, Wei Liu <wei.liu2@citrix.com> wrote:
> > On Tue, Dec 20, 2016 at 11:47:00AM -0800, Alistair Francis wrote:
> >> To avoid build errors related to missing file 'sys/sysctl.h' by removing
> >> the #include statement.
> >>
> >> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> >
> > I can find this in Linux. Maybe this is also due to the libc you're
> > using?
> 
> Yes, I think you are right. I am using musl when I see the error.
> 
> >
> > On the flip side, if this header is unused anyway I think I'm fine with
> > taking this patch.
> 
> Thanks, that is what I was thinking too.
> 

OK. I think I will need to update the commit message to:

  tools/blktap2: remove unused inclusion of sys/sysctl.h
  
  That header file is not used. Removing it would avoid build error with
  musl libc, which doesn't have that header.

If you have no objection I can make the change while committing.

Wei.

> Thanks,
> 
> Alistair
> 
> >
> >> ---
> >>  tools/blktap2/drivers/block-remus.c | 1 -
> >>  1 file changed, 1 deletion(-)
> >>
> >> diff --git a/tools/blktap2/drivers/block-remus.c b/tools/blktap2/drivers/block-remus.c
> >> index 079588d..7401800 100644
> >> --- a/tools/blktap2/drivers/block-remus.c
> >> +++ b/tools/blktap2/drivers/block-remus.c
> >> @@ -54,7 +54,6 @@
> >>  #include <netinet/in.h>
> >>  #include <arpa/inet.h>
> >>  #include <sys/param.h>
> >> -#include <sys/sysctl.h>
> >>  #include <unistd.h>
> >>  #include <sys/stat.h>
> >>
> >> --
> >> 2.7.4
> >>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel


[-- Attachment #1.2: Type: text/html, Size: 6007 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 5/5] tools/blktap2/drivers: Removenon-existent sys/sysctl.h include

[Wei Liu]

On Fri, Dec 23, 2016 at 05:09:47PM -0500, alistair23@gmail.com wrote:
> Hey Wei,
> 
> Sorry about the top post but I’m doing this from my phone.
> 
> That sounds good to me, you can use that commit message.
> 

Thanks for confirming. 

Pushed to staging.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 5/5] tools/blktap2/drivers: Remove non-existent sys/sysctl.h include

[Doug Goldstein]

[-- Attachment #1.1.1: Type: text/plain, Size: 859 bytes --]

On 12/20/16 1:47 PM, Alistair Francis wrote:
> To avoid build errors related to missing file 'sys/sysctl.h' by removing
> the #include statement.
> 
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> ---
>  tools/blktap2/drivers/block-remus.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/tools/blktap2/drivers/block-remus.c b/tools/blktap2/drivers/block-remus.c
> index 079588d..7401800 100644
> --- a/tools/blktap2/drivers/block-remus.c
> +++ b/tools/blktap2/drivers/block-remus.c
> @@ -54,7 +54,6 @@
>  #include <netinet/in.h>
>  #include <arpa/inet.h>
>  #include <sys/param.h>
> -#include <sys/sysctl.h>
>  #include <unistd.h>
>  #include <sys/stat.h>
>  
> 

A bit late to the party but I also confirmed the header was unused.

Reviewed-by: Doug Goldstein <cardoe@cardoe.com>

-- 
Doug Goldstein


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 959 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 0/5] General Build Fixes

[Konrad Rzeszutek Wilk]

On Tue, Dec 20, 2016 at 11:46:55AM -0800, Alistair Francis wrote:
> This patch series is a list of build issues that appeared when
> buildling Xen 4.8.0 in buildroot. Hopefully some of them can be

Is there an corresponding patch in buildroot for using Xen?

Thank you for reposting!

> accepted upstream to help others who are trying to build Xen in
> the future.
> 
> V2:
>  - Remove the #include path changes
>     - It turns out this only applies to musl (although it shouldn't
>       break glibc though). Instead of renaming the #include files I
>       have just removed the -Werror flags.
>  - Allow people building Xen to pass in EXTRA_CFLAGS.
> 
> Alistair Francis (5):
>   Remove hardcoded strict -Werror checking
>   config/StdGNU.mk: Allows users to pass in EXTRA_CFLAGS
>   tools/blktap2/vhd: Remove unused struct stat stats
>   tools/blktap2: Fix missing header file
>   tools/blktap2/drivers: Remove non-existent sys/sysctl.h include
> 
>  Config.mk                              | 2 +-
>  config/StdGNU.mk                       | 3 +++
>  tools/blktap2/drivers/Makefile         | 1 -
>  tools/blktap2/drivers/block-remus.c    | 1 -
>  tools/blktap2/include/atomicio.h       | 2 ++
>  tools/blktap2/vhd/lib/libvhd-journal.c | 1 -
>  tools/libxl/Makefile                   | 2 +-
>  tools/xentrace/Makefile                | 2 --
>  8 files changed, 7 insertions(+), 7 deletions(-)
> 
> -- 
> 2.7.4
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 0/5] General Build Fixes

[Alistair Francis]

On Tue, Dec 20, 2016 at 11:53 AM, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
> On Tue, Dec 20, 2016 at 11:46:55AM -0800, Alistair Francis wrote:
>> This patch series is a list of build issues that appeared when
>> buildling Xen 4.8.0 in buildroot. Hopefully some of them can be
>
> Is there an corresponding patch in buildroot for using Xen?

Not yet. Buildroot currently builds Xen 4.7.1. I sent a patch a day or
two ago to update it to version 4.8.0 with my original patch series
included.

I will have to repost to buildroot with these new patches included.

>
> Thank you for reposting!

No worries! Thanks for reviewing.

Alistair

>
>> accepted upstream to help others who are trying to build Xen in
>> the future.
>>
>> V2:
>>  - Remove the #include path changes
>>     - It turns out this only applies to musl (although it shouldn't
>>       break glibc though). Instead of renaming the #include files I
>>       have just removed the -Werror flags.
>>  - Allow people building Xen to pass in EXTRA_CFLAGS.
>>
>> Alistair Francis (5):
>>   Remove hardcoded strict -Werror checking
>>   config/StdGNU.mk: Allows users to pass in EXTRA_CFLAGS
>>   tools/blktap2/vhd: Remove unused struct stat stats
>>   tools/blktap2: Fix missing header file
>>   tools/blktap2/drivers: Remove non-existent sys/sysctl.h include
>>
>>  Config.mk                              | 2 +-
>>  config/StdGNU.mk                       | 3 +++
>>  tools/blktap2/drivers/Makefile         | 1 -
>>  tools/blktap2/drivers/block-remus.c    | 1 -
>>  tools/blktap2/include/atomicio.h       | 2 ++
>>  tools/blktap2/vhd/lib/libvhd-journal.c | 1 -
>>  tools/libxl/Makefile                   | 2 +-
>>  tools/xentrace/Makefile                | 2 --
>>  8 files changed, 7 insertions(+), 7 deletions(-)
>>
>> --
>> 2.7.4
>>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 0/5] General Build Fixes

[Wei Liu]

On Tue, Dec 20, 2016 at 11:46:55AM -0800, Alistair Francis wrote:
>   tools/blktap2/vhd: Remove unused struct stat stats
>   tools/blktap2: Fix missing header file

I've pushed these two to staging.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel