Let DriveManager own all filesystem and image mount objects
#2306
Conversation
kcgen
added
the
plumbing
|
Needs extensive testing - try lots of mount types (CDROM images and ISOs, HDD images, Floppy images, directory and overlay mounts) mixing them, unmounting them, and so on. Ideally using UBSAN-enabled builds. I'll post test results as I go. |
kcgen
force-pushed
the
kc/image-leak-2
branch
2 times, most recently
from
fb7f94f to
16c6baf
Compare
|
With all the pointers now in
The good news is that PVS Studio isn't foolishly recommending we forgo secure/safe code, because it only recommends the
So I'll back out most of these truly unnecessary |
|
@kcgen this will take me a few days, I'm busy with other stuff too. Maybe to get it fully reviewed by Monday is a safe assumption, and hopefully a bit earlier. |
|
Zero rush! Needs plenty of testing in the meantime :-) |
Previously, management of the mountable filesystems (CDROM ISOs, HDD FAT16, Floppy FAT12) and raw images (numbered HDD images and bootable floppy images) was spread across a handful of literal arrays: - `imageDiskList[]` array - `diskSwap[]` array - `Drives[]` array Without a centralized managing these, it was difficult to clear / destruct / delete them as they could often point to each-other, leading to double-free situations. This commit moves all ownership inside the `DriveManager` class where they are all held as `std::unique_ptr` objects to guarantee that only one instance exists. The other three literal arrays (`imageDiskList`, `diskSwap`, and `Drives` hold simple pointers into the drive manager's `unique_ptr->get()` content. This removes all management responsibility from the arrays. Unfortunately this is a large commit because it's essentially all-or-nothing (once the data types are switched, all the affected code needs to be switched, too).
Only the look ups without prior bounding logic were moved to .at() access, as PVS studio rightly caught many as unecessary as it duplicates logic in the code.
This commit should be functionally identical and only contain 1:1 variable renames.
This commit should be functionally identical and only contain whitespace changes.
Ready to merge; I've tested all the mount types with various combinations and there are at least no regressions. After this PR ownership is centralized but runtime state is still held in those three external global arrays (as well as some state help in the drive manager). The next phase of refactoring should eliminate thos3 external arrays by moving their state all inside the Drive Manager, so it becomes a single data and state 'registry' for all the mounts. From that point, it should be easier manage the mounts via the CLI commands (if anyone has a desire to work on that.. of course, things are work OK as-is, so no rush!) 😅 |
|
Thanks for the detailed review, @johnnovak ! |
Previously, management of the mountable filesystems (CDROM ISOs, HDD FAT16, Floppy FAT12) and raw images (numbered HDD images and bootable floppy images) was spread across a handful of literal arrays:
imageDiskList[]arraydiskSwap[]arrayDrives[]arrayThis made it difficult to properly destruct and replace objects without double-free situations.
This commit moves all ownership inside the
DriveManagerclass where they are all held asstd::unique_ptrobjects to guarantee that only one instance exists.The other three literal arrays (
imageDiskList,diskSwap, andDriveshold simple pointers into the drive manager'sunique_ptr->get()content. This removes all management responsibility from the arrays.Unfortunately this is a large commit because it's essentially all-or-nothing, as once the data types are switched, all the affected code needs to be switched, too.