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

Add decoding of VHD cookie, validation, and parsing #948

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kdedon
Copy link

@kdedon kdedon commented Jan 7, 2025

Problem

I am attempting to install Openstep 4.2 but the first major issue is that the IDE driver suffers an overflow with larger SPT values. I would like to manually set the hard drive specifications to allow for the operating system to access the hard drive.

Implementation

The VHD specification allows for hard drive metadata to be specified on a per file basis. I have added support files that provides the initial VHD footer parsing to allow for the metadata to be extracted for fixed size VHDs. I use this data in the hard drive loading path for pcxt and ao486 to allow for the file to define the CHS values.

I have created a support directory for this functionality in the case that VHD support is extended in the future to support DYNAMIC or DIFFERENTIAL types.

In the case that a VHD footer cannot be properly parsed, the functionality falls back to the current state of treating the file like a standard raw image.

@sorgelig
Copy link
Member

sorgelig commented Jan 7, 2025

I'm not sure if adding header at the end is a good idea.
May be it could be a better idea to add a text file with the same name where VHD parameters will be present? In that case, it will be easier to adjust/create than adding it in binary form.
Also, number of tracks better to omit and calculate it from file size (rounded down). So basically only sectors and heads are required.

@kdedon
Copy link
Author

kdedon commented Jan 7, 2025

I agree that manipulating the binary footer is a pain, but in testing I found that quite a few of the old files I used for testing actually had it it already having been created using tools attempting to keep in line with Microsoft's Virtual PC product set. It actually makes me wonder how many files are 512B too large. On the other hand, I do like that it keeps the configuration self contained and I don't have to manage multiple files.

The config option is fine too, and if you would prefer me work on that I can whip something up. If you have any particular file format styles (i.e.-- just multiple values separated by a space?) let me know.

A third option is to change ide_set_geometry to mirror calc_geometry in ide.cpp to attempt to fit the total size starting from lower values first. A bit more inefficient but it should only run once per file so it doesn't seem like that would be too big a waste.

In any case, let me know what needs to be done to move the solution forward. I recognize this is an edge case so I can understand the desire to minimize changes to support it.

@sorgelig
Copy link
Member

sorgelig commented Jan 8, 2025

A third option is to change ide_set_geometry to mirror calc_geometry in ide.cpp to attempt to fit the total size starting from lower values first. A bit more inefficient but it should only run once per file so it doesn't seem like that would be too big a waste.

it won't always work. You will attempt to tune the function to specific file but it will not work with many other files. So i prefer to keep CHS detection in current form (and keep it compatible with existing files) and add a config file in text form to support files where automatic CHS detection fails. Binary footer is too cryptic and requires HEX editor or other tools to edit.

So better to use a text form. It can be a simple text form like

sectors=XXX
heads=XXX
cylinders=XXX

I don't know how many real parameters are required, but better keep minimum for easier use. cylinders parameter is probably useful if you want to limit it in case if there is a "tail" at the end which may add unwanted additional cylinders. It should be an optional parameter. Probably "cylinders" can be used to extend the file on first use if original file is smaller.

I see you are motivated to do it, so it's ok if you will implement it.

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

Successfully merging this pull request may close these issues.

2 participants