| Message ID | 1340984094-5451-4-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Jun 29, 2012 at 3:34 PM, Markus Armbruster <armbru@redhat.com> wrote: > Unless parameter ":floppy:" is given, vvfat creates a virtual image > with DOS MBR defining a single partition which holds the FAT file > system. The size of the virtual image depends on the width of the > FAT: 32 MiB (CHS 64, 16, 63) for 12 bit FAT, 504 MiB (CHS 1024, 16, > 63) for 16 and 32 bit FAT, leaving (64*16-1)*63 = 64449 and > (1024*16-1)*64 = 1032129 sectors for the partition. > > However, it screws up the end of the partition in the MBR: > > FAT width param. start CHS end CHS start LBA size > :32: 0,1,1 1023,14,63 63 1032065 > :16: 0,1,1 1023,14,55 63 1032057 > :12: 0,1,1 63,14,55 63 64377 > > The actual FAT file system nevertheless assumes the partition has > 1032129 or 64449 sectors. Oops. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > block/vvfat.c | 7 ++++--- > 1 files changed, 4 insertions(+), 3 deletions(-) > > diff --git a/block/vvfat.c b/block/vvfat.c > index 0fd3367..62745b5 100644 > --- a/block/vvfat.c > +++ b/block/vvfat.c > @@ -394,11 +394,12 @@ static void init_mbr(BDRVVVFATState* s) > > /* 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 |= sector2CHS(s->bs, &partition->end_CHS, s->bs->total_sectors - 1); > > /*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); > + partition->start_sector_long = cpu_to_le32(s->first_sectors_number-1); Spaces around '-'. Thanks for fixing the other cases, BTW. > + partition->length_sector_long = cpu_to_le32(s->bs->total_sectors > + - s->first_sectors_number + 1); > > /* FAT12/FAT16/FAT32 */ > /* DOS uses different types when partition is LBA, > -- > 1.7.6.5 > >
Blue Swirl <blauwirbel@gmail.com> writes: > On Fri, Jun 29, 2012 at 3:34 PM, Markus Armbruster <armbru@redhat.com> wrote: >> Unless parameter ":floppy:" is given, vvfat creates a virtual image >> with DOS MBR defining a single partition which holds the FAT file >> system. The size of the virtual image depends on the width of the >> FAT: 32 MiB (CHS 64, 16, 63) for 12 bit FAT, 504 MiB (CHS 1024, 16, >> 63) for 16 and 32 bit FAT, leaving (64*16-1)*63 = 64449 and >> (1024*16-1)*64 = 1032129 sectors for the partition. >> >> However, it screws up the end of the partition in the MBR: >> >> FAT width param. start CHS end CHS start LBA size >> :32: 0,1,1 1023,14,63 63 1032065 >> :16: 0,1,1 1023,14,55 63 1032057 >> :12: 0,1,1 63,14,55 63 64377 >> >> The actual FAT file system nevertheless assumes the partition has >> 1032129 or 64449 sectors. Oops. >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> --- >> block/vvfat.c | 7 ++++--- >> 1 files changed, 4 insertions(+), 3 deletions(-) >> >> diff --git a/block/vvfat.c b/block/vvfat.c >> index 0fd3367..62745b5 100644 >> --- a/block/vvfat.c >> +++ b/block/vvfat.c >> @@ -394,11 +394,12 @@ static void init_mbr(BDRVVVFATState* s) >> >> /* 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 |= sector2CHS(s->bs, &partition->end_CHS, s->bs->total_sectors - 1); >> >> /*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); >> + partition->start_sector_long = cpu_to_le32(s->first_sectors_number-1); > > Spaces around '-'. Thanks for fixing the other cases, BTW. Got it, thanks! [...]
Am 29.06.2012 22:33, schrieb Blue Swirl: > On Fri, Jun 29, 2012 at 3:34 PM, Markus Armbruster <armbru@redhat.com> wrote: >> Unless parameter ":floppy:" is given, vvfat creates a virtual image >> with DOS MBR defining a single partition which holds the FAT file >> system. The size of the virtual image depends on the width of the >> FAT: 32 MiB (CHS 64, 16, 63) for 12 bit FAT, 504 MiB (CHS 1024, 16, >> 63) for 16 and 32 bit FAT, leaving (64*16-1)*63 = 64449 and >> (1024*16-1)*64 = 1032129 sectors for the partition. >> >> However, it screws up the end of the partition in the MBR: >> >> FAT width param. start CHS end CHS start LBA size >> :32: 0,1,1 1023,14,63 63 1032065 >> :16: 0,1,1 1023,14,55 63 1032057 >> :12: 0,1,1 63,14,55 63 64377 >> >> The actual FAT file system nevertheless assumes the partition has >> 1032129 or 64449 sectors. Oops. >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> --- >> block/vvfat.c | 7 ++++--- >> 1 files changed, 4 insertions(+), 3 deletions(-) >> >> diff --git a/block/vvfat.c b/block/vvfat.c >> index 0fd3367..62745b5 100644 >> --- a/block/vvfat.c >> +++ b/block/vvfat.c >> @@ -394,11 +394,12 @@ static void init_mbr(BDRVVVFATState* s) >> >> /* 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 |= sector2CHS(s->bs, &partition->end_CHS, s->bs->total_sectors - 1); >> >> /*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); >> + partition->start_sector_long = cpu_to_le32(s->first_sectors_number-1); > > Spaces around '-'. Thanks for fixing the other cases, BTW. For compensation there's an extra space before the '='. >> + partition->length_sector_long = cpu_to_le32(s->bs->total_sectors >> + - s->first_sectors_number + 1); Just wondering... This should be the same as s->sector_count, right? This whole geometry calculation code in vvfat is way too convoluted to understand when you haven't looked at it for some months... Kevin
Kevin Wolf <kwolf@redhat.com> writes: > Am 29.06.2012 22:33, schrieb Blue Swirl: >> On Fri, Jun 29, 2012 at 3:34 PM, Markus Armbruster <armbru@redhat.com> wrote: >>> Unless parameter ":floppy:" is given, vvfat creates a virtual image >>> with DOS MBR defining a single partition which holds the FAT file >>> system. The size of the virtual image depends on the width of the >>> FAT: 32 MiB (CHS 64, 16, 63) for 12 bit FAT, 504 MiB (CHS 1024, 16, >>> 63) for 16 and 32 bit FAT, leaving (64*16-1)*63 = 64449 and >>> (1024*16-1)*64 = 1032129 sectors for the partition. >>> >>> However, it screws up the end of the partition in the MBR: >>> >>> FAT width param. start CHS end CHS start LBA size >>> :32: 0,1,1 1023,14,63 63 1032065 >>> :16: 0,1,1 1023,14,55 63 1032057 >>> :12: 0,1,1 63,14,55 63 64377 >>> >>> The actual FAT file system nevertheless assumes the partition has >>> 1032129 or 64449 sectors. Oops. >>> >>> Signed-off-by: Markus Armbruster <armbru@redhat.com> >>> --- >>> block/vvfat.c | 7 ++++--- >>> 1 files changed, 4 insertions(+), 3 deletions(-) >>> >>> diff --git a/block/vvfat.c b/block/vvfat.c >>> index 0fd3367..62745b5 100644 >>> --- a/block/vvfat.c >>> +++ b/block/vvfat.c >>> @@ -394,11 +394,12 @@ static void init_mbr(BDRVVVFATState* s) >>> >>> /* 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 |= sector2CHS(s->bs, &partition->end_CHS, s->bs->total_sectors - 1); >>> >>> /*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); >>> + partition->start_sector_long = cpu_to_le32(s->first_sectors_number-1); >> >> Spaces around '-'. Thanks for fixing the other cases, BTW. > > For compensation there's an extra space before the '='. The original lined up the two '='. I preserved that. Not that I care for it. Want me to drop the extra space? >>> + partition->length_sector_long = cpu_to_le32(s->bs->total_sectors >>> + - s->first_sectors_number + 1); > > Just wondering... This should be the same as s->sector_count, right? Hmm. vvfat_open() assigns: s->sector_count = cyls * heads * secs - (s->first_sectors_number - 1); bs->total_sectors = cyls * heads * secs; But it then changes it minds and does: s->sector_count = s->faked_sectors + s->sectors_per_cluster*s->cluster_count; > This whole geometry calculation code in vvfat is way too convoluted to > understand when you haven't looked at it for some months... Yes, it is. I was briefly tempted to replace it, but then decided to limit my footprint in this buck-crazy block driver to minimal bug fixes.
Am 05.07.2012 11:23, schrieb Markus Armbruster: > Kevin Wolf <kwolf@redhat.com> writes: > >> Am 29.06.2012 22:33, schrieb Blue Swirl: >>> On Fri, Jun 29, 2012 at 3:34 PM, Markus Armbruster <armbru@redhat.com> wrote: >>>> Unless parameter ":floppy:" is given, vvfat creates a virtual image >>>> with DOS MBR defining a single partition which holds the FAT file >>>> system. The size of the virtual image depends on the width of the >>>> FAT: 32 MiB (CHS 64, 16, 63) for 12 bit FAT, 504 MiB (CHS 1024, 16, >>>> 63) for 16 and 32 bit FAT, leaving (64*16-1)*63 = 64449 and >>>> (1024*16-1)*64 = 1032129 sectors for the partition. >>>> >>>> However, it screws up the end of the partition in the MBR: >>>> >>>> FAT width param. start CHS end CHS start LBA size >>>> :32: 0,1,1 1023,14,63 63 1032065 >>>> :16: 0,1,1 1023,14,55 63 1032057 >>>> :12: 0,1,1 63,14,55 63 64377 >>>> >>>> The actual FAT file system nevertheless assumes the partition has >>>> 1032129 or 64449 sectors. Oops. >>>> >>>> Signed-off-by: Markus Armbruster <armbru@redhat.com> >>>> --- >>>> block/vvfat.c | 7 ++++--- >>>> 1 files changed, 4 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/block/vvfat.c b/block/vvfat.c >>>> index 0fd3367..62745b5 100644 >>>> --- a/block/vvfat.c >>>> +++ b/block/vvfat.c >>>> @@ -394,11 +394,12 @@ static void init_mbr(BDRVVVFATState* s) >>>> >>>> /* 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 |= sector2CHS(s->bs, &partition->end_CHS, s->bs->total_sectors - 1); >>>> >>>> /*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); >>>> + partition->start_sector_long = cpu_to_le32(s->first_sectors_number-1); >>> >>> Spaces around '-'. Thanks for fixing the other cases, BTW. >> >> For compensation there's an extra space before the '='. > > The original lined up the two '='. I preserved that. Not that I care > for it. Want me to drop the extra space? Ah, didn't notice that. I don't mind then. >>>> + partition->length_sector_long = cpu_to_le32(s->bs->total_sectors >>>> + - s->first_sectors_number + 1); >> >> Just wondering... This should be the same as s->sector_count, right? > > Hmm. vvfat_open() assigns: > > s->sector_count = cyls * heads * secs - (s->first_sectors_number - 1); > bs->total_sectors = cyls * heads * secs; > > But it then changes it minds and does: > > s->sector_count = s->faked_sectors + s->sectors_per_cluster*s->cluster_count; Which probably means that they differ if some sub-cluster sized space is left unused at the end of the disk. It's not useful to have this space included in the partition, but it doesn't hurt either. So I suppose either way is fine. >> This whole geometry calculation code in vvfat is way too convoluted to >> understand when you haven't looked at it for some months... > > Yes, it is. I was briefly tempted to replace it, but then decided to > limit my footprint in this buck-crazy block driver to minimal bug fixes. Heh, quite understandable. Kevin
Kevin Wolf <kwolf@redhat.com> writes: > Am 05.07.2012 11:23, schrieb Markus Armbruster: >> Kevin Wolf <kwolf@redhat.com> writes: >> >>> Am 29.06.2012 22:33, schrieb Blue Swirl: >>>> On Fri, Jun 29, 2012 at 3:34 PM, Markus Armbruster <armbru@redhat.com> wrote: >>>>> Unless parameter ":floppy:" is given, vvfat creates a virtual image >>>>> with DOS MBR defining a single partition which holds the FAT file >>>>> system. The size of the virtual image depends on the width of the >>>>> FAT: 32 MiB (CHS 64, 16, 63) for 12 bit FAT, 504 MiB (CHS 1024, 16, >>>>> 63) for 16 and 32 bit FAT, leaving (64*16-1)*63 = 64449 and >>>>> (1024*16-1)*64 = 1032129 sectors for the partition. >>>>> >>>>> However, it screws up the end of the partition in the MBR: >>>>> >>>>> FAT width param. start CHS end CHS start LBA size >>>>> :32: 0,1,1 1023,14,63 63 1032065 >>>>> :16: 0,1,1 1023,14,55 63 1032057 >>>>> :12: 0,1,1 63,14,55 63 64377 >>>>> >>>>> The actual FAT file system nevertheless assumes the partition has >>>>> 1032129 or 64449 sectors. Oops. >>>>> >>>>> Signed-off-by: Markus Armbruster <armbru@redhat.com> >>>>> --- >>>>> block/vvfat.c | 7 ++++--- >>>>> 1 files changed, 4 insertions(+), 3 deletions(-) >>>>> >>>>> diff --git a/block/vvfat.c b/block/vvfat.c >>>>> index 0fd3367..62745b5 100644 >>>>> --- a/block/vvfat.c >>>>> +++ b/block/vvfat.c >>>>> @@ -394,11 +394,12 @@ static void init_mbr(BDRVVVFATState* s) >>>>> >>>>> /* 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 |= sector2CHS(s->bs, &partition->end_CHS, s->bs->total_sectors - 1); >>>>> >>>>> /*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); >>>>> + partition->start_sector_long = cpu_to_le32(s->first_sectors_number-1); >>>> >>>> Spaces around '-'. Thanks for fixing the other cases, BTW. >>> >>> For compensation there's an extra space before the '='. >> >> The original lined up the two '='. I preserved that. Not that I care >> for it. Want me to drop the extra space? > > Ah, didn't notice that. I don't mind then. > >>>>> + partition->length_sector_long = cpu_to_le32(s->bs->total_sectors >>>>> + - s->first_sectors_number + 1); >>> >>> Just wondering... This should be the same as s->sector_count, right? >> >> Hmm. vvfat_open() assigns: >> >> s->sector_count = cyls * heads * secs - (s->first_sectors_number - 1); >> bs->total_sectors = cyls * heads * secs; >> >> But it then changes it minds and does: >> >> s->sector_count = s->faked_sectors + s->sectors_per_cluster*s->cluster_count; > > Which probably means that they differ if some sub-cluster sized space is > left unused at the end of the disk. It's not useful to have this space > included in the partition, but it doesn't hurt either. So I suppose > either way is fine. Complication: the partition should end on a cylinder boundary. Shaving off an unused tail may well interfere with that. Let's stick to v1 here. [...]
Am 05.07.2012 13:10, schrieb Markus Armbruster: > Kevin Wolf <kwolf@redhat.com> writes: > >> Am 05.07.2012 11:23, schrieb Markus Armbruster: >>> Kevin Wolf <kwolf@redhat.com> writes: >>> >>>> Am 29.06.2012 22:33, schrieb Blue Swirl: >>>>> On Fri, Jun 29, 2012 at 3:34 PM, Markus Armbruster <armbru@redhat.com> wrote: >>>>>> + partition->length_sector_long = cpu_to_le32(s->bs->total_sectors >>>>>> + - s->first_sectors_number + 1); >>>> >>>> Just wondering... This should be the same as s->sector_count, right? >>> >>> Hmm. vvfat_open() assigns: >>> >>> s->sector_count = cyls * heads * secs - (s->first_sectors_number - 1); >>> bs->total_sectors = cyls * heads * secs; >>> >>> But it then changes it minds and does: >>> >>> s->sector_count = s->faked_sectors + s->sectors_per_cluster*s->cluster_count; >> >> Which probably means that they differ if some sub-cluster sized space is >> left unused at the end of the disk. It's not useful to have this space >> included in the partition, but it doesn't hurt either. So I suppose >> either way is fine. > > Complication: the partition should end on a cylinder boundary. Shaving > off an unused tail may well interfere with that. Let's stick to v1 > here. Good point. If anything, maybe add a comment. Kevin
diff --git a/block/vvfat.c b/block/vvfat.c index 0fd3367..62745b5 100644 --- a/block/vvfat.c +++ b/block/vvfat.c @@ -394,11 +394,12 @@ static void init_mbr(BDRVVVFATState* s) /* 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 |= sector2CHS(s->bs, &partition->end_CHS, s->bs->total_sectors - 1); /*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); + partition->start_sector_long = cpu_to_le32(s->first_sectors_number-1); + partition->length_sector_long = cpu_to_le32(s->bs->total_sectors + - s->first_sectors_number + 1); /* FAT12/FAT16/FAT32 */ /* DOS uses different types when partition is LBA,
Unless parameter ":floppy:" is given, vvfat creates a virtual image with DOS MBR defining a single partition which holds the FAT file system. The size of the virtual image depends on the width of the FAT: 32 MiB (CHS 64, 16, 63) for 12 bit FAT, 504 MiB (CHS 1024, 16, 63) for 16 and 32 bit FAT, leaving (64*16-1)*63 = 64449 and (1024*16-1)*64 = 1032129 sectors for the partition. However, it screws up the end of the partition in the MBR: FAT width param. start CHS end CHS start LBA size :32: 0,1,1 1023,14,63 63 1032065 :16: 0,1,1 1023,14,55 63 1032057 :12: 0,1,1 63,14,55 63 64377 The actual FAT file system nevertheless assumes the partition has 1032129 or 64449 sectors. Oops. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- block/vvfat.c | 7 ++++--- 1 files changed, 4 insertions(+), 3 deletions(-)