Re: [PATCH v5 5/5] iotests: Add `vvfat` tests

[Amjad Alsharafi]

On Wed, Jun 12, 2024 at 08:43:26PM +0800, Amjad Alsharafi wrote:
> 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 
> ---
>  tests/qemu-iotests/check   |   2 +-
>  tests/qemu-iotests/fat16.py| 673 +
>  tests/qemu-iotests/testenv.py  |   2 +-
>  tests/qemu-iotests/tests/vvfat | 457 
>  tests/qemu-iotests/tests/vvfat.out |   5 +
>  5 files changed, 1137 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/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/fat16.py b/tests/qemu-iotests/fat16.py
> new file mode 100644
> index 00..abe4ce1f8f
> --- /dev/null
> +++ b/tests/qemu-iotests/fat16.py
> @@ -0,0 +1,673 @@
> +# A simple FAT16 driver that is used to test the `vvfat` driver in QEMU.
> +#
> +# Copyright (C) 2024 Amjad Alsharafi 
> +#
> +# 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 List
> +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:
> +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], "

[PATCH v5 4/5] vvfat: Fix reading files with non-continuous clusters

[Amjad Alsharafi]

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 
---
 block/vvfat.c | 23 ---
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index b63ac5d045..fc570d0610 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -1360,15 +1360,24 @@ static int open_file(BDRVVVFATState* s,mapping_t* 
mapping)
 {
 if(!mapping)
 return -1;
+int new_path = 1;
 if(!s->current_mapping ||
-strcmp(s->current_mapping->path,mapping->path)) {
-/* open file */
-int fd = qemu_open_old(mapping->path,
+s->current_mapping->info.file.offset
+!= mapping->info.file.offset ||
+(new_path = strcmp(s->current_mapping->path, mapping->path))) {
+
+if (new_path) {
+/* open file */
+int fd = qemu_open_old(mapping->path,
O_RDONLY | O_BINARY | O_LARGEFILE);
-if(fd<0)
-return -1;
-vvfat_close_current_file(s);
-s->current_fd = fd;
+if (fd < 0) {
+return -1;
+}
+vvfat_close_current_file(s);
+
+s->current_fd = fd;
+}
+assert(s->current_fd);
 s->current_mapping = mapping;
 }
 return 0;
-- 
2.45.1

[PATCH v5 5/5] iotests: Add `vvfat` tests

[Amjad Alsharafi]

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 
---
 tests/qemu-iotests/check   |   2 +-
 tests/qemu-iotests/fat16.py| 673 +
 tests/qemu-iotests/testenv.py  |   2 +-
 tests/qemu-iotests/tests/vvfat | 457 
 tests/qemu-iotests/tests/vvfat.out |   5 +
 5 files changed, 1137 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/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/fat16.py b/tests/qemu-iotests/fat16.py
new file mode 100644
index 00..abe4ce1f8f
--- /dev/null
+++ b/tests/qemu-iotests/fat16.py
@@ -0,0 +1,673 @@
+# A simple FAT16 driver that is used to test the `vvfat` driver in QEMU.
+#
+# Copyright (C) 2024 Amjad Alsharafi 
+#
+# 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 List
+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:
+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"

[PATCH v5 3/5] vvfat: Fix wrong checks for cluster mappings invariant

[Amjad Alsharafi]

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 
---
 block/vvfat.c | 10 ++
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index 247b232608..b63ac5d045 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.1

[PATCH v5 2/5] vvfat: Fix usage of `info.file.offset`

[Amjad Alsharafi]

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 
---
 block/vvfat.c | 11 +++
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index 19da009a5b..247b232608 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.1

[PATCH v5 0/5] vvfat: Fix write bugs for large files and add iotests

[Amjad Alsharafi]

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.

v5:
  - Fix a bug in reading non-contiguous clusters in vvfat for more than 2 
mappings.
  - Added a test for adding more clusters where they are non-contiguous and
result in 3 mappings (for the above fix).
  - Split PATCH 2/4 into 2 patches and a better fix for the `abort` issue (now 
in PATCH 3/5).
  - Other fixes and improvements in `fat16.py` module used for iotests.

v4:
  Applied some suggestions from Kevin Wolf :
  - Fixed code formatting by following the coding style in 
`scripts/checkpatch.pl`
  - Reduced changes related to `iotests` by setting `vvfat` format as 
non-generic.
  - Added another test to cover the fix done in `PATCH 2/4` and `PATCH 3/4` for 
handling reading/writing files with non-continuous clusters.

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-amjadsharaf...@gmail.com/
  Fix the issue of writing to the middle of the file in vvfat

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

 block/vvfat.c  |  47 +-
 tests/qemu-iotests/check   |   2 +-
 tests/qemu-iotests/fat16.py| 673 +
 tests/qemu-iotests/testenv.py  |   2 +-
 tests/qemu-iotests/tests/vvfat | 457 
 tests/qemu-iotests/tests/vvfat.out |   5 +
 6 files changed, 1164 insertions(+), 22 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.1

[PATCH v5 1/5] vvfat: Fix bug in writing to middle of file

[Amjad Alsharafi]

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 
Reviewed-by: Kevin Wolf 
Tested-by: Kevin Wolf 
---
 block/vvfat.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index 9d050ba3ae..19da009a5b 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.1

Re: [PATCH v4 2/4] vvfat: Fix usage of `info.file.offset`

[Amjad Alsharafi]

On Tue, Jun 11, 2024 at 04:30:53PM +0200, Kevin Wolf wrote:
> Am 11.06.2024 um 14:31 hat Amjad Alsharafi geschrieben:
> > On Mon, Jun 10, 2024 at 06:49:43PM +0200, Kevin Wolf wrote:
> > > Am 05.06.2024 um 02:58 hat Amjad Alsharafi geschrieben:
> > > > 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.
> > > > 
> > > > Additionally, removed the `abort` when `first_mapping_index` does not
> > > > match, as this matches the case when adding new clusters for files, and
> > > > its inevitable that we reach this condition when doing that if the
> > > > clusters are not after one another, so there is no reason to `abort`
> > > > here, execution continues and the new clusters are written to disk
> > > > correctly.
> > > > 
> > > > Signed-off-by: Amjad Alsharafi 
> > > 
> > > Can you help me understand how first_mapping_index really works?
> > > 
> > > It seems to me that you get a chain of mappings for each file on the FAT
> > > filesystem, which are just the contiguous areas in it, and
> > > first_mapping_index refers to the mapping at the start of the file. But
> > > for much of the time, it actually doesn't seem to be set at all, so you
> > > have mapping->first_mapping_index == -1. Do you understand the rules
> > > around when it's set and when it isn't?
> > 
> > Yeah. So `first_mapping_index` is the index of the first mapping, each
> > mapping is a group of clusters that are contiguous in the file.
> > Its mostly `-1` because the first mapping will have the value set as
> > `-1` and not its own index, this value will only be set when the file
> > contain more than one mapping, and this will only happen when you add
> > clusters to a file that are not contiguous with the existing clusters.
> 
> Ah, that makes some sense. Not sure if it's optimal, but it's a rule I
> can work with. So just to confirm, this is the invariant that we think
> should always hold true, right?
> 
> assert((mapping->mode & MODE_DIRECTORY) ||
>!mapping->info.file.offset ||
>mapping->first_mapping_index > 0);
> 

Yes.

We can add this into `get_cluster_count_for_direntry` loop.
I'm thinking of also converting those `abort` into `assert`, since
the line `copy_it = 1;` was confusing me, since it was after the `abort`.

> > And actually, thanks to that I noticed another bug not fixed in PATCH 3, 
> > We are doing this check 
> > `s->current_mapping->first_mapping_index != mapping->first_mapping_index`
> > to know if we should switch to the new mapping or not. 
> > If we were reading from the first mapping (`first_mapping_index == -1`)
> > and we jumped to the second mapping (`first_mapping_index == n`), we
> > will catch this condition and switch to the new mapping.
> > 
> > But if the file has more than 2 mappings, and we jumped to the 3rd
> > mapping, we will not catch this since (`first_mapping_index == n`) for
> > both of them haha. I think a better check is to check the `mapping`
> > pointer directly. (I'll add it also in the next series together with a
> > test for it.)
> 
> This comparison is exactly what confused me. I didn't realise that the
> first mapping in the chain has a different value here, so I thought this
> must mean that we're looking at a different file now - but of course I
> couldn't see a reason for that because we're iterating through a single
> file in this function.
> 
> But even now that I know that the condition triggers when switching from
> the first to the second mapping, it doesn't make sense to me. We don't
> have to copy things around just because a file is non-contiguous.
> 
> What we want to catch is if the order of mappings has changed compared
> to the old state. Do we need a linked list, maybe a prev_mapping_index,
> instead of first_mapping_index so that we can compare if it is still the
> same as before?

I think this would be the better design (tbh, that's what I thought 
`first_mapping_index` would do), though not sure if other components
depend so much into the current design that it would be hard to change.

I'll try to implement this `prev_mapping_index` and see how it goes.

> 
> Or actually, I suppose that's the first block with an abort() in the
> code, just that it doesn't compare mappings, but their offsets.

I think, I'm still confused on the whole logic there, the function
`get_cluster_count_for_direntry` is a mess, and it doesn't just
*get* the clus

Re: [PATCH v4 2/4] vvfat: Fix usage of `info.file.offset`

[Amjad Alsharafi]

On Mon, Jun 10, 2024 at 06:49:43PM +0200, Kevin Wolf wrote:
> Am 05.06.2024 um 02:58 hat Amjad Alsharafi geschrieben:
> > 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.
> > 
> > Additionally, removed the `abort` when `first_mapping_index` does not
> > match, as this matches the case when adding new clusters for files, and
> > its inevitable that we reach this condition when doing that if the
> > clusters are not after one another, so there is no reason to `abort`
> > here, execution continues and the new clusters are written to disk
> > correctly.
> > 
> > Signed-off-by: Amjad Alsharafi 
> 
> Can you help me understand how first_mapping_index really works?
> 
> It seems to me that you get a chain of mappings for each file on the FAT
> filesystem, which are just the contiguous areas in it, and
> first_mapping_index refers to the mapping at the start of the file. But
> for much of the time, it actually doesn't seem to be set at all, so you
> have mapping->first_mapping_index == -1. Do you understand the rules
> around when it's set and when it isn't?

Yeah. So `first_mapping_index` is the index of the first mapping, each
mapping is a group of clusters that are contiguous in the file.
Its mostly `-1` because the first mapping will have the value set as
`-1` and not its own index, this value will only be set when the file
contain more than one mapping, and this will only happen when you add
clusters to a file that are not contiguous with the existing clusters.

And actually, thanks to that I noticed another bug not fixed in PATCH 3, 
We are doing this check 
`s->current_mapping->first_mapping_index != mapping->first_mapping_index`
to know if we should switch to the new mapping or not. 
If we were reading from the first mapping (`first_mapping_index == -1`)
and we jumped to the second mapping (`first_mapping_index == n`), we
will catch this condition and switch to the new mapping.

But if the file has more than 2 mappings, and we jumped to the 3rd
mapping, we will not catch this since (`first_mapping_index == n`) for
both of them haha. I think a better check is to check the `mapping`
pointer directly. (I'll add it also in the next series together with a
test for it.)

> 
> >  block/vvfat.c | 12 +++-
> >  1 file changed, 7 insertions(+), 5 deletions(-)
> > 
> > diff --git a/block/vvfat.c b/block/vvfat.c
> > index 19da009a5b..f0642ac3e4 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;
> > @@ -1944,7 +1947,6 @@ get_cluster_count_for_direntry(BDRVVVFATState* s, 
> > direntry_t* direntry, const ch
> >  
> >  if (mapping->first_mapping_index != first_mapping_index
> >  && mapping->info.file.offset > 0) {
> > -abort();
> >  copy_it = 1;
> >  }
> 
> I'm unsure which case this represents. If first_mapping_index refers to
> the mapping of the first cluster in the file, does this mean we got a
> mapping for a different file here? Or is the comparison between -1 and a
> real value?

Now that I think more about it, I think this `abort` is actually
correct, 

Re: [PATCH v4 4/4] iotests: Add `vvfat` tests

[Amjad Alsharafi]

On Mon, Jun 10, 2024 at 02:01:24PM +0200, Kevin Wolf wrote:
> Am 05.06.2024 um 02:58 hat Amjad Alsharafi geschrieben:
> > 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 
> > ---
> >  tests/qemu-iotests/check   |   2 +-
> >  tests/qemu-iotests/fat16.py| 635 +
> >  tests/qemu-iotests/testenv.py  |   2 +-
> >  tests/qemu-iotests/tests/vvfat | 440 
> >  tests/qemu-iotests/tests/vvfat.out |   5 +
> >  5 files changed, 1082 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/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/fat16.py b/tests/qemu-iotests/fat16.py
> > new file mode 100644
> > index 00..baf801b4d5
> > --- /dev/null
> > +++ b/tests/qemu-iotests/fat16.py
> > @@ -0,0 +1,635 @@
> > +# A simple FAT16 driver that is used to test the `vvfat` driver in QEMU.
> > +#
> > +# Copyright (C) 2024 Amjad Alsharafi 
> > +#
> > +# 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 List
> > +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 

[PATCH v4 4/4] iotests: Add `vvfat` tests

[Amjad Alsharafi]

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 
---
 tests/qemu-iotests/check   |   2 +-
 tests/qemu-iotests/fat16.py| 635 +
 tests/qemu-iotests/testenv.py  |   2 +-
 tests/qemu-iotests/tests/vvfat | 440 
 tests/qemu-iotests/tests/vvfat.out |   5 +
 5 files changed, 1082 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/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/fat16.py b/tests/qemu-iotests/fat16.py
new file mode 100644
index 00..baf801b4d5
--- /dev/null
+++ b/tests/qemu-iotests/fat16.py
@@ -0,0 +1,635 @@
+# A simple FAT16 driver that is used to test the `vvfat` driver in QEMU.
+#
+# Copyright (C) 2024 Amjad Alsharafi 
+#
+# 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 List
+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:
+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")
+self.media_descriptor = data[21]
+self.fat_size = int.from_bytes(data[22:24], "little")
+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")
+self.total_sectors = int.from_bytes(data[32:36], "little")
+self.drive_number = data[36]
+self.volume_id = int.from_bytes(data[39:43], "little")
+self.volume_label = data[43:54].decode("ascii

[PATCH v4 2/4] vvfat: Fix usage of `info.file.offset`

[Amjad Alsharafi]

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.

Additionally, removed the `abort` when `first_mapping_index` does not
match, as this matches the case when adding new clusters for files, and
its inevitable that we reach this condition when doing that if the
clusters are not after one another, so there is no reason to `abort`
here, execution continues and the new clusters are written to disk
correctly.

Signed-off-by: Amjad Alsharafi 
---
 block/vvfat.c | 12 +++-
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index 19da009a5b..f0642ac3e4 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;
@@ -1944,7 +1947,6 @@ get_cluster_count_for_direntry(BDRVVVFATState* s, 
direntry_t* direntry, const ch
 
 if (mapping->first_mapping_index != first_mapping_index
 && mapping->info.file.offset > 0) {
-abort();
 copy_it = 1;
 }
 
@@ -2404,7 +2406,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.1

[PATCH v4 3/4] vvfat: Fix reading files with non-continuous clusters

[Amjad Alsharafi]

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 
---
 block/vvfat.c | 23 ---
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index f0642ac3e4..8b4d162aa1 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -1360,15 +1360,24 @@ static int open_file(BDRVVVFATState* s,mapping_t* 
mapping)
 {
 if(!mapping)
 return -1;
+int new_path = 1;
 if(!s->current_mapping ||
-strcmp(s->current_mapping->path,mapping->path)) {
-/* open file */
-int fd = qemu_open_old(mapping->path,
+s->current_mapping->first_mapping_index
+!= mapping->first_mapping_index ||
+(new_path = strcmp(s->current_mapping->path, mapping->path))) {
+
+if (new_path) {
+/* open file */
+int fd = qemu_open_old(mapping->path,
O_RDONLY | O_BINARY | O_LARGEFILE);
-if(fd<0)
-return -1;
-vvfat_close_current_file(s);
-s->current_fd = fd;
+if (fd < 0) {
+return -1;
+}
+vvfat_close_current_file(s);
+
+s->current_fd = fd;
+}
+assert(s->current_fd);
 s->current_mapping = mapping;
 }
 return 0;
-- 
2.45.1

[PATCH v4 1/4] vvfat: Fix bug in writing to middle of file

[Amjad Alsharafi]

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 
Reviewed-by: Kevin Wolf 
Tested-by: Kevin Wolf 
---
 block/vvfat.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index 9d050ba3ae..19da009a5b 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.1

[PATCH v4 0/4] vvfat: Fix write bugs for large files and add iotests

[Amjad Alsharafi]

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.

v4:
  Applied some suggestions from Kevin Wolf :
  - Fixed code formatting by following the coding style in 
`scripts/checkpatch.pl`
  - Reduced changes related to `iotests` by setting `vvfat` format as 
non-generic.
  - Added another test to cover the fix done in `PATCH 2/4` and `PATCH 3/4` for 
handling reading/writing files with non-continuous clusters.

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-amjadsharaf...@gmail.com/
  Fix the issue of writing to the middle of the file in vvfat

Amjad Alsharafi (4):
  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

 block/vvfat.c  |  38 +-
 tests/qemu-iotests/check   |   2 +-
 tests/qemu-iotests/fat16.py| 635 +
 tests/qemu-iotests/testenv.py  |   2 +-
 tests/qemu-iotests/tests/vvfat | 440 
 tests/qemu-iotests/tests/vvfat.out |   5 +
 6 files changed, 1107 insertions(+), 15 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.1

Re: [PATCH v3 0/6] vvfat: Fix write bugs for large files and add iotests

[Amjad Alsharafi]

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

[PATCH v3 1/6] vvfat: Fix bug in writing to middle of file

[Amjad Alsharafi]

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 
---
 block/vvfat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index 9d050ba3ae..ab342f0743 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -2525,7 +2525,7 @@ 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 = s->cluster_size; 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);
-- 
2.45.0

[PATCH v3 6/6] iotests: Add `create_file` test for `vvfat` driver

[Amjad Alsharafi]

We test the ability to create new files in the filesystem, this is done by
adding an entry in the desired directory list.
The file will also be created in the host filesystem with matching filename.

Signed-off-by: Amjad Alsharafi 
---
 tests/qemu-iotests/fat16.py| 124 +++--
 tests/qemu-iotests/tests/vvfat |  29 +--
 tests/qemu-iotests/tests/vvfat.out |   4 +-
 3 files changed, 144 insertions(+), 13 deletions(-)

diff --git a/tests/qemu-iotests/fat16.py b/tests/qemu-iotests/fat16.py
index 6ac5508d8d..e86bdd0b10 100644
--- a/tests/qemu-iotests/fat16.py
+++ b/tests/qemu-iotests/fat16.py
@@ -16,9 +16,11 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 from typing import List
+import string
 
 SECTOR_SIZE = 512
 DIRENTRY_SIZE = 32
+ALLOWED_FILE_CHARS = set("!#$%&'()-@^_`{}~" + string.digits + 
string.ascii_uppercase)
 
 
 class MBR:
@@ -265,7 +267,7 @@ def write_fat_entry(self, cluster: int, value: int):
 + self.fats[fat_offset + 2 :]
 )
 self.fats_dirty_sectors.add(fat_offset // SECTOR_SIZE)
-
+
 def flush_fats(self):
 """
 Write the FATs back to the disk.
@@ -293,7 +295,7 @@ def next_cluster(self, cluster: int) -> int | None:
 raise Exception("Invalid FAT entry")
 else:
 return fat_entry
-
+
 def next_free_cluster(self) -> int:
 """
 Find the next free cluster.
@@ -338,6 +340,67 @@ def read_directory(self, cluster: int) -> 
List[FatDirectoryEntry]:
 cluster = self.next_cluster(cluster)
 return entries
 
+def add_direntry(self, cluster: int | None, name: str, ext: str, 
attributes: int):
+"""
+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):
+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, 0x)
+
+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):
 """
 Write the directory entry back to the disk.
@@ -406,9 +469,10 @@ def truncate_file(self, entry: FatDirectoryEntry, 
new_size: int):
 raise Exception(f"{entry.whole_name()} is a directory")
 
 def clusters_from_size(size: int):
-return (size + self.boot_sector.cluster_bytes() - 1) // 
self.boot_sector.cluster_bytes()
+return (
+size + self.boot_sector.cluster_bytes() - 1
+) // self.boot_sector.cluster_bytes()
 
-
 # First, allocate new FATs if we need to
 required_clust

[PATCH v3 0/6] vvfat: Fix write bugs for large files and add iotests

[Amjad Alsharafi]

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-amjadsharaf...@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

[PATCH v3 2/6] vvfat: Fix usage of `info.file.offset`

[Amjad Alsharafi]

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.

Additionally, removed the `abort` when `first_mapping_index` does not
match, as this matches the case when adding new clusters for files, and
its inevitable that we reach this condition when doing that if the
clusters are not after one another, so there is no reason to `abort`
here, execution continues and the new clusters are written to disk
correctly.

Signed-off-by: Amjad Alsharafi 
---
 block/vvfat.c | 9 -
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index ab342f0743..cb3ab81e29 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -1408,7 +1408,7 @@ 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 +1929,8 @@ 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;
@@ -1944,7 +1944,6 @@ get_cluster_count_for_direntry(BDRVVVFATState* s, 
direntry_t* direntry, const ch
 
 if (mapping->first_mapping_index != first_mapping_index
 && mapping->info.file.offset > 0) {
-abort();
 copy_it = 1;
 }
 
@@ -2404,7 +2403,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.0

[PATCH v3 5/6] iotests: Filter out `vvfat` fmt from failing tests

[Amjad Alsharafi]

`vvfat` is a special format and not all tests (even generic) can run without 
crashing.
So, added `unsupported_fmt: vvfat` to all failling tests.

Also added `vvfat` format into `meson.build`, vvfaat tests can be run on the 
`block-thorough` suite.

Signed-off-by: Amjad Alsharafi 
---
 .gitlab-ci.d/buildtest.yml | 1 +
 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/meson.build | 3 ++-
 tests/qemu-iotests/tests/export-incoming-iothread  | 2 +-
 tests/qemu-iotests/tests/fuse-allow-other  | 1 +
 tests/qemu-iotests/tests/mirror-ready-cancel-error | 2 +-
 tests/qemu-iotests/tests/regression-vhdx-log   | 1 +
 34 files changed, 35 insertions(+), 12 deletions(-)

diff --git a/.gitlab-ci.d/buildtest.yml b/.gitlab-ci.d/buildtest.yml
index cfdff175c3..a46c179a6b 100644
--- a/.gitlab-ci.d/buildtest.yml
+++ b/.gitlab-ci.d/buildtest.yml
@@ -347,6 +347,7 @@ build-tcg-disabled:
 124 132 139 142 144 145 151 152 155 157 165 194 196 200 202
 208 209 216 218 227 234 246 247 248 250 254 255 257 258
 260 261 262 263 264 270 272 273 277 279 image-fleecing
+- ./check -vvfat vvfat
 
 build-user:
   extends: .native_build_job_template
diff --git a/tests/qemu-iotests/001 b/tests/qemu-iotests/001
index 6f980fd34d..cf905b5d00 100755
--- a/tests/qemu-iotests/001
+++ b/tests/qemu-iotests/001
@@ -38,6 +38,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 . ./common.filter
 
 _supported_fmt generic
+_unsupported_fmt vvfat
 _supported_proto generic
 
 
diff --git a/tests/qemu-iotests/002 b/tests/qemu-iotests/002
index 5ce1647531..1e557fad8c 100755
--- a/tests/qemu-iotests/002
+++ b/tests/qemu-iotests/002
@@ -38,6 +38,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 . ./common.filter
 
 _supported_fmt generic
+_unsupported_fmt vvfat
 _supported_proto generic
 _unsupported_imgopts "subformat=streamOptimized"
 
diff --git a/tests/qemu-iotests/003 b/tests/qemu-iotests/003
index 03f902a83c..6e74f1faeb 100755
--- a/tests/qemu-iotests/003
+++ b/tests/qemu-iotests/003
@@ -38,6 +38,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 . ./common.filter
 
 _supported_fmt generic
+_unsupported_fmt vvfat
 _supported_proto generic
 _unsupported_imgopts "subformat=streamOptimized"
 
diff --git a/tests/qemu-iotests/005 b/tests/qemu-iotests/005
index ba377543b0..28ae66bfcd 100755
--- a/tests/qemu-iotests/005
+++ b/tests/qemu-iotests/005
@@ -41,6 +41,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 . ./common.filter
 
 _supported_fmt generic
+_unsupported_fmt vvfat
 _supported_proto generic
 _supported_os Linux
 _unsupported_imgopts "subformat=twoGbMaxExtentFlat" \
diff --git a/tests/qemu-iotests/008 b/tests/qemu-iotests/008
index fa4990b513..80850ecf12 100755
--- a/tests/qemu-iotests/008
+++ b/tests/qemu-iotests/008
@@ -38,6 +38,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 . ./common.filter
 
 _supported_fmt generic
+_unsupported_fmt vvfat
 _supported_proto generic
 
 
diff --git a/tests/qemu-iotests/009 b/tests/qemu-iotests/009
index efa852bad3..408617b0bc 100755
--- a/tests/qemu-iotests/009
+++ b/tests/qemu-iotests/009
@@ -38,6 +38,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 . ./common.filter
 
 _supported_fmt generic
+_unsupported_fmt vvfat
 _supported_proto generic
 _unsupported_imgopts "

[PATCH v3 4/6] iotests: Add `vvfat` tests

[Amjad Alsharafi]

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.

Signed-off-by: Amjad Alsharafi 
---
 tests/qemu-iotests/check   |   2 +-
 tests/qemu-iotests/fat16.py| 507 +
 tests/qemu-iotests/tests/vvfat | 400 +++
 tests/qemu-iotests/tests/vvfat.out |   5 +
 4 files changed, 913 insertions(+), 1 deletion(-)
 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/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/fat16.py b/tests/qemu-iotests/fat16.py
new file mode 100644
index 00..6ac5508d8d
--- /dev/null
+++ b/tests/qemu-iotests/fat16.py
@@ -0,0 +1,507 @@
+# A simple FAT16 driver that is used to test the `vvfat` driver in QEMU.
+#
+# Copyright (C) 2024 Amjad Alsharafi 
+#
+# 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 List
+
+SECTOR_SIZE = 512
+DIRENTRY_SIZE = 32
+
+
+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:
+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")
+self.media_descriptor = data[21]
+self.fat_size = int.from_bytes(data[22:24], "little")
+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")
+self.total_sectors = int.from_bytes(data[32:36], "little")
+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
+
+   

[PATCH v3 3/6] vvfat: Fix reading files with non-continuous clusters

[Amjad Alsharafi]

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 
---
 block/vvfat.c | 21 ++---
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index cb3ab81e29..87165abc26 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -1360,15 +1360,22 @@ static int open_file(BDRVVVFATState* s,mapping_t* 
mapping)
 {
 if(!mapping)
 return -1;
+int new_path = 1;
 if(!s->current_mapping ||
-strcmp(s->current_mapping->path,mapping->path)) {
-/* open file */
-int fd = qemu_open_old(mapping->path,
+
s->current_mapping->first_mapping_index!=mapping->first_mapping_index ||
+(new_path = strcmp(s->current_mapping->path,mapping->path))) {
+
+if (new_path) {
+/* open file */
+int fd = qemu_open_old(mapping->path,
O_RDONLY | O_BINARY | O_LARGEFILE);
-if(fd<0)
-return -1;
-vvfat_close_current_file(s);
-s->current_fd = fd;
+if(fd<0)
+return -1;
+vvfat_close_current_file(s);
+
+s->current_fd = fd;
+}
+assert(s->current_fd);
 s->current_mapping = mapping;
 }
 return 0;
-- 
2.45.0

Re: [PATCH v2 0/5] vvfat: Fix write bugs for large files and add iotests

[Amjad Alsharafi]

Explaination of the patch list:

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 and writing to the first cluster which
would pass even before this patch.

On May 4 2024, at 4:44 pm, Amjad Alsharafi  wrote:
> v2:
> Added iotests for `vvfat` driver along with a simple `fat16` module to run 
> the tests.
>
> v1:
> https://patchew.org/QEMU/20240327201231.31046-1-amjadsharaf...@gmail.com/
> Fix the issue of writing to the middle of the file in vvfat
>
> Amjad Alsharafi (5):
> 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
>
> .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 | 507 ++
> 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 | 400 ++
> tests/qemu-iotests/tests/vvfat.out | 5 +
> 39 files changed, 967 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.44.0
>

[PATCH v2 4/5] iotests: Add `vvfat` tests

[Amjad Alsharafi]

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.

Signed-off-by: Amjad Alsharafi 
---
 tests/qemu-iotests/check   |   2 +-
 tests/qemu-iotests/fat16.py| 507 +
 tests/qemu-iotests/tests/vvfat | 400 +++
 tests/qemu-iotests/tests/vvfat.out |   5 +
 4 files changed, 913 insertions(+), 1 deletion(-)
 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/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/fat16.py b/tests/qemu-iotests/fat16.py
new file mode 100644
index 00..6ac5508d8d
--- /dev/null
+++ b/tests/qemu-iotests/fat16.py
@@ -0,0 +1,507 @@
+# A simple FAT16 driver that is used to test the `vvfat` driver in QEMU.
+#
+# Copyright (C) 2024 Amjad Alsharafi 
+#
+# 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 List
+
+SECTOR_SIZE = 512
+DIRENTRY_SIZE = 32
+
+
+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:
+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")
+self.media_descriptor = data[21]
+self.fat_size = int.from_bytes(data[22:24], "little")
+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")
+self.total_sectors = int.from_bytes(data[32:36], "little")
+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
+
+   

[PATCH v2 5/5] iotests: Filter out `vvfat` fmt from failing tests

[Amjad Alsharafi]

`vvfat` is a special format and not all tests (even generic) can run without 
crashing.
So, added `unsupported_fmt: vvfat` to all failling tests.

Also added `vvfat` format into `meson.build`, vvfaat tests can be run on the 
`block-thorough` suite.

Signed-off-by: Amjad Alsharafi 
---
 .gitlab-ci.d/buildtest.yml | 1 +
 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/meson.build | 3 ++-
 tests/qemu-iotests/tests/export-incoming-iothread  | 2 +-
 tests/qemu-iotests/tests/fuse-allow-other  | 1 +
 tests/qemu-iotests/tests/mirror-ready-cancel-error | 2 +-
 tests/qemu-iotests/tests/regression-vhdx-log   | 1 +
 34 files changed, 35 insertions(+), 12 deletions(-)

diff --git a/.gitlab-ci.d/buildtest.yml b/.gitlab-ci.d/buildtest.yml
index cfdff175c3..a46c179a6b 100644
--- a/.gitlab-ci.d/buildtest.yml
+++ b/.gitlab-ci.d/buildtest.yml
@@ -347,6 +347,7 @@ build-tcg-disabled:
 124 132 139 142 144 145 151 152 155 157 165 194 196 200 202
 208 209 216 218 227 234 246 247 248 250 254 255 257 258
 260 261 262 263 264 270 272 273 277 279 image-fleecing
+- ./check -vvfat vvfat
 
 build-user:
   extends: .native_build_job_template
diff --git a/tests/qemu-iotests/001 b/tests/qemu-iotests/001
index 6f980fd34d..cf905b5d00 100755
--- a/tests/qemu-iotests/001
+++ b/tests/qemu-iotests/001
@@ -38,6 +38,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 . ./common.filter
 
 _supported_fmt generic
+_unsupported_fmt vvfat
 _supported_proto generic
 
 
diff --git a/tests/qemu-iotests/002 b/tests/qemu-iotests/002
index 5ce1647531..1e557fad8c 100755
--- a/tests/qemu-iotests/002
+++ b/tests/qemu-iotests/002
@@ -38,6 +38,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 . ./common.filter
 
 _supported_fmt generic
+_unsupported_fmt vvfat
 _supported_proto generic
 _unsupported_imgopts "subformat=streamOptimized"
 
diff --git a/tests/qemu-iotests/003 b/tests/qemu-iotests/003
index 03f902a83c..6e74f1faeb 100755
--- a/tests/qemu-iotests/003
+++ b/tests/qemu-iotests/003
@@ -38,6 +38,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 . ./common.filter
 
 _supported_fmt generic
+_unsupported_fmt vvfat
 _supported_proto generic
 _unsupported_imgopts "subformat=streamOptimized"
 
diff --git a/tests/qemu-iotests/005 b/tests/qemu-iotests/005
index ba377543b0..28ae66bfcd 100755
--- a/tests/qemu-iotests/005
+++ b/tests/qemu-iotests/005
@@ -41,6 +41,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 . ./common.filter
 
 _supported_fmt generic
+_unsupported_fmt vvfat
 _supported_proto generic
 _supported_os Linux
 _unsupported_imgopts "subformat=twoGbMaxExtentFlat" \
diff --git a/tests/qemu-iotests/008 b/tests/qemu-iotests/008
index fa4990b513..80850ecf12 100755
--- a/tests/qemu-iotests/008
+++ b/tests/qemu-iotests/008
@@ -38,6 +38,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 . ./common.filter
 
 _supported_fmt generic
+_unsupported_fmt vvfat
 _supported_proto generic
 
 
diff --git a/tests/qemu-iotests/009 b/tests/qemu-iotests/009
index efa852bad3..408617b0bc 100755
--- a/tests/qemu-iotests/009
+++ b/tests/qemu-iotests/009
@@ -38,6 +38,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 . ./common.filter
 
 _supported_fmt generic
+_unsupported_fmt vvfat
 _supported_proto generic
 _unsupported_imgopts "

[PATCH v2 2/5] vvfat: Fix usage of `info.file.offset`

[Amjad Alsharafi]

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.

Additionally, removed the `abort` when `first_mapping_index` does not
match, as this matches the case when adding new clusters for files, and
its inevitable that we reach this condition when doing that if the
clusters are not after one another, so there is no reason to `abort`
here, execution continues and the new clusters are written to disk
correctly.

Signed-off-by: Amjad Alsharafi 
---
 block/vvfat.c | 9 -
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index ab342f0743..cb3ab81e29 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -1408,7 +1408,7 @@ 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 +1929,8 @@ 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;
@@ -1944,7 +1944,6 @@ get_cluster_count_for_direntry(BDRVVVFATState* s, 
direntry_t* direntry, const ch
 
 if (mapping->first_mapping_index != first_mapping_index
 && mapping->info.file.offset > 0) {
-abort();
 copy_it = 1;
 }
 
@@ -2404,7 +2403,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.44.0

[PATCH v2 3/5] vvfat: Fix reading files with non-continuous clusters

[Amjad Alsharafi]

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 
---
 block/vvfat.c | 21 ++---
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index cb3ab81e29..87165abc26 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -1360,15 +1360,22 @@ static int open_file(BDRVVVFATState* s,mapping_t* 
mapping)
 {
 if(!mapping)
 return -1;
+int new_path = 1;
 if(!s->current_mapping ||
-strcmp(s->current_mapping->path,mapping->path)) {
-/* open file */
-int fd = qemu_open_old(mapping->path,
+
s->current_mapping->first_mapping_index!=mapping->first_mapping_index ||
+(new_path = strcmp(s->current_mapping->path,mapping->path))) {
+
+if (new_path) {
+/* open file */
+int fd = qemu_open_old(mapping->path,
O_RDONLY | O_BINARY | O_LARGEFILE);
-if(fd<0)
-return -1;
-vvfat_close_current_file(s);
-s->current_fd = fd;
+if(fd<0)
+return -1;
+vvfat_close_current_file(s);
+
+s->current_fd = fd;
+}
+assert(s->current_fd);
 s->current_mapping = mapping;
 }
 return 0;
-- 
2.44.0

[PATCH v2 0/5] vvfat: Fix write bugs for large files and add iotests

[Amjad Alsharafi]

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

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

Amjad Alsharafi (5):
  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

 .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   | 507 ++
 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| 400 ++
 tests/qemu-iotests/tests/vvfat.out|   5 +
 39 files changed, 967 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.44.0

[PATCH v2 1/5] vvfat: Fix bug in writing to middle of file

[Amjad Alsharafi]

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 
---
 block/vvfat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index 9d050ba3ae..ab342f0743 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -2525,7 +2525,7 @@ 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 = s->cluster_size; 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);
-- 
2.44.0

Re: [PATCH 0/3] vvfat: Fix bugs in writing and reading files -- PING

[Amjad Alsharafi]

Ping again

On Apr 13 2024, at 5:51 pm, Amjad Alsharafi  wrote:
> Ping to the vvfat maintainers.
>

Re: [PATCH 0/3] vvfat: Fix bugs in writing and reading files -- PING

[Amjad Alsharafi]

Ping to the vvfat maintainers.

Re: [PATCH 3/3] ffvat: Fix reading files with non-continuous clusters

[Amjad Alsharafi]

I noticed the issue in the commit message 'ffvat' should be 'vvfat',
I'll fix it in the next version.

On Thu, Mar 28, 2024 at 04:11:27AM +0800, Amjad Alsharafi wrote:
> 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 
> ---
>  block/vvfat.c | 21 ++---
>  1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/block/vvfat.c b/block/vvfat.c
> index cb3ab81e29..87165abc26 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c
> @@ -1360,15 +1360,22 @@ static int open_file(BDRVVVFATState* s,mapping_t* 
> mapping)
>  {
>  if(!mapping)
>  return -1;
> +int new_path = 1;
>  if(!s->current_mapping ||
> -strcmp(s->current_mapping->path,mapping->path)) {
> -/* open file */
> -int fd = qemu_open_old(mapping->path,
> +
> s->current_mapping->first_mapping_index!=mapping->first_mapping_index ||
> +(new_path = strcmp(s->current_mapping->path,mapping->path))) {
> +
> +if (new_path) {
> +/* open file */
> +int fd = qemu_open_old(mapping->path,
> O_RDONLY | O_BINARY | O_LARGEFILE);
> -if(fd<0)
> -return -1;
> -vvfat_close_current_file(s);
> -s->current_fd = fd;
> +if(fd<0)
> +return -1;
> +vvfat_close_current_file(s);
> +
> +s->current_fd = fd;
> +}
> +assert(s->current_fd);
>  s->current_mapping = mapping;
>  }
>  return 0;
> -- 
> 2.44.0
> 

[PATCH 1/3] vvfat: Fix bug in writing to middle of file

[Amjad Alsharafi]

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 
---
 block/vvfat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index 9d050ba3ae..ab342f0743 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -2525,7 +2525,7 @@ 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 = s->cluster_size; 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);
-- 
2.44.0

[PATCH 3/3] ffvat: Fix reading files with non-continuous clusters

[Amjad Alsharafi]

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 
---
 block/vvfat.c | 21 ++---
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index cb3ab81e29..87165abc26 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -1360,15 +1360,22 @@ static int open_file(BDRVVVFATState* s,mapping_t* 
mapping)
 {
 if(!mapping)
 return -1;
+int new_path = 1;
 if(!s->current_mapping ||
-strcmp(s->current_mapping->path,mapping->path)) {
-/* open file */
-int fd = qemu_open_old(mapping->path,
+
s->current_mapping->first_mapping_index!=mapping->first_mapping_index ||
+(new_path = strcmp(s->current_mapping->path,mapping->path))) {
+
+if (new_path) {
+/* open file */
+int fd = qemu_open_old(mapping->path,
O_RDONLY | O_BINARY | O_LARGEFILE);
-if(fd<0)
-return -1;
-vvfat_close_current_file(s);
-s->current_fd = fd;
+if(fd<0)
+return -1;
+vvfat_close_current_file(s);
+
+s->current_fd = fd;
+}
+assert(s->current_fd);
 s->current_mapping = mapping;
 }
 return 0;
-- 
2.44.0

[PATCH 0/3] vvfat: Fix bugs in writing and reading files

[Amjad Alsharafi]

These patches fix some bugs found when modifying files in vvfat.

First, there was a bug when writing to the second+ cluster of a file, it
will copy the cluster before it instead.

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.

[PATCH 2/3] vvfat: Fix usage of `info.file.offset`

[Amjad Alsharafi]

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.

Additionally, removed the `abort` when `first_mapping_index` does not
match, as this matches the case when adding new clusters for files, and
its inevitable that we reach this condition when doing that if the
clusters are not after one another, so there is no reason to `abort`
here, execution continues and the new clusters are written to disk
correctly.

Signed-off-by: Amjad Alsharafi 
---
 block/vvfat.c | 9 -
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index ab342f0743..cb3ab81e29 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -1408,7 +1408,7 @@ 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 +1929,8 @@ 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;
@@ -1944,7 +1944,6 @@ get_cluster_count_for_direntry(BDRVVVFATState* s, 
direntry_t* direntry, const ch
 
 if (mapping->first_mapping_index != first_mapping_index
 && mapping->info.file.offset > 0) {
-abort();
 copy_it = 1;
 }
 
@@ -2404,7 +2403,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.44.0