| Message ID | cb635e58663b2dd02baa8e015dbc7fd57da89e46.1718195956.git.amjadsharafi10@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vvfat: Fix write bugs for large files and add iotests | expand |
Am 12.06.2024 um 14:43 hat Amjad Alsharafi geschrieben: > How this `abort` was intended to check for was: > - if the `mapping->first_mapping_index` is not the same as > `first_mapping_index`, which **should** happen only in one case, > when we are handling the first mapping, in that case > `mapping->first_mapping_index == -1`, in all other cases, the other > mappings after the first should have the condition `true`. > - From above, we know that this is the first mapping, so if the offset > is not `0`, then abort, since this is an invalid state. > > The issue was that `first_mapping_index` is not set if we are > checking from the middle, the variable `first_mapping_index` is > only set if we passed through the check `cluster_was_modified` with the > first mapping, and in the same function call we checked the other > mappings. > > One approach is to go into the loop even if `cluster_was_modified` > is not true so that we will be able to set `first_mapping_index` for the > first mapping, but since `first_mapping_index` is only used here, > another approach is to just check manually for the > `mapping->first_mapping_index != -1` since we know that this is the > value for the only entry where `offset == 0` (i.e. first mapping). > > Signed-off-by: Amjad Alsharafi <amjadsharafi10@gmail.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
diff --git a/block/vvfat.c b/block/vvfat.c index 247b232608..b63ac5d045 100644 --- a/block/vvfat.c +++ b/block/vvfat.c @@ -1880,7 +1880,6 @@ get_cluster_count_for_direntry(BDRVVVFATState* s, direntry_t* direntry, const ch uint32_t cluster_num = begin_of_direntry(direntry); uint32_t offset = 0; - int first_mapping_index = -1; mapping_t* mapping = NULL; const char* basename2 = NULL; @@ -1942,14 +1941,9 @@ get_cluster_count_for_direntry(BDRVVVFATState* s, direntry_t* direntry, const ch if (strcmp(basename, basename2)) copy_it = 1; - first_mapping_index = array_index(&(s->mapping), mapping); - } - - if (mapping->first_mapping_index != first_mapping_index - && mapping->info.file.offset > 0) { - abort(); - copy_it = 1; } + assert(mapping->first_mapping_index == -1 + || mapping->info.file.offset > 0); /* need to write out? */ if (!was_modified && is_file(direntry)) {
How this `abort` was intended to check for was: - if the `mapping->first_mapping_index` is not the same as `first_mapping_index`, which **should** happen only in one case, when we are handling the first mapping, in that case `mapping->first_mapping_index == -1`, in all other cases, the other mappings after the first should have the condition `true`. - From above, we know that this is the first mapping, so if the offset is not `0`, then abort, since this is an invalid state. The issue was that `first_mapping_index` is not set if we are checking from the middle, the variable `first_mapping_index` is only set if we passed through the check `cluster_was_modified` with the first mapping, and in the same function call we checked the other mappings. One approach is to go into the loop even if `cluster_was_modified` is not true so that we will be able to set `first_mapping_index` for the first mapping, but since `first_mapping_index` is only used here, another approach is to just check manually for the `mapping->first_mapping_index != -1` since we know that this is the value for the only entry where `offset == 0` (i.e. first mapping). Signed-off-by: Amjad Alsharafi <amjadsharafi10@gmail.com> --- block/vvfat.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-)