dosemu2 / fdpp

FreeDOS plus-plus, 64bit DOS
GNU General Public License v3.0
198 stars 17 forks source link

fatdir: clean up swap_deleted() logic [#202] #208

Closed stsp closed 1 year ago

stsp commented 1 year ago

Use '\x1' for deleted indicator to make logic less confusing.

stsp commented 1 year ago

@bartoldeman @ecm-pushbx @andrewbird could you please review this patch to make sure I've got your explanations correctly?

ecm-pushbx commented 1 year ago

I don't understand the purpose of this. What if a name starts with U+0001? If that can't happen then U+0005 also cannot happen. The prior logic seems much simpler and robust, though I have not fully studied it / its use.

stsp commented 1 year ago

Yes, I assume both 1 and 5 are reserved so neither can happen in an actual name. And we should error out if its there cause you are not supposed to create a deleted entry.

stsp commented 1 year ago

If this "should error out" thing is wrong and we can actually pass 05 in a name (and create a deleted entry??) then this patch makes no sense and I'll just close it.

stsp commented 1 year ago

Another attempt here. Instead of doing weird stuff with mangling name, just allocate a flag for deleted indicator. Does this look good?

andrewbird commented 1 year ago

I'm not sure really, but I figure it'd likely need testing. Perhaps @lpproj might have ideas on how to create a FAT image with a character set encoding that will need to use the 0xe5 character?

stsp commented 1 year ago

Set code page 866 and create a file with name хер. E5 is х in cp866.

stsp commented 1 year ago

Yet another variant. If I am to add the flags, then I can use 2 of them and swap between them as was done with the name letter before.

Btw. https://en.wikipedia.org/wiki/Design_of_the_FAT_file_system#Directory_entry

0x0D | 1 | First character of a deleted file under Novell DOS, OpenDOS and DR-DOS 7.02 and higher.

What if we do it a DR-DOSish way and just store the first char in dentry? (in a separate PR of course) Looks much, much cleaner, and allows for a cheap undeletion. Thoughts?

stsp commented 1 year ago

In fact I am so fond of this DR-DOSish logic, that here it is, in this PR. :)

stsp commented 1 year ago

Hmm, build fails, looks like some networking problems on CI?

stsp commented 1 year ago

Build ok.

stsp commented 1 year ago

Andrew, I think besides the swap logic, it would be good to also test that w/o the patch it was probably possible to create a deleted entry by starting the filename with '\x5'. The patch should forbid that.

Note that w/o the clean-up patch, the DR-DOSish logic is impossible because the code "corrupts" the first char before you can back it up. This makes me much more confident that my rewrite is good and needed. It uses flags instead of name mangling, which allows to back up the deleted char at any point.

stsp commented 1 year ago

Andrew, can we have a dosemu2 test run upon the fdpp CI? Otherwise fdpp is only build-tested.

andrewbird commented 1 year ago

I'll have a look.