cebix / macemu

Basilisk II and SheepShaver Macintosh emulators
1.38k stars 289 forks source link

Issue in SheepShaver associated with the feature that allows the user to specify whether a Volume shall be treated as CDROM, introduced with commit 6e06a22, 27 Apr 2015 #149

Closed RonaldPR closed 6 years ago

RonaldPR commented 6 years ago

This (neat) feature allows the user to determine in the built-in GUI prefs editor whether a volume in the volumes list is to be treated as CD-ROM. When the related checkbox is checked, the "disk" item becomes a "cdrom" item in the prefs file and vice versa. To make it possible to do that both ways, "cdrom" items also appear in the volumes list in the GUI window.

This is also true for the default cdrom value "/dev/poll/cdrom". If a user unchecks the box, it will become a disk item. At next startup, MacOS wants to initialize whatever it finds there. At the same time a new default cdrom item is created and "/dev/poll/cdrom" will now appear twice in the volumes list.

There should be no user interaction in the GUI prefs editor with the default cdrom value.

I do not have the skills to do this myself, but would it be possible to make it so that a cdrom item with a value that starts with /dev/ is hidden in the GUI?

atmaxinger commented 6 years ago

I have fixed this in #163.

RonaldPR commented 6 years ago

It does indeed fix the issue for OSX 10.8 and later where physical CD's cannot be mounted in SheepShaver and where the possibility to mount images as CD is most important.

But with this fix SheepShaver looses the ability to mount physical CDs in MacOSX 10.4 through 10.7 because cdrom /dev/poll/cdrom is not just hidden in the GUI, but actually removed from the prefs file when settings are saved with the prefs editor.

I am not sure how important it is to keep a feature that will only work in older MacOSX versions.

RonaldPR commented 6 years ago

A possible problem does not arise with the string cdrom /dev/poll/cdrom, but when (through user intervention in the GUI) the string disk /dev/poll/cdrom is written to the prefs file. The best solution would be if that could be prevented.

atmaxinger commented 6 years ago

I've changed the code so that cdrom /dev/poll/cdrom always gets written into the prefs file when saving.

atmaxinger commented 6 years ago

The interesting thing is, that cdrom /dev/poll/cdrom is actually not in the default prefs file that gets created when SheepShaver first starts.

It actually looks like /dev/poll/cdrom is always mounted in SheepShaver (as long as nocdrom is false), regardless if it is specified in the prefs file or not.

RonaldPR commented 6 years ago

You beat me. Your last suggested commit appeared seconds before my comment. :)

Yes, I noticed that too. Apparently the cdrom string is written by the built-in prefs editor or Launcher and not by SheepShaver itself.

atmaxinger commented 6 years ago

No, it's not written at all actually. There is an extra option called nocdrom. If this is set to false, then /dev/poll/cdrom gets mounted automatically (or at least SheepShaver passes it to the settings window) so there is no need to save it in the preference file.

If it is set to true, then the cdrom /dev/poll/cdrom entry acutally defeats the purpose of the nocdrom setting.

I would suggest not writing cdrom /dev/poll/cdrom at all.

atmaxinger commented 6 years ago

Also I just tested it on an old Mac with a CD drive. SheepShaver automounts the CD-ROM, even if it is not /dev/poll/cdrom (/dev/disk1 in my case), so we really need to block all devices in /dev/.

RonaldPR commented 6 years ago

I cannot test it here because my OSX 10.6 laptop broke down and SheepShaver code is way too complicated for me.

I remember that cdrom /dev/disk1 used to be the default entry in the prefs file and that it was changed to cdrom /dev/poll/cdrom because /dev/disk1 did not work on all configurations.

Also changes were made to make it possible to mount a CD-ROM while SheepShaver was running. Before that, you needed to mount the CD on the host first and then launch SheepShaver to have the CD mounted in SheepShaver.

RonaldPR commented 6 years ago

I understand correctly that a CD-ROM will mount in SheepShaver on your machine even when no cdrom entry is present in the prefs file?

atmaxinger commented 6 years ago

Yes, that is what I have observed right now.

As long as nocdrom is set to false (the setting Disable CD-ROM in the GUI) SheepShaver will automount the physical CD Drive, regardless if an entry is in the prefs file or not.

The actual bug here is that the /dev/* entries were displayed and written to the prefs file. The original code did not do that.

RonaldPR commented 6 years ago

Strange. Then why has it been there all those years. With no cdrom entry in the prefs file, can you also mount a CD-ROM in SheepShaver when you insert the CD in the drive while SheepShaver is running?

atmaxinger commented 6 years ago

No this does not work on my machine. But it also doesn't work if the cdrom entry is present in the prefs file...

RonaldPR commented 6 years ago

That used to work. I will have my MacBook repaired (probably loose RAM) and check this with older builds.

In the meantime I will post a new build with your changes in Emaculation forum and ask for comments.

RonaldPR commented 6 years ago

In 2012 there was a discussion (then still in B2-devel mailing list) about the loss of mounting and ejecting CD-ROMs directly within SheepShaver caused by an error (inverted nocdrom check) in commit 2b348f7 (April 21, 2012) that was fixed in commit 889c88d (July 1, 2012).

In that discussion Robert Munafo wrote (while he was using the code without the error): Sheepshaver still mounts CD-ROMs just fine, at least it does for me, provided you have "cdrom /dev/poll/cdrom" and "nocdrom false" in your .sheepshaver_prefs (and no other lines containing "cdrom"). Example: CodeWarrior Pro version 3 MacOS Tools (the CD-ROM containing the CodeWarrior IDE, headers and libraries). Not only does it mount, but I can repeatedly unmount (by dragging to the trash in the guest OS) and re-mount the CD-ROM (by re-inserting it), and the CD-ROM shows up on the desktop in the host OS's Finder as well.

So it seems that mounting and ejecting CD-ROMs within SheepShaver would then only work with in prefs: nocdrom false and cdrom /dev/poll/cdrom and no other cdrom entry

atmaxinger commented 6 years ago

I just tried it with the version of July 15, 2012 (where the mark-as-cdrom feature doesn't exist yet) and it also does not work. I also noticed that there isn't even a /dev/poll/cdrom file on my Macs (Snow Leopard, El Capitan and High Sierra)

RonaldPR commented 6 years ago

When I build SheepShaver from the current cebix-macemu source (that is without your commits from the past days), launch that SheepShaver build, open Preferences, and click 'Save', the line cdrom /dev/poll/cdrom is added to the prefs file.

RonaldPR commented 6 years ago

That will only happen when no cdrom entry is already present. That line will appear as soon as the user configures SheepShaver for the first time. It has been so for many years.

atmaxinger commented 6 years ago

Upon further inspection I found the code that adds /dev/poll/cdrom (screenshot below) to the settings. It is done on startup. As it states in the comments, this is only a fake entry. I tested with a version from 2012 and I think I even observed a buggy behaviour: the cdrom entry gets written even if nocdrom true is specified. If you look at the code below, it nocdrom == true then it should actually not add cdrom /dev/poll/cdrom to the settings. Also, /dev/poll/cdrom gets replaced by e.g. /dev/disk1 when an actual CD is in the drive an mounted in SheepShaver and one clicks Save in the settings window.

I have restored that behaviour that I observed from the 2012 build in my branch now.

bildschirmfoto 2018-02-22 um 10 22 04
atmaxinger commented 6 years ago

There is still a bug in there, that causes non-physical CDs to appear twice in the list if you also use physical CDs (if the physical disc is ejected from within SheepShaver), most likely caused because the code handling the physical CDs still assumes that there can only be 1 cdrom entry.

RonaldPR commented 6 years ago

Ah, I see. I have not mounted CD-ROMs in SheepShaver since that became impossible in OSX 10.8 and I think I never saved settings while a CD was mounted in SheepShaver. So that is probably why I always saw /dev/poll/cdrom in the prefs file. I will see this evening what happens here after applying your latest commits.

RonaldPR commented 6 years ago

The result of 6 successive commits, some again changing or reverting changes made in a previous one. Seems fine now.

Here in macOS 10.12 it works as I think it should. The line cdrom /dev/poll/cdrom is added when settings are saved but it will not appear in the settings window so users cannot erroneously make it into a disk item. Setting disk images to be mounted in SheepShaver as CD-ROM still works fine.

I cannot test how SheepShaver and the prefs editor will behave with regard to mounting and ejecting physical CD-ROMs as I do not have a working Mac with a Mac OS X version that allows this. Is the issue with non-physical CDs appearing twice in the list still there?

atmaxinger commented 6 years ago

Oh I think I've fixed that now (appearing multiple times). It was an error in the code that saves the preferences.

Two issues that remain, however, and I don't known how to fix are:

RonaldPR commented 6 years ago

I cannot test with physical CDs and I did not try to eject a virtual CD within SheepShaver till now.

When I try to eject/remove a virtual CD within SheepShaver, it immediately returns on the desktop as "Audio CD 1". When opened, it shows the icon of an audio track "Track 1". When double clicked, Apple CD Audio Player is launched. But nothing is played as there is no audio on that virtual CD. When I try to remove "Audio CD 1", it immediately returns as "Audio CD 2", and so on. Very weird. After a restart of the emulated machine within SheepShaver the last shown "Audio CD" is again on the desktop, but I can still shut down SheepShaver normally.

I applied your changes to the current code here in cebix/macemu. Could that explain the differences in behaviour?

atmaxinger commented 6 years ago

No, I also get the Audio CD behaviour sometimes...

But then again we're talking about undefined behaviour here, as the CD ROM handling code of SheepShaver expects it to be a physical CD ROM.

RonaldPR commented 6 years ago

Well, the remaining issues with ejecting virtual CD-ROMs are already there since your pull request #59 was merged on 27 April 2015. I imagine solving these issues would not be a trivial task. I suppose that we'll have to settle for now with what we got so far.

The issue that I reported here is solved with your latest commits. It was a bit of a trial and error operation with 8 subsequent commits, some again changing or reverting what was changed in a previous one. Thanks for your work. I think it is worth applying them to the source here.