Re: [Qemu-block] [PATCH for-4.2 13/13] iotests: Test qcow2's snapshot table handling

[Max Reitz]

On 30.07.19 21:56, Eric Blake wrote:
> 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 <address@hidden>
>> ---
>>  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.

Ah, sure.

>> +    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.

To me, it doesn’t seem really useful right now, as all qemu-created
images (past 1.1) will have the same 16 bytes.

>> +
>> +    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.

It’s a test, I know exactly what the extra data is (supposed to be).

(namely “very important data\0\0\0\0\0\0\0”)

[...]

>> +# 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...)

Right now we don’t care.  Which is good for me, because anything else
would make this test even slower than it already is (writing a different
name and ID into every snapshot would be a pain).

(It’s even worse for the next case.  There is a reason I do it for v2
only, where fully-zero snapshot table entries are valid.  It takes a
long time just to write a '16' into every one of >65536 entries.)

Max

[...]

> Overall, looks like a nice test.  I'm comfortable giving:
> 
> Reviewed-by: Eric Blake <address@hidden>

Again, thanks for reviewing!

signature.asc
Description: OpenPGP digital signature