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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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@xxxxxxxxxx>
---
CC: Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>
CC: Wei Liu <wei.liu2@xxxxxxxxxx>

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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel