Hi, See the v1 cover letter here if you haven’t already: https://lists.nongnu.org/archive/html/qemu-block/2019-07/msg01290.html
I kept all patches from the previous version because Eric deemed the patches I might have dropped useful. I kept the way I implemented fixing overly long snapshot tables, namely by discarding all snapshots past the maximum end. Eric criticized this, because this is indiscriminate (and the last ones have been created the most recently, but I’m not sure whether we’d generally rather keep the oldest or the most recent ones. Maybe best would be to drop every second snapshot? :-)). I kept it as it is, because everything else would require user interaction. We currently have no interactive check mode, and I feel like this is not a place where we need it badly: We introduced our current snapshot limits in such a way that no sane image would ever hit them. If you do hit them, changes are that your image is already corrupted, so you probably no longer care about your snapshots anyway and just want to copy the active layer off. Furthermore, I felt like an interactive mode would be pretty hard to test thoroughly. I don’t want to introduce code that probably nobody will ever use and that is hardly tested. I also tried Eric’s suggestion of using bdrv_preadv() in qcow2_read_snapshots() to read all variable data at once, and while it was very promising, it doesn’t allow us to skip data (which we must do when we want to truncate extra data down to a sane size). So all in all I couldn’t make it work. So, the changes in v2: - Patch 1: Eric asked from some magic numbers to be removed (very reasonable), and endof() will help with that. - Patch 2: See, it helps! - Patch 4: Rebased conflicts (because of patch 2), and I should use BDRV_REQUEST_MAX_BYTES instead of INT_MAX when I mean the former - Patch 7: Drop magic number [Eric] - Patch 8: Drop magic number [Eric] - Patch 10: Instead of dropping all unknown extra data when there is too much, just truncate it down to the maximum [Eric] - Patch 11: When some snapshot table entry has too much extra data and we truncate it, that shortens the snapshot table length, so we should keep that in mind when calculating whether the table is too long - Patch 12: Rebase conflicts, and dropped a magic number [Eric] - Patch 14: Drop magic number [Eric] - Patch 16: Simplified padding calculation [Eric], fixed one test case (I used “sn-size” instead of “sn_size”, oops), and added a test case for what happens when a snapshot table entry is too big and contains an entry with too much extra data (the latter should be fixed first and then we should look into whether the table is still too long) git-backport-diff against v1: Key: [----] : patches are identical [####] : number of functional differences between upstream/downstream patch [down] : patch is downstream-only The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively 001/16:[down] 'include: Move endof() up from hw/virtio/virtio.h' 002/16:[down] 'qcow2: Use endof()' 003/16:[----] [--] 'qcow2: Add Error ** to qcow2_read_snapshots()' 004/16:[0012] [FC] 'qcow2: Keep unknown extra snapshot data' 005/16:[----] [--] 'qcow2: Make qcow2_write_snapshots() public' 006/16:[----] [--] 'qcow2: Put qcow2_upgrade() into an own function' 007/16:[0005] [FC] 'qcow2: Write v3-compliant snapshot list on upgrade' 008/16:[0004] [FC] 'qcow2: Separate qcow2_check_read_snapshot_table()' 009/16:[----] [--] 'qcow2: Add qcow2_check_fix_snapshot_table()' 010/16:[0038] [FC] 'qcow2: Fix broken snapshot table entries' 011/16:[down] 'qcow2: Keep track of the snapshot table length' 012/16:[0003] [FC] 'qcow2: Fix overly long snapshot tables' 013/16:[----] [--] 'qcow2: Repair snapshot table with too many entries' 014/16:[0005] [FC] 'qcow2: Fix v3 snapshot table entry compliancy' 015/16:[----] [--] 'iotests: Add peek_file* functions' 016/16:[0125] [FC] 'iotests: Test qcow2's snapshot table handling' Max Reitz (16): include: Move endof() up from hw/virtio/virtio.h qcow2: Use endof() qcow2: Add Error ** to qcow2_read_snapshots() qcow2: Keep unknown extra snapshot data qcow2: Make qcow2_write_snapshots() public qcow2: Put qcow2_upgrade() into its own function qcow2: Write v3-compliant snapshot list on upgrade qcow2: Separate qcow2_check_read_snapshot_table() qcow2: Add qcow2_check_fix_snapshot_table() qcow2: Fix broken snapshot table entries qcow2: Keep track of the snapshot table length qcow2: Fix overly long snapshot tables qcow2: Repair snapshot table with too many entries qcow2: Fix v3 snapshot table entry compliancy iotests: Add peek_file* functions iotests: Test qcow2's snapshot table handling block/qcow2.h | 15 +- include/hw/virtio/virtio.h | 7 - include/qemu/compiler.h | 7 + block/qcow2-snapshot.c | 323 +++++++++++++++++++-- block/qcow2.c | 155 +++++++++-- hw/block/virtio-blk.c | 4 +- hw/net/virtio-net.c | 10 +- tests/qemu-iotests/261 | 523 +++++++++++++++++++++++++++++++++++ tests/qemu-iotests/261.out | 346 +++++++++++++++++++++++ tests/qemu-iotests/common.rc | 20 ++ tests/qemu-iotests/group | 1 + 11 files changed, 1354 insertions(+), 57 deletions(-) create mode 100755 tests/qemu-iotests/261 create mode 100644 tests/qemu-iotests/261.out -- 2.21.0