These patches fix some bugs found when modifying files in vvfat.
First, there was a bug when writing to the cluster 2 or above of a file, it
will copy the cluster before it instead, so, when writing to cluster=2, the
content of cluster=1 will be copied into disk instead in its place.

Another issue was modifying the clusters of a file and adding new
clusters, this showed 2 issues:
- If the new cluster is not immediately after the last cluster, it will
cause issues when reading from this file in the future.
- Generally, the usage of info.file.offset was incorrect, and the
system would crash on abort() when the file is modified and a new
cluster was added.

Also, added some iotests for vvfat, covering the this fix and also
general behavior such as reading, writing, and creating files on the filesystem.
Including tests for reading/writing the first cluster which
would pass even before this patch.

v3:
  Added test for creating new files in vvfat.

v2:
  Added iotests for `vvfat` driver along with a simple `fat16` module to run the tests.

v1:
  https://patchew.org/QEMU/20240327201231.31046-1-amjadsharafi10@gmail.com/
  Fix the issue of writing to the middle of the file in vvfat

Amjad Alsharafi (6):
  vvfat: Fix bug in writing to middle of file
  vvfat: Fix usage of `info.file.offset`
  vvfat: Fix reading files with non-continuous clusters
  iotests: Add `vvfat` tests
  iotests: Filter out `vvfat` fmt from failing tests
  iotests: Add `create_file` test for `vvfat` driver

 .gitlab-ci.d/buildtest.yml                    |   1 +
 block/vvfat.c                                 |  32 +-
 tests/qemu-iotests/001                        |   1 +
 tests/qemu-iotests/002                        |   1 +
 tests/qemu-iotests/003                        |   1 +
 tests/qemu-iotests/005                        |   1 +
 tests/qemu-iotests/008                        |   1 +
 tests/qemu-iotests/009                        |   1 +
 tests/qemu-iotests/010                        |   1 +
 tests/qemu-iotests/011                        |   1 +
 tests/qemu-iotests/012                        |   1 +
 tests/qemu-iotests/021                        |   1 +
 tests/qemu-iotests/032                        |   1 +
 tests/qemu-iotests/033                        |   1 +
 tests/qemu-iotests/052                        |   1 +
 tests/qemu-iotests/094                        |   1 +
 tests/qemu-iotests/120                        |   2 +-
 tests/qemu-iotests/140                        |   1 +
 tests/qemu-iotests/145                        |   1 +
 tests/qemu-iotests/157                        |   1 +
 tests/qemu-iotests/159                        |   2 +-
 tests/qemu-iotests/170                        |   2 +-
 tests/qemu-iotests/192                        |   1 +
 tests/qemu-iotests/197                        |   2 +-
 tests/qemu-iotests/208                        |   2 +-
 tests/qemu-iotests/215                        |   2 +-
 tests/qemu-iotests/236                        |   2 +-
 tests/qemu-iotests/251                        |   1 +
 tests/qemu-iotests/307                        |   2 +-
 tests/qemu-iotests/308                        |   2 +-
 tests/qemu-iotests/check                      |   2 +-
 tests/qemu-iotests/fat16.py                   | 619 ++++++++++++++++++
 tests/qemu-iotests/meson.build                |   3 +-
 .../tests/export-incoming-iothread            |   2 +-
 tests/qemu-iotests/tests/fuse-allow-other     |   1 +
 .../tests/mirror-ready-cancel-error           |   2 +-
 tests/qemu-iotests/tests/regression-vhdx-log  |   1 +
 tests/qemu-iotests/tests/vvfat                | 419 ++++++++++++
 tests/qemu-iotests/tests/vvfat.out            |   5 +
 39 files changed, 1098 insertions(+), 26 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

-- 
2.45.0

Am 26.05.2024 um 11:56 hat Amjad Alsharafi geschrieben:
> These patches fix some bugs found when modifying files in vvfat.
> First, there was a bug when writing to the cluster 2 or above of a file, it
> will copy the cluster before it instead, so, when writing to cluster=2, the
> content of cluster=1 will be copied into disk instead in its place.
> 
> Another issue was modifying the clusters of a file and adding new
> clusters, this showed 2 issues:
> - If the new cluster is not immediately after the last cluster, it will
> cause issues when reading from this file in the future.
> - Generally, the usage of info.file.offset was incorrect, and the
> system would crash on abort() when the file is modified and a new
> cluster was added.
> 
> Also, added some iotests for vvfat, covering the this fix and also
> general behavior such as reading, writing, and creating files on the filesystem.
> Including tests for reading/writing the first cluster which
> would pass even before this patch.

I was wondering how to reproduce the bugs that patches 2 and 3 fix. So I
tried to run your iotests case, and while it does catch the bug that
patch 1 fixes, it passes even without the other two fixes.

Is this expected? If so, can we add more tests that trigger the problems
the other two patches address?

Kevin

On Fri, May 31, 2024 at 07:22:49PM +0200, Kevin Wolf wrote:
> Am 26.05.2024 um 11:56 hat Amjad Alsharafi geschrieben:
> > These patches fix some bugs found when modifying files in vvfat.
> > First, there was a bug when writing to the cluster 2 or above of a file, it
> > will copy the cluster before it instead, so, when writing to cluster=2, the
> > content of cluster=1 will be copied into disk instead in its place.
> > 
> > Another issue was modifying the clusters of a file and adding new
> > clusters, this showed 2 issues:
> > - If the new cluster is not immediately after the last cluster, it will
> > cause issues when reading from this file in the future.
> > - Generally, the usage of info.file.offset was incorrect, and the
> > system would crash on abort() when the file is modified and a new
> > cluster was added.
> > 
> > Also, added some iotests for vvfat, covering the this fix and also
> > general behavior such as reading, writing, and creating files on the filesystem.
> > Including tests for reading/writing the first cluster which
> > would pass even before this patch.
> 
> I was wondering how to reproduce the bugs that patches 2 and 3 fix. So I
> tried to run your iotests case, and while it does catch the bug that
> patch 1 fixes, it passes even without the other two fixes.
> 
> Is this expected? If so, can we add more tests that trigger the problems
> the other two patches address?
> 
> Kevin
> 

Thanks for checking, so this bug happens when you have mapping for file,
and the clusters are not contiguous.

For example, a file with clusters `12, 13, 15`, here when trying to
read from cluster 15, it will get the offset in the file by using 
the formula `cluster_size * (15-12)` (`12` is the first cluster).

This is of course is not correct, and will result in error reading the
file from outside the range.

The reason it wasn't clear when you tested it, is that since I'm
modifying `large2.txt`, and its the last file in the disk, when trying
to allocate new clusters, coincidentally, the new clusters are allocated
after the last cluster of that same file, so the issue wasn't triggered.

I'll modify the test to use the other file, so that we can trigger the
issue.

I'll also modify the other suggestions you had in the other patches and
submit a new version.

Amjad