freemint / freemint

Unix-like kernel for Atari ST and compatible computers
https://freemint.github.io/freemint
Other
152 stars 34 forks source link

Issues with F32 handling #350

Open uweseimet opened 1 month ago

uweseimet commented 1 month ago

Somebody ;-) suggested I should use a snapshot zip in order to set up a fresh MiNT installation. This worked well (on a TT), but I noticed that STZIP reports some warnings. As far as I can tell this is caused by filenames with more than 8 characters (norwegian keyboard table, for instance) in the archive. You might want to verify this.

I needed to use drive C: for the installation, but I would have preferred a different drive. Usually I select the boot drive with a keystroke when I do not want to boot from my standard C: drive. When the drive with the MiNT installation is K:, and I select K: as the boot drive, the boot process starts normally, but at the very end it reports something like "setup complete, going to reboot", and then the system reboots. I would appreciate if you could launch the MiNT distribution from other drives than C:, by unpacking the archive there and booting from the selected drive.

uweseimet commented 1 month ago

MiNT reports that XHDI level 1.30 is supported and then says "kerinfo rejected". HDDRIVER does not reject the respective XHDI call and returns E_OK (at least it should), so something might be wrong here, because I don't think anything is rejected.

More things I stumbled upon:

th-otto commented 1 month ago

I needed to use drive C: for the installation, but I would have preferred a different drive. Usually I select the boot drive

I guess the problem is https://github.com/freemint/freemint/blob/b227ddf9df1513b6fbf9b3b85779e5e917dbdb94/doc/examples/mint.cnf#L260

There is a chicken/egg problem here: that tool is supposed to create a symlink to the current mint system dir (1-19-cur, or 1-19-xxx for bootable builds), but until then it is supposed to be located on drive C.

Can you drop into a shell (eg. by commenting out GEM= and INIT= lines in mint.cnf), and use ls -l /sysdir to see where that link points to? If it is still pointing to u:/c, then subsequent commands may still load everything from C:, possibly using incompatible older drivers (or not finding them at all). You may also try to change that line to exec /mint/sysdir.tos but i'm not sure whether that work.

As far as I can tell this is caused by filenames with more than 8 characters (norwegian keyboard table, for instance) in the >archive.

Yes, thats possible. That's a long standing problem, should be fixed, but IIRC those directory names are referenced at several places. Should not be a big deal unless you need one of the tables with too-long filenames.

th-otto commented 1 month ago
* When saving the desktop settings I would have expected open windows to be re-opened after a reset. This is not the case, and I also could not find any file (at least not in C:/, where anything was saved.

That's not an issue of freemint/XaAES, but an issue of the desktop. Maybe your $HOME environment variable is still pointing to drive C?

uweseimet commented 1 month ago
* When saving the desktop settings I would have expected open windows to be re-opened after a reset. This is not the case, and I also could not find any file (at least not in C:/, where anything was saved.

That's not an issue of freemint/XaAES, but an issue of the desktop. Maybe your $HOME environment variable is still pointing to drive C?

This happens when booting from C:. I have not changed any settings in the fiiles from the distribution, and my expectation is that with an unmodified distribution saving the desktop would work.

uweseimet commented 1 month ago

Can you drop into a shell (eg. by commenting out GEM= and INIT= lines in mint.cnf), and use ls -l /sysdir to see where that link points to? If it is still pointing to u:/c, then subsequent commands may still load everything from C:, possibly using incompatible older drivers (or not finding them at all). You may also try to change that line to exec /mint/sysdir.tos but i'm not sure whether that work.

I'm afraid I cannot spend much more time on this. My goal is to just run some FAT32-related tests with MiNT (still an open issue), and for that purpose I can do without my usual environment, which would have been on drive C: and up. But if the idea of the ZIP archive is to provide an initial setup for (new) users, this is an issue, because you are (initially at least) forced to use drive C: for a working environment. The wiki does not appear to address this (correct me if I am wrong).

As far as I can tell this is caused by filenames with more than 8 characters (norwegian keyboard table, for instance) in the >archive.

Yes, thats possible. That's a long standing problem, should be fixed, but IIRC those directory names are referenced at several places. Should not be a big deal unless you need one of the tables with too-long filenames.

It's not a big deal for me, but probably for others ...

th-otto commented 1 month ago

Ah ok. Yes that's strange, teradesk writes the file to <sys-root>/opt/GEM/teradesk/teradesk.inf, but does not seem to load it when booting.

th-otto commented 1 month ago

Seems to work as designed you have to activate "Save windows" first if you want the directory windows to be saved:

Screenshot_20240802_120036

uweseimet commented 1 month ago

I see, thank you for checking this.

th-otto commented 1 month ago
* Even when enabling VFAT for drive D: in mint.cnf, accessing a FAT32 filesystem on this drive does not work. There is an alert saying that rdlen = 0, which is normal for FAT32 filesystems.

I guess the message is from https://github.com/freemint/freemint/blob/b227ddf9df1513b6fbf9b3b85779e5e917dbdb94/sys/fatfs.c#L5109

which would indicate the drive was not recognized as FAT32. The detection is done at https://github.com/freemint/freemint/blob/b227ddf9df1513b6fbf9b3b85779e5e917dbdb94/sys/fatfs.c#L5392 based solely on the number of clusters. How many clusters does your partition have?

Add a fix for it now.

uweseimet commented 1 month ago

My partition is 16 MiB. It has about 30000 sectors with 8 sectors per cluster, i.e. less than 4000 clusters.

Trying to detect FAT32 partitions based on any information related to the partition size or number of clusters etc. is not correct. There are two ways (or a combination) to determine whether you have a FAT32 partition:

I will simply try a bigger partition later, where I would not need a fix. But what I would like to know first, because it is not clear to me from the documentation: Does a FAT32 partition require any (VFAT) setting in mint.cnf? I would expect no setting to be required, because FAT32 partitions can be detected automatically.

Important note: The partition type reported by XHDI has to be used, not any potential partition data obtained by analyzing the root sector. There might be none.

th-otto commented 1 month ago

Yes, the detection based on partition type `F32' was already done, but then later completely ignored.

FAT32 and VFAT should IMHO be independent of another, ie. theoretically you could have a FAT16 partition with VFAT entries, and a FAT32 partition without. But i don't know how well that was tested, i guess most users will use ext2fs when they need long filenames.

uweseimet commented 1 month ago

Most users may prefer ext2, I also would. But MiNT also claims FAT32 support, so more thorough testing is probably not a bad idea ;-). For exchanging files with long filenames with non-Linux platforms FAT32 is the only way.

Anyway, a 2 GiB FAT32 partition was working for me (without the fix), and I could successfully test what I primarily wanted to test.

My question regarding kerinfo is still open, maybe somebody can comment on it. The background is that I am considering not storing the MiNT kernel info in HDDRIVER anymore. The XHDI specification says that a driver can use the kernel info pointer in order to call MiNT kernel functions. HDDRIVER does not call any, so there is probably no need to store anything. This also means, of course, that HDDRIVER would not return this information anymore if asked for it.

Before making a decision I would like to gain some insight into how MiNT uses the XHMintInfo() call.

DavidGZ commented 1 month ago

Hi, I did the changes in the kernel to detect the FAT partitions based on the number of cluster. My motivation was this discussion in EmuTOS mailing list [1] [2].

In the discussion is mentioned the Microsoft FAT specification [3] [4]:

To determine the FAT type, the following algorithm is used:
If(CountofClusters < 4085) {
    /* Volume is FAT12 */
} else if(CountofClusters < 65525) {
    /* Volume is FAT16 */
} else {
    /* Volume is FAT32 */
}
The above algorithm determines the FAT type. Note the following:
• A FAT12 volume cannot contain more than 4084 clusters.
• A FAT16 volume cannot contain less than 4085 clusters or more than 65,524 clusters.

This is the one and only way that FAT type is determined. There is no such thing as a FAT12 volume that has more than 4084 clusters. There is no such thing as a FAT16 volume that has less than 4085 clusters or more than 65,524 clusters. There is no such thing as a FAT32 volume that has less than 65,525 clusters. If you try to make a FAT volume that violates this rule, Microsoft operating systems will not handle them correctly because they will think the volume has a different type of FAT than what you think it does.

[1] https://sourceforge.net/p/emutos/mailman/message/36273164/ [2] https://sourceforge.net/p/emutos/mailman/message/36277248/ [3] https://academy.cba.mit.edu/classes/networking_communications/SD/FAT.pdf (page 15) [4] http://msdn.microsoft.com/en-us/windows/hardware/gg463080 (page 15)

So as I understand it there shouldn't be a FAT32 partition with less than 4000 clusters, not even a FAT16.

Are ATARI FAT partitions different? Do they have no number of cluster limits?

th-otto commented 1 month ago

In theory this is correct, and maybe MS products will reject such partitions. But partitions/filesystems are also created by other tools (like HDRUTIL, mkfatfs etc.) which may behave differently.

Also if you look at the code, it seems a bit strange to me that it first tries to determine the FAT type based on the partition type as reported by XHDI, then later completely ignores it and sets it just based on the number of clusters.

uweseimet commented 1 month ago

I have not yet verified whether the small partition I initially used for my tests also works for Microsoft. The partition was only meant to be used with TOS/MiNT. I don't think you should have complicated rules for determining the partition type when there is the explicit "F32" type. If XHDI says a partition is a F32 partition, then it should definitely be treated as FAT32. The same argument applies if the partition has one of the DOS/Windows FAT32 types.

I am going to verify whether a small FAT32 partition works with Windows or not. If it does not I will update HDDRUTIL accordlingly, so that you cannot create small FAT32 partitions anymore, at least not Windows compatible ones. But for the Atari I don't see any technical reason for a FAT32 partition size restriction.

th-otto commented 1 month ago

Before making a decision I would like to gain some insight into how MiNT uses the XHMintInfo() call.

* What does it mean if MiNT reports that kerinfo was rejected, even though unless there is a bug HDDRIVER supports the call and stores/returns the pointer passed. Maybe this is a bug in MiNT?

* What does it it mean for MiNT if these data are not stored by the driver?

* This XHDI 1.30 call is optional. What difference does it make for MiNT if a driver does not support it?

Can you tell what exactly do you mean by kerinfo rejected (i don't see any message like this in the kernel), and which scenario triggers it?

uweseimet commented 1 month ago

There is no special scenario. I see this message each time when I boot MiNT from an unmodified installation with the MiNT snapshot archive. No other software is installed, except HDDRIVER.SYS. If you extract the archive on a clean C: drive and boot you should also see it. It's might be one of the drivers that reports it.

th-otto commented 1 month ago

I haven't tried it, but if i understand the code in dosfstools correctly, it also normally does not allow to create FAT32 partitions with less than 65529 clusters (https://github.com/freemint/dosfstools/blob/17be35754ded27ee938b940cc26bb62c219b1507/src/mkfs.fat.c#L859). But i might be possible if you also override the length of the fat.

uweseimet commented 1 month ago

Assuming that DOS/Windows does not support small FAT32 partitions IMO it should still be possible to use them with the Atari, e.g. with a purely TOS compatible medium where DOS compatibility does not matter. Correct me if I am wrong: 65529 clusters with FAT3 2 means a 128 MiB partition. This is not really small, at least from the Atari perspective. Forcing a user to create partitions of at least that size just in order to have FAT32, which is often also a synonym for long filenames, should be avoided if there is no technical reason for it.

IMO the most user-friendly behavior is to detect FAT32 partitions from the XHDI partition type only, and also enable long filenames by default for these partitions, i.e. without any configuration seting in a configuration file.

th-otto commented 1 month ago

It's might be one of the drivers that reports it.

I think it is from https://github.com/freemint/freemint/blob/85b2534f0bb6c96c00cfc4c4dcfe8861ba4b419d/sys/xhdi.c#L147-L149. However, the macro NONBLOCKING_DMA is never defined, so the call to XHMiNTInfo() was not made, and the message will always be rejected. Seems to be harmless.

uweseimet commented 1 month ago

That's interesting, because I think I know the background of this code. Frank once tried to add DMA background transfers to MiNT, just like they are supported by MagiC. This never worked, and Frank could not figure out where the problem was. But the code you found is very likely still related to that. The message may be harmless, but it is misleading, because it indicates something is wrong. I am very sure that my guess about this code is correct, and I suggest to clean this up, because MiNT does not support background transfers.

uweseimet commented 1 month ago

I just tested how Windows 7 reacts on a Windows compatible 15 MB FAT32 partition created with HDDRUTIL (with MBR and also with GPT scheme): Windows 7 accepts it as a FAT32 partition and uses long filenames with it, see screenshot. Screenshot_20240803_121607 At least in practice this small FAT32 partition appears to be a legal FAT32 partition for Windows. One more argument that MiNT should derive the partition type from the XHDI partition ID only.

This might mean that also the behavior of EmuTOS might need verification.

Linux also detects a FAT32 partition and uses long filenames with it.

th-otto commented 1 month ago

The message may be harmless, but it is misleading, because it indicates something is wrong. I am very sure that my

I've changed that now to print "kerinfo not used" instead, hope that will be ok, but the message will still appear.

BTW, with the bootable aranym setup i never saw this. I think this is because the message is printed before hostfs is initialized, and the boot.log was not created yet.

DavidGZ commented 1 month ago

I don't have a strong opinion regarding the FAT32 limits, if you think the right thing to do is to revert this commit [1], It's OK for me.

Here [2] there is a further discussion about this issue in the EmuTOS mailing list. One of the emails has HTML tags and it's difficult to read, I have pasted it below in text format.

[1] https://github.com/freemint/freemint/commit/e216b1d6b6311ee78fe9d9f259e1192b18c9a87b [2] https://sourceforge.net/p/emutos/mailman/emutos-devel/thread/ca2e58349ece75403a3b79dae8353698%40quante-web.de/#msg36354395

Hi,

only thing is: It is wrong to test for that "FAT16" string. We already had this in March. The official FAT specification by Microsoft says (highlighting by me): "FAT Type Determination. There is considerable confusion over exactly 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. [...] This is the one and only way that FAT type is determined. There is no such thing as a FAT12 volume that has more than 4084 clusters. There is no such thing as a FAT16 volume that has less than 4085 clusters or more than 65,524 clusters. [...] If you try to make a FAT volume that violates this rule, Microsoft operating systems will not handle them correctly because they will think the volume has a different type of FAT than what you think it does."

About the string that was previously checked by EmuTOS, it says: "Many people think that the string in this field has something to do with the determination of what type of FAT — FAT12, FAT16, or FAT32 — that the volume has. This is not true. [...] This string is informational only [...]".

Also see that according to https://wiki.osdev.org/FAT mkdosfs can be tricked into generating invalid FAT filesystems. ("Several developers also make the error of passing -F to mkdosfs in an attempt to choose a FAT size, which often has the effect of creating a corrupt filesystem [...]").

While it is unfortunate that EmuTOS in the past was more lenient, it is correct how it's done now. I don't think we should support invalid filesystems that would also be rejected by Microsoft OSs and iirc also by Linux.

Regards Christian

uweseimet commented 1 month ago

@DavidGZ Regarding FAT32 don't the facts speak for themselves? My screenshot and my tests show that Windows 7 at least (and also Linux) support small (15 MiB) FAT32 partitions, whereas MiNT does not. These facts contradict the statement about the clusters you quoted.

There is one more thing that is relevant in this context: With a GUID Partition Table (I tested both MBR and GPT with Windows) there is no explicit FAT32 type. While Windows can deduce the FAT32 type from the MBR data it cannot do that from the GPT data. This means that at least with a GPT scheme Windows determines that a partition is a FAT32 partition from the bootsector content. But let me emphasize once again: For TOS and XHDI the XHDI type is relevant, both for an MBR and a GPT scheme. XHDI already does the required partition type evaluation.

uweseimet commented 1 month ago

@th-otto The message may be less missleading now. But I wonder what is the purpose, the benefit for the user, of such a message? I don't see any, do you? What about removing it, there are already more than enough messages on the screen while booting MiNT. This also shows once more that an emulation is never a full replacement of real hardware ;-).

Furthermore I would have thought this to be a good opportunity to potentially clean up anything related to the NONBLOCKING_DMA macro you mentioned. But maybe you already did that. If not, consider that it is sad, but I am the only one who can confirm that the experimental MiNT background DMA code never worked, and that it was never tried with a different driver than HDDRIVER. There has never been a HDDRIVER release with MiNT background DMA support, and without hard disk driver support it can never work. (MagiC background DMA also requires the driver to cooperate.) All in all, if there is still code in MiNT (or a macro) left, it's probably the perfect time to clean this up.

th-otto commented 1 month ago

The message also prints the XHDI version found, that may actually be useful as an information that the driver was found at all, but i don't mind removing it.

As for the non-blocking dma: all the does at this point is to call XHMiNTInfo (XH_MI_SETKERINFO, &kernelinfo), which just tells the driver about the kerinfo struct https://github.com/freemint/freemint/blob/aeb2bb768ba20c8b8ead2c670fd0724e29c1683b/sys/mint/kerinfo.h#L57 .

If the driver does not make use of it, it won't even make a difference to leave that call in.

uweseimet commented 1 month ago

Fine for me. I won't spend more time on this ticket or on MiNT. The tenor is too much like this for my taste:

th-otto commented 1 month ago
* Files are not correctly unpacked from the archive: No big deal, those files are not important anyway

I didn't say they are not important. It just takes time to figure out where else they are reference in order not to break other things.

FAT32 does not work as expected and is not compatible with Windows: ext2 is preferable to FAT32 anyway

Did you try the fix i did?

* There a strange messages and potentially unused code: Just change the message, useless code is fine

That's a bit harsh, isn't it? I just said above that i'm fine to remove that message if it bothers you.

uweseimet commented 1 month ago

Maybe my reaction is too harsh, but what takes me aback is that the first reaction on a (major or minor, justified or not) issue report tends to sound like "this is not really a problem" or "the affected feature is not really important".

That message as such is minor stuff, but from a code maintenance perspective there might be more to it. I am used to clean up code that is not used, and investigating why such code is there at all sometimes leads to useful insights. This is why just changing the text appears strange to me, kind of a minimum effort approach.

Regarding a fix for the FAT32 issue, have I missed the PR? There are PRs in this project, but they are not used for everything? "Add a fix for it now" sounds like a temporary work-around instead of tackling the actual problem. But maybe I am wrong. By the way, the installation archive does not contain mkfs.ext2, does it? It probably should, if users are supposed to use ext2fs in favor of FAT32.

All in all, the handling of this ticket is just not what I am used to from other (open source) projects. In order to test something I installed MiNT the first time after maybe 20 years. Initially it was a positive surprise to have an installation archive (but there are no official releases anymore, just snapshots, why?). The rest of the experience was rather sobering ... Fortunately for my Atari projects I do not need a MiNT setup, and this is why instead of spending more time on this I prefer to bail out.

th-otto commented 1 month ago

I am used to clean up code that is not used, and investigating why such code is there at all sometimes leads to useful insights.

I do that too in my own projects, but don't forget that some of the code here is really old, and often done by people which are no longer involved. It's therefor not always easy to find out why/when a change was made, and if i don't find the reason for it, i have to assume that there was one, and therefor leave the code alone. And yes, this has the consequence that there are lots of places with old/rotten code.

Regarding a fix for the FAT32 issue, have I missed the PR?

It was done by https://github.com/freemint/freemint/commit/534404e62130147403b93e8851ca540f08d82873. Typically for smaller fixes we don't use separate PRs, because they don't provide downloadable snapshots that can be tested (well not really true anymore, the archives are available in the actions tab, as assets, but that might not be that obvious).

there are no official releases anymore, just snapshots, why?)

Maybe just because no one decided when it is time to do a new release. Apart from that, the only difference would be a new version number. That may even annoy people who have already customized their setups, because that string (1-19-cur) was hardcoded in mint.cnf (and maybe other configuration files) in a lot of places.

czietz commented 1 month ago

This might mean that also the behavior of EmuTOS might need verification.

I continue to stand behind EmuTOS's detection method as it was implemented. It was developed fully compliant to the FAT specification, which unequivocally states: "There is no such thing as a FAT32 volume that has less than 65,525 clusters." (Emphasis mine.) Therefore, tools that create FAT32 volumes with fewer clusters are disregarding the specification.

However, this is a good case for the robustness principle ("be liberal in what you accept"). I have confirmed in Microsoft's source code [1] that Windows uses the 'sectors per FAT' boot sector entry to differentiate between FAT12/16 and FAT32. Hence, I have just pushed corresponding check to EmuTOS [2] as an additional safeguard.

[1] https://github.com/microsoft/Windows-driver-samples/blob/b3af8c8f9bd508f54075da2f2516b31d05cd52c8/filesys/fastfat/fat.h#L101 [2] https://github.com/emutos/emutos/commit/aca309207086d913a9246af061023c98b24fafd5

uweseimet commented 1 month ago

I do that too in my own projects, but don't forget that some of the code here is really old, and often done by people which are no longer involved. It's therefor not always easy to find out why/when a change was made, and if i don't find the reason for it, i have to assume that there was one, and therefor leave the code alone. And yes, this has the consequence that there are lots of places with old/rotten code.

This is why I pointed out what the history of this DMA-related code was, so that removing it could be considered.

Maybe just because no one decided when it is time to do a new release. Apart from that, the only difference would be a new version number. That may even annoy people who have already customized their setups, because that string (1-19-cur) was hardcoded in mint.cnf (and maybe other configuration files) in a lot of places.

IMO MiNT (like any other project) would profit from resolving such issues and providing releases. A release usually implies a testing phase, and maybe also is an incentive to update the documentation. With these respects the nonbinding nature of snapshots is rather counterproductive. Consider what happened in my case: In just one hour or so of trying to use MiNT I stumbled upon several issues. Not all of them major, but I wonder anyway: What else would have been revealed after using MiNT more?

uweseimet commented 1 month ago

However, this is a good case for the robustness principle ("be liberal in what you accept"). I have confirmed in Microsoft's source code [1] that Windows uses the 'sectors per FAT' boot sector entry to differentiate between FAT12/16 and FAT32. Hence, I have just pushed corresponding check to EmuTOS [2] as an additional safeguard.

What you found in the source code confirms my view that when there is no other information available than the boot sector, using the "sectors per FAT" fields in order to determine the FAT type is the best approach. With a GPT scheme there is no other source of information on the FAT format anyway.

When explicit FAT type information is available, in particular the XHDI partition type, this information IMO has priority. Usually looking at the explicit type and the FAT size fields will yield the same result. But what do you do if there is a mismatch, e.g. a BGM partition where the boot sector implies that it is FAT32? Maybe it is best not to assign a drive ID to such a partition but issue a warning instead? Depending on the nature of the mismatch one might lose data when writing to it.

th-otto commented 1 month ago

Depending on the nature of the mismatch one might lose data when writing to it.

Correct me if i'm wrong, but wouldn't it be even more dangerous to solely rely on the XHDI partition type in this case? If that is BGM, it would then be treated as either FAT12 or FAT16. However if the sectors per FAT in the partition is zero, it would write both the FATs and the root directory to the same location.

DavidGZ commented 1 month ago

IMO MiNT (like any other project) would profit from resolving such issues and providing releases. A release usually implies a testing phase, and maybe also is an incentive to update the documentation. With these respects the nonbinding nature of snapshots is rather counterproductive. Consider what happened in my case: In just one hour or so of trying to use MiNT I stumbled upon several issues. Not all of them major, but I wonder anyway: What else would have been revealed after using MiNT more?

@uweseimet don't forget that MiNT is now a marginal open project, it doesn't even have an official maintainer. it's alive thanks on the altruistic work of very few people, mainly @th-otto and @mikrosk. We all agree that releases would be great but for this to happen someone has to jump in and take responsibility, MiNT didn't have the luck that EmuTOS had finding this kind of persons. For the time being automated snapshots is the best compromise for this situation.

uweseimet commented 1 month ago

I am not sure whether there is a definite answer to this question. You can have a mismatch only with an MBR scheme, which is of course what you usually have with the Atari. (But there are reasons to prefer a GPT.)

But I agree, when looking at all we know now, (exclusively) using the XHDI type is not safe. From that perspective, one may wonder whether the F32 type is needed at all. Maybe not, but having it (as long as it is consistent) is still helpful, because it makes the filesystem format more explicit.

If you only rely on the bootsector (not on the partition type), you may be forced to read and analyze data you otherwise would not need. This can be inconvenient, especially for tools that know about partition types. While adding GPT to support to HDDRUTIL and extending it in HDDRIVER I stumbled upon this more than once.

So always looking at the boot sector may indeed by the only correct way, making the partition type essentially meaningless. That's the situation you have with a GPT anyway, because there is no distinct FAT32 GUID for Windows, and also not for TOS.

From my perspective this is a good time for investigating all this. It may slightly change how my software deals with partition types, at least for the regular MBR case. For the GPT case, in order to make the XHDI type for FAT32 partitions reliable (and available at all), HDDRIVER examines the boot sector of a partition defined in a GPT and derives the XHDI type from the FAT size fields, which is according to Christian how Microsoft does it. Thus for a GPT the XHDI type always matches the boot sector information.

For the MBR case HDDRIVER does not evaluate the boot sector in order to determine the partition type. Maybe it should do so, even though this may need the additional overhead of reading the boot sector in a context where it was not read before in the MBR case. In case of a mismatch I see these options:

Any opinions on that?

Initially, when I learned that for a GPT there is no distinct FAT32 GUID I was surprised and did not like the idea of having to look at the boot sector for more information. But as a result of this discussion I wonder whether this is rather an advantage instead of a disadvantage about how Microsoft defined their GUIDs.

In practice this is an exotic case, but nevertheless having a solution would be nice.

DavidGZ commented 1 month ago

When explicit FAT type information is available, in particular the XHDI partition type, this information IMO has priority. Usually looking at the explicit type and the FAT size fields will yield the same result. But what do you do if there is a mismatch, e.g. a BGM partition where the boot sector implies that it is FAT32? Maybe it is best not to assign a drive ID to such a partition but issue a warning instead? Depending on the nature of the mismatch one might lose data when writing to it.

Yesterday I was testing creating small FAT32 partitions and I saw that the partition tool in my Linux distribution (gnome-disk-utility 3.38.2) creates a partition with a FAT32 ID but with FAT16 or FAT12 tables. So this scenario could in fact happen.

f32 limits 1 f32 limits 2

uweseimet commented 1 month ago

@DavidGZ Looks as we are not the only ones struggling with this potential type mismatch ;-).

uweseimet commented 1 month ago

@uweseimet don't forget that MiNT is now a marginal open project, it doesn't even have an official maintainer. it's alive thanks on the altruistic work of very few people, mainly @th-otto and @mikrosk. We all agree that releases would be great but for this to happen someone has to jump in and take responsibility, MiNT didn't have the luck that EmuTOS had finding this kind of persons. For the time being automated snapshots is the best compromise for this situation.

I already guessed that there might be a manpower problem limiting your choices. The github page says that Alan is the kernel coordinator. You sound as if this not the case anymore? Maybe I just misunderstand the meaning of "kernel coordinator", but it sounds like this is the person with a suitable kind of responsibility.

uweseimet commented 1 month ago

@DavidGZ I have one more question, maybe you can test this: What does the tool display if you create a small purely Windows (i.e. not TOS/Windows) compatible FAT32 partition with HDDRUTIL? In this case both the partition type and the boot sector say that this is a FAT32 partition. I wonder what the tool says. Note that with such a small partition you have to force a FAT32 partition to be created by entering the respective DOS type in this case, i.e. "$0c", as the partition ID

I recommend using a current HDDRUTIL for that. Looks as if you have not downloaded a recent release yet. In case you are willing to make this test but lost your license number please drop me an email.

th-otto commented 1 month ago
* Do not assign a drive ID to this partition (and maybe display an error message)

* Internally override the XHDI type and set it to F32 (or $0c for DOS), so that the XHDI type is consistent (and maybe display an error message)

Any opinions on that?

I think overriding the partition type cannot be done by MiNT. Maybe internally, but actually that is just info as returned by XHInqDev2, and therefor the XHDI driver (HDDRIVER, emutos, or aranam) would be responsible for that.

Not assigning a drive ID in the case of mismatch would be the safest, but would force users to run their partition tool to fix the type, otherwise they are not able to access the partition at all. Given that such error messages typically scroll by very fast, and do not always appear in some boot.log, it may be very confusing.

Given that the current code seems to work (except for small FAT32 partitions, which is hopefully now fixed), i would suggest to just issue a warning, but continue to use the partition based on boot sector contents.

uweseimet commented 1 month ago

I think overriding the partition type cannot be done by MiNT. Maybe internally, but actually that is just info as returned by XHInqDev2, and therefor the XHDI driver (HDDRIVER, emutos, or aranam) would be responsible for that.

That's what I meant, i.e. my options were meant to be options for HDDRIVER, or an XHDI compatible driver in general.

Not assigning a drive ID in the case of mismatch would be the safest, but would force users to run their partition tool to fix the type, otherwise they are not able to access the partition at all. Given that such error messages typically scroll by very fast, and do not always appear in some boot.log, it may be very confusing.

Given that the current code seems to work (except for small FAT32 partitions, which is hopefully now fixed), i would suggest to just issue a warning, but continue to use the partition based on boot sector contents.

Yes, that would probably be best. I was not explicitly thinking of MiNT (before the patch), but the mismatch may also happen in other scenarious. Even on other platforms, as we can see from David's example.

DavidGZ commented 1 month ago

@DavidGZ I have one more question, maybe you can test this: What does the tool display if you create a small purely Windows (i.e. not TOS/Windows) compatible FAT32 partition with HDDRUTIL? In this case both the partition type and the boot sector say that this is a FAT32 partition. I wonder what the tool says. Note that with such a small partition you have to force a FAT32 partition to be created by entering the respective DOS type in this case, i.e. "$0c", as the partition ID

@uweseimet here are the screenshots for the test you asked for.

fat32 limit 3 FAT32 limits 4

uweseimet commented 1 month ago

After poking around a bit and checking the available options, overriding the partition type may not resolve the mismatch problem.

For an MBR, if the partition type is BGM and the FAT16 size in the boot sector is 0, the BPB is invalid, regardless of the FAT32 size. This is how it has always worked. The drive is not accessible. Internally changing the type from BGM to F32 when FAT32 size is != 0 in the hard disk driver is not correct. The driver is required to know by the boot sector content whether a partition is a valid FAT16 partition, but not whether it is a valid FAT32 partition. A valid FAT32 partition requires more than just looking at the FAT32 size. The driver might check all that, i.e. it would have to be FAT32 aware to a certain extent, but would this really make sense? The result is still an invalid BPB (but the partition type would be correct), and the FAT32 filesystem layer needs to check all this (again) anyway.

If the partition type is F32 and the FAT16 field is not 0, or the FAT32 field is 0, the driver might still replace the F32 partition type by BGM. This would have to be done regardless of whether the BPB is valid or not, because whether it is valid for a set of boot sector data also depends on the TOS version. Coupling this internal type change to the BPB being valid or not would result in different results on different TOS versions. Not very nice.

In the GPT case, when there cannot be this kind of mismatch, but without some FAT32 knowledge, the driver can only derive the type from the FAT16 and FAT32 fields. For the "Microsoft basic data" and the "Atari TOS basic data" GUIDs there are only two possible types, for the Atari either "BGM" or "F32". The type decision may be wrong if the boot sector is corrupted. In such a case it's still clear that the partition cannot be anything else than BGM or F32, because the GPT data are secured by checksums, but the real type cannot be determined. Again the BPB will be invalid, and the FAT32 driver would have to deal with detecting anything wrong with FAT32.

I tend to say that for the case we are looking at TOS suffers from the hard disk driver knowing a bit too much (about FAT16) on the one hand, but not knowing enough (about FAT32) on the other. A dumb driver that just provides access to sectors but nothing more, and the filesystem drivers doing the rest, would likely work better, but this is not how TOS works. A more intelligent driver would know about FAT32, but this would duplicate logic, because the filesystem driver also knows about it. Both drivers might not even agree on what's valid or not.

Is this an adequate view of things? I may well be missing something.

uweseimet commented 1 month ago

@uweseimet here are the screenshots for the test you asked for.

Cool, thank you. So as long as the partition type and the boot sector data are consistent and say that the partition is FAT32, the tool is fine with it, isn't it?

DavidGZ commented 1 month ago

I already guessed that there might be a manpower problem limiting your choices. The github page says that Alan is the kernel coordinator. You sound as if this not the case anymore? Maybe I just misunderstand the meaning of "kernel coordinator", but it sounds like this is the person with a suitable kind of responsibility.

Yes, Alan was the latest active maintainer, but he doesn't practice as such since years. That info in github should be updated.

DavidGZ commented 1 month ago

Cool, thank you. So as long as the partition type and the boot sector data are consistent and say that the partition is FAT32, the tool is fine with it, isn't it?

Yes, it seems so, also I could open the partition and transfer some data with no issues.

uweseimet commented 1 month ago

Yes, Alan was the latest active maintainer, but he doesn't practice as such since years. That info in github should be updated.

Maybe updating key parts of the github pages/documentation would be good. And mentioning that due to limited manpower not everything that is desirable can be done would help managing expectations.

czietz commented 1 month ago

And mentioning that due to limited manpower not everything that is desirable can be done would help managing expectations.

Sorry for veering off-topic, but imho this is sadly the norm and not the exception. Many open-source projects (also much bigger and important ones) are understaffed and overworked.

It is for a reason that this xkcd comic is four years old: xkcd 2347: Dependency ... and it was somewhat proven true by the xz thing a few months ago.

Thus, even without such the suggested sentence in the documentation, it is safer to assume for any given open-source project that "not everything that is desirable can be done". 🙁