Post Reply 
newRPL: [UPDATED April 27-2017] Firmware for testing available for download
08-08-2016, 06:28 PM (This post was last modified: 09-10-2016 01:51 PM by matthiaspaul.)
Post: #370
RE: newRPL: [UPDATED July-25-16] Firmware for testing available for download
Hi Claudio.
(08-02-2016 07:14 PM)Claudio L. Wrote:  I had a deeper look at these issues. I don't know where specifically you mention that a variant of the BPB needed to be detected.
I mentioned BPB issues as I thought they would be an obvious example, however, as it's quite complicated to reliably detect the correct BPB format, I planned to discuss this later in conjunction with other stuff related to disk login (there are several issues).

Quote:This implementation avoids anything that's non-standard, it's based directly on an official specification by MS (second external link on Wikipedia).
Unfortunately, Microsoft's "spec" is full of bugs and omissions, leaving out many important details. It is impossible to implement a 100% compatible FAT filesystem relying on this spec alone.
Quote:
Quote:The LONGMASK (0x0F) condition above is incomplete. On FAT12 and FAT16 volumes, the attribute combination 0x0F can also occur as part of a valid pending delete file under DELWATCH...
This seems to be only happening on one (yours?) non-standard implementation of FAT 12/16, incompatible with the FAT32 spec.
DELWATCH is DR DOS' dynamically loadable deletion tracking component. It hooks into and expands the FDOS to track and protect deleted files to allow reliable later undeletion, regardless of the number of subsequent writes to the volume, its fill level and fragmentation status, to a limited extent even of several versions of the same file. It can be configured to check for various patterns, filenames, attributes, ownerships, dates, ages, count of queued files, etc.). DELWATCH also enables the FDOS to use more sophisticated allocation strategies than the default ones (discussed last week).

Discussing DELWATCH's inner workings is not in the focus here, however, it is important to note that DELWATCH utilizes the volume attribute to do its magic. This volume attribute trick was later copied by Microsoft for their VFAT LFN implementation.

The good news is that both features can coexist peacefully on the same volume, but it is nevertheless important to distinguish between DELWATCH and VFAT entries. If you do not, your VFAT implementation will misinterpret DELWATCH entries as LFN entries causing all sorts of strange effects - potentially up to serious crashes or filesystem corruption (I'd have to closer analyze your code to examine how your implementation would actually react on this, but it should be obvious that it will cause problems - and at least on FAT12 and FAT16 volumes it is easy to avoid such problems).
Quote:While this is simple to support, I'm not sure it's in the best interest of the newRPL project to put much effort into supporting these minor variants.
IMO, it should be in the best interest for any filesystem implemention to work under any conditions, which can occur in practice. At the minimum, an implementation should not malfunction but bail out safely and refuse to work with volumes containing structures not supported.
So, your implementation surely does not need to implement various peculiarities for CHS access on FAT volumes any more, as they cannot occur on an SD card. But DELWATCH entries on FAT12 or FAT16 is well among the things a good implementation should be able to deal with (passively, that is).

Quote:
(08-01-2016 01:31 AM)matthiaspaul Wrote:  There's another bug when retrieving the start cluster value:

entry->FirstCluster = buffer[26] + (buffer[27]<<8) + (buffer[20]<<16) + (buffer[21]<<24);

The code above is only valid on FAT32 volumes; on FAT12 and FAT16 it would have to read as follows, as the 16-bit entry at 0x14 holds other information on those volumes:

entry->FirstCluster = buffer[0x1A] + (buffer[0x1B]<<8);

So, this could become something like:

entry->FirstCluster = buffer[0x1A] + (buffer[0x1B]<<8);
if (type == TYPE_FAT32)
entry->FirstCluster += (buffer[0x14]<<16) + (buffer[0x15]<<24);

I've seen similar code sections in quite a few other source files as well (you probably know them by heart, whereas I haven't written them down yet) and they need to be changed as well. In general, it is invalid to assume that the 16-bit entry at 0x14 in directory entries is 0 on FAT12 and FAT16 volumes.
This is fine, although the MS official specification of FAT I linked above actually says the high bytes of cluster number must be set to 0 on FAT16/FAT12 implementations.
Yes, it can be set to 0 on file creation (to initialize the reserved entry), but relying on it to be 0 as the high word of the cluster value when later opening the file will send your code to nirvana if any of the implementations for which this was declared as reserved would have changed it from 0 in the meantime.

Quote:
(08-01-2016 01:31 AM)matthiaspaul Wrote:  ...
// mainentry[12] = file->NTRes;
...
WriteInt16(mainentry+20, file->FirstCluster>>16);
WriteInt16(mainentry+26, file->FirstCluster);
...

Unless this would be on a FAT32 volume, it would be invalid to overwrite the contents of the 16-bit entry at offset 20 with the high cluster (unless this would hold the original contents when opening the file - however, this would make it necessary to change the usage of the FirstCluster variable as cluster variable).
Same as before, this code is consistent with the MS FAT specification, so it's not a bug at all and should be compatible with all other compliant FAT implementations.
Unfortunately, it isn't. Even Microsoft's own implementation distinguishes between 16-bit and 32-bit values here, as on FAT12 and FAT16 volumes this entry is used for the access rights bitmap under any version of DR DOS, PalmDOS, Novell DOS, OpenDOS, DRMK, FlexOS, 4680 OS, 4690 OS, Concurrent DOS, Multiuser DOS, System Manager and REAL/32, and for the extended attributes cluster under multitasking MS-DOS 4.0/4.1, (IIRC also: PC DOS 3.4), OS/2, eComstation and Windows NT.

Quote:
(08-01-2016 01:31 AM)matthiaspaul Wrote:  For some reasons, restoring the original value of the NTRes byte was commented out here. This byte is used for various purposes. It is important that the implementation does not change the contents of bits 7-5 and 2-0. It may change bits 3 and 4 when dealing with LFNs, and it may clear all bits when creating a new file. However, in an already existing file, the bits must not be changed.
That's exactly the reason why it's commented out: the bits must not be changed. This function does read-modify-write on an existing entry, only to update the size, date, etc. when you close the file, and it only modifies what needs to be modified, then writes back.
Okay, thanks for the explanation. Obviously, I am not familiar with the exact "dynamics" of your code yet. Clarifying this was exactly the reason why I brought this up for discussion.

Quote:
(08-01-2016 01:31 AM)matthiaspaul Wrote:  This should be changed to something like the following in order to allow files to be undeleted without having to enter the first character of the files again:

...
mainentry = buffer;

for (f = file->DirEntryNum; f > 0; --f, mainentry += 32) {
if (f == 1) {
mainentry[0x0D] = mainentry[0]; // save 1st char of SFN at 0x0D for later undeletion (overwrite no longer needed creation ms)
(u32*)mainentry[0x0E] = 0; // clear 0x0E..0x11 creation date & time of deleted file
}
mainentry[0] = 0xE5;
}
...
Is this something standard that many undelete programs use? or it's just the undelete feature of DR-DOS? I wonder if it's worth supporting or not (same as the other changes you suggested).
It's supported by the DR-DOS and PTS-DOS families, and by WinDOS. PTS-DOS even has a CONFIG.SYS directive named SAVENAME to enable or disable this feature. Not sure about FreeDOS, RxDOS and Linux right now. It is also supported by third-party suites of disk tools like Norton Utilities and PC Tools. (As Microsoft used Central Point's library, it might be even supported by Microsoft's UNDELETE utility, but I'm not sure about that any more).

(Sidenote regarding configurability: MS-DOS and Windows 9x have a CONFIG.SYS directive named ACCDATE to enable and disable the writing of access and creation date/time stamps in order to avoid potential problems with alternative uses and for performance reasons. By default, they don't write them to superfloppies and removable disks. IIRC, you use some bitflags per volume already, perhaps SAVENAME and ACCDATE should be made configurable on a drive-by-drive basis as well.)

Quote:These bugs you found so far are not actual bugs, but minor incompatibilities with a particular variant of FAT that's not widely in use today (unless you tell me Linux and Windows have incorporated these changes lately, then I'll be forced to do it).
The SAVENAME thing above is clearly an extension - I never called it a bug. I mentioned it merely because I ran into it looking at the source code and because it is so easy to implement at virtually no costs.

However, treating a variable as a 32-bit value of which only 16-bits are actually defined (as in the cluster example) is the same as using an uninitialized variable...
Anyway, my intention was not to count bugs, but simply to help improve an implemention so that it becomes more reliable.

In general, IMHO the reason to use FAT is because it is a "universal exchange format" requiring only minimal resources to implement and still give reasonable performance at least when used as a storage and exchange medium only. I like the fact that it is supported by virtually all systems and devices, big and small, new and old. This is quite an achievement, and we shouldn't give up on this lightheartly by introducing FAT implementations which are no longer compatible with existing older systems.

I mean, if you only care about compatibility with modern versions of Windows, why not implement exFAT instead of FAT?
To me, the specification that actually makes up FAT is the combination of all existing (and reasonably well coded) implementations, in particular those in desktop operating systems.

Quote:Now these code changes seem simple and doable, so I might incorporate them just for good measures to bullet-proof the code in case of corrupted data.
Not only corrupted data, but also data related to unsupported features. Utmost reliability is paramount in a filesystem implementation, IMO.
Quote:Now if you volunteer to improve and maintain newRPL's file system... that's a different story, then you can do all these changes and more while I work on RPL :-)
Unfortunately, I don't have the time for this in the foreseeable future. What I can do is analyze the code and point out weak spots where I see them, and in some cases I may also provide (pseudo) source code snippets as (more or less) drop-in replacements.

Greetings,

Matthias

PS. Perhaps all these filesystem related discussions should be split out to a new thread in order not to bore the other readers?


--
"Programs are poems for computers."
Find all posts by this user
Quote this message in a reply
Post Reply 


Messages In This Thread
RE: newRPL: [UPDATED July-25-16] Firmware for testing available for download - matthiaspaul - 08-08-2016 06:28 PM



User(s) browsing this thread: 1 Guest(s)