[Patch] New VHD support... now with added dynamics

Post new patches here!
Post Reply
shermanp
Posts: 122
Joined: Sat 18 Feb, 2017 2:09 am

[Patch] New VHD support... now with added dynamics

Post by shermanp » Fri 29 Jun, 2018 6:57 am

EDIT 2018-07-09: Updated patch available with post #2
Hi all,

I wanted to see if I could extend myself, and implement sparse, or dynamic VHD support to PCem. I am happy to announce that I appear to have succeeded.

This patch replaces my earlier VHD patch, which I consider depreciated (I'll edit the previous topic to state this). It does of course still support fixed VHD images, but with this patch, PCem can create dynamic VHD's, and read/write them. PCem created VHD images can be mounted by Windows 10 (7, 8 should also work), with the caveat that the image needs a supported filesystem (FAT, FAT32, NTFS).

Because I had to write my own image read/write functions for dynamic VHD support, I consider it experimental, until it has had more testing. There are bound to be some silly bugs still waiting to be found (there were plenty of those during development!).

This patch does NOT support differential VHD images. These are not common, and the Windows Disk Management tool cannot create them either. I have no plans to implement differencing support. I have no desire to deal with strings in C thank you very much, let alone utf16 encoded strings...

The current method of choosing between fixed and dynamic VHD creation is not very elegant at the moment. It simply displays a messagebox asking the user to choose. This area could probably use a little refinement, but it works for now.

@SarahWalker I've left in the extra logging function and (commented out) code that I used during development. I will most likely want to remove some or all of this before you merge it, unless you want it to stay there.

I am an amateur with C, so if I've done something majorly silly in the code, feel free to point it out!

Finally, could I please have some vict—er—volunteers to help test this patch for performance and stability? I am NOT a hard disk expert, just a person who can follow a rather straightforward technical document.
Last edited by shermanp on Mon 09 Jul, 2018 5:36 am, edited 1 time in total.

shermanp
Posts: 122
Joined: Sat 18 Feb, 2017 2:09 am

Re: [Patch] New VHD support... now with added dynamics

Post by shermanp » Mon 09 Jul, 2018 5:29 am

MAJOR REFACTOR

I decided that projects other than PCem may find my VHD code useful. To that end, I've disentangled the VHD code from the PCem code as much as possible. I am now calling this implementation MiniVHD, or Minimalist VHD. The code is available at https://github.com/shermp/MiniVHD, released under the same license as Pcem (GPLv2).

As well as separating out the VHD code, I've also done a more general refactor, such as improving function and variable name consistency etc.

A new patch is included in this post. Barring any issues or bugs (there may still be a few!), I think I'll call this one more or less complete.

Patch is against the latest public commit (390edaf).

EDIT 2018-11-13 Patch removed for refactoring

shermanp
Posts: 122
Joined: Sat 18 Feb, 2017 2:09 am

Re: [Patch] New VHD support... now with added dynamics

Post by shermanp » Mon 12 Nov, 2018 9:55 pm

Just a heads up that I've temporarily removed the patch while I do some more work on it.

I've since switched from manually serializing/deserializing the VHD header/footer, to using using #pragma pack. This should be much less error prone compared to the previous method, and I think modern ARM architectures support unaligned memory accesses, so it should be portable enough for modern usage.

I also want to see if I can improve the GUI side of it, so it's a little less jerry-rigged. Especially the VHD creation.

shermanp
Posts: 122
Joined: Sat 18 Feb, 2017 2:09 am

Re: [Patch] New VHD support... now with added dynamics

Post by shermanp » Mon 26 Nov, 2018 5:34 am

Ok, I think I'm finally done with this. New patch added to this post.

As stated in my previous post, the VHD changes are mostly around serialization/deserialization of VHD metadata. Tweaked VHD creation a bit as well. The core data read/write code has not changed.

Quite a few changes on the PCem side. I've tried to make the integration as non-VHD specific as I can, so hopefully it will be rather straightforward to add additional formats in the future if desired.

I think I've finally figured out the way PCem handles wxWidgets (the PCem C wrapper doesn't exactly help make it easier to figure out...) and HDD image creation is now better integrated into the GUI. No more messageboxes to set things! Note, if anyone else wants RadioBoxes, they're in PCem now :)
HDDConfigWindow.png
Unless anybody discovers any issues or bugs, I think I'm done with this.

EDIT: Patch deleted, see next post for new version.
Last edited by shermanp on Mon 24 Dec, 2018 6:55 am, edited 1 time in total.

shermanp
Posts: 122
Joined: Sat 18 Feb, 2017 2:09 am

Re: [Patch] New VHD support... now with added dynamics

Post by shermanp » Mon 24 Dec, 2018 6:54 am

shermanp wrote:
Mon 26 Nov, 2018 5:34 am
Unless anybody discovers any issues or bugs, I think I'm done with this.
So, umm, about that....

I decided to simplify the sector read/write functions. The old "fast" path has been dropped in favor of improving the old "slow" path to be, well, not slow. Basically minimized the use of fseek() where possible, which in turn minimizes the file stream buffer flushes.

Have tested by installing both Windows 98SE and Windows 2000 to dynamic VHD's. Also ran an ATTO benchmark. There doesn't appear to be any performance or stability regressions, but as always, I welcome any testers and bug reports.

I would also welcome another pair of eyes on the code if anybody is interested. The (non PCem specific) code can be found at https://github.com/shermp/MiniVHD. This patch is based on master. The PCem integration code is in this patch, I don't have an online repository for that.

I've also tried (and tried again) to get differencing VHD support working. Unfortunately, I have yet to succeed, and have just about decided to give up on that aspect. The current attempt is in the "diff-v3" branch if anyone is interested to help me figure it out. I've probably made a silly mistake somewhere that I keep missing...
Attachments
newVHD_v4.patch
(122.73 KiB) Downloaded 152 times

Joel_w
Posts: 1
Joined: Sat 16 Mar, 2019 3:33 pm

Re: [Patch] New VHD support... now with added dynamics

Post by Joel_w » Sun 17 Mar, 2019 11:28 am

This is a very useful addition! I just managed to apply the patch and compile the emulator. I did some quick tests with a VHD hardfile and it seems to work fine, from now on I will probably only use VHD files.

If it's possible to include it in the next official release that would be great, it has my vote. And a big thanks to shermanp for the patch.

shermanp
Posts: 122
Joined: Sat 18 Feb, 2017 2:09 am

Re: [Patch] New VHD support... now with added dynamics

Post by shermanp » Mon 18 Mar, 2019 3:24 am

Joel_w wrote:
Sun 17 Mar, 2019 11:28 am
This is a very useful addition! I just managed to apply the patch and compile the emulator. I did some quick tests with a VHD hardfile and it seems to work fine, from now on I will probably only use VHD files.

If it's possible to include it in the next official release that would be great, it has my vote. And a big thanks to shermanp for the patch.
It still needs some work. Another user has had issues with Winimage that have still had to be resolved. I decided to roll the (hopeful) fix for that issue into the implementation for differential VHD's. But said implementation is on hiatus at the moment until I can work up the motivation to deal with file paths in C. (Dealing with paths in C is bad enough. Dealing with paths in a cross-platform manner in C is even worse. And we won't mention the difference in path text encodings between *nix and Windows :( )

So in summary, I don't think VHD support will be ready in time for v15. Also, Sarah has been rewriting the recompiler for v15, and probably won't want to integrate VHD support until after that's released, if at all.

sards3
Posts: 3
Joined: Tue 02 Apr, 2019 7:55 pm

Re: [Patch] New VHD support... now with added dynamics

Post by sards3 » Tue 02 Apr, 2019 9:56 pm

Might it be helpful to look at how VHDs are implemented in Dosbox-X?
https://github.com/joncampbell123/dosbo ... os_vhd.cpp

shermanp
Posts: 122
Joined: Sat 18 Feb, 2017 2:09 am

Re: [Patch] New VHD support... now with added dynamics

Post by shermanp » Tue 02 Apr, 2019 10:35 pm

sards3 wrote:
Tue 02 Apr, 2019 9:56 pm
Might it be helpful to look at how VHDs are implemented in Dosbox-X?
https://github.com/joncampbell123/dosbo ... os_vhd.cpp
Interesting, I hadn't seen that before.

After a (very) brief look at the code, I think my implementation will be more complete. For example, the DosBox-X implementation only appears to load a single parent for differencing VHD's, wheras the spec allows for a chain of differential VHD's, which I have implemented. On the other hand, the code appears to be C with a sprinkling of C++, so I'm not entirely sure I've got that right...I've made life a bit difficult for myself by aiming for a pure C implementation.

There might be some useful ideas in there, but I think our architecture's are too different to be of much use. Note that MiniVHD aims to be a completely standalone library that requires no external non-system libraries to function.

sards3
Posts: 3
Joined: Tue 02 Apr, 2019 7:55 pm

Re: [Patch] New VHD support... now with added dynamics

Post by sards3 » Wed 03 Apr, 2019 4:58 pm

It does support multi-parent chains. But I was thinking more about your comment about the handling of file paths and text encodings. As far as I can tell, the Dosbox-X implementation uses no external libraries for that, and it does it with a pretty small amount of code.

Also, for the purposes of PCem, I am of the opinion that having some VHD support is better than having no VHD support. So maybe it would be easier to start with a bare minimum implementation? For example, support reading/writng but not creation; support only relative parent locator paths. The non-essential additional features can be added in time.

If you want some help, I may have some time over the coming weekends to work on this. But I would need a bit of guidance from you in terms of what the existing issues are and what needs to be implemented. It's your code and I don't want to step on your toes.

shermanp
Posts: 122
Joined: Sat 18 Feb, 2017 2:09 am

Re: [Patch] New VHD support... now with added dynamics

Post by shermanp » Wed 03 Apr, 2019 9:07 pm

sards3 wrote:
Wed 03 Apr, 2019 4:58 pm
It does support multi-parent chains. But I was thinking more about your comment about the handling of file paths and text encodings. As far as I can tell, the Dosbox-X implementation uses no external libraries for that, and it does it with a pretty small amount of code.

Also, for the purposes of PCem, I am of the opinion that having some VHD support is better than having no VHD support. So maybe it would be easier to start with a bare minimum implementation? For example, support reading/writng but not creation; support only relative parent locator paths. The non-essential additional features can be added in time.

If you want some help, I may have some time over the coming weekends to work on this. But I would need a bit of guidance from you in terms of what the existing issues are and what needs to be implemented. It's your code and I don't want to step on your toes.
I do have reading/writing working for differencing disks (well, I appear to anyway, haven't extensively tested it yet).

From my quick look at the code, it appears dosbox-x handles the text encoding issue by converting to ascii or ISO-8859-1 and hoping for the best. I have implemented full UTF-8 <> UTF-16 conversion to (hopefully) be more language agnostic. I've currently rolled my own implementation, but will probably revert to using system libraries for that (iconv on *nix systems, MultiByteToWideChar/WideCharToMultiByte on Windows). I'm also using _wfopen() on windows which accepts UTF-16 filepaths. Modern *nix systems should be happy with UTF-8 encoded paths.

The real bugbear for me has been path manipulation. I've found a nice simple path manipulation library called cwalk, which helps, but I'm still left hoping I've got my sums right in regards to path lengths. Ironically, relative paths are harder to deal with than absolute paths. Mainly because the path is relative to the VHD file, not the current working directory. Because MiniVHD is cross platform, I wasn't planning on supporting absolute parent paths at all, considering it would break if you tried to use the same VHD accross platforms.

Once the above is sorted out, will need to flesh out the rest of the support (checking parent UUID etc.) And creation support of course, but that shouldn't be too hard, at least for the library. PCem side is going to take a bit of thinking as to how to use differencing images.

TBH, VHD does not have a very robust differencing implementation. The only "official" method of determining whether the parent image has been altered after child creation is doing a file modification time check on the parent. And in practice, that doesn't work either because *****y Windows diskpart doesn't set the date in the child image :( A more robust method would be using a full disk checksum, but that would take far too long for large images. A much better imaging format for this type of usage is something like VHDX or QEMU's qcow2, both of which are significantly more complex to implement.

I welcome any sort of help with this. Don't worry about stepping on toes, I'm not under the illusion that my code is perfect (far from it! I'd say many would consider it too verbose), and a second pair of eyeballs would probably catch many issues that I've missed. My only constraint is I'm trying to stay mostly C89, with a sprinkling of C99 here or there where it makes things a bit easier. I'm doing this because I'm aware some of the PCem developers are using fairly old toolchains, and I want to remain compatible with that.

seth
Posts: 30
Joined: Tue 14 Mar, 2017 12:13 pm

Re: [Patch] New VHD support... now with added dynamics

Post by seth » Thu 18 Apr, 2019 7:26 pm

This patch would be awesome for v15, but it's not ready yet, right?

shermanp
Posts: 122
Joined: Sat 18 Feb, 2017 2:09 am

Re: [Patch] New VHD support... now with added dynamics

Post by shermanp » Thu 18 Apr, 2019 9:11 pm

seth wrote:
Thu 18 Apr, 2019 7:26 pm
This patch would be awesome for v15, but it's not ready yet, right?
I'm working on it! Hope to have it ready by the feature freeze, but no guarantees.

shermanp
Posts: 122
Joined: Sat 18 Feb, 2017 2:09 am

Re: [Patch] New VHD support... now with added dynamics

Post by shermanp » Sun 21 Apr, 2019 11:56 pm

shermanp wrote:
Thu 18 Apr, 2019 9:11 pm
seth wrote:
Thu 18 Apr, 2019 7:26 pm
This patch would be awesome for v15, but it's not ready yet, right?
I'm working on it! Hope to have it ready by the feature freeze, but no guarantees.
Nope, wasn't able to get it working in time I'm afraid.

Currently scratching my head over why Windows (or Hyper-V) doesn't like my differencing VHD. Giving the whole "The disc image file is corrupted" message, and as of yet, I haven't figured out what it doesn't like. Because, of course it doesn't tell you what is wrong.

Sigh...

shermanp
Posts: 122
Joined: Sat 18 Feb, 2017 2:09 am

Re: [Patch] New VHD support... now with added dynamics

Post by shermanp » Mon 22 Apr, 2019 5:06 am

shermanp wrote:
Sun 21 Apr, 2019 11:56 pm
shermanp wrote:
Thu 18 Apr, 2019 9:11 pm
seth wrote:
Thu 18 Apr, 2019 7:26 pm
This patch would be awesome for v15, but it's not ready yet, right?
I'm working on it! Hope to have it ready by the feature freeze, but no guarantees.
Nope, wasn't able to get it working in time I'm afraid.

Currently scratching my head over why Windows (or Hyper-V) doesn't like my differencing VHD. Giving the whole "The disc image file is corrupted" message, and as of yet, I haven't figured out what it doesn't like. Because, of course it doesn't tell you what is wrong.

Sigh...
Just an update to this. I finally found my (facepalm worthy) mistake, and differencing VHD creation seems to work fine now :) It took an embarrassingly long time to figure out my mistake too (see: https://github.com/shermp/MiniVHD/commi ... e3f9cba0a1) :oops: .

Code still needs some cleanup, and I'm currently ignoring any and all error handling when it comes to parent paths, so it isn't going to be ready for V15 (since the deadline is today), and will need some testing from other users once I've done that.

altheos
Posts: 64
Joined: Wed 24 Feb, 2016 7:27 pm

Re: [Patch] New VHD support... now with added dynamics

Post by altheos » Fri 24 May, 2019 10:14 am

Hi,

I can't apply your patch against a stock v14 : do you have a compatible version of your patch ?

Regards.

shermanp
Posts: 122
Joined: Sat 18 Feb, 2017 2:09 am

Re: [Patch] New VHD support... now with added dynamics

Post by shermanp » Fri 24 May, 2019 11:10 am

altheos wrote:
Fri 24 May, 2019 10:14 am
Hi,

I can't apply your patch against a stock v14 : do you have a compatible version of your patch ?

Regards.
Sorry, not at the moment. I need to rebase the patch on V15, and I'm not really in a PCem frame of mind at the moment, so it could be a wee while yet.

altheos
Posts: 64
Joined: Wed 24 Feb, 2016 7:27 pm

Re: [Patch] New VHD support... now with added dynamics

Post by altheos » Fri 24 May, 2019 12:43 pm

Thanks for reply.
I've made some tests with "already made" dynamic VHD and so far your patch seems to be stable : thanks for your work.

Post Reply