On 7/30/19 12:25 PM, Max Reitz wrote:
> Add a test how our qcow2 driver handles extra data in snapshot table
> entries, and how it repairs overly long snapshot tables.

May need tweaking if we drop 9 and 10.

> 
> Signed-off-by: Max Reitz <mre...@redhat.com>
> ---
>  tests/qemu-iotests/261     | 449 +++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/261.out | 321 ++++++++++++++++++++++++++
>  tests/qemu-iotests/group   |   1 +
>  3 files changed, 771 insertions(+)
>  create mode 100755 tests/qemu-iotests/261
>  create mode 100644 tests/qemu-iotests/261.out
> 
> +
> +# Parameters:
> +#   $1: image filename
> +#   $2: snapshot table entry offset in the image
> +snapshot_table_entry_size()
> +{
> +    id_len=$(peek_file_be "$1" $(($2 + 12)) 2)
> +    name_len=$(peek_file_be "$1" $(($2 + 14)) 2)
> +    extra_len=$(peek_file_be "$1" $(($2 + 36)) 4)
> +
> +    full_len=$((40 + extra_len + id_len + name_len))
> +    if [ $((full_len % 8)) = 0 ]; then
> +        echo $full_len
> +    else
> +        echo $((full_len + 8 - full_len % 8))

Could replace the entire if with:
 echo $(( (full_len + 7) / 8 * 8 ))
but what you have works.

> +    fi
> +}
> +
> +# Parameter:
> +#   $1: image filename
> +print_snapshot_table()
> +{
> +    nb_entries=$(peek_file_be "$1" 60 4)
> +    offset=$(peek_file_be "$1" 64 8)
> +
> +    echo "Snapshots in $1:" | _filter_testdir | _filter_imgfmt

Should a separate patch add support in 'qemu-img info'/'qemu-img
snapshot -l' for letting users know how much extra info is in each
snapshot?  It seems useful enough without having to recode this
low-level iotest introspection.

> +
> +    for ((i = 0; i < nb_entries; i++)); do
> +        id_len=$(peek_file_be "$1" $((offset + 12)) 2)
> +        name_len=$(peek_file_be "$1" $((offset + 14)) 2)
> +        extra_len=$(peek_file_be "$1" $((offset + 36)) 4)
> +
> +        extra_ofs=$((offset + 40))
> +        id_ofs=$((extra_ofs + extra_len))
> +        name_ofs=$((id_ofs + id_len))
> +
> +        echo "  [$i]"
> +        echo "    ID: $(peek_file_raw "$1" $id_ofs $id_len)"
> +        echo "    Name: $(peek_file_raw "$1" $name_ofs $name_len)"

We're relying on the files having sane strings at those offsets - but
that's fine for the iotest.

> +        echo "    Extra data size: $extra_len"
> +        if [ $extra_len -ge 8 ]; then
> +            echo "    VM state size: $(peek_file_be "$1" $extra_ofs 8)"
> +        fi
> +        if [ $extra_len -ge 16 ]; then
> +            echo "    Disk size: $(peek_file_be "$1" $((extra_ofs + 8)) 8)"
> +        fi
> +        if [ $extra_len -gt 16 ]; then
> +            echo '    Unknown extra data:' \
> +                "$(peek_file_raw "$1" $((extra_ofs + 16)) $((extra_len - 
> 16)) \
> +                   | tr -d '\0')"

Printing the unknown extra data seems fishy, especially if you are going
to sanitize out the NUL bytes.  An od dump of every byte might be more
useful, but I'd also be happy with just printing the number of unknown
bytes without actually worrying about printing the contents of those bytes.


> +echo
> +echo '=== Create v2 template ==='
> +echo
> +
> +# Create v2 image with a snapshot table with three entries:
> +# [0]: No extra data (valid with v2, not valid with v3)
> +# [1]: Has extra data unknown to qemu
> +# [2]: Has the 64-bit VM state size, but not the disk size (again,
> +#      valid with v2, not valid with v3)

Cool - and covers cases that are not possible to do with current
'qemu-img create -f qcow2 -o compat=v2' (if you have a CentOS 6 machine
hanging around, you can create v2 images like entry [0]; but these days,
modern qemu-img creates v2 images with the 16 extra bytes as they will
be used by v3).

> +
> +TEST_IMG="$TEST_IMG.v2.orig" IMGOPTS='compat=0.10' _make_test_img 64M
> +$QEMU_IMG snapshot -c sn0 "$TEST_IMG.v2.orig"
> +$QEMU_IMG snapshot -c sn1 "$TEST_IMG.v2.orig"
> +$QEMU_IMG snapshot -c sn2 "$TEST_IMG.v2.orig"
> +
> +# Copy out all existing snapshot table entries

> +
> +# Amend them, one by one
> +# Set sn0's extra data size to 0
> +poke_file "$TEST_DIR/sn0-pre" 36 '\x00\x00\x00\x00'
> +truncate -s 0 "$TEST_DIR/sn0-extra"
> +# Grow sn0-post to pad
> +truncate -s $(($(snapshot_table_entry_size "$TEST_DIR/sn0-pre") - 40)) \
> +    "$TEST_DIR/sn0-post"
> +
> +# Set sn1's extra data size to 42

Nice - not a multiple of 8, to test our code handling of rounding :)

> +poke_file "$TEST_DIR/sn1-pre" 36 '\x00\x00\x00\x2a'
> +truncate -s 42 "$TEST_DIR/sn1-extra"
> +poke_file "$TEST_DIR/sn1-extra" 16 'very important data'
> +# Grow sn1-post to pad
> +truncate -s $(($(snapshot_table_entry_size "$TEST_DIR/sn1-pre") - 82)) \
> +    "$TEST_DIR/sn1-post"
> +
> +# Set sn2's extra data size to 8
> +poke_file "$TEST_DIR/sn2-pre" 36 '\x00\x00\x00\x08'
> +truncate -s 8 "$TEST_DIR/sn2-extra"
> +# Grow sn2-post to pad
> +truncate -s $(($(snapshot_table_entry_size "$TEST_DIR/sn2-pre") - 48)) \
> +    "$TEST_DIR/sn2-post"
> +
> +# Construct snapshot table
> +cat "$TEST_DIR"/sn0-{pre,extra,post} \
> +    "$TEST_DIR"/sn1-{pre,extra,post} \
> +    "$TEST_DIR"/sn2-{pre,extra,post} \
> +    | dd of="$TEST_IMG.v2.orig" bs=1 seek=$sn_table_ofs conv=notrunc \
> +          &> /dev/null
> +
> +# Done!

Hairy, but looks like a valid v2 image.

> +TEST_IMG="$TEST_IMG.v2.orig" _check_test_img
> +print_snapshot_table "$TEST_IMG.v2.orig"
> +
> +echo
> +echo '=== Upgrade to v3 ==='
> +echo
> +
> +cp "$TEST_IMG.v2.orig" "$TEST_IMG.v3.orig"
> +$QEMU_IMG amend -o compat=1.1 "$TEST_IMG.v3.orig"
> +TEST_IMG="$TEST_IMG.v3.orig" _check_test_img
> +print_snapshot_table "$TEST_IMG.v3.orig"
> +
> +echo
> +echo '=== Repair botched v3 ==='
> +echo
> +
> +# Force the v2 file to be v3.  v3 requires each snapshot table entry
> +# to have at least 16 bytes of extra data, so it will not comply to
> +# the qcow2 v3 specification; but we can fix that.
> +cp "$TEST_IMG.v2.orig" "$TEST_IMG"
> +
> +# Set version
> +poke_file "$TEST_IMG" 4 '\x00\x00\x00\x03'
> +# Increase header length (necessary for v3)
> +poke_file "$TEST_IMG" 100 '\x00\x00\x00\x68'
> +# Set refcount order (necessary for v3)
> +poke_file "$TEST_IMG" 96 '\x00\x00\x00\x04'
> +
> +_check_test_img -r all
> +print_snapshot_table "$TEST_IMG"

Good - you're testing that we can now flag and fix images that we were
previously silently using in spite of their non-compliance.

> +
> +
> +# From now on, just test the qcow2 version we are supposed to test.
> +# (v3 by default, v2 by choice through $IMGOPTS.)
> +# That works because we always write all known extra data when
> +# updating the snapshot table, independent of the version.
> +
> +if echo "$IMGOPTS" | grep -q 'compat=\(0\.10\|v2\)' 2> /dev/null; then
> +    subver=v2
> +else
> +    subver=v3
> +fi
> +
> +echo
> +echo '=== Add new snapshot ==='
> +echo
> +
> +cp "$TEST_IMG.$subver.orig" "$TEST_IMG"
> +$QEMU_IMG snapshot -c sn3 "$TEST_IMG"
> +_check_test_img
> +print_snapshot_table "$TEST_IMG"
> +
> +echo
> +echo '=== Remove different snapshots ==='
> +
> +for sn in sn0 sn1 sn2; do
> +    echo
> +    echo "--- $sn ---"
> +
> +    cp "$TEST_IMG.$subver.orig" "$TEST_IMG"
> +    $QEMU_IMG snapshot -d $sn "$TEST_IMG"
> +    _check_test_img
> +    print_snapshot_table "$TEST_IMG"
> +done
> +
> +echo
> +echo '=== Reject too much unknown extra data ==='
> +echo
> +
> +cp "$TEST_IMG.$subver.orig" "$TEST_IMG"
> +$QEMU_IMG snapshot -c sn3 "$TEST_IMG"
> +
> +sn_table_ofs=$(peek_file_be "$TEST_IMG" 64 8)
> +sn0_ofs=$sn_table_ofs
> +sn1_ofs=$((sn0_ofs + $(snapshot_table_entry_size "$TEST_IMG" $sn0_ofs)))
> +sn2_ofs=$((sn1_ofs + $(snapshot_table_entry_size "$TEST_IMG" $sn1_ofs)))
> +sn3_ofs=$((sn2_ofs + $(snapshot_table_entry_size "$TEST_IMG" $sn2_ofs)))
> +
> +# 64 kB of extra data should be rejected
> +# (Note that this also induces a refcount error, because it spills
> +# over to the next cluster.  That's a good way to test that we can
> +# handle simultaneous snapshot table and refcount errors.)
> +poke_file "$TEST_IMG" $((sn3_ofs + 36)) '\x00\x01\x00\x00'
> +
> +# Print error
> +_img_info
> +echo
> +_check_test_img
> +echo
> +
> +# Should be repairable
> +_check_test_img -r all

Nice.

> +
> +echo
> +echo '=== Snapshot table too big ==='
> +echo
> +
> +sn_table_ofs=$(peek_file_be "$TEST_IMG.v3.orig" 64 8)
> +
> +# Fill a snapshot with 1 kB of extra data, a 65535-char ID, and a
> +# 65535-char name, and repeat it as many times as necessary to fill
> +# 64 MB (the maximum supported by qemu)
> +
> +touch "$TEST_DIR/sn0"
> +
> +# Full size (fixed + extra + ID + name + padding)
> +sn_size=$((40 + 1024 + 65535 + 65535 + 2))
> +
> +# We only need the fixed part, though.
> +truncate -s 40 "$TEST_DIR/sn0"
> +
> +# 65535-char ID string
> +poke_file "$TEST_DIR/sn0" 12 '\xff\xff'
> +# 65535-char name
> +poke_file "$TEST_DIR/sn0" 14 '\xff\xff'

Do we care that there are NUL bytes in the id and name?  (The spec is
clear that id and name are not NUL-terminated, but does not actually
seem to forbid the use of arbitrary binary values as names...)

> +# 1 kB of extra data
> +poke_file "$TEST_DIR/sn0" 36 '\x00\x00\x04\x00'
> +
> +# Create test image
> +_make_test_img 64M
> +
> +# Hook up snapshot table somewhere safe (at 1 MB)
> +poke_file "$TEST_IMG" 64 '\x00\x00\x00\x00\x00\x10\x00\x00'
> +
> +offset=1048576
> +size_written=0
> +sn_count=0
> +while [ $size_written -le $((64 * 1048576)) ]; do
> +    dd if="$TEST_DIR/sn0" of="$TEST_IMG" bs=1 seek=$offset conv=notrunc \
> +        &> /dev/null
> +    offset=$((offset + sn_size))
> +    size_written=$((size_written + sn_size))
> +    sn_count=$((sn_count + 1))
> +done
> +
> +# Give the last snapshot (the one to be removed) an L1 table so we can
> +# see how that is handled when repairing the image
> +# (Put it two clusters before 1 MB, and one L2 table one cluster
> +# before 1 MB)
> +poke_file "$TEST_IMG" $((offset - sn-size + 0)) \
> +    '\x00\x00\x00\x00\x00\x0e\x00\x00'
> +poke_file "$TEST_IMG" $((offset - sn-size + 8)) \
> +    '\x00\x00\x00\x01'
> +
> +# Hook up the L2 table
> +poke_file "$TEST_IMG" $((1048576 - 2 * 65536)) \
> +    '\x80\x00\x00\x00\x00\x0f\x00\x00'
> +
> +# Make sure all of the clusters we just hooked up are allocated:
> +# - The snapshot table
> +# - The last snapshot's L1 and L2 table
> +refblock0_allocate $((1048576 - 2 * 65536)) $offset
> +
> +poke_file "$TEST_IMG" 60 \
> +    "$(printf '%08x' $sn_count | sed -e 's/\(..\)/\\x\1/g')"

Wow - you went to a lot of work to produce this image :)

> +
> +# Print error
> +_img_info
> +echo
> +_check_test_img
> +echo
> +
> +# Should be repairable
> +_check_test_img -r all

Depending on our decision on 9 and 10.

> +
> +echo
> +echo "$((sn_count - 1)) snapshots should remain:"
> +echo "  qemu-img info reports $(_img_info | grep -c '^ \{34\}') snapshots"
> +echo "  Image header reports $(peek_file_be "$TEST_IMG" 60 4) snapshots"
> +
> +echo
> +echo '=== Too many snapshots ==='
> +echo
> +
> +# Create a v2 image, for speeds' sake: All-zero snapshot table entries
> +# are only valid in v2.
> +IMGOPTS='compat=0.10' _make_test_img 64M
> +
> +# Hook up snapshot table somewhere safe (at 1 MB)
> +poke_file "$TEST_IMG" 64 '\x00\x00\x00\x00\x00\x10\x00\x00'
> +# "Create" more than 65536 snapshots (twice that many here)
> +poke_file "$TEST_IMG" 60 '\x00\x02\x00\x00'
> +
> +# 40-byte all-zero snapshot table entries are valid snapshots, but
> +# only in v2 (v3 needs 16 bytes of extra data, so we would have to
> +# write 131072x '\x10').
> +truncate -s $((1048576 + 40 * 131072)) "$TEST_IMG"
> +
> +# But let us give one of the snapshots to be removed an L1 table so
> +# we can see how that is handled when repairing the image.
> +# (Put it two clusters before 1 MB, and one L2 table one cluster
> +# before 1 MB)
> +poke_file "$TEST_IMG" $((1048576 + 40 * 65536 + 0)) \
> +    '\x00\x00\x00\x00\x00\x0e\x00\x00'
> +poke_file "$TEST_IMG" $((1048576 + 40 * 65536 + 8)) \
> +    '\x00\x00\x00\x01'
> +
> +# Hook up the L2 table
> +poke_file "$TEST_IMG" $((1048576 - 2 * 65536)) \
> +    '\x80\x00\x00\x00\x00\x0f\x00\x00'
> +
> +# Make sure all of the clusters we just hooked up are allocated:
> +# - The snapshot table
> +# - The last snapshot's L1 and L2 table
> +refblock0_allocate $((1048576 - 2 * 65536)) $((1048576 + 40 * 131072))
> +

Another fun image creation.

> +# Print error
> +_img_info
> +echo
> +_check_test_img
> +echo
> +
> +# Should be repairable
> +_check_test_img -r all
> +
> +echo
> +echo '65536 snapshots should remain:'
> +echo "  qemu-img info reports $(_img_info | grep -c '^ \{34\}') snapshots"
> +echo "  Image header reports $(peek_file_be "$TEST_IMG" 60 4) snapshots"
> +
> +# success, all done
> +echo "*** done"
> +status=0

Overall, looks like a nice test.  I'm comfortable giving:

Reviewed-by: Eric Blake <ebl...@redhat.com>

> +++ b/tests/qemu-iotests/261.out
> @@ -0,0 +1,321 @@
> +QA output created by 261
> +
> +=== Create v2 template ===
> +
> +Formatting 'TEST_DIR/t.IMGFMT.v2.orig', fmt=IMGFMT size=67108864
> +No errors were found on the image.
> +Snapshots in TEST_DIR/t.IMGFMT.v2.orig:
> +  [0]
> +    ID: 1
> +    Name: sn0
> +    Extra data size: 0
> +  [1]
> +    ID: 2
> +    Name: sn1
> +    Extra data size: 42
> +    VM state size: 0
> +    Disk size: 67108864
> +    Unknown extra data: very important data

Again, if qemu-img is improved to output more information natively
instead of you hand-scraping it, I don't think that printing the
contents of extra data is viable in that context.  But it looks fun here
in this test.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature

Reply via email to