share: Restore MS-DOS 6.22 semantics

[andrewbird]

Removed out of date commit message

---

NOTE can you check

• That it's valid to call an int21 from within the int2f code?
• That it's correct to convert a char * to char FAR * with MK_FAR_SCP?

[stsp]

That it's valid to call an int21 from within the int2f code?

AFAIK yes, only int21 is not re-entrant sometimes (but in fdpp it should). But please add a comments, what DOS func do you call and what are those magic values, so that I can avoid RBILing. :)

That it's correct to convert a char to char FAR with MK_FAR_SCP?

No, that's discouraged, and I think the change is outright wrong, i.e. isn't fptr->filename already FAR? If not, why it worked before?

[andrewbird]

what DOS func do you call and what are those magic values

It's to get file attributes in order to determine if it's a read only file, here's the rbil, but yes I'll add comment to the code.

Without the conversion I got

share.cc:404:8: error: no matching function for call to 'file_is_read_only'
                        if (file_is_read_only(fptr->filename))
                            ^~~~~~~~~~~~~~~~~
share.cc:341:12: note: candidate function not viable: no known conversion from 'char [128]' to 'FarPtr<char>' for 1st argument
static int file_is_read_only(__FAR(char)filename) {
           ^
share.cc:413:8: error: no matching function for call to 'file_is_read_only'
                        if (file_is_read_only(fptr->filename))
                            ^~~~~~~~~~~~~~~~~
share.cc:341:12: note: candidate function not viable: no known conversion from 'char [128]' to 'FarPtr<char>' for 1st argument
static int file_is_read_only(__FAR(char)filename) {
           ^
2 errors generated.

[stsp]

Ah I see why it worked before: it was commented out. :) OK, let me see what should be done.

[andrewbird]

Please don't apply yet anyway, as I have to tweak some other stuff first.

[stsp]

I think marking it XFAR will deal with the problem.

[andrewbird]

It compiles cleanly but I now see this

ERROR: redundant conversion for 0x7f6cad7f2216

fdpp crashed.
ERROR: fdpp: abort at ./farptr.hpp:321
Press any key to exit.

[stsp]

Its likely 2 different problems. Try __XFAR(const char) filename, as I suppose w/o const it thinks FP_SEG() is unsafe in that context. I think I put too many sanitizers in there...

[andrewbird]

That fixed the crash, but I still see pauses in execution and these lines being emitted:

INFO: booting with comcom32, this is very experimental
ERROR: redundant conversion for 0x7fc0e9cf6216

ERROR: redundant conversion for 0x7fc0e9cf6216

ERROR: redundant conversion for 0x7fc0e9cf6216

ERROR: redundant conversion for 0x7fc0e9cf6216

Here's the log t1.zip

[andrewbird]

I added a follow up commit that fixes the share behaviour to match that of RBIL / MS-DOS 6.22. Do you prefer I use the defines or numbers in the exception definition, it seems to me that defines are easier to add/debug problems, but numbers are better visually for spotting patterns?

[stsp]

I added a follow up commit that fixes the share behaviour to match that of RBIL / MS-DOS 6.22.

But from your initial log msg I though the first commit does the same. Why is the second one needed?

prefer I use the defines or numbers in the exception definition,

If these defines already exist (as they seem to, as your patch doesn't add them), then of course I see no problem in using them. If defines are missing, I never insist to just define every possible digit.

I'll look into the log.

[andrewbird]

But from your initial log msg I though the first commit does the same.

Yes I fooled myself too! Before I had the readonly function working I tested it with just a fixed return value, somehow I got lucky(?) and it covered up the other issues. The mistake was only uncovered by Travis when I pushed the first commit, hence the hastily added comment above Please don't apply yet anyway, as I have to tweak some other stuff first. .

[stsp]

Maybe in that case it would be a good idea to swap these patches around, and see how just the additions themselves affect the tests? And amend the commit msgs.

In a mean time it seems fdpp just thinks that this pointer was far also before, and our changes are outright wrong... It basically just repeats my first comment in that thread, and I wonder what makes it so clever...

[andrewbird]

With hindsight I should have made the PR a draft whilst I got your feedback on the two questions I had.

[andrewbird]

So with just the exceptions table commit the failures dropped from 10 to 4.

    (t=('SH_COMPAT', 'R', 'SH_DENYWR', 'R', 'DENY'))                             ... FAIL
    (t=('SH_COMPAT', 'R', 'SH_DENYNO', 'R', 'DENY'))                             ... FAIL
    (t=('SH_DENYWR', 'R', 'SH_COMPAT', 'R', 'INT24'))                            ... FAIL
    (t=('SH_DENYNO', 'R', 'SH_COMPAT', 'R', 'INT24'))                            ... FAIL

[stsp]

Yes, so I suggest to just re-push them in that order then, with an updated messages (so its clear why everyone is needed).

In a mean time I've found a fix for ptrs.

[stsp]

I applied the fix for the pointer problem. Now you can just leave that code as it was before. So the checker is correct, as usual...

[andrewbird]

Getting a boot failure now

ERROR: fdpp: abort at ./farptr.hpp:568

Here's the log t2.zip

[andrewbird]

Boot crash occurs even without my commit to re-enable file_is_read_only() function.

[stsp]

OK, what a tricky beast! But I am glad the checker resisted all my attempts to fool it around, so finally I've found a bug. :) Should now be fixed.

[andrewbird]

Thanks, the boot crash is gone, but I still get a crash with my share tests even with master. The problem seems to be in do_open_check(). Here's the log and the test to trigger it t3.zip

[stsp]

Should now be fixed.

[andrewbird]

Cool, master now runs the share tests until completion with no warnings or crash. This branch also runs fine and the tests pass too.

[stsp]

Thanks!