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

[Qemu-devel] [PATCH v2 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.

v2:
- Explicit pointer cast. Not sure why my gcc didn't warn. [Max]
- Coding style. In vvfat. Will be hard to beat the pointlessness of
  that review comment. [Max]

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

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

-- 
1.8.3.1

[Qemu-devel] [PATCH v2 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 | 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));
+
+        /* The first directory entry on the filesystem is the volume name */
+        first_direntry = (direntry_t*) s->directory.pointer;
+        assert(!memcmp(first_direntry->name, s->volume_label, 11));
+
 	current_dir_index += factor;
     }
 
-- 
1.8.3.1

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

[Peter Maydell]

On 28 April 2016 at 12:36, Kevin Wolf <kwolf@redhat.com> 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.

So you couldn't use this for writing at all, and we broke this
a year ago, and nobody complained til now? Shows how little
vvfat gets used...

thanks
-- PMM

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

[Markus Armbruster]

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

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 802 bytes --]

On Thu, Apr 28, 2016 at 01:36:05PM +0200, 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 | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)

I just noticed that Wolfgang's original patch got rid of the default
"QEMU VVFAT " volume label.  It now defaults to all spaces but I'm not
sure if this causes any problems...

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

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

[Qemu-devel] [PATCH v2 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>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block/vvfat.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/block/vvfat.c b/block/vvfat.c
index ff3df35..183fc4f 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 v2 2/2] vvfat: Fix default volume label

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> 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.

Hrmph.

> This fixes the implementation to apply the advertised default.
>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/vvfat.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/block/vvfat.c b/block/vvfat.c
> index ff3df35..183fc4f 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) {

Reviewed-by: Markus Armbruster <armbru@redhat.com>

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 611 bytes --]

On Thu, Apr 28, 2016 at 01:36:06PM +0200, 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>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/vvfat.c | 2 ++
>  1 file changed, 2 insertions(+)

Nice, thanks for fixing this.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

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

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

[Peter Maydell]

On 28 April 2016 at 12:36, Kevin Wolf <kwolf@redhat.com> wrote:
> 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.
>
> v2:
> - Explicit pointer cast. Not sure why my gcc didn't warn. [Max]
> - Coding style. In vvfat. Will be hard to beat the pointlessness of
>   that review comment. [Max]
>
> Kevin Wolf (2):
>   vvfat: Fix volume name assertion
>   vvfat: Fix default volume label
>
>  block/vvfat.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)

I think we could apply this to master tomorrow; review for patch 1
would be nice.

thanks
-- PMM