| Message ID | 2871281d8ea41fb4d7ef8f9beeaba017a1717019.1716717181.git.amjadsharafi10@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | vvfat: Fix write bugs for large files and add iotests | expand |
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
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);
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(-)