Re: [Qemu-devel] [PATCH v2 1/2] vvfat: Fix volume name assertion

[Markus Armbruster]

Kevin Wolf <address@hidden> writes:

> Commit d5941dd made the volume name configurable, but it didn't consider
> that the rw code compares the volume name string to assert that the
> first directory entry is the volume name. This made vvfat crash in rw
> mode.
>
> This fixes the assertion to compare with the configured volume name
> instead of a literal string.
>
> Cc: address@hidden
> Signed-off-by: Kevin Wolf <address@hidden>
> ---
>  block/vvfat.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/block/vvfat.c b/block/vvfat.c
> index 6b85314..ff3df35 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c
> @@ -2283,12 +2283,17 @@ DLOG(fprintf(stderr, "commit_direntries for %s, 
> parent_mapping_index %d\n", mapp
>               factor * (old_cluster_count - new_cluster_count));
>  
>      for (c = first_cluster; !fat_eof(s, c); c = modified_fat_get(s, c)) {
> +        direntry_t *first_direntry;
>       void* direntry = array_get(&(s->directory), current_dir_index);
>       int ret = vvfat_read(s->bs, cluster2sector(s, c), direntry,
>               s->sectors_per_cluster);
>       if (ret)
>           return ret;
> -     assert(!strncmp(s->directory.pointer, "QEMU", 4));

Typing all of "QEMU VVAT" a third time was clearly too much.

> +
> +        /* The first directory entry on the filesystem is the volume name */
> +        first_direntry = (direntry_t*) s->directory.pointer;

I'd ask to correct the spacing to (direntry_t *)s if the spacing wasn't
similarly off all over this file.

> +        assert(!memcmp(first_direntry->name, s->volume_label, 11));
> +
>       current_dir_index += factor;
>      }

Might want to to assert is_volume_label(), too.  But even if you want
to, let's not delay the fix for that.

Reviewed-by: Markus Armbruster <address@hidden>