[PATCH for-4.7 0/7] More tools build fixes with clang

[PATCH for-4.7 0/7] More tools build fixes with clang

[Andrew Cooper]

With these fixes, tools/ can be built with clang, so enable it in the travis
configurion.

The travis results can be viewed here:
  https://travis-ci.org/andyhhp/xen/builds/126153860

Andrew Cooper (7):
  tools/xenstat: Avoid comparing '0 <= unsigned integer'
  tools/blktap2: Use abort() instead of custom crash
  tools/blktap2: Fix array initialisers for tapdisk_disk_{types,drivers}[]
  tools/blktap2: Fix use of uninitialised variable in _tap_list_join3()
  tools/kdd: Fix uninitialised variable warning
  travis: Remove clang-3.8 build
  travis: Enable tools when building with clang

 .travis.yml                              |  4 ----
 scripts/travis-build                     |  8 ++------
 tools/blktap2/control/tap-ctl-list.c     |  4 ++--
 tools/blktap2/drivers/block-vhd.c        |  2 +-
 tools/blktap2/drivers/tapdisk-disktype.c | 20 ++++++++------------
 tools/debugger/kdd/kdd.c                 |  2 +-
 tools/xenstat/libxenstat/src/xenstat.c   |  8 ++++----
 7 files changed, 18 insertions(+), 30 deletions(-)

-- 
2.1.4


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

[PATCH for-4.7 1/7] tools/xenstat: Avoid comparing '0 <= unsigned integer'

[Andrew Cooper]

Clang points out that this is tautological.

  src/xenstat.c:325:8: warning: comparison of 0 <= unsigned expression is
  always true [-Wtautological-compare]
          if (0 <= index && index < node->num_domains)
              ~ ^  ~~~~~

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 tools/xenstat/libxenstat/src/xenstat.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/xenstat/libxenstat/src/xenstat.c b/tools/xenstat/libxenstat/src/xenstat.c
index 3495f3f..fbe44f3 100644
--- a/tools/xenstat/libxenstat/src/xenstat.c
+++ b/tools/xenstat/libxenstat/src/xenstat.c
@@ -322,7 +322,7 @@ xenstat_domain *xenstat_node_domain(xenstat_node * node, unsigned int domid)
 xenstat_domain *xenstat_node_domain_by_index(xenstat_node * node,
 					     unsigned int index)
 {
-	if (0 <= index && index < node->num_domains)
+	if (index < node->num_domains)
 		return &(node->domains[index]);
 	return NULL;
 }
@@ -389,7 +389,7 @@ unsigned int xenstat_domain_num_vcpus(xenstat_domain * domain)
 
 xenstat_vcpu *xenstat_domain_vcpu(xenstat_domain * domain, unsigned int vcpu)
 {
-	if (0 <= vcpu && vcpu < domain->num_vcpus)
+	if (vcpu < domain->num_vcpus)
 		return &(domain->vcpus[vcpu]);
 	return NULL;
 }
@@ -457,7 +457,7 @@ unsigned int xenstat_domain_num_networks(xenstat_domain * domain)
 xenstat_network *xenstat_domain_network(xenstat_domain * domain,
 					unsigned int network)
 {
-	if (domain->networks && 0 <= network && network < domain->num_networks)
+	if (domain->networks && network < domain->num_networks)
 		return &(domain->networks[network]);
 	return NULL;
 }
@@ -472,7 +472,7 @@ unsigned int xenstat_domain_num_vbds(xenstat_domain * domain)
 xenstat_vbd *xenstat_domain_vbd(xenstat_domain * domain,
 				unsigned int vbd)
 {
-	if (domain->vbds && 0 <= vbd && vbd < domain->num_vbds)
+	if (domain->vbds && vbd < domain->num_vbds)
 		return &(domain->vbds[vbd]);
 	return NULL;
 }
-- 
2.1.4


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

[PATCH for-4.7 2/7] tools/blktap2: Use abort() instead of custom crash

[Andrew Cooper]

Like c/s 4d98d3ebf - there is a second instance.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 tools/blktap2/drivers/block-vhd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/blktap2/drivers/block-vhd.c b/tools/blktap2/drivers/block-vhd.c
index 76ea5bd..f7853f9 100644
--- a/tools/blktap2/drivers/block-vhd.c
+++ b/tools/blktap2/drivers/block-vhd.c
@@ -85,7 +85,7 @@ unsigned int SPB;
 		DBG(TLOG_WARN, "%s:%d: FAILED ASSERTION: '%s'\n",	\
 		    __FILE__, __LINE__, #_p);				\
 		tlog_flush();						\
-		*(int*)0 = 0;						\
+		abort();                                                \
 	}
 
 #if (DEBUGGING == 1)
-- 
2.1.4


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

[PATCH for-4.7 3/7] tools/blktap2: Fix array initialisers for tapdisk_disk_{types, drivers}[]

[Andrew Cooper]

Clang points out:

  tapdisk-disktype.c:117:2: error: initializer overrides prior initialization
  of this subobject [-Werror,-Winitializer-overrides]
          0,
          ^
  tapdisk-disktype.c:115:23: note: previous initialization is here
          [DISK_TYPE_VINDEX]      = &vhd_index_disk,
                                    ^~~~~~~~~~~~~~~

Mixing different initialiser styles should be avoided; The actual behaviour is
different to the expected behaviour.  This specific example has been broken
since its introduction in c/s 7b4dea554 "blktap2: Fix tapdisk disktype issues"
in 2010, and is caused by the '#if 0' block removing &tapdisk_{sync,vmdk}.

First of all, remove what were intended to be trailing NULL entries in
tapdisk_disk_{types,drivers}[], making consistent use of Designated
Initialisers for the initialisation.

This requires changing the loop in tapdisk_disktype_find() to be based on the
number of elements in tapdisk_disk_types[], rather than looking for the first
NULL.  This fixes a latent bug, as the use of Designated Initializers causes
to intermediate zero entries if not all indices are explicitly specified.

There is a second latent bug where tapdisk_disktype_find() assumes that
tapdisk_disk_drivers[] has at least as many entries as tapdisk_disk_types[].
This is not the case and tapdisk_disk_drivers[] had one entry fewer than
tapdisk_disk_types[], but the NULL loop bound prevented an out-of-bounds read
of tapdisk_disk_drivers[].  Fix the issue by explicitly declaring
tapdisk_disk_drivers[] to have the same number of entries as
tapdisk_disk_types[].

Finally, this leads to a linker error.  It turns out that tapdisk_vhd_index
doesn't exist, and I can't find any evidence in the source history to suggest
that it ever did.  I can only presume that it would have been #if 0'd out like
tapdisk_sync and tapdisk_vmdk had it not been for this bug preventing a build
failure.  Drop all three.

No functional change, but only because of the specific layout of
tapdisk_disk_types[].

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>

Please can someone carefully check my "No functional change" assertion?  I am
fairly sure it is correct, but this is a gnarly.
---
 tools/blktap2/drivers/tapdisk-disktype.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/tools/blktap2/drivers/tapdisk-disktype.c b/tools/blktap2/drivers/tapdisk-disktype.c
index e9a6890..e89d364 100644
--- a/tools/blktap2/drivers/tapdisk-disktype.c
+++ b/tools/blktap2/drivers/tapdisk-disktype.c
@@ -33,6 +33,8 @@
 #include "tapdisk-disktype.h"
 #include "tapdisk-message.h"
 
+#define ARRAY_SIZE(a) (sizeof (a) / sizeof *(a))
+
 static const disk_info_t aio_disk = {
        "aio",
        "raw image (aio)",
@@ -112,35 +114,25 @@ const disk_info_t *tapdisk_disk_types[] = {
 	[DISK_TYPE_LOG]	= &log_disk,
 	[DISK_TYPE_VINDEX]	= &vhd_index_disk,
 	[DISK_TYPE_REMUS]	= &remus_disk,
-	0,
 };
 
 extern struct tap_disk tapdisk_aio;
-extern struct tap_disk tapdisk_sync;
-extern struct tap_disk tapdisk_vmdk;
 extern struct tap_disk tapdisk_vhdsync;
 extern struct tap_disk tapdisk_vhd;
 extern struct tap_disk tapdisk_ram;
 extern struct tap_disk tapdisk_qcow;
 extern struct tap_disk tapdisk_block_cache;
-extern struct tap_disk tapdisk_vhd_index;
 extern struct tap_disk tapdisk_log;
 extern struct tap_disk tapdisk_remus;
 
-const struct tap_disk *tapdisk_disk_drivers[] = {
+const struct tap_disk *tapdisk_disk_drivers[ARRAY_SIZE(tapdisk_disk_types)] = {
 	[DISK_TYPE_AIO]         = &tapdisk_aio,
-#if 0
-	[DISK_TYPE_SYNC]        = &tapdisk_sync,
-	[DISK_TYPE_VMDK]        = &tapdisk_vmdk,
-#endif
 	[DISK_TYPE_VHD]         = &tapdisk_vhd,
 	[DISK_TYPE_RAM]         = &tapdisk_ram,
 	[DISK_TYPE_QCOW]        = &tapdisk_qcow,
 	[DISK_TYPE_BLOCK_CACHE] = &tapdisk_block_cache,
-	[DISK_TYPE_VINDEX]      = &tapdisk_vhd_index,
 	[DISK_TYPE_LOG]         = &tapdisk_log,
 	[DISK_TYPE_REMUS]       = &tapdisk_remus,
-	0,
 };
 
 int
@@ -149,7 +141,11 @@ tapdisk_disktype_find(const char *name)
 	const disk_info_t *info;
 	int i;
 
-	for (i = 0; info = tapdisk_disk_types[i], info != NULL; ++i) {
+	for (i = 0; i < ARRAY_SIZE(tapdisk_disk_types); ++i) {
+		info = tapdisk_disk_types[i];
+		if (!info)
+			continue;
+
 		if (strcmp(name, info->name))
 			continue;
 
-- 
2.1.4


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

[PATCH for-4.7 4/7] tools/blktap2: Fix use of uninitialised variable in _tap_list_join3()

[Andrew Cooper]

Clang points out:

  tap-ctl-list.c:457:28: error: variable 'entry' is uninitialized when
  used here [-Werror,-Wuninitialized]
          for (; *_entry != NULL; ++entry) {
                                    ^~~~~

The content of that loop clearly was meant to iterate over _entry rather than
entry, so is fixed to do so.  This presumably fixes a memory leak when
tapdisks get orphed, as only the first item on the list got freed.

There is no use of entry at all.  It is referenced in a
list_for_each_entry(tl, &tap->list, entry) construct, but this is just a
member name, and not a reference to local scope variable of the same name.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 tools/blktap2/control/tap-ctl-list.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/blktap2/control/tap-ctl-list.c b/tools/blktap2/control/tap-ctl-list.c
index c91f6f4..f8d49c3 100644
--- a/tools/blktap2/control/tap-ctl-list.c
+++ b/tools/blktap2/control/tap-ctl-list.c
@@ -400,7 +400,7 @@ int
 _tap_list_join3(int n_minors, int *minorv, int n_taps, struct tapdisk *tapv,
 		tap_list_t ***_list)
 {
-	tap_list_t **list, **_entry, *entry;
+	tap_list_t **list, **_entry;
 	int i, _m, err;
 
 	list = tap_ctl_alloc_list(n_minors + n_taps);
@@ -454,7 +454,7 @@ _tap_list_join3(int n_minors, int *minorv, int n_taps, struct tapdisk *tapv,
 	}
 
 	/* free extraneous list entries */
-	for (; *_entry != NULL; ++entry) {
+	for (; *_entry != NULL; ++_entry) {
 		free_list(*_entry);
 		*_entry = NULL;
 	}
-- 
2.1.4


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

[PATCH for-4.7 5/7] tools/kdd: Fix uninitialised variable warning

[Andrew Cooper]

Clang warns:

  kdd.c:1031:9: error: variable 'fd' is used uninitialized whenever '||'
  condition is true [-Werror,-Wsometimes-uninitialized]
      if (argc != 4
          ^~~~~~~~~
  kdd.c:1040:20: note: uninitialized use occurs here
      if (select(fd + 1, &fds, NULL, NULL, NULL) > 0)
                 ^~

This situation can't actually happen, as usage() is a terminal path.  Annotate
it appropriately.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 tools/debugger/kdd/kdd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/debugger/kdd/kdd.c b/tools/debugger/kdd/kdd.c
index 9a0225b..70f007e 100644
--- a/tools/debugger/kdd/kdd.c
+++ b/tools/debugger/kdd/kdd.c
@@ -998,7 +998,7 @@ void kdd_select_callback(kdd_state *s)
 }
 
 
-static void usage(void)
+static void __attribute__((noreturn)) usage(void)
 {
     fprintf(stderr, 
 " usage: kdd [-v] <domid> <address> <port>\n"
-- 
2.1.4


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

[PATCH for-4.7 6/7] travis: Remove clang-3.8 build

[Andrew Cooper]

The package appears to have been renamed in Ubuntu.  The only reason this test
is currently passing is because the hypervisor build falls back to clang, at
version 3.5

Add an explicit test in the build script that out desired compiler is
available.  Note that travis already performs this step, but in a way which
isn't fatal to the build.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Doug Goldstein <cardoe@cardoe.com>
---
 .travis.yml          | 4 ----
 scripts/travis-build | 2 ++
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/.travis.yml b/.travis.yml
index 0eea94e..d2e1bec 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -22,12 +22,8 @@ matrix:
           env: XEN_TARGET_ARCH=x86_64 debug=y
         - compiler: clang
           env: XEN_TARGET_ARCH=x86_64 clang=y debug=n
-        - compiler: clang-3.8
-          env: XEN_TARGET_ARCH=x86_64 clang=y debug=n
         - compiler: clang
           env: XEN_TARGET_ARCH=x86_64 clang=y debug=y
-        - compiler: clang-3.8
-          env: XEN_TARGET_ARCH=x86_64 clang=y debug=y
         - compiler: gcc
           env: XEN_TARGET_ARCH=arm32 CROSS_COMPILE=arm-linux-gnueabihf- debug=n
         - compiler: gcc
diff --git a/scripts/travis-build b/scripts/travis-build
index b553f20..9a5c1c8 100755
--- a/scripts/travis-build
+++ b/scripts/travis-build
@@ -1,5 +1,7 @@
 #!/bin/bash -ex
 
+$CC --version
+
 # random config or default config
 if [[ "${RANDCONFIG}" == "y" ]]; then
     make -C xen randconfig
-- 
2.1.4


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

[PATCH for-4.7 7/7] travis: Enable tools when building with clang

[Andrew Cooper]

tools now build under clang, so let them be tested.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Doug Goldstein <cardoe@cardoe.com>
---
 scripts/travis-build | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/scripts/travis-build b/scripts/travis-build
index 9a5c1c8..584d008 100755
--- a/scripts/travis-build
+++ b/scripts/travis-build
@@ -22,12 +22,6 @@ else
     cfgargs+=("--disable-tools") # we don't have the cross depends installed
 fi
 
-# Due to multiple build failures and the desire to get more
-# build testing (GCC only) enabled this is disabled
-if [[ "${clang}" == "y" ]]; then
-    cfgargs+=("--disable-tools")
-fi
-
 ./configure "${cfgargs[@]}"
 
 make dist
-- 
2.1.4


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

Re: [PATCH for-4.7 3/7] tools/blktap2: Fix array initialisers for tapdisk_disk_{types, drivers}[]

[Wei Liu]

On Wed, Apr 27, 2016 at 06:01:22PM +0100, Andrew Cooper wrote:
> Clang points out:
> 
>   tapdisk-disktype.c:117:2: error: initializer overrides prior initialization
>   of this subobject [-Werror,-Winitializer-overrides]
>           0,
>           ^
>   tapdisk-disktype.c:115:23: note: previous initialization is here
>           [DISK_TYPE_VINDEX]      = &vhd_index_disk,
>                                     ^~~~~~~~~~~~~~~
> 
> Mixing different initialiser styles should be avoided; The actual behaviour is
> different to the expected behaviour.  This specific example has been broken
> since its introduction in c/s 7b4dea554 "blktap2: Fix tapdisk disktype issues"
> in 2010, and is caused by the '#if 0' block removing &tapdisk_{sync,vmdk}.
> 
> First of all, remove what were intended to be trailing NULL entries in
> tapdisk_disk_{types,drivers}[], making consistent use of Designated
> Initialisers for the initialisation.
> 
> This requires changing the loop in tapdisk_disktype_find() to be based on the
> number of elements in tapdisk_disk_types[], rather than looking for the first
> NULL.  This fixes a latent bug, as the use of Designated Initializers causes
> to intermediate zero entries if not all indices are explicitly specified.
> 
> There is a second latent bug where tapdisk_disktype_find() assumes that
> tapdisk_disk_drivers[] has at least as many entries as tapdisk_disk_types[].
> This is not the case and tapdisk_disk_drivers[] had one entry fewer than
> tapdisk_disk_types[], but the NULL loop bound prevented an out-of-bounds read
> of tapdisk_disk_drivers[].  Fix the issue by explicitly declaring
> tapdisk_disk_drivers[] to have the same number of entries as
> tapdisk_disk_types[].
> 
> Finally, this leads to a linker error.  It turns out that tapdisk_vhd_index
> doesn't exist, and I can't find any evidence in the source history to suggest
> that it ever did.  I can only presume that it would have been #if 0'd out like
> tapdisk_sync and tapdisk_vmdk had it not been for this bug preventing a build
> failure.  Drop all three.
> 
> No functional change, but only because of the specific layout of
> tapdisk_disk_types[].
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@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>
> 
> Please can someone carefully check my "No functional change" assertion?  I am
> fairly sure it is correct, but this is a gnarly.

You assertion should be correct.

> ---
>  tools/blktap2/drivers/tapdisk-disktype.c | 20 ++++++++------------
>  1 file changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/tools/blktap2/drivers/tapdisk-disktype.c b/tools/blktap2/drivers/tapdisk-disktype.c
> index e9a6890..e89d364 100644
> --- a/tools/blktap2/drivers/tapdisk-disktype.c
> +++ b/tools/blktap2/drivers/tapdisk-disktype.c
> @@ -33,6 +33,8 @@
>  #include "tapdisk-disktype.h"
>  #include "tapdisk-message.h"
>  
> +#define ARRAY_SIZE(a) (sizeof (a) / sizeof *(a))
> +
>  static const disk_info_t aio_disk = {
>         "aio",
>         "raw image (aio)",
> @@ -112,35 +114,25 @@ const disk_info_t *tapdisk_disk_types[] = {
>  	[DISK_TYPE_LOG]	= &log_disk,
>  	[DISK_TYPE_VINDEX]	= &vhd_index_disk,
>  	[DISK_TYPE_REMUS]	= &remus_disk,
> -	0,
>  };
>  
>  extern struct tap_disk tapdisk_aio;
> -extern struct tap_disk tapdisk_sync;
> -extern struct tap_disk tapdisk_vmdk;
>  extern struct tap_disk tapdisk_vhdsync;
>  extern struct tap_disk tapdisk_vhd;
>  extern struct tap_disk tapdisk_ram;
>  extern struct tap_disk tapdisk_qcow;
>  extern struct tap_disk tapdisk_block_cache;
> -extern struct tap_disk tapdisk_vhd_index;
>  extern struct tap_disk tapdisk_log;
>  extern struct tap_disk tapdisk_remus;
>  
> -const struct tap_disk *tapdisk_disk_drivers[] = {
> +const struct tap_disk *tapdisk_disk_drivers[ARRAY_SIZE(tapdisk_disk_types)] = {
>  	[DISK_TYPE_AIO]         = &tapdisk_aio,
> -#if 0
> -	[DISK_TYPE_SYNC]        = &tapdisk_sync,
> -	[DISK_TYPE_VMDK]        = &tapdisk_vmdk,
> -#endif
>  	[DISK_TYPE_VHD]         = &tapdisk_vhd,
>  	[DISK_TYPE_RAM]         = &tapdisk_ram,
>  	[DISK_TYPE_QCOW]        = &tapdisk_qcow,
>  	[DISK_TYPE_BLOCK_CACHE] = &tapdisk_block_cache,
> -	[DISK_TYPE_VINDEX]      = &tapdisk_vhd_index,
>  	[DISK_TYPE_LOG]         = &tapdisk_log,
>  	[DISK_TYPE_REMUS]       = &tapdisk_remus,
> -	0,
>  };
>  
>  int
> @@ -149,7 +141,11 @@ tapdisk_disktype_find(const char *name)
>  	const disk_info_t *info;
>  	int i;
>  
> -	for (i = 0; info = tapdisk_disk_types[i], info != NULL; ++i) {
> +	for (i = 0; i < ARRAY_SIZE(tapdisk_disk_types); ++i) {
> +		info = tapdisk_disk_types[i];
> +		if (!info)
> +			continue;
> +
>  		if (strcmp(name, info->name))
>  			continue;
>  
> -- 
> 2.1.4
> 

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

Re: [PATCH for-4.7 1/7] tools/xenstat: Avoid comparing '0 <= unsigned integer'

[Wei Liu]

On Wed, Apr 27, 2016 at 06:01:20PM +0100, Andrew Cooper wrote:
> Clang points out that this is tautological.
> 
>   src/xenstat.c:325:8: warning: comparison of 0 <= unsigned expression is
>   always true [-Wtautological-compare]
>           if (0 <= index && index < node->num_domains)
>               ~ ^  ~~~~~
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@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 for-4.7 2/7] tools/blktap2: Use abort() instead of custom crash

[Wei Liu]

On Wed, Apr 27, 2016 at 06:01:21PM +0100, Andrew Cooper wrote:
> Like c/s 4d98d3ebf - there is a second instance.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@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 for-4.7 4/7] tools/blktap2: Fix use of uninitialised variable in _tap_list_join3()

[Wei Liu]

On Wed, Apr 27, 2016 at 06:01:23PM +0100, Andrew Cooper wrote:
> Clang points out:
> 
>   tap-ctl-list.c:457:28: error: variable 'entry' is uninitialized when
>   used here [-Werror,-Wuninitialized]
>           for (; *_entry != NULL; ++entry) {
>                                     ^~~~~
> 
> The content of that loop clearly was meant to iterate over _entry rather than
> entry, so is fixed to do so.  This presumably fixes a memory leak when
> tapdisks get orphed, as only the first item on the list got freed.
> 
> There is no use of entry at all.  It is referenced in a
> list_for_each_entry(tl, &tap->list, entry) construct, but this is just a
> member name, and not a reference to local scope variable of the same name.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@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/blktap2/control/tap-ctl-list.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/blktap2/control/tap-ctl-list.c b/tools/blktap2/control/tap-ctl-list.c
> index c91f6f4..f8d49c3 100644
> --- a/tools/blktap2/control/tap-ctl-list.c
> +++ b/tools/blktap2/control/tap-ctl-list.c
> @@ -400,7 +400,7 @@ int
>  _tap_list_join3(int n_minors, int *minorv, int n_taps, struct tapdisk *tapv,
>  		tap_list_t ***_list)
>  {
> -	tap_list_t **list, **_entry, *entry;
> +	tap_list_t **list, **_entry;
>  	int i, _m, err;
>  
>  	list = tap_ctl_alloc_list(n_minors + n_taps);
> @@ -454,7 +454,7 @@ _tap_list_join3(int n_minors, int *minorv, int n_taps, struct tapdisk *tapv,
>  	}
>  
>  	/* free extraneous list entries */
> -	for (; *_entry != NULL; ++entry) {
> +	for (; *_entry != NULL; ++_entry) {
>  		free_list(*_entry);
>  		*_entry = NULL;
>  	}
> -- 
> 2.1.4
> 

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

Re: [PATCH for-4.7 5/7] tools/kdd: Fix uninitialised variable warning

[Wei Liu]

On Wed, Apr 27, 2016 at 06:01:24PM +0100, Andrew Cooper wrote:
> Clang warns:
> 
>   kdd.c:1031:9: error: variable 'fd' is used uninitialized whenever '||'
>   condition is true [-Werror,-Wsometimes-uninitialized]
>       if (argc != 4
>           ^~~~~~~~~
>   kdd.c:1040:20: note: uninitialized use occurs here
>       if (select(fd + 1, &fds, NULL, NULL, NULL) > 0)
>                  ^~
> 
> This situation can't actually happen, as usage() is a terminal path.  Annotate
> it appropriately.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@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/debugger/kdd/kdd.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/debugger/kdd/kdd.c b/tools/debugger/kdd/kdd.c
> index 9a0225b..70f007e 100644
> --- a/tools/debugger/kdd/kdd.c
> +++ b/tools/debugger/kdd/kdd.c
> @@ -998,7 +998,7 @@ void kdd_select_callback(kdd_state *s)
>  }
>  
>  
> -static void usage(void)
> +static void __attribute__((noreturn)) usage(void)
>  {
>      fprintf(stderr, 
>  " usage: kdd [-v] <domid> <address> <port>\n"
> -- 
> 2.1.4
> 

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

Re: [PATCH for-4.7 6/7] travis: Remove clang-3.8 build

[Wei Liu]

On Wed, Apr 27, 2016 at 06:01:25PM +0100, Andrew Cooper wrote:
> The package appears to have been renamed in Ubuntu.  The only reason this test
> is currently passing is because the hypervisor build falls back to clang, at
> version 3.5
> 
> Add an explicit test in the build script that out desired compiler is
> available.  Note that travis already performs this step, but in a way which
> isn't fatal to the build.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

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

Re: [PATCH for-4.7 7/7] travis: Enable tools when building with clang

[Wei Liu]

On Wed, Apr 27, 2016 at 06:01:26PM +0100, Andrew Cooper wrote:
> tools now build under clang, so let them be tested.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

> ---
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Doug Goldstein <cardoe@cardoe.com>
> ---
>  scripts/travis-build | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/scripts/travis-build b/scripts/travis-build
> index 9a5c1c8..584d008 100755
> --- a/scripts/travis-build
> +++ b/scripts/travis-build
> @@ -22,12 +22,6 @@ else
>      cfgargs+=("--disable-tools") # we don't have the cross depends installed
>  fi
>  
> -# Due to multiple build failures and the desire to get more
> -# build testing (GCC only) enabled this is disabled
> -if [[ "${clang}" == "y" ]]; then
> -    cfgargs+=("--disable-tools")
> -fi
> -
>  ./configure "${cfgargs[@]}"
>  
>  make dist
> -- 
> 2.1.4
> 

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

Re: [PATCH for-4.7 0/7] More tools build fixes with clang

[Wei Liu]

On Wed, Apr 27, 2016 at 06:01:19PM +0100, Andrew Cooper wrote:
> With these fixes, tools/ can be built with clang, so enable it in the travis
> configurion.
> 
> The travis results can be viewed here:
>   https://travis-ci.org/andyhhp/xen/builds/126153860
> 
> Andrew Cooper (7):
>   tools/xenstat: Avoid comparing '0 <= unsigned integer'
>   tools/blktap2: Use abort() instead of custom crash
>   tools/blktap2: Fix array initialisers for tapdisk_disk_{types,drivers}[]
>   tools/blktap2: Fix use of uninitialised variable in _tap_list_join3()
>   tools/kdd: Fix uninitialised variable warning
>   travis: Remove clang-3.8 build
>   travis: Enable tools when building with clang
> 

Whole series:

Release-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 for-4.7 6/7] travis: Remove clang-3.8 build

[Doug Goldstein]

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

On 4/27/16 12:01 PM, Andrew Cooper wrote:
> The package appears to have been renamed in Ubuntu.  The only reason this test
> is currently passing is because the hypervisor build falls back to clang, at
> version 3.5
> 
> Add an explicit test in the build script that out desired compiler is
> available.  Note that travis already performs this step, but in a way which
> isn't fatal to the build.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-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 for-4.7 7/7] travis: Enable tools when building with clang

[Doug Goldstein]

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

On 4/27/16 12:01 PM, Andrew Cooper wrote:
> tools now build under clang, so let them be tested.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---

Acked-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 for-4.7 5/7] tools/kdd: Fix uninitialised variable warning

[Doug Goldstein]

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

On 4/27/16 12:01 PM, Andrew Cooper wrote:
> Clang warns:
> 
>   kdd.c:1031:9: error: variable 'fd' is used uninitialized whenever '||'
>   condition is true [-Werror,-Wsometimes-uninitialized]
>       if (argc != 4
>           ^~~~~~~~~
>   kdd.c:1040:20: note: uninitialized use occurs here
>       if (select(fd + 1, &fds, NULL, NULL, NULL) > 0)
>                  ^~
> 
> This situation can't actually happen, as usage() is a terminal path.  Annotate
> it appropriately.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@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 for-4.7 4/7] tools/blktap2: Fix use of uninitialised variable in _tap_list_join3()

[Doug Goldstein]

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

On 4/27/16 12:01 PM, Andrew Cooper wrote:
> Clang points out:
> 
>   tap-ctl-list.c:457:28: error: variable 'entry' is uninitialized when
>   used here [-Werror,-Wuninitialized]
>           for (; *_entry != NULL; ++entry) {
>                                     ^~~~~
> 
> The content of that loop clearly was meant to iterate over _entry rather than
> entry, so is fixed to do so.  This presumably fixes a memory leak when
> tapdisks get orphed, as only the first item on the list got freed.
> 
> There is no use of entry at all.  It is referenced in a
> list_for_each_entry(tl, &tap->list, entry) construct, but this is just a
> member name, and not a reference to local scope variable of the same name.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---

Reviewed-by: Doug Goldstein <cardoe@ardoe.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 for-4.7 3/7] tools/blktap2: Fix array initialisers for tapdisk_disk_{types, drivers}[]

[Doug Goldstein]

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

On 4/27/16 12:01 PM, Andrew Cooper wrote:
> Clang points out:
> 
>   tapdisk-disktype.c:117:2: error: initializer overrides prior initialization
>   of this subobject [-Werror,-Winitializer-overrides]
>           0,
>           ^
>   tapdisk-disktype.c:115:23: note: previous initialization is here
>           [DISK_TYPE_VINDEX]      = &vhd_index_disk,
>                                     ^~~~~~~~~~~~~~~
> 
> Mixing different initialiser styles should be avoided; The actual behaviour is
> different to the expected behaviour.  This specific example has been broken
> since its introduction in c/s 7b4dea554 "blktap2: Fix tapdisk disktype issues"
> in 2010, and is caused by the '#if 0' block removing &tapdisk_{sync,vmdk}.
> 
> First of all, remove what were intended to be trailing NULL entries in
> tapdisk_disk_{types,drivers}[], making consistent use of Designated
> Initialisers for the initialisation.
> 
> This requires changing the loop in tapdisk_disktype_find() to be based on the
> number of elements in tapdisk_disk_types[], rather than looking for the first
> NULL.  This fixes a latent bug, as the use of Designated Initializers causes
> to intermediate zero entries if not all indices are explicitly specified.
> 
> There is a second latent bug where tapdisk_disktype_find() assumes that
> tapdisk_disk_drivers[] has at least as many entries as tapdisk_disk_types[].
> This is not the case and tapdisk_disk_drivers[] had one entry fewer than
> tapdisk_disk_types[], but the NULL loop bound prevented an out-of-bounds read
> of tapdisk_disk_drivers[].  Fix the issue by explicitly declaring
> tapdisk_disk_drivers[] to have the same number of entries as
> tapdisk_disk_types[].
> 
> Finally, this leads to a linker error.  It turns out that tapdisk_vhd_index
> doesn't exist, and I can't find any evidence in the source history to suggest
> that it ever did.  I can only presume that it would have been #if 0'd out like
> tapdisk_sync and tapdisk_vmdk had it not been for this bug preventing a build
> failure.  Drop all three.
> 
> No functional change, but only because of the specific layout of
> tapdisk_disk_types[].
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---

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

Your assertion appears correct to me. Its the same conclusion I came to
and same solution I came to independently.

-- 
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 for-4.7 2/7] tools/blktap2: Use abort() instead of custom crash

[Doug Goldstein]

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

On 4/27/16 12:01 PM, Andrew Cooper wrote:
> Like c/s 4d98d3ebf - there is a second instance.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@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 for-4.7 1/7] tools/xenstat: Avoid comparing '0 <= unsigned integer'

[Doug Goldstein]

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

On 4/27/16 12:01 PM, Andrew Cooper wrote:
> Clang points out that this is tautological.
> 
>   src/xenstat.c:325:8: warning: comparison of 0 <= unsigned expression is
>   always true [-Wtautological-compare]
>           if (0 <= index && index < node->num_domains)
>               ~ ^  ~~~~~
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@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