search

LTRData / DiscUtils

Utility libraries to interact with discs, filesystem formats and more
MIT License
111 stars 24 forks source link

Support for Fixed VHDX Files, and some package Updates #12

Closed TheEndHunter closed 3 months ago

TheEndHunter commented 3 months ago

Implemented VHDX Fixed Disk Initialization code (May need to add additional Tests for more coverage as needed, let me know) Made some changes to allow newer packages versions in the Library Tests Project.

All Tests are passing across all framework versions supported as it currently stands,.

I would like another pair of eyes(or two) to double check, just to make sure i didn't do anything bad,

LTRData commented 3 months ago

Great work! I have a few things though that I want you to consider before we move forward with this.

  1. I do not want to require preview versions of SDK to build the code in this repository. We could add .NET 9.0 to the builds, but then we should do that in a separate branch so that it does not get built automatically when building nuget packages etc. So, I would suggest that you put .NET 9.0 related changes in a separate pull request.
  2. I understand that it might make the code easier to read if we rename LeaveBlocksAllocated flag to Fixed. The issue I have we it though, is that it makes it more difficult to follow how the implementation corresponds to official VHDX format specification, where the flag is called LeaveBlockAllocated. So I suggest that we do not rename this flag.
TheEndHunter commented 3 months ago

Great work! I have a few things though that I want you to consider before we move forward with this.

  1. I do not want to require preview versions of SDK to build the code in this repository. We could add .NET 9.0 to the builds, but then we should do that in a separate branch so that it does not get built automatically when building nuget packages etc. So, I would suggest that you put .NET 9.0 related changes in a separate pull request.

No problem, I do understand that, will pull that change into its own branch.

  1. I understand that it might make the code easier to read if we rename LeaveBlocksAllocated flag to Fixed. The issue I have we it though, is that it makes it more difficult to follow how the implementation corresponds to official VHDX format specification, where the flag is called LeaveBlockAllocated. So I suggest that we do not rename this flag.

I will make this change back and probably just add some comments to each enum for clarity instead.

As for tests, are there any additional test you can think of that may need to be added for this?

The only ones I can think of are to make sure it doesn't try to shrink or grow the vhdx file when it is initialized fixed, but I'm not familiar with the code base as a whole yet, so I may already be covered by another check.

LTRData commented 3 months ago

Thanks! I will check out the code tomorrow and take a look at the test cases as well!

TheEndHunter commented 3 months ago

I have pushed the changes over. Let me know if you need any more changes.

LTRData commented 3 months ago

There seems to be something wrong with the implementation. When I run VHDXCreate -t fixed -sz 2GB test.vhdx, I get a vhdx file that is just 4 MB in physical size. The virtual size is okay though, and the file seems correct in other aspects and it does not shrink when filled with data and then trimmed afterwards. So, it actually creates a dynamically allocated vhdx that can only grow and never shrink, not a vhdx with entire size allocated from start.

If you create a 2 GB fixed size vhdx file in Disk Management in Windows, you get a 2 GB fixed allocated vhdx file.

TheEndHunter commented 3 months ago

There seems to be something wrong with the implementation. When I run VHDXCreate -t fixed -sz 2GB test.vhdx, I get a vhdx file that is just 4 MB in physical size. The virtual size is okay though, and the file seems correct in other aspects and it does not shrink when filled with data and then trimmed afterwards. So, it actually creates a dynamically allocated vhdx that can only grow and never shrink, not a vhdx with entire size allocated from start.

If you create a 2 GB fixed size vhdx file in Disk Management in Windows, you get a 2 GB fixed allocated vhdx file.

I am working on a fix now, turns out i missed setting the fileEnd from OneMiB to capacity, also had to make a few other changes for the tests as they tried to allocate 16GB of memory to a fixed disk once i fixed the issue.

checked the VHDXCreate -t fixed -sz 2GB test.vhdx (and 4GB) both worked

I'll be pushing the changes up soon

LTRData commented 3 months ago

Thanks a lot for your contribution! Much appreciated! 👍

TheEndHunter commented 3 months ago

Thanks a lot for your contribution! Much appreciated! 👍

Thanks, always happy to contribute where I can, especially when I use this project for a hobby project of my own.