[PULL 00/13] Block layer patches

[PULL 00/13] Block layer patches

[Kevin Wolf]

The following changes since commit 105b07f1ba462ec48b27e5cb74ddf81c6a79364c:

  Merge remote-tracking branch 'remotes/cohuck/tags/s390x-20200127' into staging (2020-01-27 13:02:36 +0000)

are available in the Git repository at:

  git://repo.or.cz/qemu/kevin.git tags/for-upstream

for you to fetch changes up to 5fbf1d56c24018772e900a40a0955175ff82f35c:

  iscsi: Don't access non-existent scsi_lba_status_descriptor (2020-01-27 17:19:53 +0100)

----------------------------------------------------------------
Block layer patches:

- iscsi: Cap block count from GET LBA STATUS (CVE-2020-1711)
- AioContext fixes in QMP commands for backup and bitmaps
- iotests fixes

----------------------------------------------------------------
Eiichi Tsukata (1):
      block/backup: fix memory leak in bdrv_backup_top_append()

Felipe Franciosi (1):
      iscsi: Cap block count from GET LBA STATUS (CVE-2020-1711)

Kevin Wolf (1):
      iscsi: Don't access non-existent scsi_lba_status_descriptor

Max Reitz (1):
      iotests.py: Let wait_migration wait even more

Sergio Lopez (8):
      blockdev: fix coding style issues in drive_backup_prepare
      blockdev: unify qmp_drive_backup and drive-backup transaction paths
      blockdev: unify qmp_blockdev_backup and blockdev-backup transaction paths
      blockdev: honor bdrv_try_set_aio_context() context requirements
      block/backup-top: Don't acquire context while dropping top
      blockdev: Acquire AioContext on dirty bitmap functions
      blockdev: Return bs to the proper context on snapshot abort
      iotests: Test handling of AioContexts with some blockdev actions

Thomas Huth (1):
      iotests: Add more "skip_if_unsupported" statements to the python tests

 block/backup-top.c            |   7 +-
 block/backup.c                |   3 +
 block/iscsi.c                 |   7 +-
 blockdev.c                    | 393 +++++++++++++++++++++++-------------------
 tests/qemu-iotests/iotests.py |   6 +-
 tests/qemu-iotests/030        |   4 +-
 tests/qemu-iotests/040        |   2 +
 tests/qemu-iotests/041        |  39 +----
 tests/qemu-iotests/141.out    |   2 +
 tests/qemu-iotests/185.out    |   2 +
 tests/qemu-iotests/219        |   7 +-
 tests/qemu-iotests/219.out    |   8 +
 tests/qemu-iotests/234        |   8 +-
 tests/qemu-iotests/245        |   2 +
 tests/qemu-iotests/262        |   4 +-
 tests/qemu-iotests/280        |   2 +-
 tests/qemu-iotests/281        | 247 ++++++++++++++++++++++++++
 tests/qemu-iotests/281.out    |   5 +
 tests/qemu-iotests/group      |   1 +
 19 files changed, 510 insertions(+), 239 deletions(-)
 create mode 100755 tests/qemu-iotests/281
 create mode 100644 tests/qemu-iotests/281.out

Re: [PULL 00/13] Block layer patches

[Peter Maydell]

On Mon, 27 Jan 2020 at 17:56, Kevin Wolf <kwolf@redhat.com> wrote:
>
> The following changes since commit 105b07f1ba462ec48b27e5cb74ddf81c6a79364c:
>
>   Merge remote-tracking branch 'remotes/cohuck/tags/s390x-20200127' into staging (2020-01-27 13:02:36 +0000)
>
> are available in the Git repository at:
>
>   git://repo.or.cz/qemu/kevin.git tags/for-upstream
>
> for you to fetch changes up to 5fbf1d56c24018772e900a40a0955175ff82f35c:
>
>   iscsi: Don't access non-existent scsi_lba_status_descriptor (2020-01-27 17:19:53 +0100)
>
> ----------------------------------------------------------------
> Block layer patches:
>
> - iscsi: Cap block count from GET LBA STATUS (CVE-2020-1711)
> - AioContext fixes in QMP commands for backup and bitmaps
> - iotests fixes
>
> ----------------------------------------------------------------


Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/5.0
for any user-visible changes.

-- PMM

Re: [PULL 00/13] Block layer patches

[Peter Maydell]

On Wed, 11 Mar 2020 at 15:42, Kevin Wolf <kwolf@redhat.com> wrote:
>
> The following changes since commit ba29883206d92a29ad5a466e679ccfc2ee6132ef:
>
>   Merge remote-tracking branch 'remotes/borntraeger/tags/s390x-20200310' into staging (2020-03-10 16:50:28 +0000)
>
> are available in the Git repository at:
>
>   git://repo.or.cz/qemu/kevin.git tags/for-upstream
>
> for you to fetch changes up to 8bb3b023f2055054ee119cb45b42d2b14be7fc8a:
>
>   qemu-iotests: adding LUKS cleanup for non-UTF8 secret error (2020-03-11 15:54:38 +0100)
>
> ----------------------------------------------------------------
> Block layer patches:
>
> - Relax restrictions for blockdev-snapshot (allows libvirt to do live
>   storage migration with blockdev-mirror)
> - luks: Delete created files when block_crypto_co_create_opts_luks fails
> - Fix memleaks in qmp_object_add
>
> ----------------------------------------------------------------


iotest 030 hung on x86-64 Linux (Ubuntu):

petmay01 11801  0.0  0.0  34668 26112 ?        S    11:24   0:03  |
                   \_ make --output-sync -C build/alldbg check V=1 -j8
petmay01 15277  0.0  0.0   4628   792 ?        S    11:25   0:00  |
                       \_ /bin/sh
/home/petmay01/linaro/qemu-for-merges/tests/check-block.sh
petmay01 15344  0.0  0.0  14172  3360 ?        S    11:25   0:00  |
                           \_ bash ./check -makecheck -qcow2 -g auto
petmay01 27902  0.0  0.0  14172  2128 ?        S    11:25   0:00  |
                               \_ bash ./check -makecheck -qcow2 -g
auto
petmay01 27903  0.0  0.0  52660 16400 ?        S    11:25   0:00  |
                                   \_ python3 -B 030
petmay01  1728  0.0  0.1 1011792 51604 ?       Sl   11:26   0:01  |
                                       \_
/home/petmay01/linaro/qemu-for-merges/build/alldbg/tests/qemu-iotests/../../x86_64-softmmu/qemu-system-x86_64
-display none -vga none -chardev
socket,id=mon,path=/tmp/tmp.QBQTAAybTi/qemu-27903-monitor.sock -mon
chardev=mon,mode=control -qtest
unix:path=/tmp/tmp.QBQTAAybTi/qemu-27903-qtest.sock -accel qtest
-nodefaults -display none -accel qtest -drive
if=virtio,id=drive0,file=blkdebug::/home/petmay01/linaro/qemu-for-merges/build/alldbg/tests/qemu-iotests/scratch/test.img,format=qcow2,cache=writeback,aio=threads,backing.node-name=mid,backing.backing.node-name=base

I had to manually kill the offending QEMU process; resulting
output in the log:

--- /home/petmay01/linaro/qemu-for-merges/tests/qemu-iotests/030.out
 2019-07-15 17:18:35.251364738 +01
00
+++ /home/petmay01/linaro/qemu-for-merges/build/alldbg/tests/qemu-iotests/030.out.bad
  2020-03-12 13:44:
43.101182680 +0000
@@ -1,5 +1,27 @@
-...........................
+........................E..
+======================================================================
+ERROR: test_stream_pause (__main__.TestSingleDrive)
+----------------------------------------------------------------------
+Traceback (most recent call last):
+  File "030", line 93, in test_stream_pause
+    self.pause_wait('drive0')
+  File "/home/petmay01/linaro/qemu-for-merges/tests/qemu-iotests/iotests.py",
line 927, in pause_wait
+    result = self.vm.qmp('query-block-jobs')
+  File "/home/petmay01/linaro/qemu-for-merges/tests/qemu-iotests/../../python/qemu/machine.py",
line 405, in qmp
+    return self._qmp.cmd(cmd, args=qmp_args)
+  File "/home/petmay01/linaro/qemu-for-merges/tests/qemu-iotests/../../python/qemu/qmp.py",
line 215, in cmd
+    return self.cmd_obj(qmp_cmd)
+  File "/home/petmay01/linaro/qemu-for-merges/tests/qemu-iotests/../../python/qemu/qmp.py",
line 198, in cmd_obj
+    resp = self.__json_read()
+  File "/home/petmay01/linaro/qemu-for-merges/tests/qemu-iotests/../../python/qemu/qmp.py",
line 89, in __json_read
+    data = self.__sockfile.readline()
+  File "/usr/lib/python3.6/socket.py", line 586, in readinto
+    return self._sock.recv_into(b)
+  File "/home/petmay01/linaro/qemu-for-merges/tests/qemu-iotests/iotests.py",
line 383, in timeout
+    raise Exception(self.errmsg)
+Exception: Timeout waiting for job to pause
+
 ----------------------------------------------------------------------


thanks
-- PMM

Re: [PULL 00/13] Block layer patches

[Kevin Wolf]

Am 12.03.2020 um 14:46 hat Peter Maydell geschrieben:
> On Wed, 11 Mar 2020 at 15:42, Kevin Wolf <kwolf@redhat.com> wrote:
> >
> > The following changes since commit ba29883206d92a29ad5a466e679ccfc2ee6132ef:
> >
> >   Merge remote-tracking branch 'remotes/borntraeger/tags/s390x-20200310' into staging (2020-03-10 16:50:28 +0000)
> >
> > are available in the Git repository at:
> >
> >   git://repo.or.cz/qemu/kevin.git tags/for-upstream
> >
> > for you to fetch changes up to 8bb3b023f2055054ee119cb45b42d2b14be7fc8a:
> >
> >   qemu-iotests: adding LUKS cleanup for non-UTF8 secret error (2020-03-11 15:54:38 +0100)
> >
> > ----------------------------------------------------------------
> > Block layer patches:
> >
> > - Relax restrictions for blockdev-snapshot (allows libvirt to do live
> >   storage migration with blockdev-mirror)
> > - luks: Delete created files when block_crypto_co_create_opts_luks fails
> > - Fix memleaks in qmp_object_add
> >
> > ----------------------------------------------------------------
> 
> 
> iotest 030 hung on x86-64 Linux (Ubuntu):
> 
> petmay01 11801  0.0  0.0  34668 26112 ?        S    11:24   0:03  |
>                    \_ make --output-sync -C build/alldbg check V=1 -j8
> petmay01 15277  0.0  0.0   4628   792 ?        S    11:25   0:00  |
>                        \_ /bin/sh
> /home/petmay01/linaro/qemu-for-merges/tests/check-block.sh
> petmay01 15344  0.0  0.0  14172  3360 ?        S    11:25   0:00  |
>                            \_ bash ./check -makecheck -qcow2 -g auto
> petmay01 27902  0.0  0.0  14172  2128 ?        S    11:25   0:00  |
>                                \_ bash ./check -makecheck -qcow2 -g
> auto
> petmay01 27903  0.0  0.0  52660 16400 ?        S    11:25   0:00  |
>                                    \_ python3 -B 030
> petmay01  1728  0.0  0.1 1011792 51604 ?       Sl   11:26   0:01  |
>                                        \_
> /home/petmay01/linaro/qemu-for-merges/build/alldbg/tests/qemu-iotests/../../x86_64-softmmu/qemu-system-x86_64
> -display none -vga none -chardev
> socket,id=mon,path=/tmp/tmp.QBQTAAybTi/qemu-27903-monitor.sock -mon
> chardev=mon,mode=control -qtest
> unix:path=/tmp/tmp.QBQTAAybTi/qemu-27903-qtest.sock -accel qtest
> -nodefaults -display none -accel qtest -drive
> if=virtio,id=drive0,file=blkdebug::/home/petmay01/linaro/qemu-for-merges/build/alldbg/tests/qemu-iotests/scratch/test.img,format=qcow2,cache=writeback,aio=threads,backing.node-name=mid,backing.backing.node-name=base
> 
> I had to manually kill the offending QEMU process; resulting
> output in the log:
> 
> --- /home/petmay01/linaro/qemu-for-merges/tests/qemu-iotests/030.out
>  2019-07-15 17:18:35.251364738 +01
> 00
> +++ /home/petmay01/linaro/qemu-for-merges/build/alldbg/tests/qemu-iotests/030.out.bad
>   2020-03-12 13:44:
> 43.101182680 +0000
> @@ -1,5 +1,27 @@
> -...........................
> +........................E..
> +======================================================================
> +ERROR: test_stream_pause (__main__.TestSingleDrive)
> +----------------------------------------------------------------------
> +Traceback (most recent call last):
> +  File "030", line 93, in test_stream_pause
> +    self.pause_wait('drive0')
> +  File "/home/petmay01/linaro/qemu-for-merges/tests/qemu-iotests/iotests.py",
> line 927, in pause_wait
> +    result = self.vm.qmp('query-block-jobs')
> +  File "/home/petmay01/linaro/qemu-for-merges/tests/qemu-iotests/../../python/qemu/machine.py",
> line 405, in qmp
> +    return self._qmp.cmd(cmd, args=qmp_args)
> +  File "/home/petmay01/linaro/qemu-for-merges/tests/qemu-iotests/../../python/qemu/qmp.py",
> line 215, in cmd
> +    return self.cmd_obj(qmp_cmd)
> +  File "/home/petmay01/linaro/qemu-for-merges/tests/qemu-iotests/../../python/qemu/qmp.py",
> line 198, in cmd_obj
> +    resp = self.__json_read()
> +  File "/home/petmay01/linaro/qemu-for-merges/tests/qemu-iotests/../../python/qemu/qmp.py",
> line 89, in __json_read
> +    data = self.__sockfile.readline()
> +  File "/usr/lib/python3.6/socket.py", line 586, in readinto
> +    return self._sock.recv_into(b)
> +  File "/home/petmay01/linaro/qemu-for-merges/tests/qemu-iotests/iotests.py",
> line 383, in timeout
> +    raise Exception(self.errmsg)
> +Exception: Timeout waiting for job to pause
> +
>  ----------------------------------------------------------------------

For the record (discussed on IRC):

This seems to be intermittent failure where a short timeout (1 second)
might have been too short under heavy load. That this results in a hang
is a test case bug, but it already exists on master.

Peter will retry the test later.

Kevin

Re: [PULL 00/13] Block layer patches

[Peter Maydell]

On Wed, 11 Mar 2020 at 15:42, Kevin Wolf <kwolf@redhat.com> wrote:
>
> The following changes since commit ba29883206d92a29ad5a466e679ccfc2ee6132ef:
>
>   Merge remote-tracking branch 'remotes/borntraeger/tags/s390x-20200310' into staging (2020-03-10 16:50:28 +0000)
>
> are available in the Git repository at:
>
>   git://repo.or.cz/qemu/kevin.git tags/for-upstream
>
> for you to fetch changes up to 8bb3b023f2055054ee119cb45b42d2b14be7fc8a:
>
>   qemu-iotests: adding LUKS cleanup for non-UTF8 secret error (2020-03-11 15:54:38 +0100)
>
> ----------------------------------------------------------------
> Block layer patches:
>
> - Relax restrictions for blockdev-snapshot (allows libvirt to do live
>   storage migration with blockdev-mirror)
> - luks: Delete created files when block_crypto_co_create_opts_luks fails
> - Fix memleaks in qmp_object_add


Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/5.0
for any user-visible changes.

-- PMM

[PULL 00/13] Block layer patches

[Kevin Wolf]

The following changes since commit e3acc2c1961cbe22ca474cd5da4163b7bbf7cea3:

  tests/docker/dockerfiles: Bump fedora-i386-cross to fedora 34 (2021-10-05 16:40:39 -0700)

are available in the Git repository at:

  git://repo.or.cz/qemu/kevin.git tags/for-upstream

for you to fetch changes up to 3765315d4c84f9c0799744f43a314169baaccc05:

  iotests: Update for pylint 2.11.1 (2021-10-06 10:25:55 +0200)

----------------------------------------------------------------
Block layer patches

- Fix I/O errors because of incorrectly detected max_iov
- Fix not white-listed copy-before-write
- qemu-storage-daemon: Only display FUSE help when FUSE is built-in
- iotests: update environment and linting configuration

----------------------------------------------------------------
Emanuele Giuseppe Esposito (1):
      include/block.h: remove outdated comment

John Snow (5):
      iotests: add 'qemu' package location to PYTHONPATH in testenv
      iotests/linters: check mypy files all at once
      iotests/mirror-top-perms: Adjust imports
      iotests/migrate-bitmaps-test: delint
      iotests: Update for pylint 2.11.1

Paolo Bonzini (1):
      block: introduce max_hw_iov for use in scsi-generic

Philippe Mathieu-Daudé (1):
      qemu-storage-daemon: Only display FUSE help when FUSE is built-in

Vladimir Sementsov-Ogievskiy (5):
      block: implement bdrv_new_open_driver_opts()
      block: bdrv_insert_node(): fix and improve error handling
      block: bdrv_insert_node(): doc and style
      block: bdrv_insert_node(): don't use bdrv_open()
      iotests/image-fleecing: declare requirement of copy-before-write

 include/block/block.h                         |  8 ++-
 include/block/block_int.h                     |  7 +++
 include/sysemu/block-backend.h                |  1 +
 block.c                                       | 79 ++++++++++++++++++++++-----
 block/block-backend.c                         |  6 ++
 block/file-posix.c                            |  2 +-
 block/io.c                                    |  1 +
 hw/scsi/scsi-generic.c                        |  2 +-
 storage-daemon/qemu-storage-daemon.c          |  2 +
 tests/qemu-iotests/iotests.py                 |  2 -
 tests/qemu-iotests/testenv.py                 | 15 +++--
 tests/qemu-iotests/testrunner.py              |  7 ++-
 tests/qemu-iotests/235                        |  2 -
 tests/qemu-iotests/297                        | 52 +++++++-----------
 tests/qemu-iotests/300                        |  5 +-
 tests/qemu-iotests/pylintrc                   |  6 +-
 tests/qemu-iotests/tests/image-fleecing       |  1 +
 tests/qemu-iotests/tests/migrate-bitmaps-test | 50 +++++++++--------
 tests/qemu-iotests/tests/mirror-top-perms     | 12 ++--
 19 files changed, 164 insertions(+), 96 deletions(-)

Re: [PULL 00/13] Block layer patches

[Richard Henderson]

On 10/6/21 3:59 AM, Kevin Wolf wrote:
> The following changes since commit e3acc2c1961cbe22ca474cd5da4163b7bbf7cea3:
> 
>    tests/docker/dockerfiles: Bump fedora-i386-cross to fedora 34 (2021-10-05 16:40:39 -0700)
> 
> are available in the Git repository at:
> 
>    git://repo.or.cz/qemu/kevin.git tags/for-upstream
> 
> for you to fetch changes up to 3765315d4c84f9c0799744f43a314169baaccc05:
> 
>    iotests: Update for pylint 2.11.1 (2021-10-06 10:25:55 +0200)
> 
> ----------------------------------------------------------------
> Block layer patches
> 
> - Fix I/O errors because of incorrectly detected max_iov
> - Fix not white-listed copy-before-write
> - qemu-storage-daemon: Only display FUSE help when FUSE is built-in
> - iotests: update environment and linting configuration
> 
> ----------------------------------------------------------------
> Emanuele Giuseppe Esposito (1):
>        include/block.h: remove outdated comment
> 
> John Snow (5):
>        iotests: add 'qemu' package location to PYTHONPATH in testenv
>        iotests/linters: check mypy files all at once
>        iotests/mirror-top-perms: Adjust imports
>        iotests/migrate-bitmaps-test: delint
>        iotests: Update for pylint 2.11.1
> 
> Paolo Bonzini (1):
>        block: introduce max_hw_iov for use in scsi-generic
> 
> Philippe Mathieu-Daudé (1):
>        qemu-storage-daemon: Only display FUSE help when FUSE is built-in
> 
> Vladimir Sementsov-Ogievskiy (5):
>        block: implement bdrv_new_open_driver_opts()
>        block: bdrv_insert_node(): fix and improve error handling
>        block: bdrv_insert_node(): doc and style
>        block: bdrv_insert_node(): don't use bdrv_open()
>        iotests/image-fleecing: declare requirement of copy-before-write
> 
>   include/block/block.h                         |  8 ++-
>   include/block/block_int.h                     |  7 +++
>   include/sysemu/block-backend.h                |  1 +
>   block.c                                       | 79 ++++++++++++++++++++++-----
>   block/block-backend.c                         |  6 ++
>   block/file-posix.c                            |  2 +-
>   block/io.c                                    |  1 +
>   hw/scsi/scsi-generic.c                        |  2 +-
>   storage-daemon/qemu-storage-daemon.c          |  2 +
>   tests/qemu-iotests/iotests.py                 |  2 -
>   tests/qemu-iotests/testenv.py                 | 15 +++--
>   tests/qemu-iotests/testrunner.py              |  7 ++-
>   tests/qemu-iotests/235                        |  2 -
>   tests/qemu-iotests/297                        | 52 +++++++-----------
>   tests/qemu-iotests/300                        |  5 +-
>   tests/qemu-iotests/pylintrc                   |  6 +-
>   tests/qemu-iotests/tests/image-fleecing       |  1 +
>   tests/qemu-iotests/tests/migrate-bitmaps-test | 50 +++++++++--------
>   tests/qemu-iotests/tests/mirror-top-perms     | 12 ++--
>   19 files changed, 164 insertions(+), 96 deletions(-)

Applied, thanks.

r~

[PULL 00/13] Block layer patches

[Kevin Wolf]

The following changes since commit 42f6c9179be4401974dd3a75ee72defd16b5092d:

  Merge tag 'pull-ppc-20211112' of https://github.com/legoater/qemu into staging (2021-11-12 12:28:25 +0100)

are available in the Git repository at:

  git://repo.or.cz/qemu/kevin.git tags/for-upstream

for you to fetch changes up to 7461272c5f6032436ef9032c091c0118539483e4:

  softmmu/qdev-monitor: fix use-after-free in qdev_set_id() (2021-11-15 15:49:46 +0100)

----------------------------------------------------------------
Block layer patches

- Fixes to image streaming job and block layer reconfiguration to make
  iotest 030 pass again
- docs: Deprecate incorrectly typed device_add arguments
- file-posix: Fix alignment after reopen changing O_DIRECT

----------------------------------------------------------------
Hanna Reitz (10):
      stream: Traverse graph after modification
      block: Manipulate children list in .attach/.detach
      block: Unite remove_empty_child and child_free
      block: Drop detached child from ignore list
      block: Pass BdrvChild ** to replace_child_noperm
      block: Restructure remove_file_or_backing_child()
      transactions: Invoke clean() after everything else
      block: Let replace_child_tran keep indirect pointer
      block: Let replace_child_noperm free children
      iotests/030: Unthrottle parallel jobs in reverse

Kevin Wolf (2):
      docs: Deprecate incorrectly typed device_add arguments
      file-posix: Fix alignment after reopen changing O_DIRECT

Stefan Hajnoczi (1):
      softmmu/qdev-monitor: fix use-after-free in qdev_set_id()

 docs/about/deprecated.rst   |  14 +++
 include/qemu/transactions.h |   3 +
 block.c                     | 233 +++++++++++++++++++++++++++++++++-----------
 block/file-posix.c          |  20 +++-
 block/stream.c              |   7 +-
 softmmu/qdev-monitor.c      |   2 +-
 util/transactions.c         |   8 +-
 tests/qemu-iotests/030      |  11 ++-
 tests/qemu-iotests/142      |  22 +++++
 tests/qemu-iotests/142.out  |  15 +++
 10 files changed, 269 insertions(+), 66 deletions(-)

Re: [PULL 00/13] Block layer patches

[Richard Henderson]

On 11/15/21 3:53 PM, Kevin Wolf wrote:
> The following changes since commit 42f6c9179be4401974dd3a75ee72defd16b5092d:
> 
>    Merge tag 'pull-ppc-20211112' of https://github.com/legoater/qemu into staging (2021-11-12 12:28:25 +0100)
> 
> are available in the Git repository at:
> 
>    git://repo.or.cz/qemu/kevin.git tags/for-upstream
> 
> for you to fetch changes up to 7461272c5f6032436ef9032c091c0118539483e4:
> 
>    softmmu/qdev-monitor: fix use-after-free in qdev_set_id() (2021-11-15 15:49:46 +0100)
> 
> ----------------------------------------------------------------
> Block layer patches
> 
> - Fixes to image streaming job and block layer reconfiguration to make
>    iotest 030 pass again
> - docs: Deprecate incorrectly typed device_add arguments
> - file-posix: Fix alignment after reopen changing O_DIRECT
> 
> ----------------------------------------------------------------
> Hanna Reitz (10):
>        stream: Traverse graph after modification
>        block: Manipulate children list in .attach/.detach
>        block: Unite remove_empty_child and child_free
>        block: Drop detached child from ignore list
>        block: Pass BdrvChild ** to replace_child_noperm
>        block: Restructure remove_file_or_backing_child()
>        transactions: Invoke clean() after everything else
>        block: Let replace_child_tran keep indirect pointer
>        block: Let replace_child_noperm free children
>        iotests/030: Unthrottle parallel jobs in reverse
> 
> Kevin Wolf (2):
>        docs: Deprecate incorrectly typed device_add arguments
>        file-posix: Fix alignment after reopen changing O_DIRECT
> 
> Stefan Hajnoczi (1):
>        softmmu/qdev-monitor: fix use-after-free in qdev_set_id()
> 
>   docs/about/deprecated.rst   |  14 +++
>   include/qemu/transactions.h |   3 +
>   block.c                     | 233 +++++++++++++++++++++++++++++++++-----------
>   block/file-posix.c          |  20 +++-
>   block/stream.c              |   7 +-
>   softmmu/qdev-monitor.c      |   2 +-
>   util/transactions.c         |   8 +-
>   tests/qemu-iotests/030      |  11 ++-
>   tests/qemu-iotests/142      |  22 +++++
>   tests/qemu-iotests/142.out  |  15 +++
>   10 files changed, 269 insertions(+), 66 deletions(-)

This is failing iotest 142 for build-tcg-disabled.
I did retry, in case it was transitory.

https://gitlab.com/qemu-project/qemu/-/jobs/1784955950


r~

Re: [PULL 00/13] Block layer patches

[Philippe Mathieu-Daudé]

Hi Kevin,

On 11/15/21 15:53, Kevin Wolf wrote:
> The following changes since commit 42f6c9179be4401974dd3a75ee72defd16b5092d:
> 
>   Merge tag 'pull-ppc-20211112' of https://github.com/legoater/qemu into staging (2021-11-12 12:28:25 +0100)
> 
> are available in the Git repository at:
> 
>   git://repo.or.cz/qemu/kevin.git tags/for-upstream
> 
> for you to fetch changes up to 7461272c5f6032436ef9032c091c0118539483e4:
> 
>   softmmu/qdev-monitor: fix use-after-free in qdev_set_id() (2021-11-15 15:49:46 +0100)
> 
> ----------------------------------------------------------------
> Block layer patches
> 
> - Fixes to image streaming job and block layer reconfiguration to make
>   iotest 030 pass again
> - docs: Deprecate incorrectly typed device_add arguments
> - file-posix: Fix alignment after reopen changing O_DIRECT
> 
> ----------------------------------------------------------------
> Hanna Reitz (10):
>       stream: Traverse graph after modification
>       block: Manipulate children list in .attach/.detach
>       block: Unite remove_empty_child and child_free
>       block: Drop detached child from ignore list
>       block: Pass BdrvChild ** to replace_child_noperm
>       block: Restructure remove_file_or_backing_child()
>       transactions: Invoke clean() after everything else
>       block: Let replace_child_tran keep indirect pointer
>       block: Let replace_child_noperm free children
>       iotests/030: Unthrottle parallel jobs in reverse
> 
> Kevin Wolf (2):
>       docs: Deprecate incorrectly typed device_add arguments
>       file-posix: Fix alignment after reopen changing O_DIRECT
> 
> Stefan Hajnoczi (1):
>       softmmu/qdev-monitor: fix use-after-free in qdev_set_id()
> 
>  docs/about/deprecated.rst   |  14 +++
>  include/qemu/transactions.h |   3 +
>  block.c                     | 233 +++++++++++++++++++++++++++++++++-----------
>  block/file-posix.c          |  20 +++-
>  block/stream.c              |   7 +-
>  softmmu/qdev-monitor.c      |   2 +-
>  util/transactions.c         |   8 +-
>  tests/qemu-iotests/030      |  11 ++-
>  tests/qemu-iotests/142      |  22 +++++
>  tests/qemu-iotests/142.out  |  15 +++
>  10 files changed, 269 insertions(+), 66 deletions(-)

Looking at current /staging I noticed iotest#142 failed,
build-tcg-disabled job:

+++ 142.out.bad
@@ -750,6 +750,7 @@
 --- Alignment after changing O_DIRECT ---
+qemu-io: Cannot get 'write' permission without 'resize': Image size is
not a multiple of request alignment
 read 42/42 bytes at offset 42
 42 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 read 42/42 bytes at offset 42

https://gitlab.com/qemu-project/qemu/-/jobs/1784955950#L2794

Re: [PULL 00/13] Block layer patches

[Hanna Reitz]

On 15.11.21 21:55, Richard Henderson wrote:
> On 11/15/21 3:53 PM, Kevin Wolf wrote:
>> The following changes since commit 
>> 42f6c9179be4401974dd3a75ee72defd16b5092d:
>>
>>    Merge tag 'pull-ppc-20211112' of https://github.com/legoater/qemu 
>> into staging (2021-11-12 12:28:25 +0100)
>>
>> are available in the Git repository at:
>>
>>    git://repo.or.cz/qemu/kevin.git tags/for-upstream
>>
>> for you to fetch changes up to 7461272c5f6032436ef9032c091c0118539483e4:
>>
>>    softmmu/qdev-monitor: fix use-after-free in qdev_set_id() 
>> (2021-11-15 15:49:46 +0100)
>>
>> ----------------------------------------------------------------
>> Block layer patches
>>
>> - Fixes to image streaming job and block layer reconfiguration to make
>>    iotest 030 pass again
>> - docs: Deprecate incorrectly typed device_add arguments
>> - file-posix: Fix alignment after reopen changing O_DIRECT
>>
>> ----------------------------------------------------------------
>> Hanna Reitz (10):
>>        stream: Traverse graph after modification
>>        block: Manipulate children list in .attach/.detach
>>        block: Unite remove_empty_child and child_free
>>        block: Drop detached child from ignore list
>>        block: Pass BdrvChild ** to replace_child_noperm
>>        block: Restructure remove_file_or_backing_child()
>>        transactions: Invoke clean() after everything else
>>        block: Let replace_child_tran keep indirect pointer
>>        block: Let replace_child_noperm free children
>>        iotests/030: Unthrottle parallel jobs in reverse
>>
>> Kevin Wolf (2):
>>        docs: Deprecate incorrectly typed device_add arguments
>>        file-posix: Fix alignment after reopen changing O_DIRECT
>>
>> Stefan Hajnoczi (1):
>>        softmmu/qdev-monitor: fix use-after-free in qdev_set_id()
>>
>>   docs/about/deprecated.rst   |  14 +++
>>   include/qemu/transactions.h |   3 +
>>   block.c                     | 233 
>> +++++++++++++++++++++++++++++++++-----------
>>   block/file-posix.c          |  20 +++-
>>   block/stream.c              |   7 +-
>>   softmmu/qdev-monitor.c      |   2 +-
>>   util/transactions.c         |   8 +-
>>   tests/qemu-iotests/030      |  11 ++-
>>   tests/qemu-iotests/142      |  22 +++++
>>   tests/qemu-iotests/142.out  |  15 +++
>>   10 files changed, 269 insertions(+), 66 deletions(-)
>
> This is failing iotest 142 for build-tcg-disabled.
> I did retry, in case it was transitory.
>
> https://gitlab.com/qemu-project/qemu/-/jobs/1784955950

Thanks, seems like a problem that appears on block devices with sector 
sizes greater than 512 bytes.  Since Kevin is on PTO, I’ll (try to) fix 
the test and send a v2.

Hanna

[PULL 00/13] Block layer patches

[Kevin Wolf]

The following changes since commit 2e3408b3cc7de4e87a9adafc8c19bfce3abec947:

  Merge tag 'misc-pull-request' of gitlab.com:marcandre.lureau/qemu into staging (2022-05-03 09:13:17 -0700)

are available in the Git repository at:

  git://repo.or.cz/qemu/kevin.git tags/for-upstream

for you to fetch changes up to c1fe694357a328c807ae3cc6961c19e923448fcc:

  coroutine-win32: use QEMU_DEFINE_STATIC_CO_TLS() (2022-05-04 15:55:23 +0200)

----------------------------------------------------------------
Block layer patches

- Fix and re-enable GLOBAL_STATE_CODE assertions
- vhost-user: Fixes for VHOST_USER_ADD/REM_MEM_REG
- vmdk: Fix reopening bs->file
- coroutine: use QEMU_DEFINE_STATIC_CO_TLS()
- docs/qemu-img: Fix list of formats which implement check

----------------------------------------------------------------
Denis V. Lunev (1):
      qemu-img: properly list formats which have consistency check implemented

Hanna Reitz (6):
      block: Classify bdrv_get_flags() as I/O function
      qcow2: Do not reopen data_file in invalidate_cache
      Revert "main-loop: Disable GLOBAL_STATE_CODE() assertions"
      iotests: Add regression test for issue 945
      block/vmdk: Fix reopening bs->file
      iotests/reopen-file: Test reopening file child

Kevin Wolf (3):
      docs/vhost-user: Clarifications for VHOST_USER_ADD/REM_MEM_REG
      libvhost-user: Fix extra vu_add/rem_mem_reg reply
      vhost-user: Don't pass file descriptor for VHOST_USER_REM_MEM_REG

Stefan Hajnoczi (3):
      coroutine-ucontext: use QEMU_DEFINE_STATIC_CO_TLS()
      coroutine: use QEMU_DEFINE_STATIC_CO_TLS()
      coroutine-win32: use QEMU_DEFINE_STATIC_CO_TLS()

 docs/interop/vhost-user.rst                        |  17 ++++
 docs/tools/qemu-img.rst                            |   4 +-
 include/block/block-global-state.h                 |   1 -
 include/block/block-io.h                           |   1 +
 include/qemu/main-loop.h                           |   3 +-
 block.c                                            |   2 +-
 block/qcow2.c                                      | 104 ++++++++++++---------
 block/vmdk.c                                       |  56 ++++++++++-
 hw/virtio/vhost-user.c                             |   2 +-
 subprojects/libvhost-user/libvhost-user.c          |  17 ++--
 util/coroutine-ucontext.c                          |  38 +++++---
 util/coroutine-win32.c                             |  18 +++-
 util/qemu-coroutine.c                              |  41 ++++----
 tests/qemu-iotests/tests/export-incoming-iothread  |  81 ++++++++++++++++
 .../tests/export-incoming-iothread.out             |   5 +
 tests/qemu-iotests/tests/reopen-file               |  89 ++++++++++++++++++
 tests/qemu-iotests/tests/reopen-file.out           |   5 +
 17 files changed, 388 insertions(+), 96 deletions(-)
 create mode 100755 tests/qemu-iotests/tests/export-incoming-iothread
 create mode 100644 tests/qemu-iotests/tests/export-incoming-iothread.out
 create mode 100755 tests/qemu-iotests/tests/reopen-file
 create mode 100644 tests/qemu-iotests/tests/reopen-file.out

Re: [PULL 00/13] Block layer patches

[Richard Henderson]

On 5/4/22 09:25, Kevin Wolf wrote:
> The following changes since commit 2e3408b3cc7de4e87a9adafc8c19bfce3abec947:
> 
>    Merge tag 'misc-pull-request' of gitlab.com:marcandre.lureau/qemu into staging (2022-05-03 09:13:17 -0700)
> 
> are available in the Git repository at:
> 
>    git://repo.or.cz/qemu/kevin.git tags/for-upstream
> 
> for you to fetch changes up to c1fe694357a328c807ae3cc6961c19e923448fcc:
> 
>    coroutine-win32: use QEMU_DEFINE_STATIC_CO_TLS() (2022-05-04 15:55:23 +0200)
> 
> ----------------------------------------------------------------
> Block layer patches
> 
> - Fix and re-enable GLOBAL_STATE_CODE assertions
> - vhost-user: Fixes for VHOST_USER_ADD/REM_MEM_REG
> - vmdk: Fix reopening bs->file
> - coroutine: use QEMU_DEFINE_STATIC_CO_TLS()
> - docs/qemu-img: Fix list of formats which implement check

Applied, thanks.  Please update https://wiki.qemu.org/ChangeLog/7.1 as appropriate.


r~


> 
> ----------------------------------------------------------------
> Denis V. Lunev (1):
>        qemu-img: properly list formats which have consistency check implemented
> 
> Hanna Reitz (6):
>        block: Classify bdrv_get_flags() as I/O function
>        qcow2: Do not reopen data_file in invalidate_cache
>        Revert "main-loop: Disable GLOBAL_STATE_CODE() assertions"
>        iotests: Add regression test for issue 945
>        block/vmdk: Fix reopening bs->file
>        iotests/reopen-file: Test reopening file child
> 
> Kevin Wolf (3):
>        docs/vhost-user: Clarifications for VHOST_USER_ADD/REM_MEM_REG
>        libvhost-user: Fix extra vu_add/rem_mem_reg reply
>        vhost-user: Don't pass file descriptor for VHOST_USER_REM_MEM_REG
> 
> Stefan Hajnoczi (3):
>        coroutine-ucontext: use QEMU_DEFINE_STATIC_CO_TLS()
>        coroutine: use QEMU_DEFINE_STATIC_CO_TLS()
>        coroutine-win32: use QEMU_DEFINE_STATIC_CO_TLS()
> 
>   docs/interop/vhost-user.rst                        |  17 ++++
>   docs/tools/qemu-img.rst                            |   4 +-
>   include/block/block-global-state.h                 |   1 -
>   include/block/block-io.h                           |   1 +
>   include/qemu/main-loop.h                           |   3 +-
>   block.c                                            |   2 +-
>   block/qcow2.c                                      | 104 ++++++++++++---------
>   block/vmdk.c                                       |  56 ++++++++++-
>   hw/virtio/vhost-user.c                             |   2 +-
>   subprojects/libvhost-user/libvhost-user.c          |  17 ++--
>   util/coroutine-ucontext.c                          |  38 +++++---
>   util/coroutine-win32.c                             |  18 +++-
>   util/qemu-coroutine.c                              |  41 ++++----
>   tests/qemu-iotests/tests/export-incoming-iothread  |  81 ++++++++++++++++
>   .../tests/export-incoming-iothread.out             |   5 +
>   tests/qemu-iotests/tests/reopen-file               |  89 ++++++++++++++++++
>   tests/qemu-iotests/tests/reopen-file.out           |   5 +
>   17 files changed, 388 insertions(+), 96 deletions(-)
>   create mode 100755 tests/qemu-iotests/tests/export-incoming-iothread
>   create mode 100644 tests/qemu-iotests/tests/export-incoming-iothread.out
>   create mode 100755 tests/qemu-iotests/tests/reopen-file
>   create mode 100644 tests/qemu-iotests/tests/reopen-file.out
> 
> 

[PULL 00/13] Block layer patches

[Kevin Wolf]

The following changes since commit f9851d2ffef59b3a7f39513469263ab3b019480f:

  Merge tag 'migration-20240802-pull-request' of https://gitlab.com/farosas/qemu into staging (2024-08-03 07:26:26 +1000)

are available in the Git repository at:

  https://repo.or.cz/qemu/kevin.git tags/for-upstream

for you to fetch changes up to 833362e55d9acc6b7ccc1d80d8cd84688e7f5761:

  iotests/024: exclude 'backing file format' field from the output (2024-08-05 23:00:42 +0200)

----------------------------------------------------------------
Block layer patches

- scsi-block: Fix error handling with r/werror=stop
- Depend on newer clang for TSA, make WITH_GRAPH_RDLOCK_GUARD() fully
  checked, fix block-copy to add missing lock
- vvfat: Fix write bugs for large files and add iotests
- Clean up blockdev-snapshot-internal-sync doc
- Fix iotests 024 for qed

----------------------------------------------------------------
Amjad Alsharafi (5):
      vvfat: Fix bug in writing to middle of file
      vvfat: Fix usage of `info.file.offset`
      vvfat: Fix wrong checks for cluster mappings invariant
      vvfat: Fix reading files with non-continuous clusters
      iotests: Add `vvfat` tests

Andrey Drobyshev (1):
      iotests/024: exclude 'backing file format' field from the output

Kevin Wolf (6):
      block-copy: Fix missing graph lock
      block/graph-lock: Make WITH_GRAPH_RDLOCK_GUARD() fully checked
      scsi-disk: Use positive return value for status in dma_readv/writev
      scsi-block: Don't skip callback for sgio error status/driver_status
      scsi-disk: Add warning comments that host_status errors take a shortcut
      scsi-disk: Always report RESERVATION_CONFLICT to guest

Markus Armbruster (1):
      qapi-block-core: Clean up blockdev-snapshot-internal-sync doc

 qapi/block-core.json               |   7 +-
 include/block/graph-lock.h         |  21 +-
 block/block-copy.c                 |   4 +-
 block/vvfat.c                      |  27 +-
 hw/scsi/scsi-disk.c                |  73 ++--
 tests/qemu-iotests/fat16.py        | 690 +++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/testenv.py      |   2 +-
 meson.build                        |  14 +-
 tests/qemu-iotests/024             |   2 +-
 tests/qemu-iotests/024.out         |   1 -
 tests/qemu-iotests/check           |   2 +-
 tests/qemu-iotests/tests/vvfat     | 485 ++++++++++++++++++++++++++
 tests/qemu-iotests/tests/vvfat.out |   5 +
 13 files changed, 1280 insertions(+), 53 deletions(-)
 create mode 100644 tests/qemu-iotests/fat16.py
 create mode 100755 tests/qemu-iotests/tests/vvfat
 create mode 100755 tests/qemu-iotests/tests/vvfat.out

[PULL 01/13] qapi-block-core: Clean up blockdev-snapshot-internal-sync doc

[Kevin Wolf]

From: Markus Armbruster <armbru@redhat.com>

BlockdevSnapshotInternal is the arguments type of command
blockdev-snapshot-internal-sync.  Its doc comment contains this note:

    # .. note:: In a transaction, if @name is empty or any snapshot matching
    #    @name exists, the operation will fail.  Only some image formats
    #    support it; for example, qcow2, and rbd.

"In a transaction" is misleading, and "if @name is empty or any
snapshot matching @name exists, the operation will fail" is redundant
with the command's Errors documentation.  Drop.

The remainder is fine.  Move it to the command's doc comment, where it
is more prominently visible, with a slight rephrasing for clarity.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Message-ID: <20240718123609.3063055-1-armbru@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qapi/block-core.json | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index f400b334c8..994e384a71 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -6046,10 +6046,6 @@
 #
 # @name: the name of the internal snapshot to be created
 #
-# .. note:: In a transaction, if @name is empty or any snapshot matching
-#    @name exists, the operation will fail.  Only some image formats
-#    support it; for example, qcow2, and rbd.
-#
 # Since: 1.7
 ##
 { 'struct': 'BlockdevSnapshotInternal',
@@ -6070,6 +6066,9 @@
 #     - If the format of the image used does not support it,
 #       GenericError
 #
+# .. note:: Only some image formats such as qcow2 and rbd support
+#    internal snapshots.
+#
 # Since: 1.7
 #
 # .. qmp-example::
-- 
2.45.2

[PULL 02/13] block-copy: Fix missing graph lock

[Kevin Wolf]

The graph lock needs to be held when calling bdrv_co_pdiscard(). Fix
block_copy_task_entry() to take it for the call.

WITH_GRAPH_RDLOCK_GUARD() was implemented in a weak way because of
limitations in clang's Thread Safety Analysis at the time, so that it
only asserts that the lock is held (which allows calling functions that
require the lock), but we never deal with the unlocking (so even after
the scope of the guard, the compiler assumes that the lock is still
held). This is why the compiler didn't catch this locking error.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Message-ID: <20240627181245.281403-2-kwolf@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/block-copy.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/block/block-copy.c b/block/block-copy.c
index 7e3b378528..cc618e4561 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -595,7 +595,9 @@ static coroutine_fn int block_copy_task_entry(AioTask *task)
     if (s->discard_source && ret == 0) {
         int64_t nbytes =
             MIN(t->req.offset + t->req.bytes, s->len) - t->req.offset;
-        bdrv_co_pdiscard(s->source, t->req.offset, nbytes);
+        WITH_GRAPH_RDLOCK_GUARD() {
+            bdrv_co_pdiscard(s->source, t->req.offset, nbytes);
+        }
     }
 
     return ret;
-- 
2.45.2

[PULL 03/13] block/graph-lock: Make WITH_GRAPH_RDLOCK_GUARD() fully checked

[Kevin Wolf]

Upstream clang 18 (and backports to clang 17 in Fedora and RHEL)
implemented support for __attribute__((cleanup())) in its Thread Safety
Analysis, so we can now actually have a proper implementation of
WITH_GRAPH_RDLOCK_GUARD() that understands when we acquire and when we
release the lock.

-Wthread-safety is now only enabled if the compiler is new enough to
understand this pattern. In theory, we could have used some #ifdefs to
keep the existing basic checks on old compilers, but as long as someone
runs a newer compiler (and our CI does), we will catch locking problems,
so it's probably not worth keeping multiple implementations for this.

The implementation can't use g_autoptr any more because the glib macros
define wrapper functions that don't have the right TSA attributes, so
the compiler would complain about them. Just use the cleanup attribute
directly instead.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Message-ID: <20240627181245.281403-3-kwolf@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/block/graph-lock.h | 21 ++++++++++++++-------
 meson.build                | 14 +++++++++++++-
 2 files changed, 27 insertions(+), 8 deletions(-)

diff --git a/include/block/graph-lock.h b/include/block/graph-lock.h
index d7545e82d0..dc8d949184 100644
--- a/include/block/graph-lock.h
+++ b/include/block/graph-lock.h
@@ -209,31 +209,38 @@ typedef struct GraphLockable { } GraphLockable;
  * unlocked. TSA_ASSERT_SHARED() makes sure that the following calls know that
  * we hold the lock while unlocking is left unchecked.
  */
-static inline GraphLockable * TSA_ASSERT_SHARED(graph_lock) TSA_NO_TSA coroutine_fn
+static inline GraphLockable * TSA_ACQUIRE_SHARED(graph_lock) coroutine_fn
 graph_lockable_auto_lock(GraphLockable *x)
 {
     bdrv_graph_co_rdlock();
     return x;
 }
 
-static inline void TSA_NO_TSA coroutine_fn
-graph_lockable_auto_unlock(GraphLockable *x)
+static inline void TSA_RELEASE_SHARED(graph_lock) coroutine_fn
+graph_lockable_auto_unlock(GraphLockable **x)
 {
     bdrv_graph_co_rdunlock();
 }
 
-G_DEFINE_AUTOPTR_CLEANUP_FUNC(GraphLockable, graph_lockable_auto_unlock)
+#define GRAPH_AUTO_UNLOCK __attribute__((cleanup(graph_lockable_auto_unlock)))
 
+/*
+ * @var is only used to break the loop after the first iteration.
+ * @unlock_var can't be unlocked and then set to NULL because TSA wants the lock
+ * to be held at the start of every iteration of the loop.
+ */
 #define WITH_GRAPH_RDLOCK_GUARD_(var)                                         \
-    for (g_autoptr(GraphLockable) var = graph_lockable_auto_lock(GML_OBJ_()); \
+    for (GraphLockable *unlock_var GRAPH_AUTO_UNLOCK =                        \
+            graph_lockable_auto_lock(GML_OBJ_()),                             \
+            *var = unlock_var;                                                \
          var;                                                                 \
-         graph_lockable_auto_unlock(var), var = NULL)
+         var = NULL)
 
 #define WITH_GRAPH_RDLOCK_GUARD() \
     WITH_GRAPH_RDLOCK_GUARD_(glue(graph_lockable_auto, __COUNTER__))
 
 #define GRAPH_RDLOCK_GUARD(x)                                       \
-    g_autoptr(GraphLockable)                                        \
+    GraphLockable * GRAPH_AUTO_UNLOCK                               \
     glue(graph_lockable_auto, __COUNTER__) G_GNUC_UNUSED =          \
             graph_lockable_auto_lock(GML_OBJ_())
 
diff --git a/meson.build b/meson.build
index 97f63aa86c..c2a050b844 100644
--- a/meson.build
+++ b/meson.build
@@ -649,7 +649,19 @@ warn_flags = [
 ]
 
 if host_os != 'darwin'
-  warn_flags += ['-Wthread-safety']
+  tsa_has_cleanup = cc.compiles('''
+    struct __attribute__((capability("mutex"))) mutex {};
+    void lock(struct mutex *m) __attribute__((acquire_capability(m)));
+    void unlock(struct mutex *m) __attribute__((release_capability(m)));
+
+    void test(void) {
+      struct mutex __attribute__((cleanup(unlock))) m;
+      lock(&m);
+    }
+  ''', args: ['-Wthread-safety', '-Werror'])
+  if tsa_has_cleanup
+    warn_flags += ['-Wthread-safety']
+  endif
 endif
 
 # Set up C++ compiler flags
-- 
2.45.2

[PULL 04/13] scsi-disk: Use positive return value for status in dma_readv/writev

[Kevin Wolf]

In some error cases, scsi_block_sgio_complete() never calls the passed
callback, but directly completes the request. This leads to bugs because
its error paths are not exact copies of what the callback would normally
do.

In preparation to fix this, allow passing positive return values to the
callbacks that represent the status code that should be used to complete
the request.

scsi_handle_rw_error() already handles positive values for its ret
parameter because scsi_block_sgio_complete() calls directly into it.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Acked-by: Paolo Bonzini <pbonzini@redhat.com>
Message-ID: <20240731123207.27636-2-kwolf@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 hw/scsi/scsi-disk.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index a67092db6a..3ff6798bde 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -65,6 +65,10 @@ OBJECT_DECLARE_TYPE(SCSIDiskState, SCSIDiskClass, SCSI_DISK_BASE)
 
 struct SCSIDiskClass {
     SCSIDeviceClass parent_class;
+    /*
+     * Callbacks receive ret == 0 for success. Errors are represented either as
+     * negative errno values, or as positive SAM status codes.
+     */
     DMAIOFunc       *dma_readv;
     DMAIOFunc       *dma_writev;
     bool            (*need_fua_emulation)(SCSICommand *cmd);
@@ -283,7 +287,7 @@ static bool scsi_disk_req_check_error(SCSIDiskReq *r, int ret, bool acct_failed)
         return true;
     }
 
-    if (ret < 0) {
+    if (ret != 0) {
         return scsi_handle_rw_error(r, ret, acct_failed);
     }
 
@@ -360,7 +364,7 @@ static void scsi_write_do_fua(SCSIDiskReq *r)
 static void scsi_dma_complete_noio(SCSIDiskReq *r, int ret)
 {
     assert(r->req.aiocb == NULL);
-    if (scsi_disk_req_check_error(r, ret, false)) {
+    if (scsi_disk_req_check_error(r, ret, ret > 0)) {
         goto done;
     }
 
@@ -385,9 +389,10 @@ static void scsi_dma_complete(void *opaque, int ret)
     assert(r->req.aiocb != NULL);
     r->req.aiocb = NULL;
 
+    /* ret > 0 is accounted for in scsi_disk_req_check_error() */
     if (ret < 0) {
         block_acct_failed(blk_get_stats(s->qdev.conf.blk), &r->acct);
-    } else {
+    } else if (ret == 0) {
         block_acct_done(blk_get_stats(s->qdev.conf.blk), &r->acct);
     }
     scsi_dma_complete_noio(r, ret);
@@ -403,7 +408,7 @@ static void scsi_read_complete_noio(SCSIDiskReq *r, int ret)
            qemu_get_current_aio_context());
 
     assert(r->req.aiocb == NULL);
-    if (scsi_disk_req_check_error(r, ret, false)) {
+    if (scsi_disk_req_check_error(r, ret, ret > 0)) {
         goto done;
     }
 
@@ -424,9 +429,10 @@ static void scsi_read_complete(void *opaque, int ret)
     assert(r->req.aiocb != NULL);
     r->req.aiocb = NULL;
 
+    /* ret > 0 is accounted for in scsi_disk_req_check_error() */
     if (ret < 0) {
         block_acct_failed(blk_get_stats(s->qdev.conf.blk), &r->acct);
-    } else {
+    } else if (ret == 0) {
         block_acct_done(blk_get_stats(s->qdev.conf.blk), &r->acct);
         trace_scsi_disk_read_complete(r->req.tag, r->qiov.size);
     }
@@ -534,7 +540,7 @@ static void scsi_write_complete_noio(SCSIDiskReq *r, int ret)
            qemu_get_current_aio_context());
 
     assert (r->req.aiocb == NULL);
-    if (scsi_disk_req_check_error(r, ret, false)) {
+    if (scsi_disk_req_check_error(r, ret, ret > 0)) {
         goto done;
     }
 
@@ -562,9 +568,10 @@ static void scsi_write_complete(void * opaque, int ret)
     assert (r->req.aiocb != NULL);
     r->req.aiocb = NULL;
 
+    /* ret > 0 is accounted for in scsi_disk_req_check_error() */
     if (ret < 0) {
         block_acct_failed(blk_get_stats(s->qdev.conf.blk), &r->acct);
-    } else {
+    } else if (ret == 0) {
         block_acct_done(blk_get_stats(s->qdev.conf.blk), &r->acct);
     }
     scsi_write_complete_noio(r, ret);
-- 
2.45.2

[PULL 05/13] scsi-block: Don't skip callback for sgio error status/driver_status

[Kevin Wolf]

Instead of calling into scsi_handle_rw_error() directly from
scsi_block_sgio_complete() and skipping the normal callback, go through
the normal cleanup path by calling the callback with a positive error
value.

The important difference here is not only that the code path is cleaner,
but that the callbacks set r->req.aiocb = NULL. If we skip setting this
and the error action is BLOCK_ERROR_ACTION_STOP, resuming the VM runs
into an assertion failure in scsi_read_data() or scsi_write_data()
because the dangling aiocb pointer is unexpected.

Fixes: a108557bbf ("scsi: inline sg_io_sense_from_errno() into the callers.")
Buglink: https://issues.redhat.com/browse/RHEL-50000
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Acked-by: Paolo Bonzini <pbonzini@redhat.com>
Message-ID: <20240731123207.27636-3-kwolf@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 hw/scsi/scsi-disk.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 3ff6798bde..6e1a5c98df 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -2832,16 +2832,6 @@ static void scsi_block_sgio_complete(void *opaque, int ret)
         } else {
             ret = io_hdr->status;
         }
-
-        if (ret > 0) {
-            if (scsi_handle_rw_error(r, ret, true)) {
-                scsi_req_unref(&r->req);
-                return;
-            }
-
-            /* Ignore error.  */
-            ret = 0;
-        }
     }
 
     req->cb(req->cb_opaque, ret);
-- 
2.45.2

[PULL 06/13] scsi-disk: Add warning comments that host_status errors take a shortcut

[Kevin Wolf]

scsi_block_sgio_complete() has surprising behaviour in that there are
error cases in which it directly completes the request and never calls
the passed callback. In the current state of the code, this doesn't seem
to result in bugs, but with future code changes, we must be careful to
never rely on the callback doing some cleanup until this code smell is
fixed. For now, just add warnings to make people aware of the trap.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Acked-by: Paolo Bonzini <pbonzini@redhat.com>
Message-ID: <20240731123207.27636-4-kwolf@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 hw/scsi/scsi-disk.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 6e1a5c98df..69a195177e 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -68,6 +68,9 @@ struct SCSIDiskClass {
     /*
      * Callbacks receive ret == 0 for success. Errors are represented either as
      * negative errno values, or as positive SAM status codes.
+     *
+     * Beware: For errors returned in host_status, the function may directly
+     * complete the request and never call the callback.
      */
     DMAIOFunc       *dma_readv;
     DMAIOFunc       *dma_writev;
@@ -381,6 +384,7 @@ done:
     scsi_req_unref(&r->req);
 }
 
+/* May not be called in all error cases, don't rely on cleanup here */
 static void scsi_dma_complete(void *opaque, int ret)
 {
     SCSIDiskReq *r = (SCSIDiskReq *)opaque;
@@ -421,6 +425,7 @@ done:
     scsi_req_unref(&r->req);
 }
 
+/* May not be called in all error cases, don't rely on cleanup here */
 static void scsi_read_complete(void *opaque, int ret)
 {
     SCSIDiskReq *r = (SCSIDiskReq *)opaque;
@@ -560,6 +565,7 @@ done:
     scsi_req_unref(&r->req);
 }
 
+/* May not be called in all error cases, don't rely on cleanup here */
 static void scsi_write_complete(void * opaque, int ret)
 {
     SCSIDiskReq *r = (SCSIDiskReq *)opaque;
@@ -2821,6 +2827,7 @@ static void scsi_block_sgio_complete(void *opaque, int ret)
     sg_io_hdr_t *io_hdr = &req->io_header;
 
     if (ret == 0) {
+        /* FIXME This skips calling req->cb() and any cleanup in it */
         if (io_hdr->host_status != SCSI_HOST_OK) {
             scsi_req_complete_failed(&r->req, io_hdr->host_status);
             scsi_req_unref(&r->req);
-- 
2.45.2

[PULL 07/13] scsi-disk: Always report RESERVATION_CONFLICT to guest

[Kevin Wolf]

In the case of scsi-block, RESERVATION_CONFLICT is not a backend error,
but indicates that the guest tried to make a request that it isn't
allowed to execute. Pass the error to the guest so that it can decide
what to do with it.

Without this, if we stop the VM in response to a RESERVATION_CONFLICT
(as is the default policy in management software such as oVirt or
KubeVirt), it can happen that the VM cannot be resumed any more because
every attempt to resume it immediately runs into the same error and
stops the VM again.

One case that expects RESERVATION_CONFLICT errors to be visible in the
guest is running the validation tests in Windows 2019's Failover Cluster
Manager, which intentionally tries to execute invalid requests to see if
they are properly rejected.

Buglink: https://issues.redhat.com/browse/RHEL-50000
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Acked-by: Paolo Bonzini <pbonzini@redhat.com>
Message-ID: <20240731123207.27636-5-kwolf@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 hw/scsi/scsi-disk.c | 35 ++++++++++++++++++++++++++++++-----
 1 file changed, 30 insertions(+), 5 deletions(-)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 69a195177e..4d94b2b816 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -224,7 +224,7 @@ static bool scsi_handle_rw_error(SCSIDiskReq *r, int ret, bool acct_failed)
     SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
     SCSIDiskClass *sdc = (SCSIDiskClass *) object_get_class(OBJECT(s));
     SCSISense sense = SENSE_CODE(NO_SENSE);
-    int error = 0;
+    int error;
     bool req_has_sense = false;
     BlockErrorAction action;
     int status;
@@ -235,11 +235,35 @@ static bool scsi_handle_rw_error(SCSIDiskReq *r, int ret, bool acct_failed)
     } else {
         /* A passthrough command has completed with nonzero status.  */
         status = ret;
-        if (status == CHECK_CONDITION) {
+        switch (status) {
+        case CHECK_CONDITION:
             req_has_sense = true;
             error = scsi_sense_buf_to_errno(r->req.sense, sizeof(r->req.sense));
-        } else {
+            break;
+        case RESERVATION_CONFLICT:
+            /*
+             * Don't apply the error policy, always report to the guest.
+             *
+             * This is a passthrough code path, so it's not a backend error, but
+             * a response to an invalid guest request.
+             *
+             * Windows Failover Cluster validation intentionally sends invalid
+             * requests to verify that reservations work as intended. It is
+             * crucial that it sees the resulting errors.
+             *
+             * Treating a reservation conflict as a guest-side error is obvious
+             * when a pr-manager is in use. Without one, the situation is less
+             * clear, but there might be nothing that can be fixed on the host
+             * (like in the above example), and we don't want to be stuck in a
+             * loop where resuming the VM and retrying the request immediately
+             * stops it again. So always reporting is still the safer option in
+             * this case, too.
+             */
+            error = 0;
+            break;
+        default:
             error = EINVAL;
+            break;
         }
     }
 
@@ -249,8 +273,9 @@ static bool scsi_handle_rw_error(SCSIDiskReq *r, int ret, bool acct_failed)
      * are usually retried immediately, so do not post them to QMP and
      * do not account them as failed I/O.
      */
-    if (req_has_sense &&
-        scsi_sense_buf_is_guest_recoverable(r->req.sense, sizeof(r->req.sense))) {
+    if (!error || (req_has_sense &&
+                   scsi_sense_buf_is_guest_recoverable(r->req.sense,
+                                                       sizeof(r->req.sense)))) {
         action = BLOCK_ERROR_ACTION_REPORT;
         acct_failed = false;
     } else {
-- 
2.45.2

[PULL 08/13] vvfat: Fix bug in writing to middle of file

[Kevin Wolf]

From: Amjad Alsharafi <amjadsharafi10@gmail.com>

Before this commit, the behavior when calling `commit_one_file` for
example with `offset=0x2000` (second cluster), what will happen is that
we won't fetch the next cluster from the fat, and instead use the first
cluster for the read operation.

This is due to off-by-one error here, where `i=0x2000 !< offset=0x2000`,
thus not fetching the next cluster.

Signed-off-by: Amjad Alsharafi <amjadsharafi10@gmail.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Tested-by: Kevin Wolf <kwolf@redhat.com>
Message-ID: <b97c1e1f1bc2f776061ae914f95d799d124fcd73.1721470238.git.amjadsharafi10@gmail.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/vvfat.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index 086fedf474..a67cfa823b 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -2525,8 +2525,9 @@ commit_one_file(BDRVVVFATState* s, int dir_index, uint32_t offset)
         return -1;
     }
 
-    for (i = s->cluster_size; i < offset; i += s->cluster_size)
+    for (i = 0; i < offset; i += s->cluster_size) {
         c = modified_fat_get(s, c);
+    }
 
     fd = qemu_open_old(mapping->path, O_RDWR | O_CREAT | O_BINARY, 0666);
     if (fd < 0) {
-- 
2.45.2

[PULL 09/13] vvfat: Fix usage of `info.file.offset`

[Kevin Wolf]

From: Amjad Alsharafi <amjadsharafi10@gmail.com>

The field is marked as "the offset in the file (in clusters)", but it
was being used like this
`cluster_size*(nums)+mapping->info.file.offset`, which is incorrect.

Signed-off-by: Amjad Alsharafi <amjadsharafi10@gmail.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Message-ID: <72f19a7903886dda1aa78bcae0e17702ee939262.1721470238.git.amjadsharafi10@gmail.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/vvfat.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index a67cfa823b..cfde468c2e 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -1408,7 +1408,9 @@ read_cluster_directory:
 
         assert(s->current_fd);
 
-        offset=s->cluster_size*(cluster_num-s->current_mapping->begin)+s->current_mapping->info.file.offset;
+        offset = s->cluster_size *
+            ((cluster_num - s->current_mapping->begin)
+            + s->current_mapping->info.file.offset);
         if(lseek(s->current_fd, offset, SEEK_SET)!=offset)
             return -3;
         s->cluster=s->cluster_buffer;
@@ -1929,8 +1931,9 @@ get_cluster_count_for_direntry(BDRVVVFATState* s, direntry_t* direntry, const ch
                         (mapping->mode & MODE_DIRECTORY) == 0) {
 
                     /* was modified in qcow */
-                    if (offset != mapping->info.file.offset + s->cluster_size
-                            * (cluster_num - mapping->begin)) {
+                    if (offset != s->cluster_size
+                            * ((cluster_num - mapping->begin)
+                            + mapping->info.file.offset)) {
                         /* offset of this cluster in file chain has changed */
                         abort();
                         copy_it = 1;
@@ -2404,7 +2407,7 @@ static int commit_mappings(BDRVVVFATState* s,
                         (mapping->end - mapping->begin);
             } else
                 next_mapping->info.file.offset = mapping->info.file.offset +
-                        mapping->end - mapping->begin;
+                        (mapping->end - mapping->begin);
 
             mapping = next_mapping;
         }
-- 
2.45.2

[PULL 10/13] vvfat: Fix wrong checks for cluster mappings invariant

[Kevin Wolf]

From: Amjad Alsharafi <amjadsharafi10@gmail.com>

How this `abort` was intended to check for was:
- if the `mapping->first_mapping_index` is not the same as
  `first_mapping_index`, which **should** happen only in one case,
  when we are handling the first mapping, in that case
  `mapping->first_mapping_index == -1`, in all other cases, the other
  mappings after the first should have the condition `true`.
- From above, we know that this is the first mapping, so if the offset
  is not `0`, then abort, since this is an invalid state.

The issue was that `first_mapping_index` is not set if we are
checking from the middle, the variable `first_mapping_index` is
only set if we passed through the check `cluster_was_modified` with the
first mapping, and in the same function call we checked the other
mappings.

One approach is to go into the loop even if `cluster_was_modified`
is not true so that we will be able to set `first_mapping_index` for the
first mapping, but since `first_mapping_index` is only used here,
another approach is to just check manually for the
`mapping->first_mapping_index != -1` since we know that this is the
value for the only entry where `offset == 0` (i.e. first mapping).

Signed-off-by: Amjad Alsharafi <amjadsharafi10@gmail.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Message-ID: <b0fbca3ee208c565885838f6a7deeaeb23f4f9c2.1721470238.git.amjadsharafi10@gmail.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/vvfat.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index cfde468c2e..e3a83fbc88 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -1880,7 +1880,6 @@ get_cluster_count_for_direntry(BDRVVVFATState* s, direntry_t* direntry, const ch
 
     uint32_t cluster_num = begin_of_direntry(direntry);
     uint32_t offset = 0;
-    int first_mapping_index = -1;
     mapping_t* mapping = NULL;
     const char* basename2 = NULL;
 
@@ -1942,14 +1941,9 @@ get_cluster_count_for_direntry(BDRVVVFATState* s, direntry_t* direntry, const ch
 
                         if (strcmp(basename, basename2))
                             copy_it = 1;
-                        first_mapping_index = array_index(&(s->mapping), mapping);
-                    }
-
-                    if (mapping->first_mapping_index != first_mapping_index
-                            && mapping->info.file.offset > 0) {
-                        abort();
-                        copy_it = 1;
                     }
+                    assert(mapping->first_mapping_index == -1
+                            || mapping->info.file.offset > 0);
 
                     /* need to write out? */
                     if (!was_modified && is_file(direntry)) {
-- 
2.45.2

[PULL 11/13] vvfat: Fix reading files with non-continuous clusters

[Kevin Wolf]

From: Amjad Alsharafi <amjadsharafi10@gmail.com>

When reading with `read_cluster` we get the `mapping` with
`find_mapping_for_cluster` and then we call `open_file` for this
mapping.
The issue appear when its the same file, but a second cluster that is
not immediately after it, imagine clusters `500 -> 503`, this will give
us 2 mappings one has the range `500..501` and another `503..504`, both
point to the same file, but different offsets.

When we don't open the file since the path is the same, we won't assign
`s->current_mapping` and thus accessing way out of bound of the file.

From our example above, after `open_file` (that didn't open anything) we
will get the offset into the file with
`s->cluster_size*(cluster_num-s->current_mapping->begin)`, which will
give us `0x2000 * (504-500)`, which is out of bound for this mapping and
will produce some issues.

Signed-off-by: Amjad Alsharafi <amjadsharafi10@gmail.com>
Message-ID: <1f3ea115779abab62ba32c788073cdc99f9ad5dd.1721470238.git.amjadsharafi10@gmail.com>
[kwolf: Simplified the patch based on Amjad's analysis and input]
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/vvfat.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index e3a83fbc88..8ffe8b3b9b 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -1369,8 +1369,9 @@ static int open_file(BDRVVVFATState* s,mapping_t* mapping)
             return -1;
         vvfat_close_current_file(s);
         s->current_fd = fd;
-        s->current_mapping = mapping;
     }
+
+    s->current_mapping = mapping;
     return 0;
 }
 
-- 
2.45.2

[PULL 12/13] iotests: Add `vvfat` tests

[Kevin Wolf]

From: Amjad Alsharafi <amjadsharafi10@gmail.com>

Added several tests to verify the implementation of the vvfat driver.

We needed a way to interact with it, so created a basic `fat16.py` driver
that handled writing correct sectors for us.

Added `vvfat` to the non-generic formats, as its not a normal image format.

Signed-off-by: Amjad Alsharafi <amjadsharafi10@gmail.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Tested-by: Kevin Wolf <kwolf@redhat.com>
Message-ID: <bb8149c945301aefbdf470a0924c07f69f9c087d.1721470238.git.amjadsharafi10@gmail.com>
[kwolf: Made mypy and pylint happy to unbreak 297]
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/fat16.py        | 690 +++++++++++++++++++++++++++++
 tests/qemu-iotests/testenv.py      |   2 +-
 tests/qemu-iotests/check           |   2 +-
 tests/qemu-iotests/tests/vvfat     | 485 ++++++++++++++++++++
 tests/qemu-iotests/tests/vvfat.out |   5 +
 5 files changed, 1182 insertions(+), 2 deletions(-)
 create mode 100644 tests/qemu-iotests/fat16.py
 create mode 100755 tests/qemu-iotests/tests/vvfat
 create mode 100755 tests/qemu-iotests/tests/vvfat.out

diff --git a/tests/qemu-iotests/fat16.py b/tests/qemu-iotests/fat16.py
new file mode 100644
index 0000000000..2c3c1cbb3d
--- /dev/null
+++ b/tests/qemu-iotests/fat16.py
@@ -0,0 +1,690 @@
+# A simple FAT16 driver that is used to test the `vvfat` driver in QEMU.
+#
+# Copyright (C) 2024 Amjad Alsharafi <amjadsharafi10@gmail.com>
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+from typing import Callable, List, Optional, Protocol, Set
+import string
+
+SECTOR_SIZE = 512
+DIRENTRY_SIZE = 32
+ALLOWED_FILE_CHARS = set(
+    "!#$%&'()-@^_`{}~" + string.digits + string.ascii_uppercase
+)
+
+
+class MBR:
+    def __init__(self, data: bytes):
+        assert len(data) == 512
+        self.partition_table = []
+        for i in range(4):
+            partition = data[446 + i * 16 : 446 + (i + 1) * 16]
+            self.partition_table.append(
+                {
+                    "status": partition[0],
+                    "start_head": partition[1],
+                    "start_sector": partition[2] & 0x3F,
+                    "start_cylinder": ((partition[2] & 0xC0) << 2)
+                                      | partition[3],
+                    "type": partition[4],
+                    "end_head": partition[5],
+                    "end_sector": partition[6] & 0x3F,
+                    "end_cylinder": ((partition[6] & 0xC0) << 2)
+                                    | partition[7],
+                    "start_lba": int.from_bytes(partition[8:12], "little"),
+                    "size": int.from_bytes(partition[12:16], "little"),
+                }
+            )
+
+    def __str__(self):
+        return "\n".join(
+            [
+                f"{i}: {partition}"
+                for i, partition in enumerate(self.partition_table)
+            ]
+        )
+
+
+class FatBootSector:
+    # pylint: disable=too-many-instance-attributes
+    def __init__(self, data: bytes):
+        assert len(data) == 512
+        self.bytes_per_sector = int.from_bytes(data[11:13], "little")
+        self.sectors_per_cluster = data[13]
+        self.reserved_sectors = int.from_bytes(data[14:16], "little")
+        self.fat_count = data[16]
+        self.root_entries = int.from_bytes(data[17:19], "little")
+        total_sectors_16 = int.from_bytes(data[19:21], "little")
+        self.media_descriptor = data[21]
+        self.sectors_per_fat = int.from_bytes(data[22:24], "little")
+        self.sectors_per_track = int.from_bytes(data[24:26], "little")
+        self.heads = int.from_bytes(data[26:28], "little")
+        self.hidden_sectors = int.from_bytes(data[28:32], "little")
+        total_sectors_32 = int.from_bytes(data[32:36], "little")
+        assert (
+            total_sectors_16 == 0 or total_sectors_32 == 0
+        ), "Both total sectors (16 and 32) fields are non-zero"
+        self.total_sectors = total_sectors_16 or total_sectors_32
+        self.drive_number = data[36]
+        self.volume_id = int.from_bytes(data[39:43], "little")
+        self.volume_label = data[43:54].decode("ascii").strip()
+        self.fs_type = data[54:62].decode("ascii").strip()
+
+    def root_dir_start(self):
+        """
+        Calculate the start sector of the root directory.
+        """
+        return self.reserved_sectors + self.fat_count * self.sectors_per_fat
+
+    def root_dir_size(self):
+        """
+        Calculate the size of the root directory in sectors.
+        """
+        return (
+            self.root_entries * DIRENTRY_SIZE + self.bytes_per_sector - 1
+        ) // self.bytes_per_sector
+
+    def data_sector_start(self):
+        """
+        Calculate the start sector of the data region.
+        """
+        return self.root_dir_start() + self.root_dir_size()
+
+    def first_sector_of_cluster(self, cluster: int) -> int:
+        """
+        Calculate the first sector of the given cluster.
+        """
+        return (
+            self.data_sector_start() + (cluster - 2) * self.sectors_per_cluster
+        )
+
+    def cluster_bytes(self):
+        """
+        Calculate the number of bytes in a cluster.
+        """
+        return self.bytes_per_sector * self.sectors_per_cluster
+
+    def __str__(self):
+        return (
+            f"Bytes per sector: {self.bytes_per_sector}\n"
+            f"Sectors per cluster: {self.sectors_per_cluster}\n"
+            f"Reserved sectors: {self.reserved_sectors}\n"
+            f"FAT count: {self.fat_count}\n"
+            f"Root entries: {self.root_entries}\n"
+            f"Total sectors: {self.total_sectors}\n"
+            f"Media descriptor: {self.media_descriptor}\n"
+            f"Sectors per FAT: {self.sectors_per_fat}\n"
+            f"Sectors per track: {self.sectors_per_track}\n"
+            f"Heads: {self.heads}\n"
+            f"Hidden sectors: {self.hidden_sectors}\n"
+            f"Drive number: {self.drive_number}\n"
+            f"Volume ID: {self.volume_id}\n"
+            f"Volume label: {self.volume_label}\n"
+            f"FS type: {self.fs_type}\n"
+        )
+
+
+class FatDirectoryEntry:
+    # pylint: disable=too-many-instance-attributes
+    def __init__(self, data: bytes, sector: int, offset: int):
+        self.name = data[0:8].decode("ascii").strip()
+        self.ext = data[8:11].decode("ascii").strip()
+        self.attributes = data[11]
+        self.reserved = data[12]
+        self.create_time_tenth = data[13]
+        self.create_time = int.from_bytes(data[14:16], "little")
+        self.create_date = int.from_bytes(data[16:18], "little")
+        self.last_access_date = int.from_bytes(data[18:20], "little")
+        high_cluster = int.from_bytes(data[20:22], "little")
+        self.last_mod_time = int.from_bytes(data[22:24], "little")
+        self.last_mod_date = int.from_bytes(data[24:26], "little")
+        low_cluster = int.from_bytes(data[26:28], "little")
+        self.cluster = (high_cluster << 16) | low_cluster
+        self.size_bytes = int.from_bytes(data[28:32], "little")
+
+        # extra (to help write back to disk)
+        self.sector = sector
+        self.offset = offset
+
+    def as_bytes(self) -> bytes:
+        return (
+            self.name.ljust(8, " ").encode("ascii")
+            + self.ext.ljust(3, " ").encode("ascii")
+            + self.attributes.to_bytes(1, "little")
+            + self.reserved.to_bytes(1, "little")
+            + self.create_time_tenth.to_bytes(1, "little")
+            + self.create_time.to_bytes(2, "little")
+            + self.create_date.to_bytes(2, "little")
+            + self.last_access_date.to_bytes(2, "little")
+            + (self.cluster >> 16).to_bytes(2, "little")
+            + self.last_mod_time.to_bytes(2, "little")
+            + self.last_mod_date.to_bytes(2, "little")
+            + (self.cluster & 0xFFFF).to_bytes(2, "little")
+            + self.size_bytes.to_bytes(4, "little")
+        )
+
+    def whole_name(self):
+        if self.ext:
+            return f"{self.name}.{self.ext}"
+        else:
+            return self.name
+
+    def __str__(self):
+        return (
+            f"Name: {self.name}\n"
+            f"Ext: {self.ext}\n"
+            f"Attributes: {self.attributes}\n"
+            f"Reserved: {self.reserved}\n"
+            f"Create time tenth: {self.create_time_tenth}\n"
+            f"Create time: {self.create_time}\n"
+            f"Create date: {self.create_date}\n"
+            f"Last access date: {self.last_access_date}\n"
+            f"Last mod time: {self.last_mod_time}\n"
+            f"Last mod date: {self.last_mod_date}\n"
+            f"Cluster: {self.cluster}\n"
+            f"Size: {self.size_bytes}\n"
+        )
+
+    def __repr__(self):
+        # convert to dict
+        return str(vars(self))
+
+
+class SectorReader(Protocol):
+    def __call__(self, start_sector: int, num_sectors: int = 1) -> bytes: ...
+
+# pylint: disable=broad-exception-raised
+class Fat16:
+    def __init__(
+        self,
+        start_sector: int,
+        size: int,
+        sector_reader: SectorReader,
+        sector_writer: Callable[[int, bytes], None]
+    ):
+        self.start_sector = start_sector
+        self.size_in_sectors = size
+        self.sector_reader = sector_reader
+        self.sector_writer = sector_writer
+
+        self.boot_sector = FatBootSector(self.sector_reader(start_sector, 1))
+
+        fat_size_in_sectors = (
+            self.boot_sector.sectors_per_fat * self.boot_sector.fat_count
+        )
+        self.fats = self.read_sectors(
+            self.boot_sector.reserved_sectors, fat_size_in_sectors
+        )
+        self.fats_dirty_sectors: Set[int] = set()
+
+    def read_sectors(self, start_sector: int, num_sectors: int) -> bytes:
+        return self.sector_reader(start_sector + self.start_sector,
+                                  num_sectors)
+
+    def write_sectors(self, start_sector: int, data: bytes) -> None:
+        return self.sector_writer(start_sector + self.start_sector, data)
+
+    def directory_from_bytes(
+        self, data: bytes, start_sector: int
+    ) -> List[FatDirectoryEntry]:
+        """
+        Convert `bytes` into a list of `FatDirectoryEntry` objects.
+        Will ignore long file names.
+        Will stop when it encounters a 0x00 byte.
+        """
+
+        entries = []
+        for i in range(0, len(data), DIRENTRY_SIZE):
+            entry = data[i : i + DIRENTRY_SIZE]
+
+            current_sector = start_sector + (i // SECTOR_SIZE)
+            current_offset = i % SECTOR_SIZE
+
+            if entry[0] == 0:
+                break
+
+            if entry[0] == 0xE5:
+                # Deleted file
+                continue
+
+            if entry[11] & 0xF == 0xF:
+                # Long file name
+                continue
+
+            entries.append(
+                FatDirectoryEntry(entry, current_sector, current_offset)
+            )
+        return entries
+
+    def read_root_directory(self) -> List[FatDirectoryEntry]:
+        root_dir = self.read_sectors(
+            self.boot_sector.root_dir_start(), self.boot_sector.root_dir_size()
+        )
+        return self.directory_from_bytes(
+            root_dir, self.boot_sector.root_dir_start()
+        )
+
+    def read_fat_entry(self, cluster: int) -> int:
+        """
+        Read the FAT entry for the given cluster.
+        """
+        fat_offset = cluster * 2  # FAT16
+        return int.from_bytes(self.fats[fat_offset : fat_offset + 2], "little")
+
+    def write_fat_entry(self, cluster: int, value: int) -> None:
+        """
+        Write the FAT entry for the given cluster.
+        """
+        fat_offset = cluster * 2
+        self.fats = (
+            self.fats[:fat_offset]
+            + value.to_bytes(2, "little")
+            + self.fats[fat_offset + 2 :]
+        )
+        self.fats_dirty_sectors.add(fat_offset // SECTOR_SIZE)
+
+    def flush_fats(self) -> None:
+        """
+        Write the FATs back to the disk.
+        """
+        for sector in self.fats_dirty_sectors:
+            data = self.fats[sector * SECTOR_SIZE : (sector + 1) * SECTOR_SIZE]
+            sector = self.boot_sector.reserved_sectors + sector
+            self.write_sectors(sector, data)
+        self.fats_dirty_sectors = set()
+
+    def next_cluster(self, cluster: int) -> int | None:
+        """
+        Get the next cluster in the chain.
+        If its `None`, then its the last cluster.
+        The function will crash if the next cluster
+        is `FREE` (unexpected) or invalid entry.
+        """
+        fat_entry = self.read_fat_entry(cluster)
+        if fat_entry == 0:
+            raise Exception("Unexpected: FREE cluster")
+        if fat_entry == 1:
+            raise Exception("Unexpected: RESERVED cluster")
+        if fat_entry >= 0xFFF8:
+            return None
+        if fat_entry >= 0xFFF7:
+            raise Exception("Invalid FAT entry")
+
+        return fat_entry
+
+    def next_free_cluster(self) -> int:
+        """
+        Find the next free cluster.
+        """
+        # simple linear search
+        for i in range(2, 0xFFFF):
+            if self.read_fat_entry(i) == 0:
+                return i
+        raise Exception("No free clusters")
+
+    def next_free_cluster_non_continuous(self) -> int:
+        """
+        Find the next free cluster, but makes sure
+        that the cluster before and after it are not allocated.
+        """
+        # simple linear search
+        before = False
+        for i in range(2, 0xFFFF):
+            if self.read_fat_entry(i) == 0:
+                if before and self.read_fat_entry(i + 1) == 0:
+                    return i
+                else:
+                    before = True
+            else:
+                before = False
+
+        raise Exception("No free clusters")
+
+    def read_cluster(self, cluster: int) -> bytes:
+        """
+        Read the cluster at the given cluster.
+        """
+        return self.read_sectors(
+            self.boot_sector.first_sector_of_cluster(cluster),
+            self.boot_sector.sectors_per_cluster,
+        )
+
+    def write_cluster(self, cluster: int, data: bytes) -> None:
+        """
+        Write the cluster at the given cluster.
+        """
+        assert len(data) == self.boot_sector.cluster_bytes()
+        self.write_sectors(
+            self.boot_sector.first_sector_of_cluster(cluster),
+            data,
+        )
+
+    def read_directory(
+        self, cluster: Optional[int]
+    ) -> List[FatDirectoryEntry]:
+        """
+        Read the directory at the given cluster.
+        """
+        entries = []
+        while cluster is not None:
+            data = self.read_cluster(cluster)
+            entries.extend(
+                self.directory_from_bytes(
+                    data, self.boot_sector.first_sector_of_cluster(cluster)
+                )
+            )
+            cluster = self.next_cluster(cluster)
+        return entries
+
+    def add_direntry(
+        self, cluster: int | None, name: str, ext: str, attributes: int
+    ) -> FatDirectoryEntry:
+        """
+        Add a new directory entry to the given cluster.
+        If the cluster is `None`, then it will be added to the root directory.
+        """
+
+        def find_free_entry(data: bytes) -> Optional[int]:
+            for i in range(0, len(data), DIRENTRY_SIZE):
+                entry = data[i : i + DIRENTRY_SIZE]
+                if entry[0] == 0 or entry[0] == 0xE5:
+                    return i
+            return None
+
+        assert len(name) <= 8, "Name must be 8 characters or less"
+        assert len(ext) <= 3, "Ext must be 3 characters or less"
+        assert attributes % 0x15 != 0x15, "Invalid attributes"
+
+        # initial dummy data
+        new_entry = FatDirectoryEntry(b"\0" * 32, 0, 0)
+        new_entry.name = name.ljust(8, " ")
+        new_entry.ext = ext.ljust(3, " ")
+        new_entry.attributes = attributes
+        new_entry.reserved = 0
+        new_entry.create_time_tenth = 0
+        new_entry.create_time = 0
+        new_entry.create_date = 0
+        new_entry.last_access_date = 0
+        new_entry.last_mod_time = 0
+        new_entry.last_mod_date = 0
+        new_entry.cluster = self.next_free_cluster()
+        new_entry.size_bytes = 0
+
+        # mark as EOF
+        self.write_fat_entry(new_entry.cluster, 0xFFFF)
+
+        if cluster is None:
+            for i in range(self.boot_sector.root_dir_size()):
+                sector_data = self.read_sectors(
+                    self.boot_sector.root_dir_start() + i, 1
+                )
+                offset = find_free_entry(sector_data)
+                if offset is not None:
+                    new_entry.sector = self.boot_sector.root_dir_start() + i
+                    new_entry.offset = offset
+                    self.update_direntry(new_entry)
+                    return new_entry
+        else:
+            while cluster is not None:
+                data = self.read_cluster(cluster)
+                offset = find_free_entry(data)
+                if offset is not None:
+                    new_entry.sector = (
+                        self.boot_sector.first_sector_of_cluster(cluster)
+                         + (offset // SECTOR_SIZE))
+                    new_entry.offset = offset % SECTOR_SIZE
+                    self.update_direntry(new_entry)
+                    return new_entry
+                cluster = self.next_cluster(cluster)
+
+        raise Exception("No free directory entries")
+
+    def update_direntry(self, entry: FatDirectoryEntry) -> None:
+        """
+        Write the directory entry back to the disk.
+        """
+        sector = self.read_sectors(entry.sector, 1)
+        sector = (
+            sector[: entry.offset]
+            + entry.as_bytes()
+            + sector[entry.offset + DIRENTRY_SIZE :]
+        )
+        self.write_sectors(entry.sector, sector)
+
+    def find_direntry(self, path: str) -> FatDirectoryEntry | None:
+        """
+        Find the directory entry for the given path.
+        """
+        assert path[0] == "/", "Path must start with /"
+
+        path = path[1:]  # remove the leading /
+        parts = path.split("/")
+        directory = self.read_root_directory()
+
+        current_entry = None
+
+        for i, part in enumerate(parts):
+            is_last = i == len(parts) - 1
+
+            for entry in directory:
+                if entry.whole_name() == part:
+                    current_entry = entry
+                    break
+            if current_entry is None:
+                return None
+
+            if is_last:
+                return current_entry
+
+            if current_entry.attributes & 0x10 == 0:
+                raise Exception(
+                    f"{current_entry.whole_name()} is not a directory"
+                )
+
+            directory = self.read_directory(current_entry.cluster)
+
+        assert False, "Exited loop with is_last == False"
+
+    def read_file(self, entry: FatDirectoryEntry | None) -> bytes | None:
+        """
+        Read the content of the file at the given path.
+        """
+        if entry is None:
+            return None
+        if entry.attributes & 0x10 != 0:
+            raise Exception(f"{entry.whole_name()} is a directory")
+
+        data = b""
+        cluster: Optional[int] = entry.cluster
+        while cluster is not None and len(data) <= entry.size_bytes:
+            data += self.read_cluster(cluster)
+            cluster = self.next_cluster(cluster)
+        return data[: entry.size_bytes]
+
+    def truncate_file(
+        self,
+        entry: FatDirectoryEntry,
+        new_size: int,
+        allocate_non_continuous: bool = False,
+    ) -> None:
+        """
+        Truncate the file at the given path to the new size.
+        """
+        if entry is None:
+            raise Exception("entry is None")
+        if entry.attributes & 0x10 != 0:
+            raise Exception(f"{entry.whole_name()} is a directory")
+
+        def clusters_from_size(size: int) -> int:
+            return (
+                size + self.boot_sector.cluster_bytes() - 1
+            ) // self.boot_sector.cluster_bytes()
+
+        # First, allocate new FATs if we need to
+        required_clusters = clusters_from_size(new_size)
+        current_clusters = clusters_from_size(entry.size_bytes)
+
+        affected_clusters = set()
+
+        # Keep at least one cluster, easier to manage this way
+        if required_clusters == 0:
+            required_clusters = 1
+        if current_clusters == 0:
+            current_clusters = 1
+
+        cluster: Optional[int]
+
+        if required_clusters > current_clusters:
+            # Allocate new clusters
+            cluster = entry.cluster
+            to_add = required_clusters
+            for _ in range(current_clusters - 1):
+                to_add -= 1
+                assert cluster is not None, "Cluster is None"
+                affected_clusters.add(cluster)
+                cluster = self.next_cluster(cluster)
+            assert required_clusters > 0, "No new clusters to allocate"
+            assert cluster is not None, "Cluster is None"
+            assert (
+                self.next_cluster(cluster) is None
+            ), "Cluster is not the last cluster"
+
+            # Allocate new clusters
+            for _ in range(to_add - 1):
+                if allocate_non_continuous:
+                    new_cluster = self.next_free_cluster_non_continuous()
+                else:
+                    new_cluster = self.next_free_cluster()
+                self.write_fat_entry(cluster, new_cluster)
+                self.write_fat_entry(new_cluster, 0xFFFF)
+                cluster = new_cluster
+
+        elif required_clusters < current_clusters:
+            # Truncate the file
+            cluster = entry.cluster
+            for _ in range(required_clusters - 1):
+                assert cluster is not None, "Cluster is None"
+                cluster = self.next_cluster(cluster)
+            assert cluster is not None, "Cluster is None"
+
+            next_cluster = self.next_cluster(cluster)
+            # mark last as EOF
+            self.write_fat_entry(cluster, 0xFFFF)
+            # free the rest
+            while next_cluster is not None:
+                cluster = next_cluster
+                next_cluster = self.next_cluster(next_cluster)
+                self.write_fat_entry(cluster, 0)
+
+        self.flush_fats()
+
+        # verify number of clusters
+        cluster = entry.cluster
+        count = 0
+        while cluster is not None:
+            count += 1
+            affected_clusters.add(cluster)
+            cluster = self.next_cluster(cluster)
+        assert (
+            count == required_clusters
+        ), f"Expected {required_clusters} clusters, got {count}"
+
+        # update the size
+        entry.size_bytes = new_size
+        self.update_direntry(entry)
+
+        # trigger every affected cluster
+        for cluster in affected_clusters:
+            first_sector = self.boot_sector.first_sector_of_cluster(cluster)
+            first_sector_data = self.read_sectors(first_sector, 1)
+            self.write_sectors(first_sector, first_sector_data)
+
+    def write_file(self, entry: FatDirectoryEntry, data: bytes) -> None:
+        """
+        Write the content of the file at the given path.
+        """
+        if entry is None:
+            raise Exception("entry is None")
+        if entry.attributes & 0x10 != 0:
+            raise Exception(f"{entry.whole_name()} is a directory")
+
+        data_len = len(data)
+
+        self.truncate_file(entry, data_len)
+
+        cluster: Optional[int] = entry.cluster
+        while cluster is not None:
+            data_to_write = data[: self.boot_sector.cluster_bytes()]
+            if len(data_to_write) < self.boot_sector.cluster_bytes():
+                old_data = self.read_cluster(cluster)
+                data_to_write += old_data[len(data_to_write) :]
+
+            self.write_cluster(cluster, data_to_write)
+            data = data[self.boot_sector.cluster_bytes() :]
+            if len(data) == 0:
+                break
+            cluster = self.next_cluster(cluster)
+
+        assert (
+            len(data) == 0
+        ), "Data was not written completely, clusters missing"
+
+    def create_file(self, path: str) -> Optional[FatDirectoryEntry]:
+        """
+        Create a new file at the given path.
+        """
+        assert path[0] == "/", "Path must start with /"
+
+        path = path[1:]  # remove the leading /
+
+        parts = path.split("/")
+
+        directory_cluster = None
+        directory = self.read_root_directory()
+
+        parts, filename = parts[:-1], parts[-1]
+
+        for _, part in enumerate(parts):
+            current_entry = None
+            for entry in directory:
+                if entry.whole_name() == part:
+                    current_entry = entry
+                    break
+            if current_entry is None:
+                return None
+
+            if current_entry.attributes & 0x10 == 0:
+                raise Exception(
+                    f"{current_entry.whole_name()} is not a directory"
+                )
+
+            directory = self.read_directory(current_entry.cluster)
+            directory_cluster = current_entry.cluster
+
+        # add new entry to the directory
+
+        filename, ext = filename.split(".")
+
+        if len(ext) > 3:
+            raise Exception("Ext must be 3 characters or less")
+        if len(filename) > 8:
+            raise Exception("Name must be 8 characters or less")
+
+        for c in filename + ext:
+
+            if c not in ALLOWED_FILE_CHARS:
+                raise Exception("Invalid character in filename")
+
+        return self.add_direntry(directory_cluster, filename, ext, 0)
diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py
index 96d69e5696..c8848f2ec2 100644
--- a/tests/qemu-iotests/testenv.py
+++ b/tests/qemu-iotests/testenv.py
@@ -255,7 +255,7 @@ def __init__(self, source_dir: str, build_dir: str,
         self.qemu_img_options = os.getenv('QEMU_IMG_OPTIONS')
         self.qemu_nbd_options = os.getenv('QEMU_NBD_OPTIONS')
 
-        is_generic = self.imgfmt not in ['bochs', 'cloop', 'dmg']
+        is_generic = self.imgfmt not in ['bochs', 'cloop', 'dmg', 'vvfat']
         self.imgfmt_generic = 'true' if is_generic else 'false'
 
         self.qemu_io_options = f'--cache {self.cachemode} --aio {self.aiomode}'
diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index 56d88ca423..545f9ec7bd 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -84,7 +84,7 @@ def make_argparser() -> argparse.ArgumentParser:
     p.set_defaults(imgfmt='raw', imgproto='file')
 
     format_list = ['raw', 'bochs', 'cloop', 'parallels', 'qcow', 'qcow2',
-                   'qed', 'vdi', 'vpc', 'vhdx', 'vmdk', 'luks', 'dmg']
+                   'qed', 'vdi', 'vpc', 'vhdx', 'vmdk', 'luks', 'dmg', 'vvfat']
     g_fmt = p.add_argument_group(
         '  image format options',
         'The following options set the IMGFMT environment variable. '
diff --git a/tests/qemu-iotests/tests/vvfat b/tests/qemu-iotests/tests/vvfat
new file mode 100755
index 0000000000..acdc6ce8ff
--- /dev/null
+++ b/tests/qemu-iotests/tests/vvfat
@@ -0,0 +1,485 @@
+#!/usr/bin/env python3
+# group: rw vvfat
+#
+# Test vvfat driver implementation
+# Here, we use a simple FAT16 implementation and check the behavior of
+# the vvfat driver.
+#
+# Copyright (C) 2024 Amjad Alsharafi <amjadsharafi10@gmail.com>
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+import os
+import shutil
+import iotests
+from iotests import imgfmt, QMPTestCase
+from fat16 import MBR, Fat16, DIRENTRY_SIZE
+
+filesystem = os.path.join(iotests.test_dir, "filesystem")
+
+nbd_sock = iotests.file_path("nbd.sock", base_dir=iotests.sock_dir)
+nbd_uri = "nbd+unix:///disk?socket=" + nbd_sock
+
+SECTOR_SIZE = 512
+
+
+class TestVVFatDriver(QMPTestCase):
+    # pylint: disable=broad-exception-raised
+    def setUp(self) -> None:
+        if os.path.exists(filesystem):
+            if os.path.isdir(filesystem):
+                shutil.rmtree(filesystem)
+            else:
+                raise Exception(f"{filesystem} exists and is not a directory")
+
+        os.mkdir(filesystem)
+
+        # Add some text files to the filesystem
+        for i in range(10):
+            with open(os.path.join(filesystem, f"file{i}.txt"),
+                      "w", encoding="ascii") as f:
+                f.write(f"Hello, world! {i}\n")
+
+        # Add 2 large files, above the cluster size (8KB)
+        with open(os.path.join(filesystem, "large1.txt"), "wb") as f:
+            # write 'A' * 1KB, 'B' * 1KB, 'C' * 1KB, ...
+            for i in range(8 * 2):  # two clusters
+                f.write(bytes([0x41 + i] * 1024))
+
+        with open(os.path.join(filesystem, "large2.txt"), "wb") as f:
+            # write 'A' * 1KB, 'B' * 1KB, 'C' * 1KB, ...
+            for i in range(8 * 3):  # 3 clusters
+                f.write(bytes([0x41 + i] * 1024))
+
+        self.vm = iotests.VM()
+
+        self.vm.add_blockdev(
+            self.vm.qmp_to_opts(
+                {
+                    "driver": imgfmt,
+                    "node-name": "disk",
+                    "rw": "true",
+                    "fat-type": "16",
+                    "dir": filesystem,
+                }
+            )
+        )
+
+        self.vm.launch()
+
+        self.vm.qmp_log("block-dirty-bitmap-add", **{
+            "node": "disk",
+            "name": "bitmap0",
+        })
+
+        # attach nbd server
+        self.vm.qmp_log(
+            "nbd-server-start",
+            **{"addr": {"type": "unix", "data": {"path": nbd_sock}}},
+            filters=[],
+        )
+
+        self.vm.qmp_log(
+            "nbd-server-add",
+            **{"device": "disk", "writable": True, "bitmap": "bitmap0"},
+        )
+
+        self.qio = iotests.QemuIoInteractive("-f", "raw", nbd_uri)
+
+    def tearDown(self) -> None:
+        self.qio.close()
+        self.vm.shutdown()
+        # print(self.vm.get_log())
+        shutil.rmtree(filesystem)
+
+    def read_sectors(self, sector: int, num: int = 1) -> bytes:
+        """
+        Read `num` sectors starting from `sector` from the `disk`.
+        This uses `QemuIoInteractive` to read the sectors into `stdout` and
+        then parse the output.
+        """
+        self.assertGreater(num, 0)
+
+        # The output contains the content of the sector in hex dump format
+        # We need to extract the content from it
+        output = self.qio.cmd(
+            f"read -v {sector * SECTOR_SIZE} {num * SECTOR_SIZE}")
+
+        # Each row is 16 bytes long, and we are writing `num` sectors
+        rows = num * SECTOR_SIZE // 16
+        output_rows = output.split("\n")[:rows]
+
+        hex_content = "".join(
+            [(row.split(": ")[1]).split("  ")[0] for row in output_rows]
+        )
+        bytes_content = bytes.fromhex(hex_content)
+
+        self.assertEqual(len(bytes_content), num * SECTOR_SIZE)
+
+        return bytes_content
+
+    def write_sectors(self, sector: int, data: bytes) -> None:
+        """
+        Write `data` to the `disk` starting from `sector`.
+        This uses `QemuIoInteractive` to write the data into the disk.
+        """
+
+        self.assertGreater(len(data), 0)
+        self.assertEqual(len(data) % SECTOR_SIZE, 0)
+
+        temp_file = os.path.join(iotests.test_dir, "temp.bin")
+        with open(temp_file, "wb") as f:
+            f.write(data)
+
+        self.qio.cmd(
+            f"write -s {temp_file} {sector * SECTOR_SIZE} {len(data)}"
+        )
+
+        os.remove(temp_file)
+
+    def init_fat16(self):
+        mbr = MBR(self.read_sectors(0))
+        return Fat16(
+            mbr.partition_table[0]["start_lba"],
+            mbr.partition_table[0]["size"],
+            self.read_sectors,
+            self.write_sectors,
+        )
+
+    # Tests
+
+    def test_fat_filesystem(self):
+        """
+        Test that vvfat produce a valid FAT16 and MBR sectors
+        """
+        mbr = MBR(self.read_sectors(0))
+
+        self.assertEqual(mbr.partition_table[0]["status"], 0x80)
+        self.assertEqual(mbr.partition_table[0]["type"], 6)
+
+        fat16 = Fat16(
+            mbr.partition_table[0]["start_lba"],
+            mbr.partition_table[0]["size"],
+            self.read_sectors,
+            self.write_sectors,
+        )
+        self.assertEqual(fat16.boot_sector.bytes_per_sector, 512)
+        self.assertEqual(fat16.boot_sector.volume_label, "QEMU VVFAT")
+
+    def test_read_root_directory(self):
+        """
+        Test the content of the root directory
+        """
+        fat16 = self.init_fat16()
+
+        root_dir = fat16.read_root_directory()
+
+        self.assertEqual(len(root_dir), 13)  # 12 + 1 special file
+
+        files = {
+            "QEMU VVF.AT": 0,  # special empty file
+            "FILE0.TXT": 16,
+            "FILE1.TXT": 16,
+            "FILE2.TXT": 16,
+            "FILE3.TXT": 16,
+            "FILE4.TXT": 16,
+            "FILE5.TXT": 16,
+            "FILE6.TXT": 16,
+            "FILE7.TXT": 16,
+            "FILE8.TXT": 16,
+            "FILE9.TXT": 16,
+            "LARGE1.TXT": 0x2000 * 2,
+            "LARGE2.TXT": 0x2000 * 3,
+        }
+
+        for entry in root_dir:
+            self.assertIn(entry.whole_name(), files)
+            self.assertEqual(entry.size_bytes, files[entry.whole_name()])
+
+    def test_direntry_as_bytes(self):
+        """
+        Test if we can convert Direntry back to bytes, so that we can write it
+        back to the disk safely.
+        """
+        fat16 = self.init_fat16()
+
+        root_dir = fat16.read_root_directory()
+        first_entry_bytes = fat16.read_sectors(
+            fat16.boot_sector.root_dir_start(), 1)
+
+        # The first entry won't be deleted, so we can compare it with the first
+        # entry in the root directory
+        self.assertEqual(root_dir[0].as_bytes(),
+                         first_entry_bytes[:DIRENTRY_SIZE])
+
+    def test_read_files(self):
+        """
+        Test reading the content of the files
+        """
+        fat16 = self.init_fat16()
+
+        for i in range(10):
+            file = fat16.find_direntry(f"/FILE{i}.TXT")
+            self.assertIsNotNone(file)
+            self.assertEqual(
+                fat16.read_file(file), f"Hello, world! {i}\n".encode("ascii")
+            )
+
+        # test large files
+        large1 = fat16.find_direntry("/LARGE1.TXT")
+        with open(os.path.join(filesystem, "large1.txt"), "rb") as f:
+            self.assertEqual(fat16.read_file(large1), f.read())
+
+        large2 = fat16.find_direntry("/LARGE2.TXT")
+        self.assertIsNotNone(large2)
+        with open(os.path.join(filesystem, "large2.txt"), "rb") as f:
+            self.assertEqual(fat16.read_file(large2), f.read())
+
+    def test_write_file_same_content_direct(self):
+        """
+        Similar to `test_write_file_in_same_content`, but we write the file
+        directly clusters and thus we don't go through the modification of
+        direntry.
+        """
+        fat16 = self.init_fat16()
+
+        file = fat16.find_direntry("/FILE0.TXT")
+        self.assertIsNotNone(file)
+
+        data = fat16.read_cluster(file.cluster)
+        fat16.write_cluster(file.cluster, data)
+
+        with open(os.path.join(filesystem, "file0.txt"), "rb") as f:
+            self.assertEqual(fat16.read_file(file), f.read())
+
+    def test_write_file_in_same_content(self):
+        """
+        Test writing the same content to the file back to it
+        """
+        fat16 = self.init_fat16()
+
+        file = fat16.find_direntry("/FILE0.TXT")
+        self.assertIsNotNone(file)
+
+        self.assertEqual(fat16.read_file(file), b"Hello, world! 0\n")
+
+        fat16.write_file(file, b"Hello, world! 0\n")
+        self.assertEqual(fat16.read_file(file), b"Hello, world! 0\n")
+
+        with open(os.path.join(filesystem, "file0.txt"), "rb") as f:
+            self.assertEqual(f.read(), b"Hello, world! 0\n")
+
+    def test_modify_content_same_clusters(self):
+        """
+        Test modifying the content of the file without changing the number of
+        clusters
+        """
+        fat16 = self.init_fat16()
+
+        file = fat16.find_direntry("/FILE0.TXT")
+        self.assertIsNotNone(file)
+
+        new_content = b"Hello, world! Modified\n"
+        self.assertEqual(fat16.read_file(file), b"Hello, world! 0\n")
+
+        fat16.write_file(file, new_content)
+        self.assertEqual(fat16.read_file(file), new_content)
+
+        with open(os.path.join(filesystem, "file0.txt"), "rb") as f:
+            self.assertEqual(f.read(), new_content)
+
+    def test_truncate_file_same_clusters_less(self):
+        """
+        Test truncating the file without changing number of clusters
+        Test decreasing the file size
+        """
+        fat16 = self.init_fat16()
+
+        file = fat16.find_direntry("/FILE0.TXT")
+        self.assertIsNotNone(file)
+
+        self.assertEqual(fat16.read_file(file), b"Hello, world! 0\n")
+
+        fat16.truncate_file(file, 5)
+        new_content = fat16.read_file(file)
+        self.assertEqual(new_content, b"Hello")
+
+        with open(os.path.join(filesystem, "file0.txt"), "rb") as f:
+            self.assertEqual(f.read(), new_content)
+
+    def test_truncate_file_same_clusters_more(self):
+        """
+        Test truncating the file without changing number of clusters
+        Test increase the file size
+        """
+        fat16 = self.init_fat16()
+
+        file = fat16.find_direntry("/FILE0.TXT")
+        self.assertIsNotNone(file)
+
+        self.assertEqual(fat16.read_file(file), b"Hello, world! 0\n")
+
+        fat16.truncate_file(file, 20)
+        new_content = fat16.read_file(file)
+        self.assertIsNotNone(new_content)
+
+        # random pattern will be appended to the file, and its not always the
+        # same
+        self.assertEqual(new_content[:16], b"Hello, world! 0\n")
+        self.assertEqual(len(new_content), 20)
+
+        with open(os.path.join(filesystem, "file0.txt"), "rb") as f:
+            self.assertEqual(f.read(), new_content)
+
+    def test_write_large_file(self):
+        """
+        Test writing a large file
+        """
+        fat16 = self.init_fat16()
+
+        file = fat16.find_direntry("/LARGE1.TXT")
+        self.assertIsNotNone(file)
+
+        # The content of LARGE1 is A * 1KB, B * 1KB, C * 1KB, ..., P * 1KB
+        # Lets change it to be Z * 1KB, Y * 1KB, X * 1KB, ..., K * 1KB
+        # without changing the number of clusters or filesize
+        new_content = b"".join([bytes([0x5A - i] * 1024) for i in range(16)])
+        fat16.write_file(file, new_content)
+        self.assertEqual(fat16.read_file(file), new_content)
+
+        with open(os.path.join(filesystem, "large1.txt"), "rb") as f:
+            self.assertEqual(f.read(), new_content)
+
+    def test_truncate_file_change_clusters_less(self):
+        """
+        Test truncating a file by reducing the number of clusters
+        """
+        fat16 = self.init_fat16()
+
+        file = fat16.find_direntry("/LARGE1.TXT")
+        self.assertIsNotNone(file)
+
+        fat16.truncate_file(file, 1)
+        self.assertEqual(fat16.read_file(file), b"A")
+
+        with open(os.path.join(filesystem, "large1.txt"), "rb") as f:
+            self.assertEqual(f.read(), b"A")
+
+    def test_write_file_change_clusters_less(self):
+        """
+        Test truncating a file by reducing the number of clusters
+        """
+        fat16 = self.init_fat16()
+
+        file = fat16.find_direntry("/LARGE2.TXT")
+        self.assertIsNotNone(file)
+
+        new_content = b"X" * 8 * 1024 + b"Y" * 8 * 1024
+        fat16.write_file(file, new_content)
+        self.assertEqual(fat16.read_file(file), new_content)
+
+        with open(os.path.join(filesystem, "large2.txt"), "rb") as f:
+            self.assertEqual(f.read(), new_content)
+
+    def test_write_file_change_clusters_more(self):
+        """
+        Test truncating a file by increasing the number of clusters
+        """
+        fat16 = self.init_fat16()
+
+        file = fat16.find_direntry("/LARGE2.TXT")
+        self.assertIsNotNone(file)
+
+        # from 3 clusters to 4 clusters
+        new_content = (
+            b"W" * 8 * 1024 +
+            b"X" * 8 * 1024 +
+            b"Y" * 8 * 1024 +
+            b"Z" * 8 * 1024
+        )
+        fat16.write_file(file, new_content)
+        self.assertEqual(fat16.read_file(file), new_content)
+
+        with open(os.path.join(filesystem, "large2.txt"), "rb") as f:
+            self.assertEqual(f.read(), new_content)
+
+    def test_write_file_change_clusters_more_non_contiguous_2_mappings(self):
+        """
+        Test truncating a file by increasing the number of clusters Here we
+        allocate the new clusters in a way that makes them non-contiguous so
+        that we will get 2 cluster mappings for the file
+        """
+        fat16 = self.init_fat16()
+
+        file = fat16.find_direntry("/LARGE1.TXT")
+        self.assertIsNotNone(file)
+
+        # from 2 clusters to 3 clusters with non-contiguous allocation
+        fat16.truncate_file(file, 3 * 0x2000, allocate_non_continuous=True)
+        new_content = b"X" * 8 * 1024 + b"Y" * 8 * 1024 + b"Z" * 8 * 1024
+        fat16.write_file(file, new_content)
+        self.assertEqual(fat16.read_file(file), new_content)
+
+        with open(os.path.join(filesystem, "large1.txt"), "rb") as f:
+            self.assertEqual(f.read(), new_content)
+
+    def test_write_file_change_clusters_more_non_contiguous_3_mappings(self):
+        """
+        Test truncating a file by increasing the number of clusters Here we
+        allocate the new clusters in a way that makes them non-contiguous so
+        that we will get 3 cluster mappings for the file
+        """
+        fat16 = self.init_fat16()
+
+        file = fat16.find_direntry("/LARGE1.TXT")
+        self.assertIsNotNone(file)
+
+        # from 2 clusters to 4 clusters with non-contiguous allocation
+        fat16.truncate_file(file, 4 * 0x2000, allocate_non_continuous=True)
+        new_content = (
+            b"W" * 8 * 1024 +
+            b"X" * 8 * 1024 +
+            b"Y" * 8 * 1024 +
+            b"Z" * 8 * 1024
+        )
+        fat16.write_file(file, new_content)
+        self.assertEqual(fat16.read_file(file), new_content)
+
+        with open(os.path.join(filesystem, "large1.txt"), "rb") as f:
+            self.assertEqual(f.read(), new_content)
+
+    def test_create_file(self):
+        """
+        Test creating a new file
+        """
+        fat16 = self.init_fat16()
+
+        new_file = fat16.create_file("/NEWFILE.TXT")
+
+        self.assertIsNotNone(new_file)
+        self.assertEqual(new_file.size_bytes, 0)
+
+        new_content = b"Hello, world! New file\n"
+        fat16.write_file(new_file, new_content)
+        self.assertEqual(fat16.read_file(new_file), new_content)
+
+        with open(os.path.join(filesystem, "newfile.txt"), "rb") as f:
+            self.assertEqual(f.read(), new_content)
+
+    # TODO: support deleting files
+
+
+if __name__ == "__main__":
+    # This is a specific test for vvfat driver
+    iotests.main(supported_fmts=["vvfat"], supported_protocols=["file"])
diff --git a/tests/qemu-iotests/tests/vvfat.out b/tests/qemu-iotests/tests/vvfat.out
new file mode 100755
index 0000000000..b6f257674e
--- /dev/null
+++ b/tests/qemu-iotests/tests/vvfat.out
@@ -0,0 +1,5 @@
+................
+----------------------------------------------------------------------
+Ran 16 tests
+
+OK
-- 
2.45.2

[PULL 13/13] iotests/024: exclude 'backing file format' field from the output

[Kevin Wolf]

From: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

Apparently 'qemu-img info' doesn't report the backing file format field
for qed (as it does for qcow2):

$ qemu-img create -f qed base.qed 1M && qemu-img create -f qed -b base.qed -F qed top.qed 1M
$ qemu-img create -f qcow2 base.qcow2 1M && qemu-img create -f qcow2 -b base.qcow2 -F qcow2 top.qcow2 1M
$ qemu-img info top.qed | grep 'backing file format'
$ qemu-img info top.qcow2 | grep 'backing file format'
backing file format: qcow2

This leads to the 024 test failure with -qed.  Let's just filter the
field out and exclude it from the output.

This is a fixup for the commit f93e65ee51 ("iotests/{024, 271}: add
testcases for qemu-img rebase").

Reported-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
Message-ID: <20240730094701.790624-1-andrey.drobyshev@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/024     | 2 +-
 tests/qemu-iotests/024.out | 1 -
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/024 b/tests/qemu-iotests/024
index 285f17e79f..b29c76e161 100755
--- a/tests/qemu-iotests/024
+++ b/tests/qemu-iotests/024
@@ -283,7 +283,7 @@ TEST_IMG=$BASE_OLD _make_test_img -b "$BASE_NEW" -F $IMGFMT \
 CLUSTER_SIZE=$(( CLUSTER_SIZE * 2 )) TEST_IMG=$OVERLAY \
     _make_test_img -b "$BASE_OLD" -F $IMGFMT $(( CLUSTER_SIZE * 6 ))
 
-TEST_IMG=$OVERLAY _img_info
+TEST_IMG=$OVERLAY _img_info | grep -v '^backing file format:'
 
 echo
 echo "Fill backing files with data"
diff --git a/tests/qemu-iotests/024.out b/tests/qemu-iotests/024.out
index e1e8eea863..3d1e31927a 100644
--- a/tests/qemu-iotests/024.out
+++ b/tests/qemu-iotests/024.out
@@ -214,7 +214,6 @@ file format: IMGFMT
 virtual size: 384 KiB (393216 bytes)
 cluster_size: 131072
 backing file: TEST_DIR/subdir/t.IMGFMT.base_old
-backing file format: IMGFMT
 
 Fill backing files with data
 
-- 
2.45.2

Re: [PULL 00/13] Block layer patches

[Richard Henderson]

On 8/6/24 07:08, Kevin Wolf wrote:
> The following changes since commit f9851d2ffef59b3a7f39513469263ab3b019480f:
> 
>    Merge tag 'migration-20240802-pull-request' ofhttps://gitlab.com/farosas/qemu into staging (2024-08-03 07:26:26 +1000)
> 
> are available in the Git repository at:
> 
>    https://repo.or.cz/qemu/kevin.git tags/for-upstream
> 
> for you to fetch changes up to 833362e55d9acc6b7ccc1d80d8cd84688e7f5761:
> 
>    iotests/024: exclude 'backing file format' field from the output (2024-08-05 23:00:42 +0200)
> 
> ----------------------------------------------------------------
> Block layer patches
> 
> - scsi-block: Fix error handling with r/werror=stop
> - Depend on newer clang for TSA, make WITH_GRAPH_RDLOCK_GUARD() fully
>    checked, fix block-copy to add missing lock
> - vvfat: Fix write bugs for large files and add iotests
> - Clean up blockdev-snapshot-internal-sync doc
> - Fix iotests 024 for qed

Auto-merging qapi/block-core.json
CONFLICT (content): Merge conflict in qapi/block-core.json
Automatic merge failed; fix conflicts and then commit the result.

Please re-spin, thanks.


r~