[Qemu-devel] [PATCH] vvfat mbr fixes

[Qemu-devel] [PATCH] vvfat mbr fixes

[Ivan Kalvachev]

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

Hello,

I've been having problems using vvfat virtual block device.
Even linux fdisk was able to find problems with it.
The reason turned out to be simple, MBR have bogus parameters.

1. Partition size in sectors is threated as partition end,
causing the partition to have the same size as the device,
while starting 63 sectors inside it.

2. The disk CHS geometry was set to maximum allowed values,
not the maximum sizes of parameters.

Cylinders = 1024 -> 0 .. 1023
Heads     = 16   -> 0 .. 15
Sectors   = 63   -> 1 .. 63

By historical reasons sectors maximum allowed value and size match.
I've changed it to correct values. Now disks are at 504MB limit.

3. The size of partition was fixed with pre-calculated value based on
the wrong geometry.
I've implemented dynamic calculation.

4. The size of partition have been precalculated to fill the maximum geometry.
However usually this means that there is incomplete cluster at the
end of it.
I've implemented proper shrinking of the partition after init_directories().

5. WinNT clones refuse to mount disk that doesn't have NT-ID.
I set it to 'qemu'. :)

As a bonus I reworked the rest of init_mbr code, so it can handle
disk outside the CHS limits in a way that all modern systems do, using LBA.
Because I couldn't find sector->CHS function I wrote one and to simplify
its calling I changed the 3 separate bytes for CHS into an array of 3.
Also when LBA mode is detected it sets the proper FAT partition types.

I haven't touched any of the directory/boot_record/etc code.
(I haven't done automatic fat/partition/disk growing, so far it all
have to fit into the maximum CHS geometry).

I also moved the ":rw:" handling after other option parsing, as some of them
change the sector_count that is used to initialize the snapshot device.
However in qemu-0.9.0 and qemu-cvs RW mode doesn't work for me, it fails
at bdrv_open() in enable_write_target(). I'll try to debug that and
if I find solution I would send additional patch.

The attached patch is to the current CVS, it should apply to
qemu-0.9.0 with `patch -l`, as there have been only whitespace
changes.


Here is some simple table of the virtual disk, that would help you
understand the code changes.

====
MBR           - 0
---
partition_start
boot          - first_sectors_number-1
FAT1          - first_sectors_number
FAT2          - first_sectors_number+sectors_per_fat
DIR_ROOT      - first_sectors_number+2*sectors_per_fat = faked_sectors
...data...
partition_end - cluster_count*sectors_per_cluster + faked_sectors = sector_count
====

P.S.
Maybe it is good idea to use 80/2/18 CHS for 1.44MB floppy,
instead of 80/2/36 CHS for 2.88MB. Linux doesn't seem to autodetect them.
The problem is noticed first by Thomas Schwinge in mail to qemu-dev at 28 Mart.

[-- Attachment #2: qemu_vvfat_mbr.patch --]
[-- Type: application/octet-stream, Size: 4874 bytes --]

--- oldqemu/block-vvfat.c	2007-09-17 11:09:43.000000000 +0300
+++ newqemu/block-vvfat.c	2007-09-22 22:58:17.044333017 +0300
@@ -244,15 +244,11 @@ typedef struct bootsector_t {
 
 typedef struct partition_t {
     uint8_t attributes; /* 0x80 = bootable */
-    uint8_t start_head;
-    uint8_t start_sector;
-    uint8_t start_cylinder;
-    uint8_t fs_type; /* 0x1 = FAT12, 0x6 = FAT16, 0xb = FAT32 */
-    uint8_t end_head;
-    uint8_t end_sector;
-    uint8_t end_cylinder;
+    uint8_t start_CHS[3];
+    uint8_t fs_type; /* 0x1 = FAT12, 0x6 = FAT16, 0xe = FAT16_LBA, 0xb = FAT32, 0xc = FAT32_LBA */
+    uint8_t end_CHS[3];
     uint32_t start_sector_long;
-    uint32_t end_sector_long;
+    uint32_t length_sector_long;
 } __attribute__((packed)) partition_t;
 
 typedef struct mbr_t {
@@ -350,26 +346,52 @@ typedef struct BDRVVVFATState {
     int downcase_short_names;
 } BDRVVVFATState;
 
+static int convert_sector2CHS(BlockDriverState* bs, uint8_t * CHS, int spos){
+    int head,sector;
+    sector   = spos % (bs->secs);  spos/= bs->secs;
+    head     = spos % (bs->heads); spos/= bs->heads;
+    if(spos >= bs->cyls){
+        /* Windows/Dos is seid to take 1023/255/63 as nonrepresentable CHS */
+        CHS[0] = 0xFF;
+        CHS[1] = 0xFF;
+        CHS[2] = 0xFF;
+        return 1;
+    }
+    CHS[0] = (uint8_t)head;//head
+    CHS[1] = (uint8_t)( (sector+1) | ((spos>>8)<<6) ); //sector+hi(cylinder)
+    CHS[2] = (uint8_t)spos;//cylinder
+    return 0;
+}
 
 static void init_mbr(BDRVVVFATState* s)
 {
     /* TODO: if the files mbr.img and bootsect.img exist, use them */
     mbr_t* real_mbr=(mbr_t*)s->first_sectors;
     partition_t* partition=&(real_mbr->partition[0]);
+    int lba;
 
     memset(s->first_sectors,0,512);
 
+    /* Win NT Disk Signature */
+    real_mbr->ignored[0x1b8]='q';
+    real_mbr->ignored[0x1b9]='e';
+    real_mbr->ignored[0x1ba]='m';
+    real_mbr->ignored[0x1bb]='u';
+   
     partition->attributes=0x80; /* bootable */
-    partition->start_head=1;
-    partition->start_sector=1;
-    partition->start_cylinder=0;
+    
+    lba =convert_sector2CHS(s->bs, partition->start_CHS, s->first_sectors_number-1);
+    lba|=convert_sector2CHS(s->bs, partition->end_CHS, s->sector_count);
+
+    partition->start_sector_long =cpu_to_le32(s->first_sectors_number-1);
+    partition->length_sector_long=cpu_to_le32(s->sector_count - s->first_sectors_number+1);
+
     /* FAT12/FAT16/FAT32 */
-    partition->fs_type=(s->fat_type==12?0x1:s->fat_type==16?0x6:0xb);
-    partition->end_head=s->bs->heads-1;
-    partition->end_sector=0xff; /* end sector & upper 2 bits of cylinder */;
-    partition->end_cylinder=0xff; /* lower 8 bits of end cylinder */;
-    partition->start_sector_long=cpu_to_le32(s->bs->secs);
-    partition->end_sector_long=cpu_to_le32(s->sector_count);
+    if(lba)
+        /*LBA partitions are identified by start/ength_sector_long not by CHS*/
+        partition->fs_type=(s->fat_type==12?0x1:s->fat_type==16?0xe:0xc);
+    else
+        partition->fs_type=(s->fat_type==12?0x1:s->fat_type==16?0x6:0xb);
 
     real_mbr->magic[0]=0x55; real_mbr->magic[1]=0xaa;
 }
@@ -973,10 +995,10 @@ DLOG(if (stderr == NULL) {
 
     s->fat_type=16;
     /* LATER TODO: if FAT32, adjust */
-    s->sector_count=0xec04f;
     s->sectors_per_cluster=0x10;
-    /* LATER TODO: this could be wrong for FAT32 */
-    bs->cyls=1023; bs->heads=15; bs->secs=63;
+    /* Keep this geometry for disk +512MB and use LBA model*/
+    bs->cyls=1024; bs->heads=16; bs->secs=63;
+
 
     s->current_cluster=0xffffffff;
 
@@ -991,11 +1013,6 @@ DLOG(if (stderr == NULL) {
     if (!strstart(dirname, "fat:", NULL))
 	return -1;
 
-    if (strstr(dirname, ":rw:")) {
-	if (enable_write_target(s))
-	    return -1;
-	bs->read_only = 0;
-    }
 
     if (strstr(dirname, ":floppy:")) {
 	floppy = 1;
@@ -1005,6 +1022,8 @@ 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;
@@ -1015,6 +1034,12 @@ DLOG(if (stderr == NULL) {
 	s->sector_count=2880;
     }
 
+    if (strstr(dirname, ":rw:")) {
+	if (enable_write_target(s))
+	    return -1;
+	bs->read_only = 0;
+    }
+
     i = strrchr(dirname, ':') - dirname;
     assert(i >= 3);
     if (dirname[i-2] == ':' && isalpha(dirname[i-1]))
@@ -1024,11 +1049,12 @@ DLOG(if (stderr == NULL) {
 	dirname += i+1;
 
     bs->total_sectors=bs->cyls*bs->heads*bs->secs;
-    if (s->sector_count > bs->total_sectors)
-	s->sector_count = bs->total_sectors;
+
     if(init_directories(s, dirname))
 	return -1;
 
+    s->sector_count = s->faked_sectors + s->sectors_per_cluster*s->cluster_count;
+
     if(s->first_sectors_number==0x40)
 	init_mbr(s);
 

Re: [Qemu-devel] [PATCH] vvfat mbr fixes

[Johannes Schindelin]

Hi,

On Sun, 23 Sep 2007, Ivan Kalvachev wrote:

> I've been having problems using vvfat virtual block device. Even linux 
> fdisk was able to find problems with it. The reason turned out to be 
> simple, MBR have bogus parameters.

Thanks for doing this; I did not find any time for that.

Overall, I like what you did, but here are some comments (if you would 
have inlined the patch, I would have commented with references):

- I like the convert_sector2CHS() function, although I would have named it 
  sector2CHS() for brevity (although the pretty magic -- or unintuitive 
  -- detection if lba is needed would have to be done differently, which 
  I maintain would be better),

- you write the NT-ID byte-per-byte, whereas I would have used strcpy() 
  for clarity,

- I'd have introduced a member nt_id instead of hardcoding an offset into 
  the "ignored" part, and

- fat_type == 12 and lba does not make sense, or does it?

Thanks,
Dscho

[Qemu-devel] Re: [PATCH] vvfat mbr fixes

[Lorenzo Campedelli]

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

Johannes Schindelin wrote:
> Hi,
> 
> On Sun, 23 Sep 2007, Ivan Kalvachev wrote:
> 
>> I've been having problems using vvfat virtual block device. Even linux 
>> fdisk was able to find problems with it. The reason turned out to be 
>> simple, MBR have bogus parameters.
> 
> Thanks for doing this; I did not find any time for that.
> 
> Overall, I like what you did, but here are some comments (if you would 
> have inlined the patch, I would have commented with references):
> 
> - I like the convert_sector2CHS() function, although I would have named it 
>   sector2CHS() for brevity (although the pretty magic -- or unintuitive 
>   -- detection if lba is needed would have to be done differently, which 
>   I maintain would be better),
> 
> - you write the NT-ID byte-per-byte, whereas I would have used strcpy() 
>   for clarity,
> 
> - I'd have introduced a member nt_id instead of hardcoding an offset into 
>   the "ignored" part, and
> 
> - fat_type == 12 and lba does not make sense, or does it?
> 
> Thanks,
> Dscho

While you are working on vvfat issues, could you give a look to the 
attached small patch?

It tries to fix the problem with using vvfat together with -snapshot.

The patch is not complete (it doesn't fix additional options such as 
:rw:, :floppy: :32: etc,) nor clean (I guess this specific code should 
be in block-vvfat.c, not in block.c, but the backing file handling is 
done there...) but I needed a quick fix for my needs.

If anybody has suggestions on how to make a better patch to be 
integrated in CVS I'd be glad :)

While I'm here I'd add a related question: wouldn't it be useful to be 
able to specify a per-device "-snapshot" option? Is it doable?

Thanks,
Lorenzo

[-- Attachment #2: qemu-0.9.0.20070921-block.c.patch --]
[-- Type: text/x-patch, Size: 1241 bytes --]

--- qemu-0.9.0.20070921/block.c.orig	2007-09-21 21:10:53.000000000 +0200
+++ qemu-0.9.0.20070921/block.c	2007-09-22 10:09:32.000000000 +0200
@@ -331,6 +331,7 @@
 
     if (flags & BDRV_O_SNAPSHOT) {
         BlockDriverState *bs1;
+        BlockDriver *drv1;
         int64_t total_size;
 
         /* if snapshot, we create a temporary backing file and open it
@@ -346,10 +347,22 @@
             return -1;
         }
         total_size = bdrv_getlength(bs1) >> SECTOR_BITS;
+        drv1 = bs1->drv;
         bdrv_delete(bs1);
 
         get_tmp_filename(tmp_filename, sizeof(tmp_filename));
-        realpath(filename, backing_filename);
+        /*
+         * for vvfat protocol the string "fat:<options>:" should remain
+         * the prefix of the filename even after realpath() call ...
+         */
+        if (drv1 == &bdrv_vvfat) {
+            int i = strrchr(filename, ':') - filename + 1;
+
+            strncpy(backing_filename, filename, i);
+            realpath(filename + i, backing_filename + i);
+        } else {
+            realpath(filename, backing_filename);
+        }
         if (bdrv_create(&bdrv_qcow2, tmp_filename,
                         total_size, backing_filename, 0) < 0) {
             return -1;

Re: [Qemu-devel] Re: [PATCH] vvfat mbr fixes

[Johannes Schindelin]

Hi,

On Sun, 23 Sep 2007, Lorenzo Campedelli wrote:

> While you are working on vvfat issues, could you give a look to the 
> attached small patch?

Okay, this is what I would do:

- not make this handling dependent on vvfat (but this means checking if 
  the colon is the second character, because then it is most likely 
  a Windows path and does _not_ want special handling), and

- I'd use the result of strrchr() directly.

This is a sketch of the code I propose:

	const char *tmp;

	...

	get_tmp_filename(tmp_filename, sizeof(tmp_filename));
	/* Handle 'fat:rw:<filename>' */
	tmp = strrchr(backing_filename, ':');
	if (tmp - backing_filename == 2) /* DOS path */
		tmp = NULL;
	else if (tmp - backing_filename > 2 && tmp[-2] == ':')
		tmp -= 2;
	realpath(filename, tmp ? tmp : backing_filename);

Ciao,
Dscho

[Qemu-devel] Re: [PATCH] vvfat mbr fixes

[Lorenzo Campedelli]

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

Johannes Schindelin wrote:
> Hi,
> 
> On Sun, 23 Sep 2007, Lorenzo Campedelli wrote:
> 
>> While you are working on vvfat issues, could you give a look to the 
>> attached small patch?
> 
> Okay, this is what I would do:
> 
> - not make this handling dependent on vvfat (but this means checking if 
>   the colon is the second character, because then it is most likely 
>   a Windows path and does _not_ want special handling), and
> 
> - I'd use the result of strrchr() directly.
> 
> This is a sketch of the code I propose:
> 
> 	const char *tmp;
> 
> 	...
> 
> 	get_tmp_filename(tmp_filename, sizeof(tmp_filename));
> 	/* Handle 'fat:rw:<filename>' */
> 	tmp = strrchr(backing_filename, ':');
> 	if (tmp - backing_filename == 2) /* DOS path */
> 		tmp = NULL;
> 	else if (tmp - backing_filename > 2 && tmp[-2] == ':')
> 		tmp -= 2;
> 	realpath(filename, tmp ? tmp : backing_filename);
> 
> Ciao,
> Dscho
> 
> 
> 
> 
Hi Johannes,

thanks for your reply. I gave another try at the patch.

I was concerned by having this handling outside of a proper
place (i.e. block-vvfat.c), but if we want to leave it there,
wouldn't it be better to just handle cases which would otherwise
fail, and just try to fix the "fat:" format?

The attached patch only tries to fix things when realpath() fails
to find a match and when the filename starts with a "fat:" header.

This way if anybody really wanted to use an image file named "fat:foo"
could do it without us handling it as a vvfat directory...

The code to handle the search for ':' and the DOS drive name special
case is stolen from block-vvfat.c, so you should like it ;-).

Regards,
Lorenzo


[-- Attachment #2: qemu-0.9.0.20070921-block.c.patch --]
[-- Type: text/x-patch, Size: 954 bytes --]

--- qemu-0.9.0.20070921/block.c.orig	2007-09-23 17:04:21.000000000 +0200
+++ qemu-0.9.0.20070921/block.c	2007-09-23 17:18:39.000000000 +0200
@@ -349,7 +349,19 @@
         bdrv_delete(bs1);
 
         get_tmp_filename(tmp_filename, sizeof(tmp_filename));
-        realpath(filename, backing_filename);
+        if (realpath(filename, backing_filename) == NULL) {
+            if (strstart(filename, "fat:", NULL)) {
+                int i = strrchr(filename, ':') - filename;
+
+                if ((filename[i-2] == ':') && isalpha(filename[i-1]))
+                    i -= 1; /* workaround for DOS drive names */
+                else
+                    i += 1;
+
+                strncpy(backing_filename, filename, i);
+                realpath(filename + i, backing_filename + i);
+            }
+        }
         if (bdrv_create(&bdrv_qcow2, tmp_filename,
                         total_size, backing_filename, 0) < 0) {
             return -1;

[Qemu-devel] Re: [PATCH] vvfat mbr fixes

[Johannes Schindelin]

Hi,

On Sun, 23 Sep 2007, Lorenzo Campedelli wrote:

> I was concerned by having this handling outside of a proper place (i.e. 
> block-vvfat.c), but if we want to leave it there, wouldn't it be better 
> to just handle cases which would otherwise fail, and just try to fix the 
> "fat:" format?

I don't like that option, since it special-cases vvfat to much.

Ciao,
Dscho

[Qemu-devel] Re: [PATCH] vvfat mbr fixes

[Lorenzo Campedelli]

Johannes Schindelin wrote:
> Hi,
> 
> On Sun, 23 Sep 2007, Lorenzo Campedelli wrote:
> 
>> I was concerned by having this handling outside of a proper place (i.e. 
>> block-vvfat.c), but if we want to leave it there, wouldn't it be better 
>> to just handle cases which would otherwise fail, and just try to fix the 
>> "fat:" format?
> 
> I don't like that option, since it special-cases vvfat to much.
> 
> Ciao,
> Dscho
> 
> 
> 
> 
I agree the fix is ugly, otoh the problem is also a special case...
I don't see a clean way of fixing this, the code in block.c needs to 
know how to handle a vvfat specific filename format.

I'll keep my "works for me" patch waiting for some better fix.

Thanks for your time.
Lorenzo

Re: [Qemu-devel] [PATCH] vvfat mbr fixes

[Ivan Kalvachev]

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

I had a discussion with Johannes Schindelin over my patch, that I
thought is on the maillist, but apparently it wasn't. I'm subscribed,
so please don't send me mails directly, gmail web interface could be
quite misleading.

So here is the third revision of my patch. Changes include:
using more structures instead of fixed byte locations. chs and nt_id.
more detailed comments, function name shortened and if(lba) moved to
?: construct.

[-- Attachment #2: qemu_vvfat_mbr_v3.patch --]
[-- Type: application/octet-stream, Size: 5584 bytes --]

--- oldqemu/block-vvfat.c	2007-09-17 11:09:43.000000000 +0300
+++ newqemu/block-vvfat.c	2007-09-24 13:20:27.056353326 +0300
@@ -242,21 +242,25 @@ typedef struct bootsector_t {
     uint8_t magic[2];
 } __attribute__((packed)) bootsector_t;
 
+typedef struct {
+    uint8_t head;
+    uint8_t sector;
+    uint8_t cylinder;
+} mbr_chs_t;
+
 typedef struct partition_t {
     uint8_t attributes; /* 0x80 = bootable */
-    uint8_t start_head;
-    uint8_t start_sector;
-    uint8_t start_cylinder;
-    uint8_t fs_type; /* 0x1 = FAT12, 0x6 = FAT16, 0xb = FAT32 */
-    uint8_t end_head;
-    uint8_t end_sector;
-    uint8_t end_cylinder;
+    mbr_chs_t start_CHS;
+    uint8_t   fs_type; /* 0x1 = FAT12, 0x6 = FAT16, 0xe = FAT16_LBA, 0xb = FAT32, 0xc = FAT32_LBA */
+    mbr_chs_t end_CHS;
     uint32_t start_sector_long;
-    uint32_t end_sector_long;
+    uint32_t length_sector_long;
 } __attribute__((packed)) partition_t;
 
 typedef struct mbr_t {
-    uint8_t ignored[0x1be];
+    uint8_t ignored[0x1b8];
+    uint32_t nt_id;
+    uint8_t ignored2[2];
     partition_t partition[4];
     uint8_t magic[2];
 } __attribute__((packed)) mbr_t;
@@ -350,26 +354,57 @@ typedef struct BDRVVVFATState {
     int downcase_short_names;
 } BDRVVVFATState;
 
+/* take the sector position spos and convert it to Cylinder/Head/Sector position
+ * if the position is outside the specified geometry, fill maximum value for CHS
+ * and return 1 to signal overflow.
+ */
+static int sector2CHS(BlockDriverState* bs, mbr_chs_t * chs, int spos){
+    int head,sector;
+    sector   = spos % (bs->secs);  spos/= bs->secs;
+    head     = spos % (bs->heads); spos/= bs->heads;
+    if(spos >= bs->cyls){
+        /* Overflow,
+        it happens if 32bit sector positions are used, while CHS is only 24bit.
+        Windows/Dos is said to take 1023/255/63 as nonrepresentable CHS */
+        chs->head     = 0xFF;
+        chs->sector   = 0xFF;
+        chs->cylinder = 0xFF;
+        return 1;
+    }
+    chs->head     = (uint8_t)head;
+    chs->sector   = (uint8_t)( (sector+1) | ((spos>>8)<<6) );
+    chs->cylinder = (uint8_t)spos;
+    return 0;
+}
 
 static void init_mbr(BDRVVVFATState* s)
 {
     /* TODO: if the files mbr.img and bootsect.img exist, use them */
     mbr_t* real_mbr=(mbr_t*)s->first_sectors;
     partition_t* partition=&(real_mbr->partition[0]);
+    int lba;
 
     memset(s->first_sectors,0,512);
 
+    /* Win NT Disk Signature */
+    real_mbr->nt_id= cpu_to_le32(0xbe1afdfa);
+
     partition->attributes=0x80; /* bootable */
-    partition->start_head=1;
-    partition->start_sector=1;
-    partition->start_cylinder=0;
+
+    /* LBA is used when partition is outside the CHS geometry */
+    lba = sector2CHS(s->bs, &partition->start_CHS, s->first_sectors_number-1);
+    lba|= sector2CHS(s->bs, &partition->end_CHS,   s->sector_count);
+
+    /*LBA partitions are identified only by start/length_sector_long not by CHS*/
+    partition->start_sector_long =cpu_to_le32(s->first_sectors_number-1);
+    partition->length_sector_long=cpu_to_le32(s->sector_count - s->first_sectors_number+1);
+
     /* FAT12/FAT16/FAT32 */
-    partition->fs_type=(s->fat_type==12?0x1:s->fat_type==16?0x6:0xb);
-    partition->end_head=s->bs->heads-1;
-    partition->end_sector=0xff; /* end sector & upper 2 bits of cylinder */;
-    partition->end_cylinder=0xff; /* lower 8 bits of end cylinder */;
-    partition->start_sector_long=cpu_to_le32(s->bs->secs);
-    partition->end_sector_long=cpu_to_le32(s->sector_count);
+    /* DOS uses different types when partition is LBA,
+       probably to prevent older versions from using CHS on them */
+    partition->fs_type= s->fat_type==12 ? 0x1:
+                        s->fat_type==16 ? (lba?0xe:0x06):
+                         /*fat_tyoe==32*/ (lba?0xc:0x0b);
 
     real_mbr->magic[0]=0x55; real_mbr->magic[1]=0xaa;
 }
@@ -973,10 +1008,9 @@ DLOG(if (stderr == NULL) {
 
     s->fat_type=16;
     /* LATER TODO: if FAT32, adjust */
-    s->sector_count=0xec04f;
     s->sectors_per_cluster=0x10;
-    /* LATER TODO: this could be wrong for FAT32 */
-    bs->cyls=1023; bs->heads=15; bs->secs=63;
+    /* 504MB disk*/
+    bs->cyls=1024; bs->heads=16; bs->secs=63;
 
     s->current_cluster=0xffffffff;
 
@@ -991,12 +1025,6 @@ DLOG(if (stderr == NULL) {
     if (!strstart(dirname, "fat:", NULL))
 	return -1;
 
-    if (strstr(dirname, ":rw:")) {
-	if (enable_write_target(s))
-	    return -1;
-	bs->read_only = 0;
-    }
-
     if (strstr(dirname, ":floppy:")) {
 	floppy = 1;
 	s->fat_type = 12;
@@ -1005,6 +1033,8 @@ 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;
@@ -1015,6 +1045,12 @@ DLOG(if (stderr == NULL) {
 	s->sector_count=2880;
     }
 
+    if (strstr(dirname, ":rw:")) {
+	if (enable_write_target(s))
+	    return -1;
+	bs->read_only = 0;
+    }
+
     i = strrchr(dirname, ':') - dirname;
     assert(i >= 3);
     if (dirname[i-2] == ':' && isalpha(dirname[i-1]))
@@ -1024,11 +1060,12 @@ DLOG(if (stderr == NULL) {
 	dirname += i+1;
 
     bs->total_sectors=bs->cyls*bs->heads*bs->secs;
-    if (s->sector_count > bs->total_sectors)
-	s->sector_count = bs->total_sectors;
+
     if(init_directories(s, dirname))
 	return -1;
 
+    s->sector_count = s->faked_sectors + s->sectors_per_cluster*s->cluster_count;
+
     if(s->first_sectors_number==0x40)
 	init_mbr(s);
 

Re: [Qemu-devel] [PATCH] vvfat mbr fixes

[Johannes Schindelin]

Hi,

On Mon, 24 Sep 2007, Ivan Kalvachev wrote:

> I had a discussion with Johannes Schindelin over my patch, that I 
> thought is on the maillist, but apparently it wasn't. I'm subscribed, so 
> please don't send me mails directly, gmail web interface could be quite 
> misleading.
> 
> So here is the third revision of my patch. Changes include: using more 
> structures instead of fixed byte locations. chs and nt_id. more detailed 
> comments, function name shortened and if(lba) moved to ?: construct.

Almost all my comments went unheeded.

Oh well,
Dscho

Re: [Qemu-devel] [PATCH] vvfat mbr fixes

[Ivan Kalvachev]

2007/9/24, Johannes Schindelin <Johannes.Schindelin@gmx.de>:
> Hi,
>
> On Mon, 24 Sep 2007, Ivan Kalvachev wrote:
>
> > I had a discussion with Johannes Schindelin over my patch, that I
> > thought is on the maillist, but apparently it wasn't. I'm subscribed, so
> > please don't send me mails directly, gmail web interface could be quite
> > misleading.
> >
> > So here is the third revision of my patch. Changes include: using more
> > structures instead of fixed byte locations. chs and nt_id. more detailed
> > comments, function name shortened and if(lba) moved to ?: construct.
>
> Almost all my comments went unheeded.

I believe that I've answered and addressed all your comments.
If you feel sorry that they haven't been documented on the maillist
you could have forwarded them by yourself, as I do now. I just hope I
haven't missed some.

If you have more questions just ask them.


---------- Forwarded message ----------
From: Ivan Kalvachev <ikalvachev@gmail.com>
Date: 23.09.2007 03:27
Subject: Re: [Qemu-devel] [PATCH] vvfat mbr fixes
To: Johannes Schindelin <Johannes.Schindelin@gmx.de>


2007/9/23, Johannes Schindelin <Johannes.Schindelin@gmx.de>:
> Hi,
>
> On Sun, 23 Sep 2007, Ivan Kalvachev wrote:
>
> > I've been having problems using vvfat virtual block device. Even linux
> > fdisk was able to find problems with it. The reason turned out to be
> > simple, MBR have bogus parameters.
>
> Thanks for doing this; I did not find any time for that.
>
> Overall, I like what you did, but here are some comments (if you would
> have inlined the patch, I would have commented with references):
I'm happy I didn't inlined it:) And I'm sure gmail would've mangled the patch.
>
> - I like the convert_sector2CHS() function, although I would have named it
>   sector2CHS() for brevity (although the pretty magic -- or unintuitive
>   -- detection if lba is needed would have to be done differently, which
>   I maintain would be better),

Making the name shorter is not problem.
However I don't understand your comment about LBA. How do you want it
done and where.
CHS is not used anywhere else, so MBR is the logical place to handle
it. LBA just means that CHS should be ignored and only
partition_start/length_sectors_long should be used. It shouldn't
affect any part of the other code that works with sectors and
clusters.

> - you write the NT-ID byte-per-byte, whereas I would have used strcpy()
>   for clarity,

NT-ID is not supposed to be string and strcpy() implies null terminated string.
NT-ID could be any random value, I just didn't wanted it that random.
Having it memcpy-ed would make some generic calculation harder (e.g.
hash of the fat:dirname or etc).
Having it as uint32_t would bring endian issues, but I think I'd go with that.

> - I'd have introduced a member nt_id instead of hardcoding an offset into
>   the "ignored" part, and

OK, I'll change the structure to have ntid. How do you like to name
the 4 bytes after the ntid and before the partition table -
ignored2[4] ?

> - fat_type == 12 and lba does not make sense, or does it?

Your point is?
Theoretically it could work even on floppy, as long as the guest OS
ignores the CHS.
I think that the FAT_XX_LBA new id's are done to prevent older version
of DOS from trying to access them using the bogus CHS, and that new
versions that support LBA use only LBA even on normal CHS, as LBA it
is always valid.


---------- Forwarded message ----------
From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Date: 23.09.2007 04:25
Subject: Re: [Qemu-devel] [PATCH] vvfat mbr fixes
To: Ivan Kalvachev <ikalvachev@gmail.com>


Hi,

On Sun, 23 Sep 2007, Ivan Kalvachev wrote:

> 2007/9/23, Johannes Schindelin <Johannes.Schindelin@gmx.de>:
> >
> > On Sun, 23 Sep 2007, Ivan Kalvachev wrote:
> >
> > > I've been having problems using vvfat virtual block device. Even
> > > linux fdisk was able to find problems with it. The reason turned out
> > > to be simple, MBR have bogus parameters.
> >
> > Thanks for doing this; I did not find any time for that.
> >
> > Overall, I like what you did, but here are some comments (if you would
> > have inlined the patch, I would have commented with references):
>
> I'm happy I didn't inlined it:) And I'm sure gmail would've mangled the
> patch.

Hehe... and you're right, GMail's webmailer mangles patches badly.

> > - I like the convert_sector2CHS() function, although I would have named it
> >   sector2CHS() for brevity (although the pretty magic -- or
> >   unintuitive -- detection if lba is needed would have to be done
> >   differently, which I maintain would be better),
>
> Making the name shorter is not problem.
> However I don't understand your comment about LBA. How do you want it
> done and where.

Like this:

        sector2CHS(BlockDriverState* bs, int spos, int *lba)

returning the CHS value.  I like that better, since what you are really
interested in, when calling sector2CHS, are the CHS, and that should be
the return value.

But I see that you did not make a struct of the CHS, so that seems less
practicable.

> > - you write the NT-ID byte-per-byte, whereas I would have used strcpy()
> >   for clarity,
>
> NT-ID is not supposed to be string and strcpy() implies null terminated
> string.

Ah, I thought it was something like volume id, which is supposed to be
0-padded.

> NT-ID could be any random value, I just didn't wanted it that random.
> Having it memcpy-ed would make some generic calculation harder (e.g.
> hash of the fat:dirname or etc).

I do not see that.  Just do a memcpy(real_mbr->nt_id, "qemu", 4);

> > - I'd have introduced a member nt_id instead of hardcoding an offset
> >   into the "ignored" part, and
>
> OK, I'll change the structure to have ntid. How do you like to name
> the 4 bytes after the ntid and before the partition table -
> ignored2[4] ?

Yep.

> > - fat_type == 12 and lba does not make sense, or does it?
>
> Your point is?

I meant that

+    if(lba)
+        /*LBA partitions are identified by start/ength_sector_long not by CHS*/
+        partition->fs_type=(s->fat_type==12?0x1:s->fat_type==16?0xe:0xc);
+    else
+        partition->fs_type=(s->fat_type==12?0x1:s->fat_type==16?0x6:0xb);

which suggests that fat_type == 12 is valid even with lba.

I'd rather have done something like

        /*LBA partitions are identified by start/ength_sector_long not by CHS*/
        partition->fs_type = s->fat_type == 12 ? 0x1 :
                s->fat_type == 16 ? (lba ? 0xe : 0x6) : (lba ? 0xc : 0xb);

But then, I probably miss something.  It's been a long time since I wrote
it, and I am quite amazed that you understood the code enough to fix it so
well.

Thanks,
Dscho


---------- Forwarded message ----------
From: Ivan Kalvachev <ikalvachev@gmail.com>
Date: 23.09.2007 18:58
Subject: Re: [Qemu-devel] [PATCH] vvfat mbr fixes
To: Johannes Schindelin <Johannes.Schindelin@gmx.de>


2007/9/23, Johannes Schindelin <Johannes.Schindelin@gmx.de>:
> Hi,
>
> On Sun, 23 Sep 2007, Ivan Kalvachev wrote:
>
> > 2007/9/23, Johannes Schindelin <Johannes.Schindelin@gmx.de>:
> > >
> > > On Sun, 23 Sep 2007, Ivan Kalvachev wrote:
[..]
> > > - I like the convert_sector2CHS() function, although I would have named it
> > >   sector2CHS() for brevity (although the pretty magic -- or
> > >   unintuitive -- detection if lba is needed would have to be done
> > >   differently, which I maintain would be better),
> >
> > Making the name shorter is not problem.
> > However I don't understand your comment about LBA. How do you want it
> > done and where.
>
> Like this:
>
>         sector2CHS(BlockDriverState* bs, int spos, int *lba)
>
> returning the CHS value.  I like that better, since what you are really
> interested in, when calling sector2CHS, are the CHS, and that should be
> the return value.
>
> But I see that you did not make a struct of the CHS, so that seems less
> practicable.

I do not like returning structures. I prefer to work directly with
pointers instead of hoping that the compiler would do something smart,
like using the return pointer directly instead of memcpy the local
copy of the structure.

Still, I see that you prefer having proper names, so I changed the
array to structure. As it is composed only by bytes it shouldn't need
packed attribute.

BTW I noticed something strange in the code, you define both structure
and typedefs with exactly the same names, but you don't use them as
structures (and there are no pointers to the structure itself).

> > > - you write the NT-ID byte-per-byte, whereas I would have used strcpy()
> > >   for clarity,
> >
> > NT-ID is not supposed to be string and strcpy() implies null terminated
> > string.
>
> Ah, I thought it was something like volume id, which is supposed to be
> 0-padded.
>
> > NT-ID could be any random value, I just didn't wanted it that random.
> > Having it memcpy-ed would make some generic calculation harder (e.g.
> > hash of the fat:dirname or etc).
>
> I do not see that.  Just do a memcpy(real_mbr->nt_id, "qemu", 4);

Done, in a little different way. I noticed another _id in the fat code,
and made this one similar.

> > > - I'd have introduced a member nt_id instead of hardcoding an offset
> > >   into the "ignored" part, and
> >
> > OK, I'll change the structure to have ntid. How do you like to name
> > the 4 bytes after the ntid and before the partition table -
> > ignored2[4] ?
>
> Yep.

Done

> > > - fat_type == 12 and lba does not make sense, or does it?
> >
> > Your point is?
>
> I meant that
>
> +    if(lba)
> +        /*LBA partitions are identified by start/ength_sector_long not by CHS*/
> +        partition->fs_type=(s->fat_type==12?0x1:s->fat_type==16?0xe:0xc);
> +    else
> +        partition->fs_type=(s->fat_type==12?0x1:s->fat_type==16?0x6:0xb);
>
> which suggests that fat_type == 12 is valid even with lba.

Let's say we have fat12 and it happens to be lba. How would you like
to handle this situation:
 - abort() and exit()?
 - write some other partition type
 - write the fat12 type and let the host sort it out.

I prefer to let it try. As I already explained there is quite big
chance that the host could work with it, because CHS should be used
only by old boot sector code.

> I'd rather have done something like
>
>         /*LBA partitions are identified by start/ength_sector_long not by CHS*/
>         partition->fs_type = s->fat_type == 12 ? 0x1 :
>                 s->fat_type == 16 ? (lba ? 0xe : 0x6) : (lba ? 0xc : 0xb);

Done.

I though of such construct, but I also don't like ?: ,  Having 3 level
of nested expressions is nasty.
But if you prefer it, I'd do it so (it saves one condition).

If it was up to me, I'd make fat_type take 0,1,2 values and handle all
the stuff with small tables. But it is already used in quite many
places to make this change non-trivial.


Here is second version of the patch. Hope it is OK.


---------- Forwarded message ----------
From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Date: 23.09.2007 22:28
Subject: Re: [Qemu-devel] [PATCH] vvfat mbr fixes
To: Ivan Kalvachev <ikalvachev@gmail.com>


Hi,

On Sun, 23 Sep 2007, Ivan Kalvachev wrote:

> 2007/9/23, Johannes Schindelin <Johannes.Schindelin@gmx.de>:
>
> > Like this:
> >
> >         sector2CHS(BlockDriverState* bs, int spos, int *lba)
> >
> > returning the CHS value.  I like that better, since what you are
> > really interested in, when calling sector2CHS, are the CHS, and that
> > should be the return value.
> >
> > But I see that you did not make a struct of the CHS, so that seems
> > less practicable.
>
> I do not like returning structures.

That is exactly the reason why I mentioned "that seems less practicable".
However, returning as value an indicator if lba is to be activated,
without even mentioning it, is bad.

> BTW I noticed something strange in the code, you define both structure
> and typedefs with exactly the same names, but you don't use them as
> structures (and there are no pointers to the structure itself).

Yeah, I'd probably do many things differently these days.

> > > > - fat_type == 12 and lba does not make sense, or does it?
> > >
> > > Your point is?
> >
> > I meant that
> >
> > +    if(lba)
> > +        /*LBA partitions are identified by start/ength_sector_long not by CHS*/
> > +        partition->fs_type=(s->fat_type==12?0x1:s->fat_type==16?0xe:0xc);
> > +    else
> > +        partition->fs_type=(s->fat_type==12?0x1:s->fat_type==16?0x6:0xb);
> >
> > which suggests that fat_type == 12 is valid even with lba.
>
> Let's say we have fat12 and it happens to be lba. How would you like
> to handle this situation:
>  - abort() and exit()?
>  - write some other partition type
>  - write the fat12 type and let the host sort it out.
>
> I prefer to let it try. As I already explained there is quite big
> chance that the host could work with it, because CHS should be used
> only by old boot sector code.

I have not enough knowledge about the issue to have an informed opinion on
that, so I'll just go with whatever you deem appropriate.

> If it was up to me, I'd make fat_type take 0,1,2 values and handle all
> the stuff with small tables.

That makes sense.

Ciao,
Dscho


---------- Forwarded message ----------
From: Ivan Kalvachev <ikalvachev@gmail.com>
Date: 24.09.2007 00:43
Subject: Re: [Qemu-devel] [PATCH] vvfat mbr fixes
To: Johannes Schindelin <Johannes.Schindelin@gmx.de>


2007/9/23, Johannes Schindelin <Johannes.Schindelin@gmx.de>:
> Hi,
>
> On Sun, 23 Sep 2007, Ivan Kalvachev wrote:
>
> > 2007/9/23, Johannes Schindelin <Johannes.Schindelin@gmx.de>:
> >
> > > Like this:
> > >
> > >         sector2CHS(BlockDriverState* bs, int spos, int *lba)
> > >
> > > returning the CHS value.  I like that better, since what you are
> > > really interested in, when calling sector2CHS, are the CHS, and that
> > > should be the return value.
> > >
> > > But I see that you did not make a struct of the CHS, so that seems
> > > less practicable.
> >
> > I do not like returning structures.
>
> That is exactly the reason why I mentioned "that seems less practicable".
> However, returning as value an indicator if lba is to be activated,
> without even mentioning it, is bad.

I still don't understand what do you mean with that.

The LBA mode is not activated. LBA is just a way to handle disk that
are bigger than the maximum possible CHS geometry.

For example, you can simply ignore the overflow and write clipped
values. In that case the partition would look like it ends much
earlier, in real cases it may even look like the end is before the
beginning. That's why in case of overflow the CHS are marked with
maximum values (1023/255/63 Windows) . As I said marking partitions
with another type by (win)DOS is just a way to prevent older versions
from using bogus CHS parameters.

The return value is just flag, it could be ignored if we are sure that
the partition would never be bigger than the geometry. That is the
case at the moment.
It is possible to not use the flag at all,  just check the returned
CHS for maximum values, however it would be much uglier solution.
My current code would get into full use when vvfat starts working with
virtual disks bigger than 8GB. (512*2^24).


---------- Forwarded message ----------
From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Date: 24.09.2007 01:12
Subject: Re: [Qemu-devel] [PATCH] vvfat mbr fixes
To: Ivan Kalvachev <ikalvachev@gmail.com>


Hi,

On Mon, 24 Sep 2007, Ivan Kalvachev wrote:

> 2007/9/23, Johannes Schindelin <Johannes.Schindelin@gmx.de>:
> > Hi,
> >
> > On Sun, 23 Sep 2007, Ivan Kalvachev wrote:
> >
> > > 2007/9/23, Johannes Schindelin <Johannes.Schindelin@gmx.de>:
> > >
> > > > Like this:
> > > >
> > > >         sector2CHS(BlockDriverState* bs, int spos, int *lba)
> > > >
> > > > returning the CHS value.  I like that better, since what you are
> > > > really interested in, when calling sector2CHS, are the CHS, and that
> > > > should be the return value.
> > > >
> > > > But I see that you did not make a struct of the CHS, so that seems
> > > > less practicable.
> > >
> > > I do not like returning structures.
> >
> > That is exactly the reason why I mentioned "that seems less practicable".
> > However, returning as value an indicator if lba is to be activated,
> > without even mentioning it, is bad.
>
> I still don't understand what do you mean with that.
>
> The LBA mode is not activated. LBA is just a way to handle disk that
> are bigger than the maximum possible CHS geometry.
>
> For example, you can simply ignore the overflow and write clipped
> values. In that case the partition would look like it ends much
> earlier, in real cases it may even look like the end is before the
> beginning. That's why in case of overflow the CHS are marked with
> maximum values (1023/255/63 Windows) . As I said marking partitions
> with another type by (win)DOS is just a way to prevent older versions
> >from using bogus CHS parameters.
>
> The return value is just flag, it could be ignored if we are sure that
> the partition would never be bigger than the geometry. That is the
> case at the moment.
> It is possible to not use the flag at all,  just check the returned
> CHS for maximum values, however it would be much uglier solution.
> My current code would get into full use when vvfat starts working with
> virtual disks bigger than 8GB. (512*2^24).

The thing is: you use the return value of the function
convert_sectors2CHS() to trigger lba handling or not.

But I did not see any documentation that the return value is such an
indicator.  I had to read the code (and not the code of
convert_sectors2CHS(), but the caller) to understand it.

That is my gripe.

Ciao,
Dscho


---------- Forwarded message ----------
From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Date: 24.09.2007 04:07
Subject: Re: [Qemu-devel] [PATCH] vvfat mbr fixes
To: Ivan Kalvachev <ikalvachev@gmail.com>


Hi,

On Mon, 24 Sep 2007, Ivan Kalvachev wrote:

> 2007/9/24, Johannes Schindelin <Johannes.Schindelin@gmx.de>:
> >
> > On Mon, 24 Sep 2007, Ivan Kalvachev wrote:
> >
> > > 2007/9/23, Johannes Schindelin <Johannes.Schindelin@gmx.de>:
> > > >
> > > > On Sun, 23 Sep 2007, Ivan Kalvachev wrote:
> > > >
> > > > > 2007/9/23, Johannes Schindelin <Johannes.Schindelin@gmx.de>:
> > > > >
> > > > > > Like this:
> > > > > >
> > > > > >         sector2CHS(BlockDriverState* bs, int spos, int *lba)
> > > > > >
> > > > > > returning the CHS value.  I like that better, since what you are
> > > > > > really interested in, when calling sector2CHS, are the CHS, and that
> > > > > > should be the return value.
> > > > > >
> > > > > > But I see that you did not make a struct of the CHS, so that seems
> > > > > > less practicable.
> > > > >
> > > > > I do not like returning structures.
> > > >
> > > > That is exactly the reason why I mentioned "that seems less practicable".
> > > > However, returning as value an indicator if lba is to be activated,
> > > > without even mentioning it, is bad.
> > >
> > > I still don't understand what do you mean with that.
> > >
> > > The LBA mode is not activated. LBA is just a way to handle disk that
> > > are bigger than the maximum possible CHS geometry.
> > >
> > > For example, you can simply ignore the overflow and write clipped
> > > values. In that case the partition would look like it ends much
> > > earlier, in real cases it may even look like the end is before the
> > > beginning. That's why in case of overflow the CHS are marked with
> > > maximum values (1023/255/63 Windows) . As I said marking partitions
> > > with another type by (win)DOS is just a way to prevent older versions
> > > >from using bogus CHS parameters.
> > >
> > > The return value is just flag, it could be ignored if we are sure that
> > > the partition would never be bigger than the geometry. That is the
> > > case at the moment.
> > > It is possible to not use the flag at all,  just check the returned
> > > CHS for maximum values, however it would be much uglier solution.
> > > My current code would get into full use when vvfat starts working with
> > > virtual disks bigger than 8GB. (512*2^24).
> >
> > The thing is: you use the return value of the function
> > convert_sectors2CHS() to trigger lba handling or not.
> >
> > But I did not see any documentation that the return value is such an
> > indicator.  I had to read the code (and not the code of
> > convert_sectors2CHS(), but the caller) to understand it.
> >
> > That is my gripe.
>
> I didn't documented it because I though it is obvious.
> So I assume you don't have any more objections and you haven't found
> any bugs in my code.

Right, except for the lacking documentation (you really should add that,
since it is _not_ obvious).

> Somebody else?

Umm.  You culled the Cc: list pretty early in our discussion.  I thought
it was on purpose...  So there is nobody else listening, but yours truly.

Ciao,
Dscho


---------- Forwarded message ----------
From: Ivan Kalvachev <ikalvachev@gmail.com>
Date: 24.09.2007 12:29
Subject: Re: [Qemu-devel] [PATCH] vvfat mbr fixes
To: Johannes Schindelin <Johannes.Schindelin@gmx.de>


2007/9/24, Johannes Schindelin <Johannes.Schindelin@gmx.de>:
> Hi,
>
> On Mon, 24 Sep 2007, Ivan Kalvachev wrote:
>
> > 2007/9/24, Johannes Schindelin <Johannes.Schindelin@gmx.de>:
> > >
> > > On Mon, 24 Sep 2007, Ivan Kalvachev wrote:
> > >
> > > > 2007/9/23, Johannes Schindelin <Johannes.Schindelin@gmx.de>:
> > > > >
> > > > > On Sun, 23 Sep 2007, Ivan Kalvachev wrote:
> > > > >
> > > > > > 2007/9/23, Johannes Schindelin <Johannes.Schindelin@gmx.de>:
> > > > > >
> > > > > > > Like this:
> > > > > > >
> > > > > > >         sector2CHS(BlockDriverState* bs, int spos, int *lba)
> > > > > > >
> > > > > > > returning the CHS value.  I like that better, since what you are
> > > > > > > really interested in, when calling sector2CHS, are the CHS, and that
> > > > > > > should be the return value.
> > > > > > >
> > > > > > > But I see that you did not make a struct of the CHS, so that seems
> > > > > > > less practicable.
> > > > > >
> > > > > > I do not like returning structures.
> > > > >
> > > > > That is exactly the reason why I mentioned "that seems less practicable".
> > > > > However, returning as value an indicator if lba is to be activated,
> > > > > without even mentioning it, is bad.
> > > >
> > > > I still don't understand what do you mean with that.
> > > >
> > > > The LBA mode is not activated. LBA is just a way to handle disk that
> > > > are bigger than the maximum possible CHS geometry.
> > > >
> > > > For example, you can simply ignore the overflow and write clipped
> > > > values. In that case the partition would look like it ends much
> > > > earlier, in real cases it may even look like the end is before the
> > > > beginning. That's why in case of overflow the CHS are marked with
> > > > maximum values (1023/255/63 Windows) . As I said marking partitions
> > > > with another type by (win)DOS is just a way to prevent older versions
> > > > >from using bogus CHS parameters.
> > > >
> > > > The return value is just flag, it could be ignored if we are sure that
> > > > the partition would never be bigger than the geometry. That is the
> > > > case at the moment.
> > > > It is possible to not use the flag at all,  just check the returned
> > > > CHS for maximum values, however it would be much uglier solution.
> > > > My current code would get into full use when vvfat starts working with
> > > > virtual disks bigger than 8GB. (512*2^24).
> > >
> > > The thing is: you use the return value of the function
> > > convert_sectors2CHS() to trigger lba handling or not.
> > >
> > > But I did not see any documentation that the return value is such an
> > > indicator.  I had to read the code (and not the code of
> > > convert_sectors2CHS(), but the caller) to understand it.
> > >
> > > That is my gripe.
> >
> > I didn't documented it because I though it is obvious.
> > So I assume you don't have any more objections and you haven't found
> > any bugs in my code.
>
> Right, except for the lacking documentation (you really should add that,
> since it is _not_ obvious).
>
> > Somebody else?
>
> Umm.  You culled the Cc: list pretty early in our discussion.  I thought
> it was on purpose...  So there is nobody else listening, but yours truly.

Sometimes I really hate gmail, it hides details on purpose and makes
really easy making such mistakes.

I'll add some note and repost the patch.

Re: [Qemu-devel] [PATCH] vvfat mbr fixes

[Johannes Schindelin]

Hi,

On Mon, 24 Sep 2007, Ivan Kalvachev wrote:

> 2007/9/24, Johannes Schindelin <Johannes.Schindelin@gmx.de>:
>
> > On Mon, 24 Sep 2007, Ivan Kalvachev wrote:
> >
> > > I had a discussion with Johannes Schindelin over my patch, that I 
> > > thought is on the maillist, but apparently it wasn't. I'm 
> > > subscribed, so please don't send me mails directly, gmail web 
> > > interface could be quite misleading.
> > >
> > > So here is the third revision of my patch. Changes include: using 
> > > more structures instead of fixed byte locations. chs and nt_id. more 
> > > detailed comments, function name shortened and if(lba) moved to ?: 
> > > construct.
> >
> > Almost all my comments went unheeded.
> 
> I believe that I've answered and addressed all your comments.

Ooops.  I think I mixed up your patch with the other patch for vvfat that 
floated around recently.  (Probably because the patch was not inlined...)

FWIW if we're talking about qemu_vvfat_mbr_v3.patch, I have no more 
gripes.

Thanks,
Dscho

Re: [Qemu-devel] [PATCH] vvfat mbr fixes

[Ivan Kalvachev]

2007/9/24, Johannes Schindelin <Johannes.Schindelin@gmx.de>:
> Hi,
> On Mon, 24 Sep 2007, Ivan Kalvachev wrote:
[...]
> > I believe that I've answered and addressed all your comments.
>
> Ooops.  I think I mixed up your patch with the other patch for vvfat that
> floated around recently.  (Probably because the patch was not inlined...)
>
> FWIW if we're talking about qemu_vvfat_mbr_v3.patch, I have no more
> gripes.


Great. Now if somebody is willing to commit it?

[Qemu-devel] Re: [PATCH] vvfat mbr fixes

[Lorenzo Campedelli]

Johannes Schindelin wrote:
> Hi,
> 
> On Mon, 24 Sep 2007, Ivan Kalvachev wrote:
> 
>> 2007/9/24, Johannes Schindelin <Johannes.Schindelin@gmx.de>:
>>
>>> On Mon, 24 Sep 2007, Ivan Kalvachev wrote:
>>>
>>>> I had a discussion with Johannes Schindelin over my patch, that I 
>>>> thought is on the maillist, but apparently it wasn't. I'm 
>>>> subscribed, so please don't send me mails directly, gmail web 
>>>> interface could be quite misleading.
>>>>
>>>> So here is the third revision of my patch. Changes include: using 
>>>> more structures instead of fixed byte locations. chs and nt_id. more 
>>>> detailed comments, function name shortened and if(lba) moved to ?: 
>>>> construct.
>>> Almost all my comments went unheeded.
>> I believe that I've answered and addressed all your comments.
> 
> Ooops.  I think I mixed up your patch with the other patch for vvfat that 
> floated around recently.  (Probably because the patch was not inlined...)
> 
> FWIW if we're talking about qemu_vvfat_mbr_v3.patch, I have no more 
> gripes.
> 
> Thanks,
> Dscho
> 
> 
> 
I think you were referring to the small patch I sent.
I actually gave up with it, as I don't see how to make
it in a clean way.

Honestly I found your suggestion to try to have it
less special-casing vvfat a bit puzzling...
vvfat is the only case in which there's any need to
override realpath() behaviour, so I tried to make it
as clear as possible.
Why is it better to affect code paths which don't need
any change?

Regards,
Lorenzo

[Qemu-devel] Re: [PATCH] vvfat mbr fixes

[Johannes Schindelin]

Hi,

On Mon, 24 Sep 2007, Lorenzo Campedelli wrote:

> I think you were referring to the small patch I sent. I actually gave up 
> with it, as I don't see how to make it in a clean way.
> 
> Honestly I found your suggestion to try to have it less special-casing 
> vvfat a bit puzzling... vvfat is the only case in which there's any need 
> to override realpath() behaviour, so I tried to make it as clear as 
> possible.

It makes the code ugly as hell, and it limits (unnecessarily) future 
extensions.

But since you made quite clear that you do not want to change your patch, 
I will stop wasting my time.

Ciao,
Dscho