Re: [PATCH v4 2/4] vvfat: Fix usage of `info.file.offset`

[Kevin Wolf <kwolf@redhat.com>]

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