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

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

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

Post by shermanp »

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: 175
Joined: Sat 18 Feb, 2017 2:09 am

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

Post by shermanp »

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: 175
Joined: Sat 18 Feb, 2017 2:09 am

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

Post by shermanp »

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: 175
Joined: Sat 18 Feb, 2017 2:09 am

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

Post by shermanp »

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: 175
Joined: Sat 18 Feb, 2017 2:09 am

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

Post by shermanp »

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...
Last edited by shermanp on Sat 14 Nov, 2020 6:49 am, edited 1 time in total.
Joel_w
Posts: 4
Joined: Sat 16 Mar, 2019 3:33 pm

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

Post by Joel_w »

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: 175
Joined: Sat 18 Feb, 2017 2:09 am

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

Post by shermanp »

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: 7
Joined: Tue 02 Apr, 2019 7:55 pm

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

Post by sards3 »

Might it be helpful to look at how VHDs are implemented in Dosbox-X?
https://github.com/joncampbell123/dosbo ... os_vhd.cpp
shermanp
Posts: 175
Joined: Sat 18 Feb, 2017 2:09 am

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

Post by shermanp »

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: 7
Joined: Tue 02 Apr, 2019 7:55 pm

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

Post by sards3 »

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: 175
Joined: Sat 18 Feb, 2017 2:09 am

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

Post by shermanp »

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: 34
Joined: Tue 14 Mar, 2017 12:13 pm

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

Post by seth »

This patch would be awesome for v15, but it's not ready yet, right?
shermanp
Posts: 175
Joined: Sat 18 Feb, 2017 2:09 am

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

Post by shermanp »

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: 175
Joined: Sat 18 Feb, 2017 2:09 am

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

Post by shermanp »

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: 175
Joined: Sat 18 Feb, 2017 2:09 am

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

Post by shermanp »

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: 72
Joined: Wed 24 Feb, 2016 7:27 pm

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

Post by altheos »

Hi,

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

Regards.
shermanp
Posts: 175
Joined: Sat 18 Feb, 2017 2:09 am

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

Post by shermanp »

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: 72
Joined: Wed 24 Feb, 2016 7:27 pm

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

Post by altheos »

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.
shermanp
Posts: 175
Joined: Sat 18 Feb, 2017 2:09 am

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

Post by shermanp »

With the assistance of @sards3, here's a new patch based on PCem v16.

I'm hoping that this sneaks in just before the v17 feature freeze.

This basically has full VHD support. Fixed, sparse (dynamic) and differencing images all work. Creation of differencing images works.

Be very careful when using differencing images that you do not modify the parent image. There's some timestamp checking to help prevent whoopsies, but it's pretty fragile. Unfortunately, the VHD format doesn't have a more robust method of protecting the parent/child integrity (that's probably why Microsoft created the VHDX format).

A huge thanks to @sards3 for helping to fix some bugs in the core VHD library, and for providing the bulk of the current PCem GUI integration.

The PCem/VHD integration code can be found at my PCem repo at https://github.com/shermp/pcem/tree/minivhd

One problem that I've noticed, and I'm not sure how to resolve it, is that when stopping emulation, then attempting to select the same VHD image (or image that references it as a parent), in the config, this doesn't work. This is because for some reason, PCem does not close the HDD image files when the emulation stops.

@SarahWalker I prefer pull requests if you're happy to accept them, but here's a git diff for this thread. Please let me know if there's a problem with the diff.
Attachments
pcem-minivhd-v5.zip
(70.42 KiB) Downloaded 359 times
User avatar
SarahWalker
Site Admin
Posts: 2054
Joined: Thu 24 Apr, 2014 4:18 pm

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

Post by SarahWalker »

Committed at 602d6f1c.

I'll have a look at the image not closing bug. Hopefully won't be too difficult to fix.
shermanp
Posts: 175
Joined: Sat 18 Feb, 2017 2:09 am

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

Post by shermanp »

SarahWalker wrote: Sat 14 Nov, 2020 7:13 pm Committed at 602d6f1c.

I'll have a look at the image not closing bug. Hopefully won't be too difficult to fix.
Thank you very much.

When I was running it through the debugger, the image only seems to be closed during the ide reset, which only seems to happen when emulation starts. (this is of course for IDE disks, not sure about SCSI).

Also, one other thing to note, while I don't THINK it's an issue, it might be best to double check, I changed the hdd_file_t.f from a FILE pointer to a void pointer. All usages I could find outside of hdd_file.c appeared to be NULL checks, so it should be ok, but I don't know enough about the PCem internals to be absolutely certain.
sards3
Posts: 7
Joined: Tue 02 Apr, 2019 7:55 pm

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

Post by sards3 »

Without actually debugging it, looks like you just need to call hdd_close inside the (currently empty) ide_close.
Laucian
Posts: 11
Joined: Mon 28 Nov, 2016 6:49 pm

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

Post by Laucian »

SarahWalker wrote: Sat 14 Nov, 2020 7:13 pm Committed at 602d6f1c.
Currently when building under Linux there is a build error due case sensitiveness. The "minivhd" folder is specified as "MiniVHD" in wx-config.c. The build continues after that is fixed.

Code: Select all

wx-config.c:23:10: fatal error: MiniVHD/minivhd.h: No such file or directory
   23 | #include "MiniVHD/minivhd.h"
      |          ^~~~~~~~~~~~~~~~~~~
The next error is as follows:

Code: Select all

/usr/bin/ld: minivhd/pcem-minivhd_create.o: in function `mvhd_create_sparse_diff':
minivhd_create.c:(.text+0x33a): undefined reference to `strcpy_s'
/usr/bin/ld: minivhd/pcem-minivhd_manage.o: in function `mvhd_get_diff_parent_path':
minivhd_manage.c:(.text+0x1be): undefined reference to `strncpy_s'
/usr/bin/ld: minivhd_manage.c:(.text+0x312): undefined reference to `strncpy_s'
/usr/bin/ld: minivhd_manage.c:(.text+0x447): undefined reference to `strncpy_s'
/usr/bin/ld: minivhd/pcem-minivhd_manage.o: in function `mvhd_open':
minivhd_manage.c:(.text+0x666): undefined reference to `strcpy_s'
collect2: error: ld returned 1 exit status
As far as I am aware, strncpy_s is a Microsoft extension and unsupported by Glibc/GCC.
shermanp
Posts: 175
Joined: Sat 18 Feb, 2017 2:09 am

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

Post by shermanp »

Laucian wrote: Sun 15 Nov, 2020 1:06 am
SarahWalker wrote: Sat 14 Nov, 2020 7:13 pm Committed at 602d6f1c.
Currently when building under Linux there is a build error due case sensitiveness. The "minivhd" folder is specified as "MiniVHD" in wx-config.c. The build continues after that is fixed.

Code: Select all

wx-config.c:23:10: fatal error: MiniVHD/minivhd.h: No such file or directory
   23 | #include "MiniVHD/minivhd.h"
      |          ^~~~~~~~~~~~~~~~~~~
The next error is as follows:

Code: Select all

/usr/bin/ld: minivhd/pcem-minivhd_create.o: in function `mvhd_create_sparse_diff':
minivhd_create.c:(.text+0x33a): undefined reference to `strcpy_s'
/usr/bin/ld: minivhd/pcem-minivhd_manage.o: in function `mvhd_get_diff_parent_path':
minivhd_manage.c:(.text+0x1be): undefined reference to `strncpy_s'
/usr/bin/ld: minivhd_manage.c:(.text+0x312): undefined reference to `strncpy_s'
/usr/bin/ld: minivhd_manage.c:(.text+0x447): undefined reference to `strncpy_s'
/usr/bin/ld: minivhd/pcem-minivhd_manage.o: in function `mvhd_open':
minivhd_manage.c:(.text+0x666): undefined reference to `strcpy_s'
collect2: error: ld returned 1 exit status
As far as I am aware, strncpy_s is a Microsoft extension and unsupported by Glibc/GCC.
Arrgh. Sorry about this. @sards3 changed this in MiniVHD, I assume to cause less issues for MSVC. I should have checked its compatibility with other compilers (it works with MinGW-w64, which is why I didn't catch it).

I'll see about fixing this (and the directory name issue) soon.
User avatar
leilei
Posts: 1039
Joined: Fri 25 Apr, 2014 4:47 pm

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

Post by leilei »

I'm seeing the same strcpy/strncpy build failures with MinGW 4.7.2
shermanp
Posts: 175
Joined: Sat 18 Feb, 2017 2:09 am

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

Post by shermanp »

leilei wrote: Sun 15 Nov, 2020 3:24 am I'm seeing the same strcpy/strncpy build failures with MinGW 4.7.2
Siigh... Thanks for the feedback.

I'll either revert back to what I had originally (strcpy, strncpy), or switch to a straight memcpy alternative (I'm copying between fixed arrays in some cases, so that's a viable option.
sards3
Posts: 7
Joined: Tue 02 Apr, 2019 7:55 pm

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

Post by sards3 »

Sorry, didn't realize that strncpy_s, etc., are not available in all compilers.
shermanp
Posts: 175
Joined: Sat 18 Feb, 2017 2:09 am

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

Post by shermanp »

Alright, here's some fixes.

And yes, I actually tested compiling this on linux this time...

Diff is against the latest pcem master as of writing this (5ae201b).

If you don't want to deal with the diff, the code is at https://github.com/shermp/pcem/tree/vhd ... tion-fixes
Attachments
pcem-vhd-compilation-fixes.zip
(1.56 KiB) Downloaded 332 times
sards3
Posts: 7
Joined: Tue 02 Apr, 2019 7:55 pm

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

Post by sards3 »

@shermanp I noticed one more issue:

In hdd_file.c, line 80, you have:

Code: Select all

	if (hdd->img_type == HDD_IMG_VHD)
        {
                MVHDMeta *vhdm = (MVHDMeta*)hdd->f;
                MVHDGeom geom = mvhd_get_geometry(vhdm);
                hdd->spt = geom.spt;
                hdd->hpc = geom.heads;
                hdd->tracks = geom.cyl;
        }
        else if (hdd->img_type == HDD_IMG_RAW)
        {
                hdd->spt = spt;
                hdd->hpc = hpc;
                hdd->tracks = tracks;
        }
I think we should actually not use mvhd_get_geometry() here. That will use the geometry from the VHD file, instead of the geometry in the PCem config. In the case of the geometry adjustment for large VHDs, (or in case the user has changed the geometry in the config for some reason) they won't be the same. So we could replace the above with:

Code: Select all

hdd->spt = spt;
hdd->hpc = hpc;
hdd->tracks = tracks;
User avatar
leilei
Posts: 1039
Joined: Fri 25 Apr, 2014 4:47 pm

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

Post by leilei »

Progress, but I still land on this error at the linking part

Code: Select all

minivhd_util.o:minivhd_util.c:(.text+0x42c): undefined reference to `_wfopen_s'
Post Reply