[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v4 2/4] vvfat: Fix usage of `info.file.offset`
From: |
Kevin Wolf |
Subject: |
Re: [PATCH v4 2/4] vvfat: Fix usage of `info.file.offset` |
Date: |
Mon, 10 Jun 2024 18:49:43 +0200 |
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 <amjadsharafi10@gmail.com>
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?
> 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?
In any case it doesn't seem to be the case that the comment at the
declaration of copy_it describes.
>
> @@ -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;
> }
Kevin
[PATCH v4 3/4] vvfat: Fix reading files with non-continuous clusters, Amjad Alsharafi, 2024/06/04
[PATCH v4 4/4] iotests: Add `vvfat` tests, Amjad Alsharafi, 2024/06/04