[Qemu-devel] [PATCH 0/6] misc vvfat fixes

[Qemu-devel] [PATCH 0/6] misc vvfat fixes

[Paolo Bonzini]

It occurred to me that, if there's one thing vvfat ought to be good
at, it is creating disk images with qemu-img convert (a driver disk
in my case).

It turns out the use case is really broken.  qemu-img doesn't
complete at all, the resulting images often do not pass fsck,
and it's impossible to create a 1.44 MB disk image.  This
series fixes all of the small problems I found.

Coding standard in this file is such a pain that I hardly bothered
about it.


Paolo Bonzini (6):
  vvfat: fix out of bounds array_get usage
  vvfat: do not fail if the disk has spare sectors
  vvfat: need to use first_sectors_number to distinguish fdd/hdd
  vvfat: unify and correct computation of sector count
  vvfat: do not hardcode sector counts in error message
  vvfat: reorganize computation of disk geometry

 block/vvfat.c |   50 ++++++++++++++++++++++++--------------------------
 3 files changed, 26 insertions(+), 28 deletions(-)

-- 
1.7.6

[Qemu-devel] [PATCH 1/6] vvfat: fix out of bounds array_get usage

[Paolo Bonzini]

When reading the address of the first free entry, you cannot
use array_get without first marking all entries as occupied.

This is visible if you change the sectors per cluster on a
floppy from 2 to 1.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/vvfat.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index f567c9a..cee3971 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -799,6 +799,7 @@ static int read_directory(BDRVVVFATState* s, int mapping_index)
 	/* root directory */
 	int cur = s->directory.next;
 	array_ensure_allocated(&(s->directory), ROOT_ENTRIES - 1);
+	s->directory.next = ROOT_ENTRIES;
 	memset(array_get(&(s->directory), cur), 0,
 		(ROOT_ENTRIES - cur) * sizeof(direntry_t));
     }
-- 
1.7.6

[Qemu-devel] [PATCH 2/6] vvfat: do not fail if the disk has spare sectors

[Paolo Bonzini]

If the number of "faked sectors" + the number of sectors that are
part of a cluster does not sum up to the total number of sectors,
qemu-img convert fails.  Read these spare sectors as all zeros.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/vvfat.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index cee3971..bc2dd4c 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -1294,7 +1294,7 @@ static int vvfat_read(BlockDriverState *bs, int64_t sector_num,
     int i;
 
     for(i=0;i<nb_sectors;i++,sector_num++) {
-	if (sector_num >= s->sector_count)
+	if (sector_num >= bs->total_sectors)
 	   return -1;
 	if (s->qcow) {
 	    int n;
@@ -1320,7 +1320,7 @@ DLOG(fprintf(stderr, "sector %d not allocated\n", (int)sector_num));
 	    uint32_t sector=sector_num-s->faked_sectors,
 	    sector_offset_in_cluster=(sector%s->sectors_per_cluster),
 	    cluster_num=sector/s->sectors_per_cluster;
-	    if(read_cluster(s, cluster_num) != 0) {
+	    if(cluster_num > s->cluster_count || read_cluster(s, cluster_num) != 0) {
 		/* LATER TODO: strict: return -1; */
 		memset(buf+i*0x200,0,0x200);
 		continue;
-- 
1.7.6

[Qemu-devel] [PATCH 3/6] vvfat: need to use first_sectors_number to distinguish fdd/hdd

[Paolo Bonzini]

This is consistent with what "real" floppies have, so file(1)
now actually recognizes the VVFAT image as a 1.44 MB floppy.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/vvfat.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index bc2dd4c..eb91642 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -968,7 +968,7 @@ static int init_directories(BDRVVVFATState* s,
     bootsector->number_of_fats=0x2; /* number of FATs */
     bootsector->root_entries=cpu_to_le16(s->sectors_of_root_directory*0x10);
     bootsector->total_sectors16=s->sector_count>0xffff?0:cpu_to_le16(s->sector_count);
-    bootsector->media_type=(s->fat_type!=12?0xf8:s->sector_count==5760?0xf9:0xf8); /* media descriptor */
+    bootsector->media_type=(s->first_sectors_number>1?0xf8:0xf0); /* media descriptor (f8=hd, f0=3.5 fd)*/
     s->fat.pointer[0] = bootsector->media_type;
     bootsector->sectors_per_fat=cpu_to_le16(s->sectors_per_fat);
     bootsector->sectors_per_track=cpu_to_le16(s->bs->secs);
@@ -977,7 +977,7 @@ static int init_directories(BDRVVVFATState* s,
     bootsector->total_sectors=cpu_to_le32(s->sector_count>0xffff?s->sector_count:0);
 
     /* LATER TODO: if FAT32, this is wrong */
-    bootsector->u.fat16.drive_number=s->fat_type==12?0:0x80; /* assume this is hda (TODO) */
+    bootsector->u.fat16.drive_number=s->first_sectors_number==1?0:0x80; /* fda=0, hda=0x80 */
     bootsector->u.fat16.current_head=0;
     bootsector->u.fat16.signature=0x29;
     bootsector->u.fat16.id=cpu_to_le32(0xfabe1afd);
-- 
1.7.6

[Qemu-devel] [PATCH 4/6] vvfat: unify and correct computation of sector count

[Paolo Bonzini]

The sector count is stored in the partition and hence must not include the
sectors before its start.  At the same time, remove the useless special
casing for 1.44 MB floppies.  This fixes fsck on VVFAT hard disks,
which otherwise tries to seek past the end of the disk.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/vvfat.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index eb91642..a682eae 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -1040,8 +1040,6 @@ DLOG(if (stderr == NULL) {
 	bs->cyls = 80; bs->heads = 2; bs->secs = 36;
     }
 
-    s->sector_count=bs->cyls*bs->heads*bs->secs;
-
     if (strstr(dirname, ":32:")) {
 	fprintf(stderr, "Big fat greek warning: FAT32 has not been tested. You are welcome to do so!\n");
 	s->fat_type = 32;
@@ -1049,9 +1047,11 @@ DLOG(if (stderr == NULL) {
 	s->fat_type = 16;
     } else if (strstr(dirname, ":12:")) {
 	s->fat_type = 12;
-	s->sector_count=2880;
+	bs->secs = 18;
     }
 
+    s->sector_count=bs->cyls*bs->heads*bs->secs-(s->first_sectors_number-1);
+
     if (strstr(dirname, ":rw:")) {
 	if (enable_write_target(s))
 	    return -1;
-- 
1.7.6

[Qemu-devel] [PATCH 5/6] vvfat: do not hardcode sector counts in error message

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/vvfat.c |    7 ++-----
 1 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index a682eae..fb3d62e 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -930,11 +930,8 @@ static int init_directories(BDRVVVFATState* s,
 	cluster = mapping->end;
 
 	if(cluster > s->cluster_count) {
-	    fprintf(stderr,"Directory does not fit in FAT%d (capacity %s)\n",
-		    s->fat_type,
-		    s->fat_type == 12 ? s->sector_count == 2880 ? "1.44 MB"
-								: "2.88 MB"
-				      : "504MB");
+	    fprintf(stderr,"Directory does not fit in FAT%d (capacity %.2f MB)\n",
+		    s->fat_type, s->sector_count / 2000.0);
 	    return -EINVAL;
 	}
 
-- 
1.7.6

[Qemu-devel] [PATCH 6/6] vvfat: reorganize computation of disk geometry

[Paolo Bonzini]

First determine FAT12/16/32, then compute geometry from that for both
FDD and HDD.  For floppies, change to 1 sector/cluster since the limit
for that is ~2 MB for FAT12 (which we use for 1.44 MB floppies only)
and well over 2.88 MB for FAT16.

This matches the actual format used by 1.44 MB floppies.  Real 2.88 MB
floppies used FAT12 with 2 sectors/cluster.  This patch does not make
things worse than they were before it, though.

FAT32 is totally broken.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/vvfat.c |   30 +++++++++++++++---------------
 1 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index fb3d62e..1a0216c 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -996,7 +996,6 @@ static int is_consistent(BDRVVVFATState *s);
 static int vvfat_open(BlockDriverState *bs, const char* dirname, int flags)
 {
     BDRVVVFATState *s = bs->opaque;
-    int floppy = 0;
     int i;
 
 #ifdef DEBUG
@@ -1013,8 +1012,6 @@ DLOG(if (stderr == NULL) {
     s->fat_type=16;
     /* LATER TODO: if FAT32, adjust */
     s->sectors_per_cluster=0x10;
-    /* 504MB disk*/
-    bs->cyls=1024; bs->heads=16; bs->secs=63;
 
     s->current_cluster=0xffffffff;
 
@@ -1029,14 +1026,6 @@ DLOG(if (stderr == NULL) {
     if (!strstart(dirname, "fat:", NULL))
 	return -1;
 
-    if (strstr(dirname, ":floppy:")) {
-	floppy = 1;
-	s->fat_type = 12;
-	s->first_sectors_number = 1;
-	s->sectors_per_cluster=2;
-	bs->cyls = 80; bs->heads = 2; bs->secs = 36;
-    }
-
     if (strstr(dirname, ":32:")) {
 	fprintf(stderr, "Big fat greek warning: FAT32 has not been tested. You are welcome to do so!\n");
 	s->fat_type = 32;
@@ -1044,7 +1033,18 @@ DLOG(if (stderr == NULL) {
 	s->fat_type = 16;
     } else if (strstr(dirname, ":12:")) {
 	s->fat_type = 12;
-	bs->secs = 18;
+    }
+
+    if (strstr(dirname, ":floppy:")) {
+	/* Real ED floppies used FAT12, 2 sec/cluster.  Take a shortcut.  */
+	s->first_sectors_number = 1;
+	s->sectors_per_cluster=1;
+	bs->cyls=80; bs->heads=2;
+	bs->secs=(s->fat_type == 12 ? 18 : 36);
+    } else {
+	/* 32MB or 504MB disk*/
+	bs->cyls=(s->fat_type == 12 ? 64 : 1024);
+	bs->heads=16; bs->secs=63;
     }
 
     s->sector_count=bs->cyls*bs->heads*bs->secs-(s->first_sectors_number-1);
@@ -1072,10 +1072,10 @@ DLOG(if (stderr == NULL) {
 
     if(s->first_sectors_number==0x40)
 	init_mbr(s);
-
-    /* for some reason or other, MS-DOS does not like to know about CHS... */
-    if (floppy)
+    else {
+        /* MS-DOS does not like to know about CHS (?). */
 	bs->heads = bs->cyls = bs->secs = 0;
+    }
 
     //    assert(is_consistent(s));
     return 0;
-- 
1.7.6

[Qemu-devel] [PATCH 6/6 v2] vvfat: reorganize computation of disk geometry

[Paolo Bonzini]

First determine FAT12/16/32, then compute geometry from that for both
FDD and HDD.  For 1.44MB floppies, and 2.88MB floppies using FAT16,
change to 1 sector/cluster.  The default remains 2.88MB with FAT12
and 2 sectors/cluster.  Both DOS and mkdosfs by default format a 2.88MB
floppy as FAT12.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
	v1->v2: do not change default 2.88MB format to FAT16

 block/vvfat.c |   38 ++++++++++++++++++++++++----------------
 1 files changed, 24 insertions(+), 16 deletions(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index faf2947..8511fe5 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -982,7 +982,6 @@ static int is_consistent(BDRVVVFATState *s);
 static int vvfat_open(BlockDriverState *bs, const char* dirname, int flags)
 {
     BDRVVVFATState *s = bs->opaque;
-    int floppy = 0;
     int i;
 
 #ifdef DEBUG
@@ -996,11 +995,8 @@ DLOG(if (stderr == NULL) {
 
     s->bs = bs;
 
-    s->fat_type=16;
     /* LATER TODO: if FAT32, adjust */
     s->sectors_per_cluster=0x10;
-    /* 504MB disk*/
-    bs->cyls=1024; bs->heads=16; bs->secs=63;
 
     s->current_cluster=0xffffffff;
 
@@ -1015,14 +1011,6 @@ DLOG(if (stderr == NULL) {
     if (!strstart(dirname, "fat:", NULL))
 	return -1;
 
-    if (strstr(dirname, ":floppy:")) {
-	floppy = 1;
-	s->fat_type = 12;
-	s->first_sectors_number = 1;
-	s->sectors_per_cluster=2;
-	bs->cyls = 80; bs->heads = 2; bs->secs = 36;
-    }
-
     if (strstr(dirname, ":32:")) {
 	fprintf(stderr, "Big fat greek warning: FAT32 has not been tested. You are welcome to do so!\n");
 	s->fat_type = 32;
@@ -1030,7 +1018,27 @@ DLOG(if (stderr == NULL) {
 	s->fat_type = 16;
     } else if (strstr(dirname, ":12:")) {
 	s->fat_type = 12;
-	bs->secs = 18;
+    }
+
+    if (strstr(dirname, ":floppy:")) {
+	/* 1.44MB or 2.88MB floppy.  2.88MB can be FAT12 (default) or FAT16. */
+	if (!s->fat_type) {
+	    s->fat_type = 12;
+	    bs->secs = 36;
+	    s->sectors_per_cluster=2;
+	} else {
+	    bs->secs=(s->fat_type == 12 ? 18 : 36);
+	    s->sectors_per_cluster=1;
+	}
+	s->first_sectors_number = 1;
+	bs->cyls=80; bs->heads=2;
+    } else {
+	/* 32MB or 504MB disk*/
+	if (!s->fat_type) {
+	    s->fat_type = 16;
+	}
+	bs->cyls=(s->fat_type == 12 ? 64 : 1024);
+	bs->heads=16; bs->secs=63;
     }
 
     s->sector_count=bs->cyls*bs->heads*bs->secs-(s->first_sectors_number-1);
@@ -1058,10 +1066,10 @@ DLOG(if (stderr == NULL) {
 
     if(s->first_sectors_number==0x40)
 	init_mbr(s);
-
-    /* for some reason or other, MS-DOS does not like to know about CHS... */
-    if (floppy)
+    else {
+        /* MS-DOS does not like to know about CHS (?). */
 	bs->heads = bs->cyls = bs->secs = 0;
+    }
 
     //    assert(is_consistent(s));
     qemu_co_mutex_init(&s->lock);
-- 
1.7.6.4

Re: [Qemu-devel] [PATCH 6/6 v2] vvfat: reorganize computation of disk geometry

[Kevin Wolf]

Am 04.11.2011 17:21, schrieb Paolo Bonzini:
> First determine FAT12/16/32, then compute geometry from that for both
> FDD and HDD.  For 1.44MB floppies, and 2.88MB floppies using FAT16,
> change to 1 sector/cluster.  The default remains 2.88MB with FAT12
> and 2 sectors/cluster.  Both DOS and mkdosfs by default format a 2.88MB
> floppy as FAT12.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> 	v1->v2: do not change default 2.88MB format to FAT16

Thanks, applied all to the block-stable branch (for 1.0).

Kevin

[Qemu-devel] ping Re: [PATCH 0/6] misc vvfat fixes

[Paolo Bonzini]

On 10/05/2011 09:12 AM, Paolo Bonzini wrote:
> It occurred to me that, if there's one thing vvfat ought to be good
> at, it is creating disk images with qemu-img convert (a driver disk
> in my case).
>
> It turns out the use case is really broken.  qemu-img doesn't
> complete at all, the resulting images often do not pass fsck,
> and it's impossible to create a 1.44 MB disk image.  This
> series fixes all of the small problems I found.
>
> Coding standard in this file is such a pain that I hardly bothered
> about it.
>
>
> Paolo Bonzini (6):
>    vvfat: fix out of bounds array_get usage
>    vvfat: do not fail if the disk has spare sectors
>    vvfat: need to use first_sectors_number to distinguish fdd/hdd
>    vvfat: unify and correct computation of sector count
>    vvfat: do not hardcode sector counts in error message
>    vvfat: reorganize computation of disk geometry
>
>   block/vvfat.c |   50 ++++++++++++++++++++++++--------------------------
>   3 files changed, 26 insertions(+), 28 deletions(-)
>

ping?

Paolo

Re: [Qemu-devel] ping Re: [PATCH 0/6] misc vvfat fixes

[Kevin Wolf]

Am 27.10.2011 13:46, schrieb Paolo Bonzini:
> On 10/05/2011 09:12 AM, Paolo Bonzini wrote:
>> It occurred to me that, if there's one thing vvfat ought to be good
>> at, it is creating disk images with qemu-img convert (a driver disk
>> in my case).
>>
>> It turns out the use case is really broken.  qemu-img doesn't
>> complete at all, the resulting images often do not pass fsck,
>> and it's impossible to create a 1.44 MB disk image.  This
>> series fixes all of the small problems I found.
>>
>> Coding standard in this file is such a pain that I hardly bothered
>> about it.
>>
>>
>> Paolo Bonzini (6):
>>    vvfat: fix out of bounds array_get usage
>>    vvfat: do not fail if the disk has spare sectors
>>    vvfat: need to use first_sectors_number to distinguish fdd/hdd
>>    vvfat: unify and correct computation of sector count
>>    vvfat: do not hardcode sector counts in error message
>>    vvfat: reorganize computation of disk geometry
>>
>>   block/vvfat.c |   50 ++++++++++++++++++++++++--------------------------
>>   3 files changed, 26 insertions(+), 28 deletions(-)
>>
> 
> ping?

Looked at it a week or two ago, didn't immediately understand the first
patch and decided that there's more important stuff for 1.0...

Not sure what to do with it. The subject clearly says "fixes", so it
should qualify for 1.0, but someone must review it.

Kevin

Re: [Qemu-devel] ping Re: [PATCH 0/6] misc vvfat fixes

[Paolo Bonzini]

On 10/27/2011 02:38 PM, Kevin Wolf wrote:
> Am 27.10.2011 13:46, schrieb Paolo Bonzini:
>> On 10/05/2011 09:12 AM, Paolo Bonzini wrote:
>>> It occurred to me that, if there's one thing vvfat ought to be good
>>> at, it is creating disk images with qemu-img convert (a driver disk
>>> in my case).
>>>
>>> It turns out the use case is really broken.  qemu-img doesn't
>>> complete at all, the resulting images often do not pass fsck,
>>> and it's impossible to create a 1.44 MB disk image.  This
>>> series fixes all of the small problems I found.
>>>
>>> Coding standard in this file is such a pain that I hardly bothered
>>> about it.
>>>
>>>
>>> Paolo Bonzini (6):
>>>     vvfat: fix out of bounds array_get usage
>>>     vvfat: do not fail if the disk has spare sectors the
>>>     vvfat: need to use first_sectors_number to distinguish fdd/hdd
>>>     vvfat: unify and correct computation of sector count
>>>     vvfat: do not hardcode sector counts in error message
>>>     vvfat: reorganize computation of disk geometry
>>>
>>>    block/vvfat.c |   50 ++++++++++++++++++++++++--------------------------
>>>    3 files changed, 26 insertions(+), 28 deletions(-)
>>>
>>
>> ping?
>
> Looked at it a week or two ago, didn't immediately understand the first
> patch and decided that there's more important stuff for 1.0...

Yeah.  It can probably go in during the freeze.

Regarding the first patch, we simply fail this assert:

static inline void* array_get(array_t* array,unsigned int index) {
     assert(index < array->next);
     return array->pointer + index * array->item_size;
}

so you need to first set s->directory.next like array_get_next does.

Paolo

Re: [Qemu-devel] ping Re: [PATCH 0/6] misc vvfat fixes

[Kevin Wolf]

Am 27.10.2011 15:10, schrieb Paolo Bonzini:
> On 10/27/2011 02:38 PM, Kevin Wolf wrote:
>> Am 27.10.2011 13:46, schrieb Paolo Bonzini:
>>> On 10/05/2011 09:12 AM, Paolo Bonzini wrote:
>>>> It occurred to me that, if there's one thing vvfat ought to be good
>>>> at, it is creating disk images with qemu-img convert (a driver disk
>>>> in my case).
>>>>
>>>> It turns out the use case is really broken.  qemu-img doesn't
>>>> complete at all, the resulting images often do not pass fsck,
>>>> and it's impossible to create a 1.44 MB disk image.  This
>>>> series fixes all of the small problems I found.
>>>>
>>>> Coding standard in this file is such a pain that I hardly bothered
>>>> about it.
>>>>
>>>>
>>>> Paolo Bonzini (6):
>>>>     vvfat: fix out of bounds array_get usage
>>>>     vvfat: do not fail if the disk has spare sectors the
>>>>     vvfat: need to use first_sectors_number to distinguish fdd/hdd
>>>>     vvfat: unify and correct computation of sector count
>>>>     vvfat: do not hardcode sector counts in error message
>>>>     vvfat: reorganize computation of disk geometry
>>>>
>>>>    block/vvfat.c |   50 ++++++++++++++++++++++++--------------------------
>>>>    3 files changed, 26 insertions(+), 28 deletions(-)
>>>>
>>>
>>> ping?
>>
>> Looked at it a week or two ago, didn't immediately understand the first
>> patch and decided that there's more important stuff for 1.0...
> 
> Yeah.  It can probably go in during the freeze.
> 
> Regarding the first patch, we simply fail this assert:
> 
> static inline void* array_get(array_t* array,unsigned int index) {
>      assert(index < array->next);
>      return array->pointer + index * array->item_size;
> }
> 
> so you need to first set s->directory.next like array_get_next does.

So is this combination of array_ensure_allocated(), setting
s->directory.next and memset() basically an open-coded array_set_size()
that initialises new elements with zeros?

Kevin