[Qemu-devel] [PATCH 0/2] vvfat: Fix regressions introduced in 2.4

[Qemu-devel] [PATCH 0/2] vvfat: Fix regressions introduced in 2.4

[Kevin Wolf]

Commit d5941dd added a little new feature and broke a few things (including the
whole write support) while doing so. Let's fix this now and do a bit more
careful testing when touching the driver in the future.

Kevin Wolf (2):
  vvfat: Fix volume name assertion
  vvfat: Fix default volume label

 block/vvfat.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

-- 
1.8.3.1

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

[Kevin Wolf]

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: qemu-stable@nongnu.org
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 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;
+        assert(!memcmp(first_direntry->name, s->volume_label, 11));
+
 	current_dir_index += factor;
     }
 
-- 
1.8.3.1

[Qemu-devel] [PATCH 2/2] vvfat: Fix default volume label

[Kevin Wolf]

Commit d5941dd documented that it leaves the default volume name as it
was ("QEMU VVFAT"), but it doesn't actually implement this. You get an
empty name (eleven space characters) instead.

This fixes the implementation to apply the advertised default.

Cc: qemu-stable@nongnu.org
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/vvfat.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/block/vvfat.c b/block/vvfat.c
index f9d4e82..e4817dd 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -1109,6 +1109,8 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
             goto fail;
         }
         memcpy(s->volume_label, label, label_length);
+    } else {
+        memcpy(s->volume_label, "QEMU VVFAT", 10);
     }
 
     if (floppy) {
-- 
1.8.3.1

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

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 1904 bytes --]

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: qemu-stable@nongnu.org
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  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;
>      }
>  
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 2/2] vvfat: Fix default volume label

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 505 bytes --]

On 27.04.2016 14:24, Kevin Wolf wrote:
> Commit d5941dd documented that it leaves the default volume name as it
> was ("QEMU VVFAT"), but it doesn't actually implement this. You get an
> empty name (eleven space characters) instead.
> 
> This fixes the implementation to apply the advertised default.
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/vvfat.c | 2 ++
>  1 file changed, 2 insertions(+)

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]