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