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

[Max Reitz]

On 27.04.2016 14:24, Kevin Wolf wrote:
> 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 | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/block/vvfat.c b/block/vvfat.c
> index 6b85314..f9d4e82 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c
> @@ -2288,7 +2288,11 @@ DLOG(fprintf(stderr, "commit_direntries for %s, 
> parent_mapping_index %d\n", mapp
>               s->sectors_per_cluster);
>       if (ret)
>           return ret;
> -     assert(!strncmp(s->directory.pointer, "QEMU", 4));
> +
> +        /* The first directory entry on the filesystem is the volume name */
> +        direntry_t *first_direntry = s->directory.pointer;

I am afraid this does not conform to the QEMU coding style. Declaration
of variables ("first_direntry" in this case) needs to be done at the
beginning of the block, and this block starts with the for () loop.

Fortunately, the vvfat code provides plenty of examples on how to do
this right. I strongly support choosing the well-established style of
creating a non-conditional block just around these three lines so that
the declaration does not need to be moved. See the declaration of the
direntry_t pointer "entry" in init_directories(), for instance.

> +        assert(!memcmp(first_direntry->name, s->volume_label, 11));

The code mentioned above also teaches us that we can use
sizeof(first_direntry->name) instead of 11. However, I'll leave that up
to you.

Max

> +
>       current_dir_index += factor;
>      }
>  
>

signature.asc
Description: OpenPGP digital signature