Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP : Run as snapshot for macos "Apple Virtualisation" #3792

Closed
wants to merge 23 commits into from

Conversation

PicoCreator
Copy link

@PicoCreator PicoCreator commented Mar 19, 2022

#2688

The QEMU counterpart is

#3067

The general idea is to do a APFS clone (Meaning it will only require the delta additional space, and make no changes on the original) on VM startup. ie. cp -c command. This allows us to mimic a qemu <disk> --snapshot like behaviour.

However, while I have coded in C, i'm pretty much a swift noob, and is learning on the fly here. And would want feedback regarding this pull request. The code done here, has not been tested either, as I could not get it built on my machine, due to the lack of an apple developer account (unless i missed something else)

The goal is to support the disposable VM workflow (not so much on the snapshot management)

Copy link
Contributor

@ktprograms ktprograms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, this is a great start!

Few notes (just from looking at the code):

  • In order to not complicate it with saving and loading the snapshot bookmark, call cleanupDriveSnapShot when the VM stops.
  • Does it make sense that enabling snapshot is done in the Drive settings for each drive? The QEMU version currently uses adds an item to the context (right click) menu in the main screen.
  • Looking more at the code, I don't really like this architecture. It doesn't seem right that resetDriveSnapShot is always called and internally it handles the logic based on whether each diskImage has runAsSnapshot set. Maybe if the function was renamed to something more descriptive ("reset" doesn't really make sense for what it's doing) it would be okay.
  • Use either snapshot or Snapshot in your function names, but not SnapShot, since snapshot is one word.
  • In swift, spaces are not put on the inside of parentheses.
// Good:
foo(bar: baz)
// Bad:
foo( bar: baz )

Configuration/UTMAppleConfiguration.swift Outdated Show resolved Hide resolved
Configuration/UTMAppleConfiguration.swift Outdated Show resolved Hide resolved
Configuration/UTMAppleConfiguration.swift Outdated Show resolved Hide resolved
Configuration/UTMAppleConfiguration.swift Outdated Show resolved Hide resolved
Managers/UTMAppleVirtualMachine.swift Outdated Show resolved Hide resolved
@PicoCreator
Copy link
Author

PicoCreator commented Mar 20, 2022

@ktprograms - hmm i think the flag can be done on either a drive by drive level - or on an entire VM level.

I was just generally following the QEMU API convention, where you can specify this on a drive level. The VM level option, is from what I understand, simply an alias to apply on all drives.

While there are use cases (like having a /tmp drive always running as a snapshot). Or inversely, having a storage drive persist for specific configs. Not sure how much demand this is for this flow.

Separately, doing so at the drive level makes it more obvious that the "--snapshot" does not apply to shared drive mounts.

Either option fulfils my goal of a "disposable VM".

For the rest, I will be applying your suggestions

@PicoCreator
Copy link
Author

PicoCreator commented Mar 20, 2022

Also for my workflow ( from qubesOS ) - the disposable VM's is always reuse and rested for every run.
Once designated as the untrusted sandbox, Rarely does it run "not as disposable" - unless im doing so just for updates.

Right-Click, to run as --snapshot, is a flow where I can accidentally misclick on. While not end of the world (as I would have a cloned VM), will be annoying when doing malware analysis.

@ktprograms
Copy link
Contributor

Maybe the QEMU -snapshot flag internally applies to each drive individually, but it's a single flag for the entire VM from the CLI.

Correct me if I'm wrong, though.

@PicoCreator
Copy link
Author

PicoCreator commented Mar 21, 2022

For the drive options, it can be done with snapshot=on like the following

qemu 
     < other args >
     -drive id=<id>,if=<if>,format=<format>,file="<IMG_FILEPATH>",snapshot=on

It's terribly buried within the docs. But both options are available.

Docs reference : https://manpages.debian.org/jessie/qemu-system-x86/qemu-system-x86_64.1.en.html
Search for: snapshot=snapshot

@ktprograms
Copy link
Contributor

Ah, I see.

@osy what do you think about this? Should it be:

  • A single context menu option
  • A persistent single option to always run the VM as snapshot
  • Snapshot option per-drive

@PicoCreator
Copy link
Author

PicoCreator commented Mar 21, 2022

I vote for either of the last two =)

  • A persistent single option to always run the VM as snapshot
  • Snapshot option per-drive

@PicoCreator
Copy link
Author

PicoCreator commented Apr 8, 2022

@ktprograms - applied the suggested changes, and added toggle for both disk level and system level.

Note: because i can't build this on my laptop due to #3873 , please do a test build prior to merger >_<"

I do wish there was an easier way for me to build this + validate on my end.

@ktprograms
Copy link
Contributor

You can download the sysroot for master from GitHub Actions, see the MacDevelopment document for instructions.

Building the full sysroot is only necessary if you changed stuff with Qemu or the patches.

@PicoCreator
Copy link
Author

Got it built, with some buggy behaviour with the current master branch (cant seem to boot an apple VM with just the master branch) - However, when i revert to v3.1.5 - i am able to get new apple VM's working again.

Will be reapplying changes from v3.1.5 commit to avoid confusion with other changes (wip)


On another note, i noticed in the config file there seems to be a pattern of using isX as a property. Should I be using runAsSnapshot, or isRunAsSnapshot ?

@osy
Copy link
Contributor

osy commented Apr 8, 2022

I've switched to isX and hasX naming for bools to be consistent with apple libraries. So probably a good idea.

@PicoCreator
Copy link
Author

PicoCreator commented Apr 11, 2022

Iterating on a few things, one thing i notice, for some reason the packaging command does not seem to be complete while being unsigned for the following command.

./scripts/package_mac.sh unsigned "$(pwd)/build/UTM.xcarchive" "$(pwd)/build/"

Last few lines of the command output

/tmp/UTM.xcarchive/Products/Applications/UTM.app/Contents/XPCServices/QEMUHelper.xpc/Contents/MacOS/QEMULauncher.app/Contents/MacOS/QEMULauncher: replacing existing signature
/tmp/UTM.xcarchive/Products/Applications/UTM.app/Contents/XPCServices/QEMUHelper.xpc/Contents/MacOS/QEMUHelper: replacing existing signature
/tmp/UTM.xcarchive/Products/Applications/UTM.app/Contents/MacOS/UTM: replacing existing signature

So from what i see, it builds the UTM.app in the tmp drive, but never moves it back to the build directory provided - not sure if the script is broken, or if the docs is outdated. (the built .app works to be clear)

Anyway, been using the following as my general workaround / separate build script. And found it really useful when iterating on small changes. (in case anyone else finds it useful)

#!/bin/zsh

# Kill any existing UTM instance
killall UTM

# Packaging seems to be incomplete, and does not remove this file
# causes errors on rerun, hence we remove if it exists
rm -rf /tmp/signed/Applications/UTM.app

# Remove previous build 
rm -rf "$(pwd)/build/UTM.app"

# Build the UTM.xarchive file, exit on failure
./scripts/build_utm.sh -p macos -a arm64 -o "$(pwd)/build/UTM" || exit 1

# Self sign and convert the .xarchive to a .app
./scripts/package_mac.sh unsigned "$(pwd)/build/UTM.xcarchive" "$(pwd)/build/"

# For some reason the UTM.dmg is not building, so we copy out the .app from /tmp
cp -r /tmp/signed/Applications/UTM.app "$(pwd)/build"

# Run the app
open "$(pwd)/build/UTM.app"

@PicoCreator
Copy link
Author

Update it now works - moving this to the new PR, as this was redone from a known "working commit" instead : #3893

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants