Add support for running VM as temporary snapshot #3067
Conversation
|
BTW some of this code (the parts with custom modifiers) should probably be rewritten when Swift 5.5 is available with it's |
|
@conath I think I'll need some help adding the Run as Snapshot option for older iOS versions of the app. I'm not sure how the UI should be implemented and where the code for it should be. |
ktprograms
force-pushed
the
command-temporary-snapshot
branch
from
28cbda4 to
e2b8058
Compare
|
Thanks for working on this! See also #2688. As a remark: I find the terms / language for this option a bit unusual (but of course that began with the qemu option name) and not very intuitive / understandable. Snapshot usually means something frozen, something one is keeping. But here (if I understood it correctly), it rather means to run the VM as a "disposable VM", so everything one changes while running it is disposed as soon as it is shutdown. Qubes OS calls this a "disposable VM". But also if one considers the perspective of a non-admin, non-developer user: they likely would not associate "run as snapshot" with "disposable" or "temporary / throw away". Also, hiding this behind an additional key to hold down is not very intuitive / discoverable - would anybody even find this without reading the docs (are there docs?)? |
|
Good point @ThomasWaldmann. There should definitely be some kind of explanation whenever this feature is activated, possibly an alert explaining what will happen, with the option to run normally and to not show it again. The running VM window should probably also gain an indicator that changes won't be saved. I'm not sure on the wording either, "Run without saving changes" could be a more verbose and more obvious name (I can't think of a concise way of phrasing it). Though if someone is looking for the QEMU feature "snapshot" then they wouldn't find it. In terms of UI, as ktprograms mentioned there is the context menu action so it's discoverable. Maybe a menu bar item could be added as well, if there isn't already one included with the PR. The menu bar has the advantage of being searchable from the Help menu. |
|
Maybe this feature could have similar UX to a web browser's incognito window where the history is deleted when the browser window is closed. There could be a little ghost icon in the toolbar whenever a VM is running in this "snapshot" mode. |
|
When using the Qemu Monitor, it's possible to commit changes to disk even if it was started as |
There was a problem hiding this comment.
Change not storing into UTMConfiguration and also address previous comments about visibility to users. Maybe to make things easier, we remove the Cmd+Click option? I don't think this will be a feature that will be widely used and therefore it seems weird to assign a hot key to it when other features do not have such assignment.
| @@ -38,6 +38,8 @@ | |||
| const NSString *const kUTMConfigIconCustomKey = @"IconCustom"; | |||
| const NSString *const kUTMConfigNotesKey = @"Notes"; | |||
|
|
|||
| const NSString *const kUTMConfigSnapshotKey = @"RunAsSnapshot"; | |||
There was a problem hiding this comment.
Don't add temporary (per launch) settings to UTMConfiguration. Check out UTMViewState and how we save things like displayScale.
There was a problem hiding this comment.
Actually I'm reading on and I take back this suggestion. Read the next comment.
Managers/UTMQemuSystem.m
Outdated
| @@ -645,6 +645,10 @@ - (void)argsFromConfiguration { | |||
| // fix windows time issues | |||
| [self pushArgv:@"-rtc"]; | |||
| [self pushArgv:@"base=localtime"]; | |||
|
|
|||
| if (self.configuration.runAsSnapshot) { | |||
There was a problem hiding this comment.
So actually, it may make more sense to add a new @ property to UTMQemuSystem for runAsSnapshot and to UTMVirtualMachine where the property in UTMVirtualMachine is called isNextRunAsSnapshot, which the set handler will set UTMQemuSystem's property.
There was a problem hiding this comment.
So where would the property in UTMVirtualMachine be set?
There was a problem hiding this comment.
And what would the UI to enable it be like?
There was a problem hiding this comment.
I mean in UTMVirtualMachine we add @property (nonatomic) BOOL isNextRunAsSnapshot; and then for - (void)setIsNextRunAsSnapshot it will propagate to UTMQemuSystem which has its own property as well. Then from the UI you set UTMVirtualMachine's property. Additionally, in start it should un-set isNextRunAsSnapshot.
There was a problem hiding this comment.
I think the context menu UI is good enough. I would remove the hot key.
There was a problem hiding this comment.
Agreed; this also makes it consistent with other VM systems. On some UIs though, there's the start button > with a drop-down button on press-and-hold that lets you select between Run, Run Snapshot, Run Shadow Copy.
Of course, the issue there is that then you get into shadow copy management, where you need the ability somewhere to manage/delete shadow copies.
As for UI labeling, Snapshot is definitely problematic, especially when most popular VMs use the term the way QEMU refers to shadow copies. Even the QEMU documentation has to differentiate between -snapshot and VM Snapshots: https://qemu.readthedocs.io/en/latest/system/images.html
On the plus side, that documentation now has a clear explanation of what VM snapshotting and -snapshot do, which might be useful to incorporate into UTM documentation as well. Of course, with no access to the monitor, there's currently no way to manage VM snapshots on UTM?
Mark me down as someone who would love the ability to use -snapshot, plus the ability to optionally merge the snapshot into the image at the end of the session. I do lots of demonstration sessions on various OSes, and would love the ability to automatically discard changes unless I explicitly choose to merge them. A dialog when closing the VM stating "This session has unsaved changes. Would you like to save them?" with UTM handling the response by doing nothing if the answer is No, or sending "-a s" to the serial console if the answer is Yes.
There was a problem hiding this comment.
@adespoton I plan to merge the PR as-is but your suggestions are good. Feel free to open a new issue with the suggestions once it's merged.
ktprograms
force-pushed
the
command-temporary-snapshot
branch
from
e2b8058 to
42fb253
Compare
|
@osy Did your requested changes (completely rewrote it against current master) |
#2688
I tried adding the
-snapshotoption in theQEMUsettings and it works, so I've changed the UI to use it.The
-snapshotoption is enabled when you click the Play button while holding thecommandkey on macOS, and in a long press context menu on iOS 14.I can change the UI if this design isn't wanted.
Also I'm not sure how to change the UI on legacy (11 - 13) iOS.