Add macOS disk resize support#678
Conversation
balcsida
force-pushed
the
feat/macos-disk-resize-compact
branch
2 times, most recently
from
f5ac0a5 to
2cfd5fe
Compare
balcsida
force-pushed
the
feat/macos-disk-resize-compact
branch
from
2cfd5fe to
5e9a160
Compare
insidegui
left a comment
insidegui
left a comment
There was a problem hiding this comment.
Added some comments about the code, mostly just nitpicks.
I couldn't test it yet though, as I couldn't figure out how to access the disk resizing feature. At first I thought the resizing arrow icon was a button, but it's just an indicator, and the ellipsis button that gives access to the disk settings remains disabled for the boot disk, even when it indicates that it's resizable.
| public var displayName: String { | ||
| switch self { | ||
| case .raw: | ||
| return "Raw Image" | ||
| case .dmg: | ||
| return "Disk Image (DMG)" | ||
| case .sparse: | ||
| return "Sparse Image" | ||
| case .asif: | ||
| return "Apple Sparse Image Format (ASIF)" | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Nitpick: reformat this to not use an explicit return. There's probably older code in the repo that still does it, but this is more idiomatic in modern Swift:
public var displayName: String {
switch self {
case .raw: "Raw Image"
case .dmg: "Disk Image (DMG)"
case .sparse: "Sparse Image"
case .asif: "Apple Sparse Image Format (ASIF)"
}
}| public var canBeResized: Bool { | ||
| switch format { | ||
| case .raw, .sparse: | ||
| return true | ||
| case .dmg, .asif: | ||
| return false | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Same nitpick about the explicit return :)
| return current | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Since this is an extension that's all about disk resizing, let's move it into a separate file
| if case let VBDiskResizeError.apfsVolumesLocked(container) = error { | ||
| let alert = NSAlert() | ||
| alert.messageText = "Unlock FileVault to Finish Resizing" | ||
| alert.informativeText = "VirtualBuddy enlarged the disk image, but the APFS container \(container) is still locked. Start the guest, sign in to unlock FileVault, then use Disk Utility (or run 'diskutil apfs resizeContainer disk0s2 0') inside the guest to claim the newly added space." |
There was a problem hiding this comment.
Not requesting a change here, just noting that this is probably something we can make the VirtualBuddyGuest app do automatically in a future development 🤔
| alert.runModal() | ||
| } | ||
| // Log resize errors but don't fail VM start | ||
| NSLog("Warning: Failed to resize disk images: \(error)") |
There was a problem hiding this comment.
A couple of things:
1 - We don't use NSLog; use a Logger instance if logging is needed
2 - Shouldn't the user be told about the error in the UI, even if it doesn't prevent the VM from continuing startup?
| import VirtualCore | ||
|
|
||
| struct ManagedDiskImageEditor: View { | ||
| @EnvironmentObject var viewModel: VMConfigurationViewModel |
There was a problem hiding this comment.
It doesn't look like the view is observing any property of the view model, it's using it to call checkFileVaultForDiskImage on the underlying VBVirtualMachine. In general we should try to avoid having the isolated configuration UI components depend on the configuration view model directly, so in this case this property could probably be a let virtualMachine: VBVirtualMachine.
| } | ||
| .onChange(of: image) { _, newValue in | ||
| onSave(newValue) | ||
| if isExistingDiskImage && canResize && newValue.size != minimumSize { |
There was a problem hiding this comment.
Don't do this right now, but add a // TODO: comment here to extract this slider with confirmation behavior into a separate component 🤓
| if canResize { | ||
| return "Boot disk can be expanded, but not shrunk. Choose your size carefully." | ||
| } else { | ||
| return "Be sure to reserve enough space, since it won't be possible to change the size of the disk later." | ||
| } | ||
| } else { | ||
| return "It's not possible to change the size of an existing storage device." | ||
| if canResize { | ||
| return "This disk can be expanded to a larger size, but cannot be shrunk." | ||
| } else { | ||
| return "It's not possible to change the size of an existing storage device." | ||
| } |
There was a problem hiding this comment.
Same nitpick about the explicit return
2 participants
What the resize feature does is that when a guest VM starts the next time, before the guest OS boots, VirtualBuddy inspects the guest disk image by temporarily attaching it on the host with
hdiutiland reading the partition layout. If it finds an APFS-on-GPT layout, it treats it as a macOS guest disk and checks for locked APFS volumes / FileVault first:diskutilexpands the GPT layout and the APFS container to match. If APFS does not pick up the new ceiling on the first pass, which sometimes happens, the resizer applies a small shrink-and-grow nudge to force APFS to recompute the available space.