[PATCH v2 for-4.7 00/14] Fixes for compiling with clang

[PATCH v2 for-4.7 00/14] Fixes for compiling with clang

[Roger Pau Monne]

Hello,

This is a set of bug fixes for compiling both the hypervisor and the 
toolstack with clang. I've only tested it with clang 3.8.0 from base 
FreeBSD, so I'm not sure if it's going to work with _all_ clang versions, 
but should be better than nothing.

AFAICT, most of the issues that clang found on the toolstack are actual 
bugs, so it's worth trying to fix them before the release.

Patches 1-4 are for the hypervisor, while patches 5-14 are for the 
toolstack.

Thanks, Roger.


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

[PATCH v2 for-4.7 01/14] build: make HOSTCC conditional on the value of clang

[Roger Pau Monne]

Previously HOSTCC was always hardcoded to gcc

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Tim Deegan <tim@xen.org>
---
Changes since v1:
 - Use ?= instead of =
---
 Config.mk | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Config.mk b/Config.mk
index a0e6d4e..fd8371a 100644
--- a/Config.mk
+++ b/Config.mk
@@ -36,7 +36,6 @@ CONFIG_$(XEN_OS) := y
 SHELL     ?= /bin/sh
 
 # Tools to run on system hosting the build
-HOSTCC      = gcc
 HOSTCFLAGS  = -Wall -Werror -Wstrict-prototypes -O2 -fomit-frame-pointer
 HOSTCFLAGS += -fno-strict-aliasing
 
@@ -50,8 +49,10 @@ DESTDIR     ?= /
 clang ?= n
 ifeq ($(clang),n)
 gcc := y
+HOSTCC ?= gcc
 else
 gcc := n
+HOSTCC ?= clang
 endif
 
 
-- 
2.6.4 (Apple Git-63)


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

Re: [PATCH v2 for-4.7 01/14] build: make HOSTCC conditional on the value of clang

[Andrew Cooper]

On 26/04/16 15:52, Roger Pau Monne wrote:
> Previously HOSTCC was always hardcoded to gcc
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Re: [PATCH v2 for-4.7 01/14] build: make HOSTCC conditional on the value of clang

[Doug Goldstein]

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

On 4/26/16 9:52 AM, Roger Pau Monne wrote:
> Previously HOSTCC was always hardcoded to gcc
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

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: 126 bytes --]

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

[PATCH v2 for-4.7 02/14] build: set HOSTCXX based on clang value for Kconfig xconfig target

[Roger Pau Monne]

The xconfig Kconfig target requires a C++ compiler because it uses Qt.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Tim Deegan <tim@xen.org>
---
 Config.mk | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Config.mk b/Config.mk
index fd8371a..4a5ebed 100644
--- a/Config.mk
+++ b/Config.mk
@@ -50,9 +50,11 @@ clang ?= n
 ifeq ($(clang),n)
 gcc := y
 HOSTCC ?= gcc
+HOSTCXX ?= g++
 else
 gcc := n
 HOSTCC ?= clang
+HOSTCXX ?= clang++
 endif
 
 
-- 
2.6.4 (Apple Git-63)


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

Re: [PATCH v2 for-4.7 02/14] build: set HOSTCXX based on clang value for Kconfig xconfig target

[Andrew Cooper]

On 26/04/16 15:52, Roger Pau Monne wrote:
> The xconfig Kconfig target requires a C++ compiler because it uses Qt.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Re: [PATCH v2 for-4.7 02/14] build: set HOSTCXX based on clang value for Kconfig xconfig target

[Doug Goldstein]

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

On 4/26/16 9:52 AM, Roger Pau Monne wrote:
> The xconfig Kconfig target requires a C++ compiler because it uses Qt.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

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: 126 bytes --]

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

[PATCH v2 for-4.7 03/14] build: pass HOST{CC/CXX} value down to Kconfig

[Roger Pau Monne]

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Tim Deegan <tim@xen.org>
---
 xen/Makefile | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/xen/Makefile b/xen/Makefile
index c908544..b483823 100644
--- a/xen/Makefile
+++ b/xen/Makefile
@@ -242,14 +242,14 @@ kconfig := silentoldconfig oldconfig config menuconfig defconfig \
 	randconfig
 .PHONY: $(kconfig)
 $(kconfig):
-	$(MAKE) -f $(BASEDIR)/tools/kconfig/Makefile.kconfig ARCH=$(ARCH) SRCARCH=$(SRCARCH) $@
+	$(MAKE) -f $(BASEDIR)/tools/kconfig/Makefile.kconfig ARCH=$(ARCH) SRCARCH=$(SRCARCH) HOSTCC=$(HOSTCC) HOSTCXX=$(HOSTCXX) $@
 
 include/config/%.conf: include/config/auto.conf.cmd $(KCONFIG_CONFIG)
-	$(MAKE) -f $(BASEDIR)/tools/kconfig/Makefile.kconfig ARCH=$(ARCH) SRCARCH=$(SRCARCH) silentoldconfig
+	$(MAKE) -f $(BASEDIR)/tools/kconfig/Makefile.kconfig ARCH=$(ARCH) SRCARCH=$(SRCARCH) HOSTCC=$(HOSTCC) HOSTCXX=$(HOSTCXX) 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) defconfig
+	$(MAKE) -f $(BASEDIR)/tools/kconfig/Makefile.kconfig ARCH=$(ARCH) SRCARCH=$(SRCARCH) HOSTCC=$(HOSTCC) HOSTCXX=$(HOSTCXX) defconfig
 
 # Break the dependency chain for the first run
 include/config/auto.conf.cmd: ;
-- 
2.6.4 (Apple Git-63)


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

Re: [PATCH v2 for-4.7 03/14] build: pass HOST{CC/CXX} value down to Kconfig

[Andrew Cooper]

On 26/04/16 15:52, Roger Pau Monne wrote:
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Re: [PATCH v2 for-4.7 03/14] build: pass HOST{CC/CXX} value down to Kconfig

[Doug Goldstein]

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

On 4/26/16 9:52 AM, Roger Pau Monne wrote:
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

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: 126 bytes --]

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

[PATCH v2 for-4.7 04/14] build: remove Kconfig forced gcc selection

[Roger Pau Monne]

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Doug Goldstein <cardoe@cardoe.com>
---
 xen/tools/kconfig/Makefile.kconfig | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/tools/kconfig/Makefile.kconfig b/xen/tools/kconfig/Makefile.kconfig
index 815f306..dbd8912 100644
--- a/xen/tools/kconfig/Makefile.kconfig
+++ b/xen/tools/kconfig/Makefile.kconfig
@@ -36,8 +36,8 @@ KBUILD_DEFCONFIG := $(ARCH)_defconfig
 CONFIG_SHELL := $(SHELL)
 
 # provide the host compiler
-HOSTCC := gcc
-HOSTCXX := g++
+HOSTCC ?= gcc
+HOSTCXX ?= g++
 
 # force target
 PHONY += FORCE
-- 
2.6.4 (Apple Git-63)


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

Re: [PATCH v2 for-4.7 04/14] build: remove Kconfig forced gcc selection

[Doug Goldstein]

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

On 4/26/16 9:52 AM, Roger Pau Monne wrote:
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Cc: Doug Goldstein <cardoe@cardoe.com>
> ---
>  xen/tools/kconfig/Makefile.kconfig | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/tools/kconfig/Makefile.kconfig b/xen/tools/kconfig/Makefile.kconfig
> index 815f306..dbd8912 100644
> --- a/xen/tools/kconfig/Makefile.kconfig
> +++ b/xen/tools/kconfig/Makefile.kconfig
> @@ -36,8 +36,8 @@ KBUILD_DEFCONFIG := $(ARCH)_defconfig
>  CONFIG_SHELL := $(SHELL)
>  
>  # provide the host compiler
> -HOSTCC := gcc
> -HOSTCXX := g++
> +HOSTCC ?= gcc
> +HOSTCXX ?= g++
>  
>  # force target
>  PHONY += FORCE
> 

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

Regardless of the other patches in this series, this one should be
merged in.

-- 
Doug Goldstein


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

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

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

Re: [PATCH v2 for-4.7 04/14] build: remove Kconfig forced gcc selection

[Andrew Cooper]

On 26/04/16 15:52, Roger Pau Monne wrote:
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

[PATCH v2 for-4.7 05/14] tools/headers: prevent adding two __align8__ to uint64_t in ARM headers

[Roger Pau Monne]

Due to the fact that on ARM headers types are substituted to uint64_t and
then uint64_t is also substituted to contain the aligment, this would lead
to some types containing two __align8__ directives. Fix this by first
expanding Xen specific types to uint64_t only, and then replacing all the
uint64_t types to __align8__ uint64_t. This relies on the fact that all
Xen-specific types will have longer names, so they will always be replaced
first.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 tools/include/xen-foreign/mkheader.py | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/include/xen-foreign/mkheader.py b/tools/include/xen-foreign/mkheader.py
index 0504cb8..0e42e14 100644
--- a/tools/include/xen-foreign/mkheader.py
+++ b/tools/include/xen-foreign/mkheader.py
@@ -20,8 +20,8 @@ footer = {};
 inttypes["arm32"] = {
     "unsigned long" : "__danger_unsigned_long_on_arm32",
     "long"          : "__danger_long_on_arm32",
-    "xen_pfn_t"     : "__align8__ uint64_t",
-    "xen_ulong_t"   : "__align8__ uint64_t",
+    "xen_pfn_t"     : "uint64_t",
+    "xen_ulong_t"   : "uint64_t",
     "uint64_t"      : "__align8__ uint64_t",
 };
 header["arm32"] = """
@@ -41,8 +41,8 @@ footer["arm32"] = """
 inttypes["arm64"] = {
     "unsigned long" : "__danger_unsigned_long_on_arm64",
     "long"          : "__danger_long_on_arm64",
-    "xen_pfn_t"     : "__align8__ uint64_t",
-    "xen_ulong_t"   : "__align8__ uint64_t",
+    "xen_pfn_t"     : "uint64_t",
+    "xen_ulong_t"   : "uint64_t",
     "uint64_t"      : "__align8__ uint64_t",
 };
 header["arm64"] = """
-- 
2.6.4 (Apple Git-63)


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

Re: [PATCH v2 for-4.7 05/14] tools/headers: prevent adding two __align8__ to uint64_t in ARM headers

[Andrew Cooper]

On 26/04/16 15:52, Roger Pau Monne wrote:
> Due to the fact that on ARM headers types are substituted to uint64_t and
> then uint64_t is also substituted to contain the aligment, this would lead
> to some types containing two __align8__ directives. Fix this by first
> expanding Xen specific types to uint64_t only, and then replacing all the
> uint64_t types to __align8__ uint64_t. This relies on the fact that all
> Xen-specific types will have longer names, so they will always be replaced
> first.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Re: [PATCH v2 for-4.7 05/14] tools/headers: prevent adding two __align8__ to uint64_t in ARM headers

[Doug Goldstein]

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

On 4/26/16 9:52 AM, Roger Pau Monne wrote:
> Due to the fact that on ARM headers types are substituted to uint64_t and
> then uint64_t is also substituted to contain the aligment, this would lead
> to some types containing two __align8__ directives. Fix this by first
> expanding Xen specific types to uint64_t only, and then replacing all the
> uint64_t types to __align8__ uint64_t. This relies on the fact that all
> Xen-specific types will have longer names, so they will always be replaced
> first.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

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: 126 bytes --]

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

Re: [PATCH v2 for-4.7 05/14] tools/headers: prevent adding two __align8__ to uint64_t in ARM headers

[Wei Liu]

On Tue, Apr 26, 2016 at 04:52:14PM +0200, Roger Pau Monne wrote:
> Due to the fact that on ARM headers types are substituted to uint64_t and
> then uint64_t is also substituted to contain the aligment, this would lead
> to some types containing two __align8__ directives. Fix this by first
> expanding Xen specific types to uint64_t only, and then replacing all the
> uint64_t types to __align8__ uint64_t. This relies on the fact that all
> Xen-specific types will have longer names, so they will always be replaced
> first.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

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

[PATCH v2 for-4.7 06/14] xen/tools: fix substitution of __align8__ uint64_t inside of headers

[Roger Pau Monne]

The current seedery doesn't work with BSD sed, so remove the try to match
int64_t also (since there's none at the moment). Also, apply the same
treatment to all arch headers, currently this is only done to x86_64
headers.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 tools/include/xen-foreign/Makefile | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/tools/include/xen-foreign/Makefile b/tools/include/xen-foreign/Makefile
index 270a975..e395011 100644
--- a/tools/include/xen-foreign/Makefile
+++ b/tools/include/xen-foreign/Makefile
@@ -25,18 +25,30 @@ check-headers: checker
 	rm tmp.size
 
 arm32.h: mkheader.py structs.py $(ROOT)/arch-arm.h $(ROOT)/xen.h
-	$(PYTHON) $< $* $@ $(filter %.h,$^)
+	$(PYTHON) $< $* $@.tmp $(filter %.h,$^)
+	#Avoid mixing an alignment directive with a uint64_t cast or sizeof expression
+	sed 's/(__align8__ \(uint64_t\))/(\1)/g' < $@.tmp > $@.tmp2
+	rm $@.tmp
+	$(call move-if-changed,$@.tmp2,$@)
 
 arm64.h: mkheader.py structs.py $(ROOT)/arch-arm.h $(ROOT)/xen.h
-	$(PYTHON) $< $* $@ $(filter %.h,$^)
+	$(PYTHON) $< $* $@.tmp $(filter %.h,$^)
+	#Avoid mixing an alignment directive with a uint64_t cast or sizeof expression
+	sed 's/(__align8__ \(uint64_t\))/(\1)/g' < $@.tmp > $@.tmp2
+	rm $@.tmp
+	$(call move-if-changed,$@.tmp2,$@)
 
 x86_32.h: mkheader.py structs.py $(ROOT)/arch-x86/xen-x86_32.h $(ROOT)/arch-x86/xen.h $(ROOT)/xen.h
-	$(PYTHON) $< $* $@ $(filter %.h,$^)
+	$(PYTHON) $< $* $@.tmp $(filter %.h,$^)
+	#Avoid mixing an alignment directive with a uint64_t cast or sizeof expression
+	sed 's/(__align8__ \(uint64_t\))/(\1)/g' < $@.tmp > $@.tmp2
+	rm $@.tmp
+	$(call move-if-changed,$@.tmp2,$@)
 
 x86_64.h: mkheader.py structs.py $(ROOT)/arch-x86/xen-x86_64.h $(ROOT)/arch-x86/xen.h $(ROOT)/xen.h
 	$(PYTHON) $< $* $@.tmp $(filter %.h,$^)
 	#Avoid mixing an alignment directive with a uint64_t cast or sizeof expression
-	sed 's/(__align8__ \(u\?int64_t\))/(\1)/g' < $@.tmp > $@.tmp2
+	sed 's/(__align8__ \(uint64_t\))/(\1)/g' < $@.tmp > $@.tmp2
 	rm $@.tmp
 	$(call move-if-changed,$@.tmp2,$@)
 
-- 
2.6.4 (Apple Git-63)


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

Re: [PATCH v2 for-4.7 06/14] xen/tools: fix substitution of __align8__ uint64_t inside of headers

[Andrew Cooper]

On 26/04/16 15:52, Roger Pau Monne wrote:
> The current seedery doesn't work with BSD sed, so remove the try to match
> int64_t also (since there's none at the moment). Also, apply the same
> treatment to all arch headers, currently this is only done to x86_64
> headers.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

That is likely a side effect of me only getting the x86 bit of
tools+clang working.

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Re: [PATCH v2 for-4.7 06/14] xen/tools: fix substitution of __align8__ uint64_t inside of headers

[Doug Goldstein]

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

On 4/26/16 9:52 AM, Roger Pau Monne wrote:
> The current seedery doesn't work with BSD sed, so remove the try to match
> int64_t also (since there's none at the moment). Also, apply the same
> treatment to all arch headers, currently this is only done to x86_64
> headers.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

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: 126 bytes --]

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

Re: [PATCH v2 for-4.7 06/14] xen/tools: fix substitution of __align8__ uint64_t inside of headers

[Wei Liu]

On Tue, Apr 26, 2016 at 04:52:15PM +0200, Roger Pau Monne wrote:
> The current seedery doesn't work with BSD sed, so remove the try to match
> int64_t also (since there's none at the moment). Also, apply the same
> treatment to all arch headers, currently this is only done to x86_64
> headers.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

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

[PATCH v2 for-4.7 07/14] libxc: fix uninitialized variable

[Roger Pau Monne]

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxc/xc_dom_bzimageloader.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/libxc/xc_dom_bzimageloader.c b/tools/libxc/xc_dom_bzimageloader.c
index 7fde42a..0a4041c 100644
--- a/tools/libxc/xc_dom_bzimageloader.c
+++ b/tools/libxc/xc_dom_bzimageloader.c
@@ -482,7 +482,7 @@ static int xc_try_lzo1x_decode(
         if ( !dst_len )
         {
             msg = "Error registering stream output";
-            if ( xc_dom_register_external(dom, out_buf, out_len) )
+            if ( xc_dom_register_external(dom, out_buf, 0) )
                 break;
 
             return 0;
-- 
2.6.4 (Apple Git-63)


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

Re: [PATCH v2 for-4.7 07/14] libxc: fix uninitialized variable

[Wei Liu]

On Tue, Apr 26, 2016 at 04:52:16PM +0200, Roger Pau Monne wrote:
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> ---
>  tools/libxc/xc_dom_bzimageloader.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/libxc/xc_dom_bzimageloader.c b/tools/libxc/xc_dom_bzimageloader.c
> index 7fde42a..0a4041c 100644
> --- a/tools/libxc/xc_dom_bzimageloader.c
> +++ b/tools/libxc/xc_dom_bzimageloader.c
> @@ -482,7 +482,7 @@ static int xc_try_lzo1x_decode(
>          if ( !dst_len )
>          {
>              msg = "Error registering stream output";
> -            if ( xc_dom_register_external(dom, out_buf, out_len) )
> +            if ( xc_dom_register_external(dom, out_buf, 0) )

I fail to figure out why this is correct.

I think the out_len should be moved out of the loop and initialise as 0.
We still need to use out_len here.

Wei.

>                  break;
>  
>              return 0;
> -- 
> 2.6.4 (Apple Git-63)
> 

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

Re: [PATCH v2 for-4.7 07/14] libxc: fix uninitialized variable

[Roger Pau Monne]

On Tue, Apr 26, 2016 at 04:16:51PM +0100, Wei Liu wrote:
> On Tue, Apr 26, 2016 at 04:52:16PM +0200, Roger Pau Monne wrote:
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> > Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> > Cc: Wei Liu <wei.liu2@citrix.com>
> > ---
> >  tools/libxc/xc_dom_bzimageloader.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/tools/libxc/xc_dom_bzimageloader.c b/tools/libxc/xc_dom_bzimageloader.c
> > index 7fde42a..0a4041c 100644
> > --- a/tools/libxc/xc_dom_bzimageloader.c
> > +++ b/tools/libxc/xc_dom_bzimageloader.c
> > @@ -482,7 +482,7 @@ static int xc_try_lzo1x_decode(
> >          if ( !dst_len )
> >          {
> >              msg = "Error registering stream output";
> > -            if ( xc_dom_register_external(dom, out_buf, out_len) )
> > +            if ( xc_dom_register_external(dom, out_buf, 0) )
> 
> I fail to figure out why this is correct.
> 
> I think the out_len should be moved out of the loop and initialise as 0.
> We still need to use out_len here.

I'm not following here. AFAICT out_len is always uninitialized at this 
point (from loop 0 to N), so I assume 0 is what was intended to be passed 
here. Or it this supposed to be passing in the last out_len value from the 
previous iteration of the loop?

Roger.

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

Re: [PATCH v2 for-4.7 07/14] libxc: fix uninitialized variable

[Andrew Cooper]

On 27/04/16 09:57, Roger Pau Monne wrote:
> On Tue, Apr 26, 2016 at 04:16:51PM +0100, Wei Liu wrote:
>> On Tue, Apr 26, 2016 at 04:52:16PM +0200, Roger Pau Monne wrote:
>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>> ---
>>> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
>>> Cc: Wei Liu <wei.liu2@citrix.com>
>>> ---
>>>  tools/libxc/xc_dom_bzimageloader.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/tools/libxc/xc_dom_bzimageloader.c b/tools/libxc/xc_dom_bzimageloader.c
>>> index 7fde42a..0a4041c 100644
>>> --- a/tools/libxc/xc_dom_bzimageloader.c
>>> +++ b/tools/libxc/xc_dom_bzimageloader.c
>>> @@ -482,7 +482,7 @@ static int xc_try_lzo1x_decode(
>>>          if ( !dst_len )
>>>          {
>>>              msg = "Error registering stream output";
>>> -            if ( xc_dom_register_external(dom, out_buf, out_len) )
>>> +            if ( xc_dom_register_external(dom, out_buf, 0) )
>> I fail to figure out why this is correct.
>>
>> I think the out_len should be moved out of the loop and initialise as 0.
>> We still need to use out_len here.
> I'm not following here. AFAICT out_len is always uninitialized at this 
> point (from loop 0 to N), so I assume 0 is what was intended to be passed 
> here. Or it this supposed to be passing in the last out_len value from the 
> previous iteration of the loop?

xc_dom_register_external() is completely mad, and it is not obvious as
to its purpose; tt is doing some kind of allocation tracking for
malloc()-allocated blocks.   It looks like the output length is only
uses for stats purposes, as the memory in out_buf will be freed properly
by free().

So I think the correct value should be the actual size of the allocation
of out_buf, but I also don't think it makes any real difference either way.

~Andrew

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

Re: [PATCH v2 for-4.7 07/14] libxc: fix uninitialized variable

[Wei Liu]

On Wed, Apr 27, 2016 at 10:57:17AM +0200, Roger Pau Monne wrote:
> On Tue, Apr 26, 2016 at 04:16:51PM +0100, Wei Liu wrote:
> > On Tue, Apr 26, 2016 at 04:52:16PM +0200, Roger Pau Monne wrote:
> > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > > ---
> > > Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> > > Cc: Wei Liu <wei.liu2@citrix.com>
> > > ---
> > >  tools/libxc/xc_dom_bzimageloader.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/tools/libxc/xc_dom_bzimageloader.c b/tools/libxc/xc_dom_bzimageloader.c
> > > index 7fde42a..0a4041c 100644
> > > --- a/tools/libxc/xc_dom_bzimageloader.c
> > > +++ b/tools/libxc/xc_dom_bzimageloader.c
> > > @@ -482,7 +482,7 @@ static int xc_try_lzo1x_decode(
> > >          if ( !dst_len )
> > >          {
> > >              msg = "Error registering stream output";
> > > -            if ( xc_dom_register_external(dom, out_buf, out_len) )
> > > +            if ( xc_dom_register_external(dom, out_buf, 0) )
> > 
> > I fail to figure out why this is correct.
> > 
> > I think the out_len should be moved out of the loop and initialise as 0.
> > We still need to use out_len here.
> 
> I'm not following here. AFAICT out_len is always uninitialized at this 
> point (from loop 0 to N), so I assume 0 is what was intended to be passed 
> here. Or it this supposed to be passing in the last out_len value from the 
> previous iteration of the loop?
> 

Note that out_buf is defined out of the scope of this loop, which points
to the buffer returns to caller when decompression completes (note the
return 0 in that block).  The second argument to
xc_dom_register_external should reflect the true size of the buffer.

I got confused by the name of out_len because it is actually used for
something else. So please ignore what I said in my earlier email about
using out_len. From reading the code I think we need to use *size or
something. Does this make sense?

Wei.

> Roger.

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

[PATCH v2 for-4.7 08/14] libxl: fix shutdown_reason type in list_domains

[Roger Pau Monne]

It should be an enum, not an unsigned.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/xl_cmdimpl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 6346017..8ff54e1 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -4254,7 +4254,7 @@ static void list_domains(bool verbose, bool context, bool claim, bool numa,
     printf("\n");
     for (i = 0; i < nb_domain; i++) {
         char *domname;
-        unsigned shutdown_reason;
+        libxl_shutdown_reason shutdown_reason;
         domname = libxl_domid_to_name(ctx, info[i].domid);
         shutdown_reason = info[i].shutdown ? info[i].shutdown_reason : 0;
         printf("%-40s %5d %5lu %5d     %c%c%c%c%c%c  %8.1f",
-- 
2.6.4 (Apple Git-63)


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

Re: [PATCH v2 for-4.7 08/14] libxl: fix shutdown_reason type in list_domains

[Wei Liu]

On Tue, Apr 26, 2016 at 04:52:17PM +0200, Roger Pau Monne wrote:
> It should be an enum, not an unsigned.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

> ---
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> ---
>  tools/libxl/xl_cmdimpl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 6346017..8ff54e1 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -4254,7 +4254,7 @@ static void list_domains(bool verbose, bool context, bool claim, bool numa,
>      printf("\n");
>      for (i = 0; i < nb_domain; i++) {
>          char *domname;
> -        unsigned shutdown_reason;
> +        libxl_shutdown_reason shutdown_reason;
>          domname = libxl_domid_to_name(ctx, info[i].domid);
>          shutdown_reason = info[i].shutdown ? info[i].shutdown_reason : 0;
>          printf("%-40s %5d %5lu %5d     %c%c%c%c%c%c  %8.1f",
> -- 
> 2.6.4 (Apple Git-63)
> 

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

Re: [PATCH v2 for-4.7 08/14] libxl: fix shutdown_reason type in list_domains

[Doug Goldstein]

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

On 4/26/16 9:52 AM, Roger Pau Monne wrote:
> It should be an enum, not an unsigned.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

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

I had to drop the shutdown_reason >= 0 check a few lines below in my
version of this patch. But maybe its not necessary after all.

-- 
Doug Goldstein


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

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

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

[PATCH v2 for-4.7 09/14] xl: fix usage of libxl_get_scheduler

[Roger Pau Monne]

It returns an int, not a libxl_scheduler.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/xl_cmdimpl.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 8ff54e1..2ebca0a 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -5894,16 +5894,18 @@ static void output_xeninfo(void)
 {
     const libxl_version_info *info;
     libxl_scheduler sched;
+    int sched_rc;
 
     if (!(info = libxl_get_version_info(ctx))) {
         fprintf(stderr, "libxl_get_version_info failed.\n");
         return;
     }
 
-    if ((sched = libxl_get_scheduler(ctx)) < 0) {
+    if ((sched_rc = libxl_get_scheduler(ctx)) < 0) {
         fprintf(stderr, "get_scheduler sysctl failed.\n");
         return;
     }
+    sched = sched_rc;
 
     printf("xen_major              : %d\n", info->xen_version_major);
     printf("xen_minor              : %d\n", info->xen_version_minor);
@@ -7991,7 +7993,7 @@ int main_cpupoolcreate(int argc, char **argv)
     libxl_scheduler sched = 0;
     XLU_ConfigList *cpus;
     XLU_ConfigList *nodes;
-    int n_cpus, n_nodes, i, n;
+    int n_cpus, n_nodes, i, n, sched_rc;
     libxl_bitmap freemap;
     libxl_bitmap cpumap;
     libxl_uuid uuid;
@@ -8084,10 +8086,12 @@ int main_cpupoolcreate(int argc, char **argv)
             goto out_cfg;
         }
     } else {
-        if ((sched = libxl_get_scheduler(ctx)) < 0) {
+        if ((sched_rc = libxl_get_scheduler(ctx)) < 0) {
             fprintf(stderr, "get_scheduler sysctl failed.\n");
+            rc = EXIT_FAILURE;
             goto out_cfg;
         }
+        sched = sched_rc;
     }
 
     if (libxl_get_freecpus(ctx, &freemap)) {
-- 
2.6.4 (Apple Git-63)


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

Re: [PATCH v2 for-4.7 09/14] xl: fix usage of libxl_get_scheduler

[Doug Goldstein]

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

On 4/26/16 9:52 AM, Roger Pau Monne wrote:
> It returns an int, not a libxl_scheduler.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

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: 126 bytes --]

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

Re: [PATCH v2 for-4.7 09/14] xl: fix usage of libxl_get_scheduler

[Wei Liu]

On Tue, Apr 26, 2016 at 04:52:18PM +0200, Roger Pau Monne wrote:
> It returns an int, not a libxl_scheduler.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>

At the risk of nitpicking too much, I think we should enforce more
consistent style, especially in xl which is already quite messy at the
moment.

> ---
>  tools/libxl/xl_cmdimpl.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 8ff54e1..2ebca0a 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -5894,16 +5894,18 @@ static void output_xeninfo(void)
>  {
>      const libxl_version_info *info;
>      libxl_scheduler sched;
> +    int sched_rc;
>  

CODING_STYLE says that variable holds return value of libxl API should
be rc, so sched_rc should be rc.

>      if (!(info = libxl_get_version_info(ctx))) {
>          fprintf(stderr, "libxl_get_version_info failed.\n");
>          return;
>      }
>  
> -    if ((sched = libxl_get_scheduler(ctx)) < 0) {
> +    if ((sched_rc = libxl_get_scheduler(ctx)) < 0) {
>          fprintf(stderr, "get_scheduler sysctl failed.\n");
>          return;
>      }
> +    sched = sched_rc;
>  
>      printf("xen_major              : %d\n", info->xen_version_major);
>      printf("xen_minor              : %d\n", info->xen_version_minor);
> @@ -7991,7 +7993,7 @@ int main_cpupoolcreate(int argc, char **argv)
>      libxl_scheduler sched = 0;
>      XLU_ConfigList *cpus;
>      XLU_ConfigList *nodes;
> -    int n_cpus, n_nodes, i, n;
> +    int n_cpus, n_nodes, i, n, sched_rc;

Same here.

The code itself is fine.

Let me know if changing sched_rc to rc would cause a lot of troubles...

Wei.

>      libxl_bitmap freemap;
>      libxl_bitmap cpumap;
>      libxl_uuid uuid;
> @@ -8084,10 +8086,12 @@ int main_cpupoolcreate(int argc, char **argv)
>              goto out_cfg;
>          }
>      } else {
> -        if ((sched = libxl_get_scheduler(ctx)) < 0) {
> +        if ((sched_rc = libxl_get_scheduler(ctx)) < 0) {
>              fprintf(stderr, "get_scheduler sysctl failed.\n");
> +            rc = EXIT_FAILURE;
>              goto out_cfg;
>          }
> +        sched = sched_rc;
>      }
>  
>      if (libxl_get_freecpus(ctx, &freemap)) {
> -- 
> 2.6.4 (Apple Git-63)
> 

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

[PATCH v2 for-4.7 10/14] libxl: add the printf-like attributes to a couple of functions

[Roger Pau Monne]

Or else clang complains with:

error: format string is not a string literal

This looks quite pedantic, but I don't know of any other way to get rid of
those errors.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/libxl_internal.c    | 16 ----------------
 tools/libxl/libxl_internal.h    | 11 ++++++-----
 tools/libxl/libxl_save_helper.c |  1 +
 3 files changed, 7 insertions(+), 21 deletions(-)

diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index e7b765b..3b30f8a 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -554,22 +554,6 @@ void libxl__update_domain_configuration(libxl__gc *gc,
     dst->b_info.video_memkb = src->b_info.video_memkb;
 }
 
-char *libxl__device_model_xs_path(libxl__gc *gc, uint32_t dm_domid,
-                                  uint32_t domid, const char *format,  ...)
-{
-    char *s, *fmt;
-    va_list ap;
-
-    fmt = GCSPRINTF("/local/domain/%u/device-model/%u%s", dm_domid,
-                    domid, format);
-
-    va_start(ap, format);
-    s = libxl__vsprintf(gc, fmt, ap);
-    va_end(ap);
-
-    return s;
-}
-
 /*
  * Local variables:
  * mode: C
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 8e2cf3e..0879e4c 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -636,7 +636,7 @@ _hidden void *libxl__realloc(libxl__gc *gc_opt, void *ptr, size_t new_size) NN1;
 /* print @fmt into an allocated string large enoughto contain the result.
  * (similar to gc'd asprintf(3)). */
 _hidden char *libxl__sprintf(libxl__gc *gc_opt, const char *fmt, ...) PRINTF_ATTRIBUTE(2, 3) NN1;
-_hidden char *libxl__vsprintf(libxl__gc *gc, const char *format, va_list ap);
+_hidden char *libxl__vsprintf(libxl__gc *gc, const char *format, va_list ap) PRINTF_ATTRIBUTE(2, 0);
 /* duplicate the string @c (similar to a gc'd strdup(3)). */
 _hidden char *libxl__strdup(libxl__gc *gc_opt,
                             const char *c /* may be NULL */) NN1;
@@ -709,7 +709,7 @@ _hidden char *libxl__xs_libxl_path(libxl__gc *gc, uint32_t domid);
  */
 
 int libxl__xs_vprintf(libxl__gc *gc, xs_transaction_t t,
-                      const char *path, const char *fmt, va_list ap);
+                      const char *path, const char *fmt, va_list ap) PRINTF_ATTRIBUTE(4, 0);
 int libxl__xs_printf(libxl__gc *gc, xs_transaction_t t,
                      const char *path, const char *fmt, ...) PRINTF_ATTRIBUTE(4, 5);
 
@@ -1995,9 +1995,10 @@ _hidden libxl__json_object *libxl__json_parse(libxl__gc *gc_opt, const char *s);
 _hidden int libxl__device_model_version_running(libxl__gc *gc, uint32_t domid);
   /* Return the system-wide default device model */
 _hidden libxl_device_model_version libxl__default_device_model(libxl__gc *gc);
-_hidden char *libxl__device_model_xs_path(libxl__gc *gc, uint32_t dm_domid,
-                                          uint32_t domid,
-                                          const char *format, ...) PRINTF_ATTRIBUTE(4, 5);
+
+#define libxl__device_model_xs_path(gc, dm_domid, domid, fmt, _a...)       \
+    libxl__sprintf(gc, "/local/domain/%u/device-model/%u" fmt, dm_domid,   \
+                   domid, ##_a)
 
 /*
  * Calling context and GC for event-generating functions:
diff --git a/tools/libxl/libxl_save_helper.c b/tools/libxl/libxl_save_helper.c
index 5fe642a..d3def6b 100644
--- a/tools/libxl/libxl_save_helper.c
+++ b/tools/libxl/libxl_save_helper.c
@@ -52,6 +52,7 @@
 
 /*----- logger -----*/
 
+__attribute__((format(printf, 5, 0)))
 static void tellparent_vmessage(xentoollog_logger *logger_in,
                                 xentoollog_level level,
                                 int errnoval,
-- 
2.6.4 (Apple Git-63)


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

Re: [PATCH v2 for-4.7 10/14] libxl: add the printf-like attributes to a couple of functions

[Wei Liu]

On Tue, Apr 26, 2016 at 04:52:19PM +0200, Roger Pau Monne wrote:
[...]
> @@ -1995,9 +1995,10 @@ _hidden libxl__json_object *libxl__json_parse(libxl__gc *gc_opt, const char *s);
>  _hidden int libxl__device_model_version_running(libxl__gc *gc, uint32_t domid);
>    /* Return the system-wide default device model */
>  _hidden libxl_device_model_version libxl__default_device_model(libxl__gc *gc);
> -_hidden char *libxl__device_model_xs_path(libxl__gc *gc, uint32_t dm_domid,
> -                                          uint32_t domid,
> -                                          const char *format, ...) PRINTF_ATTRIBUTE(4, 5);

Why does this not work with clang?

Wei.

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

Re: [PATCH v2 for-4.7 10/14] libxl: add the printf-like attributes to a couple of functions

[Andrew Cooper]

On 26/04/16 16:29, Wei Liu wrote:
> On Tue, Apr 26, 2016 at 04:52:19PM +0200, Roger Pau Monne wrote:
> [...]
>> @@ -1995,9 +1995,10 @@ _hidden libxl__json_object *libxl__json_parse(libxl__gc *gc_opt, const char *s);
>>  _hidden int libxl__device_model_version_running(libxl__gc *gc, uint32_t domid);
>>    /* Return the system-wide default device model */
>>  _hidden libxl_device_model_version libxl__default_device_model(libxl__gc *gc);
>> -_hidden char *libxl__device_model_xs_path(libxl__gc *gc, uint32_t dm_domid,
>> -                                          uint32_t domid,
>> -                                          const char *format, ...) PRINTF_ATTRIBUTE(4, 5);
> Why does this not work with clang?

It is a security consideration.

Passing anything other than a string literal to a printf-style function
is opening a can of worms if an untrusted entity can influence the
content of the string.

I guess clang is better at spotting parameters passed like this than GCC.

~Andrew

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

Re: [PATCH v2 for-4.7 10/14] libxl: add the printf-like attributes to a couple of functions

[Wei Liu]

On Tue, Apr 26, 2016 at 04:30:36PM +0100, Andrew Cooper wrote:
> On 26/04/16 16:29, Wei Liu wrote:
> > On Tue, Apr 26, 2016 at 04:52:19PM +0200, Roger Pau Monne wrote:
> > [...]
> >> @@ -1995,9 +1995,10 @@ _hidden libxl__json_object *libxl__json_parse(libxl__gc *gc_opt, const char *s);
> >>  _hidden int libxl__device_model_version_running(libxl__gc *gc, uint32_t domid);
> >>    /* Return the system-wide default device model */
> >>  _hidden libxl_device_model_version libxl__default_device_model(libxl__gc *gc);
> >> -_hidden char *libxl__device_model_xs_path(libxl__gc *gc, uint32_t dm_domid,
> >> -                                          uint32_t domid,
> >> -                                          const char *format, ...) PRINTF_ATTRIBUTE(4, 5);
> > Why does this not work with clang?
> 
> It is a security consideration.
> 
> Passing anything other than a string literal to a printf-style function
> is opening a can of worms if an untrusted entity can influence the
> content of the string.
> 

I see. I didn't look closely into the function body.

> I guess clang is better at spotting parameters passed like this than GCC.
> 

Sigh. I can't say I like turning that into a macro though. On the other
hand there doesn't seem to be an elegant way of solving that.

Roger, please at least make it look like a macro. Say, name it
DEVICE_MODEL_XS_PATH or something.

Wei.

> ~Andrew

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

Re: [PATCH v2 for-4.7 10/14] libxl: add the printf-like attributes to a couple of functions

[Ian Jackson]

Wei Liu writes ("Re: [Xen-devel] [PATCH v2 for-4.7 10/14] libxl: add the printf-like attributes to a couple of functions"):
> Sigh. I can't say I like turning that into a macro though. On the other
> hand there doesn't seem to be an elegant way of solving that.

Well, other than suppressing the warning somehow.

> Roger, please at least make it look like a macro. Say, name it
> DEVICE_MODEL_XS_PATH or something.

I'm not sure I agree.  I think if we can make it a function-like
macro, then it is fine to give it a function-like name.  (Note that
many libc functions might be macros; getc is traditionally a #define.)

Obviously it can't be function-like in that you must always pass it a
literal, but violations of that constraint will be detected by the
compiler.

Ian.

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

Re: [PATCH v2 for-4.7 10/14] libxl: add the printf-like attributes to a couple of functions

[Wei Liu]

On Thu, Apr 28, 2016 at 06:26:09PM +0100, Ian Jackson wrote:
> Wei Liu writes ("Re: [Xen-devel] [PATCH v2 for-4.7 10/14] libxl: add the printf-like attributes to a couple of functions"):
> > Sigh. I can't say I like turning that into a macro though. On the other
> > hand there doesn't seem to be an elegant way of solving that.
> 
> Well, other than suppressing the warning somehow.
> 

I don't think we want to suppress the warning. It's useful.

> > Roger, please at least make it look like a macro. Say, name it
> > DEVICE_MODEL_XS_PATH or something.
> 
> I'm not sure I agree.  I think if we can make it a function-like
> macro, then it is fine to give it a function-like name.  (Note that
> many libc functions might be macros; getc is traditionally a #define.)
> 
> Obviously it can't be function-like in that you must always pass it a
> literal, but violations of that constraint will be detected by the
> compiler.

I've pushed the series from Roger to staging because it fixes clang
compilation. I don't have strong opinion on what the name of macro
should look like so feel free to send a patch to change that.

Wei.

> 
> Ian.

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

[PATCH v2 for-4.7 11/14] libxl: add explicit casts from yajl_gen_status to yajl_status

[Roger Pau Monne]

Or else clang complains with:

implicit conversion from enumeration type 'yajl_gen_status' to different
enumeration type 'yajl_status' [-Werror,-Wenum-conversion]

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/libxl_json.c | 29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/tools/libxl/libxl_json.c b/tools/libxl/libxl_json.c
index 3b695dd..6bb0695 100644
--- a/tools/libxl/libxl_json.c
+++ b/tools/libxl/libxl_json.c
@@ -617,42 +617,48 @@ yajl_status libxl__json_object_to_yajl_gen(libxl__gc *gc,
     int idx = 0;
     yajl_status rc;
 
+#define CONVERT_YAJL_GEN_TO_STATUS(gen) \
+    ((gen) == yajl_gen_status_ok ? yajl_status_ok : yajl_status_error);
+
     switch (obj->type) {
     case JSON_NULL:
-        return yajl_gen_null(hand);
+        return CONVERT_YAJL_GEN_TO_STATUS(yajl_gen_null(hand));
     case JSON_BOOL:
-        return yajl_gen_bool(hand, obj->u.b);
+        return CONVERT_YAJL_GEN_TO_STATUS(yajl_gen_bool(hand, obj->u.b));
     case JSON_INTEGER:
-        return yajl_gen_integer(hand, obj->u.i);
+        return CONVERT_YAJL_GEN_TO_STATUS(yajl_gen_integer(hand, obj->u.i));
     case JSON_DOUBLE:
-        return yajl_gen_double(hand, obj->u.d);
+        return CONVERT_YAJL_GEN_TO_STATUS(yajl_gen_double(hand, obj->u.d));
     case JSON_NUMBER:
-        return yajl_gen_number(hand, obj->u.string, strlen(obj->u.string));
+        return CONVERT_YAJL_GEN_TO_STATUS(
+                  yajl_gen_number(hand, obj->u.string, strlen(obj->u.string)));
     case JSON_STRING:
-        return libxl__yajl_gen_asciiz(hand, obj->u.string);
+        return CONVERT_YAJL_GEN_TO_STATUS(
+                        libxl__yajl_gen_asciiz(hand, obj->u.string));
     case JSON_MAP: {
         libxl__json_map_node *node = NULL;
 
-        rc = yajl_gen_map_open(hand);
+        rc = CONVERT_YAJL_GEN_TO_STATUS(yajl_gen_map_open(hand));
         if (rc != yajl_status_ok)
             return rc;
         for (idx = 0; idx < obj->u.map->count; idx++) {
             if (flexarray_get(obj->u.map, idx, (void**)&node) != 0)
                 break;
 
-            rc = libxl__yajl_gen_asciiz(hand, node->map_key);
+            rc = CONVERT_YAJL_GEN_TO_STATUS(
+                            libxl__yajl_gen_asciiz(hand, node->map_key));
             if (rc != yajl_status_ok)
                 return rc;
             rc = libxl__json_object_to_yajl_gen(gc, hand, node->obj);
             if (rc != yajl_status_ok)
                 return rc;
         }
-        return yajl_gen_map_close(hand);
+        return CONVERT_YAJL_GEN_TO_STATUS(yajl_gen_map_close(hand));
     }
     case JSON_ARRAY: {
         libxl__json_object *node = NULL;
 
-        rc = yajl_gen_array_open(hand);
+        rc = CONVERT_YAJL_GEN_TO_STATUS(yajl_gen_array_open(hand));
         if (rc != yajl_status_ok)
             return rc;
         for (idx = 0; idx < obj->u.array->count; idx++) {
@@ -662,13 +668,14 @@ yajl_status libxl__json_object_to_yajl_gen(libxl__gc *gc,
             if (rc != yajl_status_ok)
                 return rc;
         }
-        return yajl_gen_array_close(hand);
+        return CONVERT_YAJL_GEN_TO_STATUS(yajl_gen_array_close(hand));
     }
     case JSON_ANY:
         /* JSON_ANY is not a valid value for obj->type. */
         ;
     }
     abort();
+#undef CONVERT_YAJL_GEN_TO_STATUS
 }
 
 
-- 
2.6.4 (Apple Git-63)


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

Re: [PATCH v2 for-4.7 11/14] libxl: add explicit casts from yajl_gen_status to yajl_status

[Wei Liu]

On Tue, Apr 26, 2016 at 04:52:20PM +0200, Roger Pau Monne wrote:
> Or else clang complains with:
> 
> implicit conversion from enumeration type 'yajl_gen_status' to different
> enumeration type 'yajl_status' [-Werror,-Wenum-conversion]
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

> ---
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> ---
>  tools/libxl/libxl_json.c | 29 ++++++++++++++++++-----------
>  1 file changed, 18 insertions(+), 11 deletions(-)
> 
> diff --git a/tools/libxl/libxl_json.c b/tools/libxl/libxl_json.c
> index 3b695dd..6bb0695 100644
> --- a/tools/libxl/libxl_json.c
> +++ b/tools/libxl/libxl_json.c
> @@ -617,42 +617,48 @@ yajl_status libxl__json_object_to_yajl_gen(libxl__gc *gc,
>      int idx = 0;
>      yajl_status rc;
>  
> +#define CONVERT_YAJL_GEN_TO_STATUS(gen) \
> +    ((gen) == yajl_gen_status_ok ? yajl_status_ok : yajl_status_error);
> +

Extraneous ";" at the end.

I can fix that up while committing.

Wei.

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

[PATCH v2 for-4.7 12/14] libxl: fix passing the type argument to xc_psr_*

[Roger Pau Monne]

The xc_psr_* functions expect the type to be xc_psr_cat_type instead of
libxl_psr_cbm_type, so do the conversion.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/libxl_psr.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/libxl_psr.c b/tools/libxl/libxl_psr.c
index 3d0dc61..40f2d5f 100644
--- a/tools/libxl/libxl_psr.c
+++ b/tools/libxl/libxl_psr.c
@@ -298,6 +298,7 @@ int libxl_psr_cat_set_cbm(libxl_ctx *ctx, uint32_t domid,
                           uint64_t cbm)
 {
     GC_INIT(ctx);
+    BUILD_BUG_ON(sizeof(libxl_psr_cbm_type) != sizeof(xc_psr_cat_type));
     int rc;
     int socketid, nr_sockets;
 
@@ -310,7 +311,8 @@ int libxl_psr_cat_set_cbm(libxl_ctx *ctx, uint32_t domid,
     libxl_for_each_set_bit(socketid, *target_map) {
         if (socketid >= nr_sockets)
             break;
-        if (xc_psr_cat_set_domain_data(ctx->xch, domid, type, socketid, cbm)) {
+        if (xc_psr_cat_set_domain_data(ctx->xch, domid, (xc_psr_cat_type)type,
+                                       socketid, cbm)) {
             libxl__psr_cat_log_err_msg(gc, errno);
             rc = ERROR_FAIL;
         }
@@ -328,7 +330,8 @@ int libxl_psr_cat_get_cbm(libxl_ctx *ctx, uint32_t domid,
     GC_INIT(ctx);
     int rc = 0;
 
-    if (xc_psr_cat_get_domain_data(ctx->xch, domid, type, target, cbm_r)) {
+    if (xc_psr_cat_get_domain_data(ctx->xch, domid, (xc_psr_cat_type)type,
+                                   target, cbm_r)) {
         libxl__psr_cat_log_err_msg(gc, errno);
         rc = ERROR_FAIL;
     }
-- 
2.6.4 (Apple Git-63)


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

Re: [PATCH v2 for-4.7 12/14] libxl: fix passing the type argument to xc_psr_*

[Wei Liu]

On Tue, Apr 26, 2016 at 04:52:21PM +0200, Roger Pau Monne wrote:
> The xc_psr_* functions expect the type to be xc_psr_cat_type instead of
> libxl_psr_cbm_type, so do the conversion.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

> ---
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> ---
>  tools/libxl/libxl_psr.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/libxl/libxl_psr.c b/tools/libxl/libxl_psr.c
> index 3d0dc61..40f2d5f 100644
> --- a/tools/libxl/libxl_psr.c
> +++ b/tools/libxl/libxl_psr.c
> @@ -298,6 +298,7 @@ int libxl_psr_cat_set_cbm(libxl_ctx *ctx, uint32_t domid,
>                            uint64_t cbm)
>  {
>      GC_INIT(ctx);
> +    BUILD_BUG_ON(sizeof(libxl_psr_cbm_type) != sizeof(xc_psr_cat_type));
>      int rc;
>      int socketid, nr_sockets;
>  
> @@ -310,7 +311,8 @@ int libxl_psr_cat_set_cbm(libxl_ctx *ctx, uint32_t domid,
>      libxl_for_each_set_bit(socketid, *target_map) {
>          if (socketid >= nr_sockets)
>              break;
> -        if (xc_psr_cat_set_domain_data(ctx->xch, domid, type, socketid, cbm)) {
> +        if (xc_psr_cat_set_domain_data(ctx->xch, domid, (xc_psr_cat_type)type,
> +                                       socketid, cbm)) {
>              libxl__psr_cat_log_err_msg(gc, errno);
>              rc = ERROR_FAIL;
>          }
> @@ -328,7 +330,8 @@ int libxl_psr_cat_get_cbm(libxl_ctx *ctx, uint32_t domid,
>      GC_INIT(ctx);
>      int rc = 0;
>  
> -    if (xc_psr_cat_get_domain_data(ctx->xch, domid, type, target, cbm_r)) {
> +    if (xc_psr_cat_get_domain_data(ctx->xch, domid, (xc_psr_cat_type)type,
> +                                   target, cbm_r)) {
>          libxl__psr_cat_log_err_msg(gc, errno);
>          rc = ERROR_FAIL;
>      }
> -- 
> 2.6.4 (Apple Git-63)
> 

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

Re: [PATCH v2 for-4.7 12/14] libxl: fix passing the type argument to xc_psr_*

[Ian Jackson]

Wei Liu writes ("Re: [PATCH v2 for-4.7 12/14] libxl: fix passing the type argument to xc_psr_*"):
> On Tue, Apr 26, 2016 at 04:52:21PM +0200, Roger Pau Monne wrote:
> > The xc_psr_* functions expect the type to be xc_psr_cat_type instead of
> > libxl_psr_cbm_type, so do the conversion.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Acked-by: Wei Liu <wei.liu2@citrix.com>

Nacked-by: Ian Jackson <ian.jackson@eu.citrix.com>

> > -        if (xc_psr_cat_set_domain_data(ctx->xch, domid, type, socketid,
 cbm)) {
> > +        if (xc_psr_cat_set_domain_data(ctx->xch, domid, (xc_psr_cat_typ
e)type,
> > +                                       socketid, cbm)) {

I'm very much against introducing casts which are not absolutely
necessary.  Casts are a big hammer which can suppress important
warnings (such as conversions between integers and pointers).

This anomaly with the same enum defined in two places with two names
is pretty poor.  But if we are to perpetuate it, as perhaps we must,
then rather than casting at each conversion point, we should introduce
an inline function which contains the cast.  That way each call site
remains more typesafe.

Thanks,
Ian.

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

Re: [PATCH v2 for-4.7 12/14] libxl: fix passing the type argument to xc_psr_*

[Wei Liu]

On Thu, Apr 28, 2016 at 06:29:03PM +0100, Ian Jackson wrote:
> Wei Liu writes ("Re: [PATCH v2 for-4.7 12/14] libxl: fix passing the type argument to xc_psr_*"):
> > On Tue, Apr 26, 2016 at 04:52:21PM +0200, Roger Pau Monne wrote:
> > > The xc_psr_* functions expect the type to be xc_psr_cat_type instead of
> > > libxl_psr_cbm_type, so do the conversion.
> > > 
> > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > 
> > Acked-by: Wei Liu <wei.liu2@citrix.com>
> 
> Nacked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> 
> > > -        if (xc_psr_cat_set_domain_data(ctx->xch, domid, type, socketid,
>  cbm)) {
> > > +        if (xc_psr_cat_set_domain_data(ctx->xch, domid, (xc_psr_cat_typ
> e)type,
> > > +                                       socketid, cbm)) {
> 
> I'm very much against introducing casts which are not absolutely
> necessary.  Casts are a big hammer which can suppress important
> warnings (such as conversions between integers and pointers).
> 
> This anomaly with the same enum defined in two places with two names
> is pretty poor.  But if we are to perpetuate it, as perhaps we must,
> then rather than casting at each conversion point, we should introduce
> an inline function which contains the cast.  That way each call site
> remains more typesafe.
> 

The two enums aren't going away any time soon.

Does the following diff meet your requirement?

---8<---
diff --git a/tools/libxl/libxl_psr.c b/tools/libxl/libxl_psr.c
index 40f2d5f..7a34c04 100644
--- a/tools/libxl/libxl_psr.c
+++ b/tools/libxl/libxl_psr.c
@@ -293,12 +293,18 @@ out:
     return rc;
 }
 
+static inline xc_psr_cat_type libxl_psr_cbm_type_to_libxc_psr_cat_type(
+    libxl_psr_cbm_type type)
+{
+    BUILD_BUG_ON(sizeof(libxl_psr_cbm_type) != sizeof(xc_psr_cat_type));
+    return (xc_psr_cat_type)type;
+}
+
 int libxl_psr_cat_set_cbm(libxl_ctx *ctx, uint32_t domid,
                           libxl_psr_cbm_type type, libxl_bitmap *target_map,
                           uint64_t cbm)
 {
     GC_INIT(ctx);
-    BUILD_BUG_ON(sizeof(libxl_psr_cbm_type) != sizeof(xc_psr_cat_type));
     int rc;
     int socketid, nr_sockets;
 
@@ -309,9 +315,13 @@ int libxl_psr_cat_set_cbm(libxl_ctx *ctx, uint32_t domid,
     }
 
     libxl_for_each_set_bit(socketid, *target_map) {
+        xc_psr_cat_type xc_type;
+
         if (socketid >= nr_sockets)
             break;
-        if (xc_psr_cat_set_domain_data(ctx->xch, domid, (xc_psr_cat_type)type,
+
+        xc_type = libxl_psr_cbm_type_to_libxc_psr_cat_type(type);
+        if (xc_psr_cat_set_domain_data(ctx->xch, domid, xc_type,
                                        socketid, cbm)) {
             libxl__psr_cat_log_err_msg(gc, errno);
             rc = ERROR_FAIL;
@@ -329,8 +339,9 @@ int libxl_psr_cat_get_cbm(libxl_ctx *ctx, uint32_t domid,
 {
     GC_INIT(ctx);
     int rc = 0;
+    xc_psr_cat_type xc_type = libxl_psr_cbm_type_to_libxc_psr_cat_type(type);
 
-    if (xc_psr_cat_get_domain_data(ctx->xch, domid, (xc_psr_cat_type)type,
+    if (xc_psr_cat_get_domain_data(ctx->xch, domid, xc_type,
                                    target, cbm_r)) {
         libxl__psr_cat_log_err_msg(gc, errno);
         rc = ERROR_FAIL;

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

Re: [PATCH v2 for-4.7 12/14] libxl: fix passing the type argument to xc_psr_*

[Roger Pau Monne]

On Thu, Apr 28, 2016 at 09:49:11PM +0100, Wei Liu wrote:
> On Thu, Apr 28, 2016 at 06:29:03PM +0100, Ian Jackson wrote:
> > Wei Liu writes ("Re: [PATCH v2 for-4.7 12/14] libxl: fix passing the type argument to xc_psr_*"):
> > > On Tue, Apr 26, 2016 at 04:52:21PM +0200, Roger Pau Monne wrote:
> > > > The xc_psr_* functions expect the type to be xc_psr_cat_type instead of
> > > > libxl_psr_cbm_type, so do the conversion.
> > > > 
> > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > > 
> > > Acked-by: Wei Liu <wei.liu2@citrix.com>
> > 
> > Nacked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> > 
> > > > -        if (xc_psr_cat_set_domain_data(ctx->xch, domid, type, socketid,
> >  cbm)) {
> > > > +        if (xc_psr_cat_set_domain_data(ctx->xch, domid, (xc_psr_cat_typ
> > e)type,
> > > > +                                       socketid, cbm)) {
> > 
> > I'm very much against introducing casts which are not absolutely
> > necessary.  Casts are a big hammer which can suppress important
> > warnings (such as conversions between integers and pointers).
> > 
> > This anomaly with the same enum defined in two places with two names
> > is pretty poor.  But if we are to perpetuate it, as perhaps we must,
> > then rather than casting at each conversion point, we should introduce
> > an inline function which contains the cast.  That way each call site
> > remains more typesafe.
> > 
> 
> The two enums aren't going away any time soon.
> 
> Does the following diff meet your requirement?

Hello,

Thanks for doing this.
 
> ---8<---
> diff --git a/tools/libxl/libxl_psr.c b/tools/libxl/libxl_psr.c
> index 40f2d5f..7a34c04 100644
> --- a/tools/libxl/libxl_psr.c
> +++ b/tools/libxl/libxl_psr.c
> @@ -293,12 +293,18 @@ out:
>      return rc;
>  }
>  
> +static inline xc_psr_cat_type libxl_psr_cbm_type_to_libxc_psr_cat_type(
                                      ^ extra _ needed.
> +    libxl_psr_cbm_type type)
> +{
> +    BUILD_BUG_ON(sizeof(libxl_psr_cbm_type) != sizeof(xc_psr_cat_type));
> +    return (xc_psr_cat_type)type;

In order to prevent using a cast, we could use a union:

union {
    libxl_psr_cbm_type libxl_psr;
    xc_psr_cat_type xc_psr;
} u;

u.libxl_psr = type;
return u.xc_psr;

> +}
> +
>  int libxl_psr_cat_set_cbm(libxl_ctx *ctx, uint32_t domid,
>                            libxl_psr_cbm_type type, libxl_bitmap *target_map,
>                            uint64_t cbm)
>  {
>      GC_INIT(ctx);
> -    BUILD_BUG_ON(sizeof(libxl_psr_cbm_type) != sizeof(xc_psr_cat_type));
>      int rc;
>      int socketid, nr_sockets;
>  
> @@ -309,9 +315,13 @@ int libxl_psr_cat_set_cbm(libxl_ctx *ctx, uint32_t domid,
>      }
>  
>      libxl_for_each_set_bit(socketid, *target_map) {
> +        xc_psr_cat_type xc_type;
> +
>          if (socketid >= nr_sockets)
>              break;
> -        if (xc_psr_cat_set_domain_data(ctx->xch, domid, (xc_psr_cat_type)type,
> +
> +        xc_type = libxl_psr_cbm_type_to_libxc_psr_cat_type(type);
> +        if (xc_psr_cat_set_domain_data(ctx->xch, domid, xc_type,
>                                         socketid, cbm)) {
>              libxl__psr_cat_log_err_msg(gc, errno);
>              rc = ERROR_FAIL;
> @@ -329,8 +339,9 @@ int libxl_psr_cat_get_cbm(libxl_ctx *ctx, uint32_t domid,
>  {
>      GC_INIT(ctx);
>      int rc = 0;
> +    xc_psr_cat_type xc_type = libxl_psr_cbm_type_to_libxc_psr_cat_type(type);
>  
> -    if (xc_psr_cat_get_domain_data(ctx->xch, domid, (xc_psr_cat_type)type,
> +    if (xc_psr_cat_get_domain_data(ctx->xch, domid, xc_type,
>                                     target, cbm_r)) {
>          libxl__psr_cat_log_err_msg(gc, errno);
>          rc = ERROR_FAIL;

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

Re: [PATCH v2 for-4.7 12/14] libxl: fix passing the type argument to xc_psr_* [and 1 more messages]

[Ian Jackson]

Wei Liu writes ("Re: [PATCH v2 for-4.7 12/14] libxl: fix passing the type argument to xc_psr_*"):
> On Thu, Apr 28, 2016 at 06:29:03PM +0100, Ian Jackson wrote:
> > I'm very much against introducing casts which are not absolutely
> > necessary.  Casts are a big hammer which can suppress important
> > warnings (such as conversions between integers and pointers).
> > 
> > This anomaly with the same enum defined in two places with two names
> > is pretty poor.  But if we are to perpetuate it, as perhaps we must,
> > then rather than casting at each conversion point, we should introduce
> > an inline function which contains the cast.  That way each call site
> > remains more typesafe.
> 
> The two enums aren't going away any time soon.

Sadly, I think you're right.

> Does the following diff meet your requirement?

Yes, that is exactly the kind of thing I was thinking of.  It makes
the cast non-routine by putting it in a dedicated function whose
authors/readers know to check it's right.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

(with a suitable commit message)

Roger Pau Monne writes ("Re: [PATCH v2 for-4.7 12/14] libxl: fix passing the type argument to xc_psr_*"):
> On Thu, Apr 28, 2016 at 09:49:11PM +0100, Wei Liu wrote:
> > +    BUILD_BUG_ON(sizeof(libxl_psr_cbm_type) != sizeof(xc_psr_cat_type));
> > +    return (xc_psr_cat_type)type;
> 
> In order to prevent using a cast, we could use a union:
> 
> union {
>     libxl_psr_cbm_type libxl_psr;
>     xc_psr_cat_type xc_psr;
> } u;
> 
> u.libxl_psr = type;
> return u.xc_psr;

No, not really.

Firstly, although we compile with -fno-strict-aliasing, this is a bad
example in these days of hostile optimisers: without
-fno-strict-aliasing, the compiler is allowed to assume that the loads
and stores are to different variables, even though the contrary is
evident.

Secondly, it's clumsy.

Thirdly, this kind of union aliasing is normally used for
representation (structure) punning.


Thanks,
Ian.

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

Re: [PATCH v2 for-4.7 12/14] libxl: fix passing the type argument to xc_psr_* [and 1 more messages]

[Wei Liu]

On Wed, May 18, 2016 at 03:45:22PM +0100, Ian Jackson wrote:
> Wei Liu writes ("Re: [PATCH v2 for-4.7 12/14] libxl: fix passing the type argument to xc_psr_*"):
> > On Thu, Apr 28, 2016 at 06:29:03PM +0100, Ian Jackson wrote:
> > > I'm very much against introducing casts which are not absolutely
> > > necessary.  Casts are a big hammer which can suppress important
> > > warnings (such as conversions between integers and pointers).
> > > 
> > > This anomaly with the same enum defined in two places with two names
> > > is pretty poor.  But if we are to perpetuate it, as perhaps we must,
> > > then rather than casting at each conversion point, we should introduce
> > > an inline function which contains the cast.  That way each call site
> > > remains more typesafe.
> > 
> > The two enums aren't going away any time soon.
> 
> Sadly, I think you're right.
> 
> > Does the following diff meet your requirement?
> 
> Yes, that is exactly the kind of thing I was thinking of.  It makes
> the cast non-routine by putting it in a dedicated function whose
> authors/readers know to check it's right.
> 
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> 
> (with a suitable commit message)

Thanks. I will submit a proper patch with your ack.

Wei.

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

[PATCH v2 for-4.7 13/14] oxenstored: fix error when shifting negative value

[Roger Pau Monne]

By explicitly casting it to unsigned.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 tools/ocaml/xenstored/systemd_stubs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/ocaml/xenstored/systemd_stubs.c b/tools/ocaml/xenstored/systemd_stubs.c
index 1bd5dea..a78a72b 100644
--- a/tools/ocaml/xenstored/systemd_stubs.c
+++ b/tools/ocaml/xenstored/systemd_stubs.c
@@ -124,7 +124,7 @@ CAMLprim value ocaml_sd_listen_fds(value connect_to)
 	CAMLparam1(connect_to);
 	CAMLlocal1(sock_ret);
 
-	sock_ret = Val_int(-1);
+	sock_ret = Val_int(-1U);
 
 	CAMLreturn(sock_ret);
 }
@@ -144,7 +144,7 @@ CAMLprim value ocaml_sd_notify_ready(value ignore)
 	CAMLparam1(ignore);
 	CAMLlocal1(ret);
 
-	ret = Val_int(-1);
+	ret = Val_int(-1U);
 
 	CAMLreturn(ret);
 }
-- 
2.6.4 (Apple Git-63)


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

Re: [PATCH v2 for-4.7 13/14] oxenstored: fix error when shifting negative value

[Wei Liu]

The title is a bit too cryptic to me. Where do that shift happen?

Wei.

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

Re: [PATCH v2 for-4.7 13/14] oxenstored: fix error when shifting negative value

[Andrew Cooper]

On 26/04/16 16:35, Wei Liu wrote:
> The title is a bit too cryptic to me. Where do that shift happen?

Ocaml stores integers shifted left by one, and with the bottom bit set.

Values with the bottom bit clear are pointers into the GC'd heap. 
Values with the bottom bit set are integers, and need to be shifted by 1
bit to have calculations performed.

Underlying patch Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Re: [PATCH v2 for-4.7 13/14] oxenstored: fix error when shifting negative value

[Wei Liu]

On Tue, Apr 26, 2016 at 04:37:49PM +0100, Andrew Cooper wrote:
> On 26/04/16 16:35, Wei Liu wrote:
> > The title is a bit too cryptic to me. Where do that shift happen?
> 
> Ocaml stores integers shifted left by one, and with the bottom bit set.
> 
> Values with the bottom bit clear are pointers into the GC'd heap. 
> Values with the bottom bit set are integers, and need to be shifted by 1
> bit to have calculations performed.
> 

This is better.

Roger, can you add the above paragraphs to commit message? Thanks.

Wei.

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

Re: [PATCH v2 for-4.7 13/14] oxenstored: fix error when shifting negative value

[Andrew Cooper]

On 26/04/16 16:43, Wei Liu wrote:
> On Tue, Apr 26, 2016 at 04:37:49PM +0100, Andrew Cooper wrote:
>> On 26/04/16 16:35, Wei Liu wrote:
>>> The title is a bit too cryptic to me. Where do that shift happen?
>> Ocaml stores integers shifted left by one, and with the bottom bit set.
>>
>> Values with the bottom bit clear are pointers into the GC'd heap. 
>> Values with the bottom bit set are integers, and need to be shifted by 1
>> bit to have calculations performed.
>>
> This is better.
>
> Roger, can you add the above paragraphs to commit message? Thanks.

P.S. this is why Ocaml integers are 31 or 63 bits wide, and cause all
kinds of "fun" issues when interfacing with C which makes use of all
bits available in an integer.

~Andrew

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

[PATCH v2 for-4.7 14/14] tools/python: corrently use LDFLAGS and CFLAGS

[Roger Pau Monne]

It is incorrect to add the LDFLAGS to the CFLAGS, and some compilers will
error out if linker flags are passed when creating object files. Fix this by
properly passing CFLAGS and LDFLAGS, instead of putting everything in
CFLAGS.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 tools/python/Makefile | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/tools/python/Makefile b/tools/python/Makefile
index 2363537..da08f46 100644
--- a/tools/python/Makefile
+++ b/tools/python/Makefile
@@ -4,7 +4,8 @@ include $(XEN_ROOT)/tools/Rules.mk
 .PHONY: all
 all: build
 
-PY_CFLAGS = $(CFLAGS) $(PY_NOOPT_CFLAGS) $(LDFLAGS) $(APPEND_LDFLAGS)
+PY_CFLAGS = $(CFLAGS) $(PY_NOOPT_CFLAGS)
+PY_LDFLAGS = $(LDFLAGS) $(APPEND_LDFLAGS)
 
 .PHONY: build
 build:
@@ -14,8 +15,9 @@ build:
 install:
 	$(INSTALL_DIR) $(DESTDIR)$(LIBEXEC_BIN)
 
-	CC="$(CC)" CFLAGS="$(PY_CFLAGS)" $(PYTHON) setup.py install \
-		$(PYTHON_PREFIX_ARG) --root="$(DESTDIR)" --force
+	CC="$(CC)" CFLAGS="$(PY_CFLAGS)" LDFLAGS="$(PY_LDFLAGS)" $(PYTHON) \
+		setup.py install $(PYTHON_PREFIX_ARG) --root="$(DESTDIR)"  \
+		--force
 
 	$(INSTALL_PROG) scripts/convert-legacy-stream $(DESTDIR)$(LIBEXEC_BIN)
 	$(INSTALL_PROG) scripts/verify-stream-v2 $(DESTDIR)$(LIBEXEC_BIN)
-- 
2.6.4 (Apple Git-63)


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

Re: [PATCH v2 for-4.7 14/14] tools/python: corrently use LDFLAGS and CFLAGS

[Doug Goldstein]

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

On 4/26/16 9:52 AM, Roger Pau Monne wrote:
> It is incorrect to add the LDFLAGS to the CFLAGS, and some compilers will
> error out if linker flags are passed when creating object files. Fix this by
> properly passing CFLAGS and LDFLAGS, instead of putting everything in
> CFLAGS.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---

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: 126 bytes --]

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

Re: [PATCH v2 for-4.7 14/14] tools/python: corrently use LDFLAGS and CFLAGS

[Wei Liu]

On Tue, Apr 26, 2016 at 04:52:23PM +0200, Roger Pau Monne wrote:
> It is incorrect to add the LDFLAGS to the CFLAGS, and some compilers will
> error out if linker flags are passed when creating object files. Fix this by
> properly passing CFLAGS and LDFLAGS, instead of putting everything in
> CFLAGS.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

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

Re: [PATCH v2 for-4.7 00/14] Fixes for compiling with clang

[Wei Liu]

On Tue, Apr 26, 2016 at 04:52:09PM +0200, Roger Pau Monne wrote:
> Hello,
> 
> This is a set of bug fixes for compiling both the hypervisor and the 
> toolstack with clang. I've only tested it with clang 3.8.0 from base 
> FreeBSD, so I'm not sure if it's going to work with _all_ clang versions, 
> but should be better than nothing.
> 
> AFAICT, most of the issues that clang found on the toolstack are actual 
> bugs, so it's worth trying to fix them before the release.
> 
> Patches 1-4 are for the hypervisor, while patches 5-14 are for the 
> toolstack.
> 
> Thanks, Roger.
> 

I went through the series. Most patches are trivial, so I'm inclined to
take this series in 4.7.

There are a few patches that require updating. You can either resend the
whole series with all tags folded in or just update those patches that
require updating. Do whatever most convenient for you.

Wei.

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

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

Re: [PATCH v2 for-4.7 00/14] Fixes for compiling with clang

[Doug Goldstein]

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

On 4/26/16 11:12 AM, Wei Liu wrote:
> On Tue, Apr 26, 2016 at 04:52:09PM +0200, Roger Pau Monne wrote:
>> Hello,
>>
>> This is a set of bug fixes for compiling both the hypervisor and the 
>> toolstack with clang. I've only tested it with clang 3.8.0 from base 
>> FreeBSD, so I'm not sure if it's going to work with _all_ clang versions, 
>> but should be better than nothing.
>>
>> AFAICT, most of the issues that clang found on the toolstack are actual 
>> bugs, so it's worth trying to fix them before the release.
>>
>> Patches 1-4 are for the hypervisor, while patches 5-14 are for the 
>> toolstack.
>>
>> Thanks, Roger.
>>
> 
> I went through the series. Most patches are trivial, so I'm inclined to
> take this series in 4.7.
> 
> There are a few patches that require updating. You can either resend the
> whole series with all tags folded in or just update those patches that
> require updating. Do whatever most convenient for you.
> 
> Wei.
> 

FWIW, a lot of these are individual patches and don't build on each
other. If some end up being controversial we can push the ones that have
a broad consensus.

-- 
Doug Goldstein


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

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

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

Re: [PATCH v2 for-4.7 00/14] Fixes for compiling with clang

[Wei Liu]

On Tue, Apr 26, 2016 at 12:20:30PM -0500, Doug Goldstein wrote:
> On 4/26/16 11:12 AM, Wei Liu wrote:
> > On Tue, Apr 26, 2016 at 04:52:09PM +0200, Roger Pau Monne wrote:
> >> Hello,
> >>
> >> This is a set of bug fixes for compiling both the hypervisor and the 
> >> toolstack with clang. I've only tested it with clang 3.8.0 from base 
> >> FreeBSD, so I'm not sure if it's going to work with _all_ clang versions, 
> >> but should be better than nothing.
> >>
> >> AFAICT, most of the issues that clang found on the toolstack are actual 
> >> bugs, so it's worth trying to fix them before the release.
> >>
> >> Patches 1-4 are for the hypervisor, while patches 5-14 are for the 
> >> toolstack.
> >>
> >> Thanks, Roger.
> >>
> > 
> > I went through the series. Most patches are trivial, so I'm inclined to
> > take this series in 4.7.
> > 
> > There are a few patches that require updating. You can either resend the
> > whole series with all tags folded in or just update those patches that
> > require updating. Do whatever most convenient for you.
> > 
> > Wei.
> > 
> 
> FWIW, a lot of these are individual patches and don't build on each
> other. If some end up being controversial we can push the ones that have
> a broad consensus.
> 

Right. I will see about pushing some of them today.

Wei.

> -- 
> Doug Goldstein
> 




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


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