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

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

Signed-off-by: Amjad Alsharafi <amjadsharafi10@gmail.com>
---
 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

Am 26.05.2024 um 11:56 hat Amjad Alsharafi geschrieben:
> Before this commit, the behavior when calling `commit_one_file` for
> example with `offset=0x2000` (second cluster), what will happen is that
> we won't fetch the next cluster from the fat, and instead use the first
> cluster for the read operation.
> 
> This is due to off-by-one error here, where `i=0x2000 !< offset=0x2000`,
> thus not fetching the next cluster.
> 
> Signed-off-by: Amjad Alsharafi <amjadsharafi10@gmail.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Tested-by: Kevin Wolf <kwolf@redhat.com>

> 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);

While your change results in the correct behaviour, I think I would
prefer the code to be changed like this so that at the start of each
loop iteration, 'i' always refers to the offset that matches 'c':

    for (i = 0; i < offset; i += s->cluster_size) {
        c = modified_fat_get(s, c);
    }

I'm also adding braces here to make the code conform with the QEMU
coding style. You can use scripts/checkpatch.pl to make sure that all
code you add has the correct style. Much of the vvfat code predates the
coding style, so you'll often have to change style when you touch a
line. (Which is good, because it slowly fixes the existing mess.)

You can keep my Reviewed/Tested-by if you change this.

Kevin