msdosfs header unification patch
Bruce Evans
brde at optusnet.com.au
Fri Jul 13 13:54:13 UTC 2007
On Fri, 13 Jul 2007, Brian Chu wrote:
> The included diff is the work I've done on header consolidation in
> msdosfs. The three areas I concentrated on where:
>
> - sys/fs/msdosfs/
> - sys/geom/label/g_label_msdosfs.*
> - sbin/fsck_msdosfs/
>
> I decided to preserve the headers in sys/fs/msdosfs since the bulk of
> the code lives there and is most widely understood (don't want to be
> pulling the rug from under other developers here).
Good.
> There were several places where a meta-structure describing
> FAT12/FAT16/FAT32 bootsectors and bpb's were kept intact because they
> were more compact than the actual on disk bs/bpb's.
>
> Please feel free to comment!
Somthing seems to have corrupted most of the tabs before it was mailed.
% --- //depot/projects/soc2007/chub-msdosfs2/sbin/fsck_msdosfs/boot.c 2007/06/29 00:42:42
% +++ //depot/projects/soc2007/chub-msdosfs2/sbin/fsck_msdosfs/boot.c 2007/07/08 08:28:50
% @@ -30,8 +30,8 @@
% * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
% */
%
% +#include <sys/cdefs.h>
%
% -#include <sys/cdefs.h>
% #ifndef lint
% __RCSID("$NetBSD: boot.c,v 1.9 2003/07/24 19:25:46 ws Exp $");
% static const char rcsid[] =
Just adds a style bug.
% @@ -44,131 +44,164 @@
% #include <stdio.h>
% #include <unistd.h>
%
% +#include <fs/msdosfs/bootsect.h>
% +#include <fs/msdosfs/bpb.h>
% #include "ext.h"
% #include "fsutil.h"
%
% int
% -readboot(dosfs, boot)
% - int dosfs;
% - struct bootblock *boot;
% +readboot(int dosfs, struct bootblock *boot)
% {
% - u_char block[DOSBOOTBLOCKSIZE];
% - u_char fsinfo[2 * DOSBOOTBLOCKSIZE];
% - u_char backup[DOSBOOTBLOCKSIZE];
% + union bootsector buffer;
% + union bootsector backup;
% + struct byte_bpb50 *pfat50;
% + struct byte_bpb710 *pfat710;
% + struct byte_Extboot *pfatext;
% + struct fsinfo fsstruct;
The tab lossage mostly looks like this.
% int ret = FSOK;
%
% - if (read(dosfs, block, sizeof block) < sizeof block) {
% + if (read(dosfs, &buffer, sizeof(union bootsector))
% + < sizeof(union bootsector)) {
% perror("could not read boot block");
% return FSFATAL;
% }
The old code above (declarations and read()'s third arg) had fewer
bugs. read() should read into character arrays, not into structs.
It is necessary to do i/o in units of the sector size. readboot()
must try various sizes, since the sector size is unknown initially.
After reading something, it should get the sector size from the BPB
after doing a few sanity checks. Buffers can then be sized using
expressions like roundup(sizeof(foo), boot->BytesPerSec) or just
boot->BytesPerSec since most data structures fit in one 512-byte
sector and sector sizes < 512 are not supported.
All i/o here except for fsinfo currently fails if the sector size is
!= 512. fsinfo has the wrong size 1024 (without your changes), so
i/o to fsinfo would accidentally succeed for sector size 1024 if
execution reached that far.
% ...
% + /* decode bios parameter block and store in a compacted
% + * architecture independent data structure for future use
% + */
Various style bugs here.
% ...
% + boot->BytesPerSec = getushort(pfat50->bpbBytesPerSec);
% + boot->SecPerClust = pfat50->bpbSecPerClust;
% + boot->ResSectors = getushort(pfat50->bpbResSectors);
% + boot->FATs = pfat50->bpbFATs;
% + boot->RootDirEnts = getushort(pfat50->bpbRootDirEnts);
% + boot->Sectors = getushort(pfat50->bpbSectors);
% + boot->Media = pfat50->bpbMedia;
% + boot->SecPerTrack = getushort(pfat50->bpbSecPerTrack);
% + boot->Heads = getushort(pfat50->bpbHeads);
% + boot->HiddenSecs = getulong(pfat50->bpbHiddenSecs);
% + boot->HugeSectors = getulong(pfat50->bpbHugeSectors);
This part is fine.
% ...
% - /* Check backup FSInfo? XXX */
% +
% + /* Unnecessary to check the backup FSInfo because there isn't
% + * an physical backup copy of the FSInfo block. There's only a
% + * backup of the FSInfo block number, which was checked (bsBPB) above.
% + */
% }
But there is a physical backup FSinfo.
The normal layout is:
main boot sector (sector 0)
fsinfo (sector 1, but can be anywhere)
second boot sector (next sector? -- no pointer to this)
fsisig4 checks/clobbers here
backup main boot sector (sector 3, but can be anywhere)
backup fsinfo (next sector? -- no pointer to this, since backup
main boot sector can't have a different pointer than main bs)
backup second boot sector (next sector?)
backup fsisig4 would check/clobber here
newfs_msdos omits the second boot sector and creates the following layout:
main boot sector (sector 0)
fsinfo (sector 1)
backup main boot sector (sector 2)
fsisig4 checks/clobbers here
backup fsinfo (sector 3)
backup fsisig4 would check/clobber outside of all boot sectors
This is valid (WinXP accepts it) for sector size 512, but newfs_msdos
puts all fsinfo stuff except fsisig1 in a logically wrong place, at
the end of the sector, so it creates invalid fsinfo (FreeBSD rejects
it) if the sector size != 512.
% --- //depot/projects/soc2007/chub-msdosfs2/sys/fs/msdosfs/bootsect.h 2007/06/29 00:42:42
% +++ //depot/projects/soc2007/chub-msdosfs2/sys/fs/msdosfs/bootsect.h 2007/07/08 08:01:09
% @@ -17,6 +20,8 @@
% * October 1992
% */
%
% +#include <fs/msdosfs/bpb.h>
% +
% /*
% * Format of a boot sector. This is the first sector on a DOS floppy disk
% * or the fist sector of a partition on a hard disk. But, it is not the
Namespace pollution.
% @@ -25,7 +30,8 @@
% struct bootsector33 {
% u_int8_t bsJump[3]; /* jump inst E9xxxx or EBxx90 */
% int8_t bsOemName[8]; /* OEM name and version */
% - int8_t bsBPB[19]; /* BIOS parameter block */
% + // int8_t bsBPB[19]; /* BIOS parameter block */
% + struct bpb33 bsBPB; /* BIOS parameter block */
This gained independence of bpb.h by not using a struct.
If a struct is used here, then it has to be struct byte_bpb. struct_bpb33
has size != 19 due to padding.
% @@ -47,8 +63,8 @@
% struct bootsector50 {
% u_int8_t bsJump[3]; /* jump inst E9xxxx or EBxx90 */
% int8_t bsOemName[8]; /* OEM name and version */
% - int8_t bsBPB[25]; /* BIOS parameter block */
% - int8_t bsExt[26]; /* Bootsector Extension */
% + struct bpb50 bsBPB; /* BIOS parameter block */
% + struct extboot bsExt; /* Bootsector Extension */
% int8_t bsBootCode[448]; /* pad so structure is 512b */
% u_int8_t bsBootSectSig0;
% u_int8_t bsBootSectSig1;
% @@ -59,8 +75,8 @@
% struct bootsector710 {
% u_int8_t bsJump[3]; /* jump inst E9xxxx or EBxx90 */
% int8_t bsOEMName[8]; /* OEM name and version */
% - int8_t bsBPB[53]; /* BIOS parameter block */
% - int8_t bsExt[26]; /* Bootsector Extension */
% + struct bpb710 bsBPB; /* BIOS parameter block */
% + struct extboot bsExt; /* Bootsector Extension */
% int8_t bsBootCode[420]; /* pad so structure is 512b */
% u_int8_t bsBootSectSig0;
% u_int8_t bsBootSectSig1;
As above. You declared byte_extboot, but don't use it here. Also in
fsck_msdosfs, extboot is used where byte_extboot seems to be needed.
% --- //depot/projects/soc2007/chub-msdosfs2/sys/fs/msdosfs/bpb.h 2007/06/29 00:42:42
% +++ //depot/projects/soc2007/chub-msdosfs2/sys/fs/msdosfs/bpb.h 2007/07/08 08:01:09
% @@ -78,7 +81,7 @@
% u_int32_t bpbRootClust; /* start cluster for root directory */
% u_int16_t bpbFSInfo; /* filesystem info structure sector */
% u_int16_t bpbBackup; /* backup boot sector */
% - /* There is a 12 byte filler here, but we ignore it */
% + u_int8_t bpbReserved[12]; /* 12 byte filler here */
% };
%
% /*
% @@ -153,7 +156,7 @@
% u_int8_t bpbRootClust[4]; /* start cluster for root directory */
% u_int8_t bpbFSInfo[2]; /* filesystem info structure sector */
% u_int8_t bpbBackup[2]; /* backup boot sector */
% - /* There is a 12 byte filler here, but we ignore it */
% + u_int8_t bpbReserved[12]; /* 12 byte filler here */
% };
%
% /*
% @@ -170,3 +173,6 @@
% u_int8_t fsifill3[508];
% u_int8_t fsisig4[4];
% };
% +
% +#endif
% +// _FS_MSDOSFS_BPB_H
The vendor (NetBSD) version has similar changes.
[...]
There is also newfs_msdosfs. It doesn't even use headers for msdsofs
things, but declares everything in its own style.
Bruce
More information about the freebsd-fs
mailing list