fix region file locking

[stsp]

http://www.burtonsys.com/download/TESTLOCK.ZIP Fails region file locking test.

[stsp]

Doesn't work in 1.1 again. :(

[stsp]

Test 3 failed. Test 8 failed. Critical error handler crashes.

[andrewbird]

I'm now seeing a crash with FDPP test 'test_fat_ds3_lock_readlckd' and others, but that's a small simple test example.

ERROR: bad dos helper function: AX=0x0e83
ERROR: general protection at 0x7fa2b670f200: 0

But that error seems to be random, sometimes I see the 'bad dos helper', other times not. I always see the general protection error but its address varies.

[stsp]

Can you think of a minimal reproducer? I tried to sniff the args from your tests, and do:

dosemu.bin -f test-imagedir/dosemu.conf -n -o test_dos.PPDOSGITTestCase.test_fat_ds3_lock_readlckd.log -td --Fimagedir test-imagedir -E testit.bat

but no crash that way.

[stsp]

I also simplified testit.bat:

rm foo.dat
lckreadl primary
rem end

[andrewbird]

latest Doemu2 devel branch with latest fdpp master branch

Here's the simplified command line

bin/dosemu \                                                      
        -n -f test-imagedir/dosemu.conf -o test.log \             
        --Fimagedir `pwd`/test-imagedir

and the directory contents t1.zip

[stsp]

Interestingly, the change I did to testit.bat is what responsible to no crash.

[andrewbird]

I wondered if yours was okay because of -E?

[andrewbird]

So it tests the current drive. For fat test that's d:, so if -E tested on temporary drive that'll be Mfs, I think.

[stsp]

Yes, your thoughts seem correct. Should now be fixed. I forgot that fdpp relies on BP sometimes...

[andrewbird]

That looks better. Just retesting all FDPP now.

[andrewbird]

So that helped a lot, thanks. There are some tests on MFS that give errors as we might expect due to the recent attr changes. There are a couple of MFS test failures that surprise me

test_mfs_fcb_rename_simple (and other renames)

test_mfs_ds2_file_seek_2_position_back

This last one just seems to be very very slow

[andrewbird]

Both problems are the same on MS-DOS too, so doubtful that they they are FDPP problems. Will raise an issue there.

[stsp]

I fixed the problem with test 3. It appears SHARING_VIOLATION must be returned instead of RBIL-specified ACCESS_DENIED. What do you think? An RBIL bug?

[stsp]

test 8 was broken because share didn't check an fd. There was a PSP check which I removed, but an fd check had to be added instead. I am surprised it passed our tests with a bug like that. Would be good if you add tests that make sure that conflicting locks on same fd do not conflict, or maybe they do? I would be surprised if DOS can do all those advanced tricks with lock region coalescing, so I suspect my current fix is wrong again. Would be good if you add the test that makes sure that the locked region can still be accessed (read/written) from the same fd where the lock was applied.

Lets re-open until these are addressed.

[andrewbird]

So here's the first test that shows current locking behaviour with single process and DOS fd. https://github.com/andrewbird/dosemu2/commit/782ab6dbe50a47b17613b13a861477d4dff88d6e. It's consistent with there being no lock coalescing, else I'd expect the partial and wholly contained second lock attempts to succeed. I also see the same behaviour across all MFS and FAT with DR-DOS 7.01, MS-DOS 6.22, FreeDOS 1.20 and FDPP.

I guess you want a second test to see if we can read data from those same locked ranges?

[stsp]

also see the same behaviour across all MFS and FAT with DR-DOS 7.01, MS-DOS 6.22, FreeDOS 1.20 and FDPP.

Makes no sense to me. On fdpp+fat I currently only check other FDs, so I suppose everything on a single FD should be allowed. On mfs we have a linux semantic with coalescing (which I think it good to keep as an extension, but you say its not there).

I guess you want a second test to see if we can read data from those same locked ranges?

Yes. The bug was that the locked range was "locked" even for the current FD, and none of our tests failed (but testlock.exe did).

[andrewbird]

On mfs we have a linux semantic with coalescing (which I think it good to keep as an extension, but you say its not there).

Yes, it's not what I was expecting either.

Yes. The bug was that the locked range was "locked" even for the current FD, and none of our tests failed (but testlock.exe did).

That's really strange as we were certainly able to read from a different Dos process and FD. Remember discussion about partial reads. https://github.com/dosemu2/dosemu2/issues/270#issuecomment-729152450

[andrewbird]

So with latest test in travis-ci-42 I guess I'm seeing the failure you were expecting in FDPP

======================================================================
FAIL: Test PP-DOS-GIT FAT DOSv3 lock read one handle                                  
----------------------------------------------------------------------
Traceback (most recent call last):
  File "./test/test_dos.py", line 5153, in test_fat_ds3_lock_one_handle
    ds3_lock_one_handle(self, "FAT")
  File "/clients/common/dosemu2.git/test/func_ds3_lock_one_handle.py", line 111, in ds3_lock_one_handle
    self.assertNotIn("FAIL:", results)
AssertionError: 'FAIL:' unexpectedly found in "testit.bat\r\r\n\r\r\nC:\\>d:\r\r\nD:\\>rem Internal share\r\r\nD:\\>c:\\lckreado\r\r\nOKAY: Acquired lock on file 'FOO.DAT'\r\nFAIL: Unable to read locked data from file, err=5 cnt=0\r\n\r\r\nD:\\>"

I find it strange that I don't see this in the test with one process but two handles. I'll recheck!

[stsp]

Unable to read locked data from file

Really? But I thought this is fixed. That test was supposed to pass... What I expected is locks on same fd to never conflict.

[stsp]

That's really strange as we were certainly able to read from a different Dos process and FD. Remember discussion about partial reads. dosemu2/dosemu2#270 (comment)

The discussion says quite the opposite:

when tested properly shows that reading the locked range from the second handle is also denied!

[stsp]

i tried your tests, and they behave exactly as expected.

Test PP-DOS-GIT FAT DOSv3 lock read one handle                                   ... ok (  1.52s)
Test PP-DOS-GIT MFS DOSv3 lock read one handle                                   ... ok (  1.78s)
======================================================================
FAIL: Test PP-DOS-GIT FAT DOSv3 lock file conflict                                    
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/stas/src/dosemu2-devel/test/test_dos.py", line 5145, in test_fat_ds3_lock_conflict
    ds3_lock_conflict(self, "FAT")
  File "/home/stas/src/dosemu2-devel/test/func_ds3_lock_conflict.py", line 132, in ds3_lock_conflict
    self.assertNotIn("FAIL:", results)
AssertionError: 'FAIL:' unexpectedly found in "testit.bat\r\r\n\r\r\nC:\\>d:\r\r\nD:\\>rem Internal share\r\r\nD:\\>c:\\lckcnflt\r\r\nINFO: Created 'FOO.DAT'\r\nINFO: Wrote test data to file\r\nINFO: 10,5 First lock acquired on file\r\nINFO: 0,5 Second non conflicting lock acquired and released on file\r\nFAIL: 8,5 Got second partially overlapping lock on file\r\nINFO: 10,5 First lock released on file\r\nINFO: File closed\r\n\r\r\nD:\\>"

[stsp]

Bad test:

diff --git a/test/test_dos.py b/test/test_dos.py
index b6061426a..db959f41c 100755
--- a/test/test_dos.py
+++ b/test/test_dos.py
@@ -5138,7 +5138,7 @@ $_floppy_a = ""

     def test_mfs_ds3_lock_conflict(self):
         """MFS DOSv3 lock file conflict"""
-        ds3_lock_readlckd(self, "MFS")
+        ds3_lock_conflict(self, "MFS")

     def test_fat_ds3_lock_conflict(self):
         """FAT DOSv3 lock file conflict"""

[stsp]

Now: Test PP-DOS-GIT FAT DOSv3 lock file conflict ... ok ( 1.55s)

[andrewbird]

So when you fix my mistake with the Mfs test do you see an error now?

[stsp]

Yes, 2 tests were failing. Now I fixed share, and only mfs does.

[stsp]

But mfs is not a problem, its an extension. It can coalesce the locks.

[andrewbird]

Tomorrow I'll fix the test you've shown and see how I can handle the Mfs extension.

[stsp]

Thanks. The extension can be handled simply: if you managed to apply containing or overlapping locks, then you can try to unlock some bytes in an overlapping region, and see if you can read from those but not others. If so - extension. Say, Lock 10,20 Lock 13,5 (containing) Success? Then perhaps extension, try to unlock 14,2. Success? Very likely an extension. Try to read 14,5. Returned 2? Then definitely an extension. Returned LOCK_VIOLATION? Try to read 14,2. Success? Still extension (but the one without partial reads).

[stsp]

region, and see if you can read from those

... on another fd... Which makes this test alone quite useless. Its better to just enrich the 2-fd test: try to lock something overlapped, see if the overlap region cannot be read on another fd and can - on same fd. Which means, separate same-fd test also have quite a small value. So looks like its better to add things to 2fd test, and not add anything separate.

[stsp]

So would you like to add the same-fd read test either as a new test or just a few lines to the one of an existing locking tests?

[andrewbird]

Sorry I've not been around for the last few days. I've had, and still have, lots of household jobs to catch up on. Will be back fairly soon I hope!