Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CI: Add label create tests #1801

Merged
merged 3 commits into from
Apr 27, 2023
Merged

CI: Add label create tests #1801

merged 3 commits into from
Apr 27, 2023

Conversation

andrewbird
Copy link
Member

@andrewbird andrewbird commented Sep 29, 2022

Note: These tests identify the following problems:

1/ FreeDOS 1.3 on FAT does:
a) With existing file, volume is set but the existing file is deleted
b) With existing directory, the function returns failure.
c) No Volume field is set in the BPB.

2/ MS-DOS 6.22 does:
a) No support for FAT32

3/ DR-DOS 7.01 does:
a) No support for FAT32
b) No Volume field is set in the BPB.

Note: Currently MFS does not allow setting of volume label

@stsp
Copy link
Member

stsp commented Sep 30, 2022

Seems failing?

@andrewbird
Copy link
Member Author

andrewbird commented Sep 30, 2022

Yes there's two things going on here:

  • Something wrong with the current implementation of partition image support in devel and the drive seems partially exist (enough to exist, but gives the following when trying to access)
Write fault error writing drive D^M                                             
Abort, Retry, Fail?                                                             

You can run the simple test locally by doing python test/test_dos.py MSDOS622TestCase.test_fat_label_create_simple

  • Once the first problem is fixed you'll see the second as I'm no longer referencing my test t2 branch of FDPP, so the problems that FreeDOS has with label will be evident here too.

@stsp
Copy link
Member

stsp commented Sep 30, 2022

I won't be able to get to fixing
it withis a few days, so you can
beat me on that. :)

@andrewbird
Copy link
Member Author

Actually it's fine with PR #1798 applied as that's how I tested dosemu2/fdpp#202. I didn't actually narrow down what the problem is here, it just got fixed when I rewrote partition support to use build_pi().

@stsp
Copy link
Member

stsp commented Sep 30, 2022

But will you remove mallocs in
#1798?

@andrewbird
Copy link
Member Author

Yes mallocs removed now.

@stsp stsp closed this Oct 1, 2022
@stsp stsp reopened this Oct 1, 2022
@stsp
Copy link
Member

stsp commented Oct 1, 2022

Still fails.

@andrewbird
Copy link
Member Author

Yep, because of part two of my explanation above. It needs the label fixes to fdpp to be applied (dosemu2/fdpp#202)

@andrewbird andrewbird force-pushed the ci-01 branch 3 times, most recently from e05cecf to 715c1a4 Compare November 6, 2022 12:27
@andrewbird andrewbird force-pushed the ci-01 branch 8 times, most recently from 9ea764f to 698ff56 Compare November 16, 2022 00:44
@andrewbird andrewbird force-pushed the ci-01 branch 2 times, most recently from 6ce3be8 to 97035bf Compare April 20, 2023 08:39
Note: These tests identify the following problems:
1/ FreeDOS 1.3 does:
   a) With existing file, volume is set but the existing file is deleted
   b) With existing directory, the function returns failure.
   c) No Volume field is set in the BPB.
   d) More than one file with the volume bit set can be created.

2/ MS-DOS 6.22 does:
   a) No support for FAT32

3/ DR-DOS 7.01 does:
   a) No support for FAT32
   b) No Volume field is set in the BPB.

Note: Currently MFS does not allow setting of volume label
Since LFNs are stored in FAT using the VOLID bit (amongst others), we
need to check that:
  when creating a label that we don't error on their existence.
  when deleting a label that we don't destroy or corrupt them.

Note:
   MS-DOS 6.22 and DR-DOS 7.01 fail to create a label if there are LFNs
existing, which is presumably due to their lack of LFN support.
@andrewbird
Copy link
Member Author

I think this is good to add now that FDPP has functioning label code. I also tested on FRDOS 1.3, DRDOS 7.01 and MSDOS 622 to apply the known failures / unsupported actions where necessary.

@stsp stsp merged commit 81bf77c into dosemu2:devel Apr 27, 2023
@stsp
Copy link
Member

stsp commented Apr 27, 2023

Thanks!

@andrewbird andrewbird deleted the ci-01 branch April 27, 2023 15:12
@andrewbird
Copy link
Member Author

I'm just extending the label create test to check if the set label is propagated to int21/69 (get serial number). It seems that FreeDOS/FDPP implement getting the label via this service, but ignores label on the corresponding set. See https://fd.lod.bz/rbil/interrup/dos_kernel/2169.html#3212 . I think this is probably fine, otherwise to synchronise the two labels stored we'd have to delete / create a new volume name file in fatfs too. Do you agree?

@stsp
Copy link
Member

stsp commented Oct 12, 2024

I think volume dir entry should never
be read or written by DOS. Only the
one in BPB. So I don't see the reason
why we should sync, or even think
about the root dentry label.

@andrewbird
Copy link
Member Author

I suppose for completeness I need to check if MS-DOS does update the root dentry label if an application calls int21/69 to change the volume label.

On another note, I found a bug with label setting via FCB. It seems that the dos_open() function is passed a pathname including the dot, but the writelabelBPB() function I wrote was expecting the volume name in FCB format. A patch to use ConvertNameSZToName83() is on the way.

@stsp
Copy link
Member

stsp commented Oct 14, 2024

Could you please look into this build?
https://github.com/dosemu2/dosemu2/actions/runs/11318596166/job/31473391666
Besides the (usual) ASAN failure, it also
has the failure uploading logs because
of the name collision.

@stsp
Copy link
Member

stsp commented Oct 14, 2024

Also there is a crash on Focal...
Does anyone have Focal here?
Is the crash reproducible?

@andrewbird
Copy link
Member Author

Log zip file name seems to be per workflow, not per job. I'll see if there's some job specific variable I can suffix it with.

I have to go out now for the day, back later.

@stsp
Copy link
Member

stsp commented Oct 14, 2024

I wonder why other packaged
tests passed, only focal failed.
I seem to have unbootable situation
with any packaged version, as usual. :(

@stsp
Copy link
Member

stsp commented Oct 14, 2024

It seems, most packaged tests failed
because tap is inaccessible.
Could you please make it so that for
the packaged tests, tap is set for
user dosemu2 rather than the current
user?

@stsp
Copy link
Member

stsp commented Oct 14, 2024

Also if you have Focal around somewhere,
it would be good to reproduce the focal-specific
crash. As we don't use dbgsyms for Focal
tests, its not possible to see where it failed.

@stsp
Copy link
Member

stsp commented Oct 14, 2024

I think I've nailed the problem on Focal.
It appears to have an old gcc where the
-fcommon mode is default. In addition
it might have some linker bug that allows
the common symbols to become undefined
even in -Bsymbolic linking mode. Both
problems seem to be already fixed, as I
can't reproduce the removal of common
symbols in -Bsymbolic linking mode on
fedora, even if I compile dj64 with -fcommon.
I suppose -fno-common would be enough
to get things running though. Trying that now.

In addition it appears that debuild adds the
.note.package section for which I didn't
specify the address. Tried to remove that
section, but hit this:
https://sourceware.org/bugzilla/show_bug.cgi?id=32271
So had to specify its address explicitly...
finding at the same time that I had
-section-start instead of --section-start -
a typo but linker didn't complain. :(
stsp/dj64dev@e85e45f
Quite unlucky stuff.

@stsp
Copy link
Member

stsp commented Oct 14, 2024

Yep, fno-common worked.
I also managed to shut up
the crash from an asan build.

So what remains is a TAP problem
and log upload problem.

@andrewbird
Copy link
Member Author

Working on the log upload problem now.

@stsp
Copy link
Member

stsp commented Oct 14, 2024

Cool! :)

@andrewbird
Copy link
Member Author

So I did the int21/6901 test to check writing the BPB

  • MS-DOS 6.22 writes the serial number, writes the BPB volume name, writes the FSTYPE, but doesn't update/create any volume name file in the root
  • DR-DOS 7.01 writes the serial number, writes the BPB volume name, writes the FSTYPE, but doesn't update/create any volume name file in the root
  • FreeDOS 1.3 doesn't write anything
  • FDPP writes only the serial number

I think this is probably alright for FDPP.

@stsp
Copy link
Member

stsp commented Oct 18, 2024

I think this is probably alright for FDPP.

I think its just how you did that in fdpp. :)
Tools like format of fdisk will likely disagree.

@stsp
Copy link
Member

stsp commented Oct 18, 2024

Also IIRC you had some code that
also updates BPB volume when
creating root dir volume?

@andrewbird
Copy link
Member Author

Also IIRC you had some code that
also updates BPB volume when
creating root dir volume?

Yes and I'm happy with that, but I wanted to check what happened in the reverse direction i.e. using int21/6901 to set the BPB volume.

Tools like format of fdisk will likely disagree.

There is an effort to look at that now, FDOS/fdisk#86 (comment)

@stsp
Copy link
Member

stsp commented Oct 18, 2024

Oh, he wants Mac and OpenSUSE builds...
And I guess I broke both. :(
Should OpenSUSE builds be resurrected?
Do you want to maintain them btw? :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants