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

[Doug Goldstein <cardoe@xxxxxxxxxx>]

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@xxxxxxxxxx>
> ---

Reviewed-by: Doug Goldstein <cardoe@xxxxxxxxxx>

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

-- 
Doug Goldstein

Attachment: signature.asc
Description: OpenPGP digital signature

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