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

[Ivan Kalvachev]

2007/9/24, Johannes Schindelin <address@hidden>:
> 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 <address@hidden>
Date: 23.09.2007 03:27
Subject: Re: [Qemu-devel] [PATCH] vvfat mbr fixes
To: Johannes Schindelin <address@hidden>


2007/9/23, Johannes Schindelin <address@hidden>:
> 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 <address@hidden>
Date: 23.09.2007 04:25
Subject: Re: [Qemu-devel] [PATCH] vvfat mbr fixes
To: Ivan Kalvachev <address@hidden>


Hi,

On Sun, 23 Sep 2007, Ivan Kalvachev wrote:

> 2007/9/23, Johannes Schindelin <address@hidden>:
> >
> > 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 <address@hidden>
Date: 23.09.2007 18:58
Subject: Re: [Qemu-devel] [PATCH] vvfat mbr fixes
To: Johannes Schindelin <address@hidden>


2007/9/23, Johannes Schindelin <address@hidden>:
> Hi,
>
> On Sun, 23 Sep 2007, Ivan Kalvachev wrote:
>
> > 2007/9/23, Johannes Schindelin <address@hidden>:
> > >
> > > 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 <address@hidden>
Date: 23.09.2007 22:28
Subject: Re: [Qemu-devel] [PATCH] vvfat mbr fixes
To: Ivan Kalvachev <address@hidden>


Hi,

On Sun, 23 Sep 2007, Ivan Kalvachev wrote:

> 2007/9/23, Johannes Schindelin <address@hidden>:
>
> > 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 <address@hidden>
Date: 24.09.2007 00:43
Subject: Re: [Qemu-devel] [PATCH] vvfat mbr fixes
To: Johannes Schindelin <address@hidden>


2007/9/23, Johannes Schindelin <address@hidden>:
> Hi,
>
> On Sun, 23 Sep 2007, Ivan Kalvachev wrote:
>
> > 2007/9/23, Johannes Schindelin <address@hidden>:
> >
> > > 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 <address@hidden>
Date: 24.09.2007 01:12
Subject: Re: [Qemu-devel] [PATCH] vvfat mbr fixes
To: Ivan Kalvachev <address@hidden>


Hi,

On Mon, 24 Sep 2007, Ivan Kalvachev wrote:

> 2007/9/23, Johannes Schindelin <address@hidden>:
> > Hi,
> >
> > On Sun, 23 Sep 2007, Ivan Kalvachev wrote:
> >
> > > 2007/9/23, Johannes Schindelin <address@hidden>:
> > >
> > > > 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 <address@hidden>
Date: 24.09.2007 04:07
Subject: Re: [Qemu-devel] [PATCH] vvfat mbr fixes
To: Ivan Kalvachev <address@hidden>


Hi,

On Mon, 24 Sep 2007, Ivan Kalvachev wrote:

> 2007/9/24, Johannes Schindelin <address@hidden>:
> >
> > On Mon, 24 Sep 2007, Ivan Kalvachev wrote:
> >
> > > 2007/9/23, Johannes Schindelin <address@hidden>:
> > > >
> > > > On Sun, 23 Sep 2007, Ivan Kalvachev wrote:
> > > >
> > > > > 2007/9/23, Johannes Schindelin <address@hidden>:
> > > > >
> > > > > > 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 <address@hidden>
Date: 24.09.2007 12:29
Subject: Re: [Qemu-devel] [PATCH] vvfat mbr fixes
To: Johannes Schindelin <address@hidden>


2007/9/24, Johannes Schindelin <address@hidden>:
> Hi,
>
> On Mon, 24 Sep 2007, Ivan Kalvachev wrote:
>
> > 2007/9/24, Johannes Schindelin <address@hidden>:
> > >
> > > On Mon, 24 Sep 2007, Ivan Kalvachev wrote:
> > >
> > > > 2007/9/23, Johannes Schindelin <address@hidden>:
> > > > >
> > > > > On Sun, 23 Sep 2007, Ivan Kalvachev wrote:
> > > > >
> > > > > > 2007/9/23, Johannes Schindelin <address@hidden>:
> > > > > >
> > > > > > > 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.