| Message ID | cover.1718195956.git.amjadsharafi10@gmail.com |
|---|---|
| Headers | show |
| Series | vvfat: Fix write bugs for large files and add iotests | expand |
Can I get review for this patch? Thanks, Best regards, Amjad On Wed, Jun 12, 2024 at 08:43:21PM +0800, Amjad Alsharafi wrote: > 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 <kwolf@redhat.com>: > - 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-amjadsharafi10@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 >
12.06.2024 15:43, Amjad Alsharafi wrote: > 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. ... > 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 Actually, maybe the whole series is a good candidate for -stable, not just the first fix. What do you think? Thanks, /mjt
On Aug 11 2024, at 3:51 pm, Michael Tokarev <mjt@tls.msk.ru> wrote: > 12.06.2024 15:43, Amjad Alsharafi wrote: >> 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. > ... >> 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 > > Actually, maybe the whole series is a good candidate for -stable, not > just the > first fix. What do you think? > > Thanks, > > /mjt Hello Michael, This actually has been reviewed and approved (last version was v6 here: https://patchew.org/QEMU/cover.1721470238.git.amjadsharafi10@gmail.com/) It has been merged into upstream here: https://gitlab.com/qemu-project/qemu/-/commit/6d00c6f982562222adbd0613966285792125abe5 Or do you perhaps mean something else by `-stable`? Thanks, Amjad > > -- > GPG Key transition (from rsa2048 to rsa4096) since 2024-04-24. > New key: rsa4096/61AD3D98ECDF2C8E 9D8B E14E 3F2A 9DD7 9199 28F1 61AD > 3D98 ECDF 2C8E > Old key: rsa2048/457CE0A0804465C5 6EE1 95D1 886E 8FFB 810D 4324 457C > E0A0 8044 65C5 > Transition statement: http://www.corpit.ru/mjt/gpg-transition-2024.txt > >
11.08.2024 12:52, Amjad Alsharafi wrote: Hi! > This actually has been reviewed and approved (last version was v6 here: https://patchew.org/QEMU/cover.1721470238.git.amjadsharafi10@gmail.com/) > It has been merged into upstream here: https://gitlab.com/qemu-project/qemu/-/commit/6d00c6f982562222adbd0613966285792125abe5 Yes I know, this is how I spotted it - by checking which patches has been applied to qemu master branch. > Or do you perhaps mean something else by `-stable`? Please see: https://www.qemu.org/docs/master/devel/stable-process.html Current active qemu stable series (branches) are 7.2.x (stable-7.2, staging-7.2) and 9.0.x (stable-9.0, staging-9.0). Thanks, /mjt
On Aug 11 2024, at 6:09 pm, Michael Tokarev <mjt@tls.msk.ru> wrote: > 11.08.2024 12:52, Amjad Alsharafi wrote: > > Hi! > >> This actually has been reviewed and approved (last version was v6 >> here: https://patchew.org/QEMU/cover.1721470238.git.amjadsharafi10@gmail.com/) >> It has been merged into upstream here: https://gitlab.com/qemu-project/qemu/-/commit/6d00c6f982562222adbd0613966285792125abe5 > > Yes I know, this is how I spotted it - by checking which patches has been > applied to qemu master branch. > >> Or do you perhaps mean something else by `-stable`? > > Please see: https://www.qemu.org/docs/master/devel/stable-process.html > > Current active qemu stable series (branches) are 7.2.x (stable-7.2, staging-7.2) > and 9.0.x (stable-9.0, staging-9.0). > > Thanks, > > /mjt > Thanks for the info Michael. It would be great to include this patch in `stable`. Thank you. Best, Amjad
11.08.2024 13:19, Amjad Alsharafi wrote:
...
> It would be great to include this patch in `stable`. Thank you.
The question here is whenever I should include whole series
(5 patches) or just one? I picked up all 5 for now.
Thanks,
/mjt
On Aug 11 2024, at 10:45 pm, Michael Tokarev <mjt@tls.msk.ru> wrote: > The question here is whenever I should include whole series > (5 patches) or just one? I picked up all 5 for now. Yeah, we should include all of them (at least the first 4 are required for the actual fixes), the last one is unit test for these fixes. Thanks, Amjad
Am 11.08.2024 um 09:51 hat Michael Tokarev geschrieben: > 12.06.2024 15:43, Amjad Alsharafi wrote: > > 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. > ... > > 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 > > Actually, maybe the whole series is a good candidate for -stable, not > just the first fix. What do you think? Yes, if you consider vvfat relevant for stable at all, then I think you want to take all of the fixes, each one fixes some corruption in read-write mode. (Though as far as I can tell, read-write support is still broken even after this series.) Kevin
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 <kwolf@redhat.com>: - 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-amjadsharafi10@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