FATFS: Add special case for label creation

[andrewbird]

Opening draft PR so we can discuss things

[andrewbird]

I thought it about time that I addressed the FAT label issue FDOS/label#17 on FDPP. Currently the kernel sees identically named volume labels and files/directories as in conflict. MS-DOS 6.22 and DR-DOS 7.01 allow them to coexist. I added some tests to my topic branch dosemu2/dosemu2@ff58161 to show the problem.

Currently (without this patch):
1 - creating a label with same name as an existing file succeeds but the original file is deleted.
2 - creating a label with same name as an existing directory fails with error.
3 - creating a new file with same name as existing label succeeds.
4 - creating a new directory with same name as existing label succeeds.

So you can see that at the very least the current behaviour is asymmetric based on creation order.

Now for a discussion about the patch. I've created a special case that only comes into play if we have the volid set, are in the root directory and are doing file create/truncate. This seems to work quite nicely, however there's a strange behaviour with the find_fname() function.

  status = find_fname(path, D_VOLID, fnp);
  if ((status == SUCCESS) && (fnp->f_dir.dir_attrib & D_VOLID)) {

I thought that by specifying the D_VOLID attribute I'd only be finding the volume names. However it seems that plain files are also found, which I why I added the additional check (fnp->f_dir.dir_attrib & D_VOLID). I think this is why in case 1 above an existing plain file gets deleted when creating a label. I'm loathed to change find_fname() since it is used all over the place though and I don't necessarily understand the consequences of doing so.

Thoughts?

[stsp]

if find_fname() finds both and you
explicitly allow duplicates, then in
some other code find_fname() will
find vol instead of real file, which
would break something.
So I think if you want to fix that, you
need to start from fixing find_fname().

Or alternatively maybe you can use
your light-weight approach which works
fine but doesn't allow writing to a vol
files? By writing volume to the boot
sector.

[andrewbird]

if find_fname() finds both and you
explicitly allow duplicates, then in
some other code find_fname() will
find vol instead of real file, which
would break something.

Yes, that's a very good point! As I note in cases 3 and 4 above the duplicate filename (file/directory) with label problem already exists, but only in the case that the vol was set first and the file/directory added afterwards. I can imagine that any search with find_fname() is already returning the volume first. I'll have to see if those results are being further filtered by the caller, else I can't explain why things seeming work okay now.

So I think if you want to fix that, you
need to start from fixing find_fname().

Mmm, I was afraid you'd say that. I suppose I'll need to find all its callers, then I'll know what to check afterwards.

By writing volume to the boot
sector.

I think I will have to do that in addition to what I have here because there's already the case in the wild of BPB volname and root directory volid file contents getting out of sync. This is what MS-DOS 6.22 does to avoid that problem.

[stsp]

can't explain why things seeming work okay now.

Maybe because no one creates
vol+file with the same name?
But obviously making vol just an
attr for a regular file, was not the
brightest idea of MS...

[andrewbird]

It seems findfile/next returning other files with VOLID bit set was DOS 2 behaviour, with 3 and later it only returns the label. I've added some code to do this. The tests all pass now.

I now need to think about setting the volume name in BPB too. Have you any thoughts on how best to do this cleanly?

[stsp]

What will this give us?
If it is never referenced by DOS itself,
maybe we won't bother?

[andrewbird]

What will this give us?

Ensure that disk utilities like FDISK/format will report the same name that a user set and that reports like this one https://gitlab.com/FreeDOS/issue-reporting/-/issues/31 won't occur.

[stsp]

I really don't know how that should
be done. I think format.com creates
bpb and I suspect vol command
should alter bpb for it?

[andrewbird]

Looks like here

fdpp/kernel/dsk.c

Lines 682 to 711 in fbd9ee2

	case 0x46: /* set volume serial number */
	{
	struct Gioc_media FAR *gioc = rp->r_gioc;
	struct FS_info FAR *fs;
	ret = getbpb(pddt);
	if (ret != 0)
	return (ret);
	/* return error if media lacks extended BPB with serial # */
	{
	BYTE extended_BPB_signature =
	DiskTransferBuffer[(pddt->ddt_bpb.bpb_nfsect != 0 ? 0x26 : 0x42)];
	/* 0x29 is usual signature value for serial#,vol label,& fstype;
	0x28 older EBPB signature indicating only serial# is valid */
	if ((extended_BPB_signature != 0x29) && (extended_BPB_signature != 0x28))
	return failure(E_MEDIA);
	}
	/* otherwise, store serial # in extended BPB */
	fs = (struct FS_info FAR *)&DiskTransferBuffer
	[(pddt->ddt_bpb.bpb_nfsect != 0 ? 0x27 : 0x43)];
	fs->serialno = gioc->ioc_serialno;
	pddt->ddt_serialno = fs->serialno;
	ret = RWzero(pddt, LBA_WRITE);
	if (ret != 0)
	return (dskerr(ret));
	}
	break;

is a good place to start.

[stsp]

Serial number??
I really think the good place to
start is a vol command.

[andrewbird]

Serial number??

Yes serial number is the adjacent field in the bpb. This code shows me how to get the bpb (location can vary if FAT32), test for proper bpb version, set the field to be changed (in my case will be volume name), and write it back to disk.

I really think the good place to
start is a vol command.

MS-DOS 6.22 sets bpb->volume_name within the kernel at the same time as writing the file with volid set. I want to avoid changing all the userspace tools.

[stsp]

Sounds like a plan actually.
http://www.ctyme.com/intr/rb-2896.htm#Table1560
Here we see this note:

(generally CL=66h only, but MS-DOS 5.0 will write the
given filesystem type to the disk)

Which means that all we need is to "abuse"
0x46 a bit. So yes, your idea looks good to me.

[andrewbird]

This seems to work.

[stsp]

Ok I thought you can use fn 0x46,
but your code looks simple enough.

[andrewbird]

Do you know if there's an easy way to wrap a partition table around disk image file? I want to do this:

$ dd if=/dev/zero of=fat32.img bs=1024k count=50
$ mkfs.fat -F 32 fat32.img 
$ fatlabel fat32.img INITIAL
# do something to make fat32.img compatible with dosemu2

Now run test
1 - modify the label in FDPP
2 - shutdown dosemu
3 - check that the label appears in the correct place in the image file

I specifically don't want to have to use root privilege for something like kpartx

[stsp]

I can think of partitioning the hd image
with fdisk and then dd the partition image
over hd image with an offset.
Will that work? I think the default offset
of a first partition is fixed.

[stsp]

Oh, try fuseloop tool - seems to be
exactly for that.

[andrewbird]

Thanks, I'll give them both a try.

[andrewbird]

I have now successfully tested these patches with FAT12, FAT16 and FAT32 filesystems.

[andrewbird]

Don't apply yet, as I need to consider the unlikely but valid case where bpb is v7 and fs is not fat32. It says "A common misconception is that this BPB is exclusive to 32-bit FAT. It is not. Just as there was no reason that the version 4.0 BPB could not be used for 12-bit FAT, there is no reason that the version 7.0 BPB cannot be used for filesystem formats other than 32-bit FAT. "

[ecm-pushbx]

mtools just had this edge case fixed. The truth is that the word (16-bit) "sectors per FAT" field being zero indicates using both the larger EBPB and FAT32. It doesn't really matter how many clusters there are, you can create a FAT32 file system with less than 64 Ki clusters. That seems to be what most drivers, including Microsoft's, actually do.

[ecm-pushbx]

Quoting from the mailing list for mtools: https://lists.gnu.org/archive/html/info-mtools/2022-09/msg00003.html

Actually, according to Microsoft's specification, number of FAT bits of
an existing filesystem is SOLELY determined by the number of clusters.
This applies to all three bit numbers: 12/16/32

Yes, the (in)famous fatgen103.pdf document, retired from Microsoft's own
site, but still available at
https://web.archive.org/web/20210723100623/http://download.microsoft.com/download/1/6/1/161ba512-40e2-4cc9-843a-923143f3456c/fatgen103.doc

It sternly says at page 14:

"It is really quite simple how this works. The FAT type— one of FAT12,
FAT16, or FAT32—is determined by the count of clusters on the volume
and nothing else."

But see below...:

[...]

But as Alain pointed Microsoft FAT implementation (fastfat.sys) for
FAT32 detection does not use number of clusters, but rather explicit
marking of FAT32. Same behavior has Linux kernel implementation
(msdos.ko and vfat.ko) and same in dosfstools project. Hence this
non-standard FAT32 can be still created by mkfs.fat.

Actually, what I meant in my previous mail was that everything else
that is referred to in the FAT32 extended boot block (root directory
location, etc.) would depend on this explicit marking
(fatLen/bigFatLen), but not the number of FAT bits themselves.

However, your remark got me thinking, and so I tried it out on a Windows
10 VM.

... and lo and behold: the mere presence of a FAT32 extended boot block
(as signaled by fat_len being 0) was enough to make it use 32 fat bits,
even if the number of clusters was too low for FAT32.

Sorry for having been to easily misled by an assertive, but nonfactual,
Microsoft statement.

Given this finding, I'll shortly make a new mtools release that will
consider presence of FAT32 extended boot block as well for picking
appropriate number of FAT bits.

[ecm-pushbx]

if (DiskTransferBuffer[0x26] == 0x29)         // BPB v4.1
    offset = 0x27;

This seems wrong. If you have a FAT32 EBPB then the byte at offset 26h is part of the ebpbSectorsPerFATLarge field, for which 29h would be a valid value. You should check bpbSectorsPerFAT == 0 to determine whether to use the EBPB or BPB new fields.

[andrewbird]

So back on this now. I wrote an additional test for LFNs (andrewbird/dosemu2@6ce3be8) , and my current code fails it. When creating a new volume label, I need to scan the root directory to ensure that there is not already a label. I use find_in_dir() but that gives me a false positive if there's any LFNs defined.

[stsp]

I use find_in_dir() but that gives me a false positive if there's any LFNs defined.

There was probably a brackets error in
a condition, not sure how harmful it was.
Fixed now.
Can you re-try?
If it doesn't help you might be able to
identify the problem as the condition in
find_in_dir() is not too complex.

[andrewbird]

Can you re-try?

Thanks, but it didn't fix my issue here.

If it doesn't help you might be able to
identify the problem as the condition in
find_in_dir() is not too complex.

So would you expect that specifying find_in_dir(D_VOLID, D_VOLID | D_RDONLY | D_ARCHIVE, fnp); should ignore LFN entries (D_RDONLY | D_HIDDEN | D_SYSTEM | D_VOLID) because the attr_allow argument doesn't have D_HIDDEN | D_SYSTEM set?

[stsp]

Yes, I think so.
Why its not the case?

[andrewbird]

I don't think so, but I'll have to instrument the code a little to be sure.

[andrewbird]

Why its not the case?

Sorry, my mistake in rebasing the December label test code onto current devel without reviewing. I had already set a volume label in the common code that makes the image, and this test didn't delete it first!

[andrewbird]

Mmm, a comment from my test case!

/* it's very important that the disk has never had a label before */

[ecm-pushbx]

So would you expect that specifying find_in_dir(D_VOLID, D_VOLID | D_RDONLY | D_ARCHIVE, fnp); should ignore LFN entries (D_RDONLY | D_HIDDEN | D_SYSTEM | D_VOLID) because the attr_allow argument doesn't have D_HIDDEN | D_SYSTEM set?

I think findfirst/findnext types of calls should never return LFN entries on an LFN-aware kernel (MS-DOS v7 compatibility). I don't know any reason that find_in_dir should return LFN entries.

[stsp]

I don't think it does, it was just a
speculation based in your mis-information.

[andrewbird]

it was just a
speculation based in your mis-information.

Mine, not @ecm-pushbx 's

[stsp]

Mine, not @ecm-pushbx 's

Correct, sorry for confusion!
I simply haven't looked at who's an msg author. :(

[stsp]

So what's the status of this?

[stsp]

OK, thanks.

[ecm-pushbx]

I read that it's required to set volume id as you can't open an existing label as a file, but I don't have access to my DOS book for the next few days to check. I did some tests in an earlier ticket which showed dosemu2/dosemu2#1771 there was very little you could do other than create the label (no others can exist on the volume), write, seek on it, and close. Afterwards all you could do was find and delete it.

In EDR-DOS you can (probably) FCB open or FCB create a label, but DOS will immediately close the FCB before the control flow returns from the int 21h: https://hg.pushbx.org/ecm/edrdos/file/b390f7f10fd0/drdos/fcbs.a86#l217 So no seeking or writing to it is possible.