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.

Sign up for GitHub

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

gui: Watch vm state to terminate when it's stopped #154

Closed
wants to merge 2 commits into from
Closed

gui: Watch vm state to terminate when it's stopped #154

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6cfd2d4
gui-linux: Add reproducer for hardstop issue
cfergeau Nov 10, 2023
999f00a
gui: Watch vm state to terminate when it's stopped
cfergeau Nov 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions example/gui-linux/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,17 @@ func run(ctx context.Context) error {
}
}()

go func() {
if !vm.CanStop() {
log.Println("cannot stop vm forcefully")
return
}
time.Sleep(10 * time.Second)
log.Println("calling vm.Stop()")

vm.Stop()
}()

// cleanup is this function is useful when finished graphic application.
cleanup := func() {
for i := 1; vm.CanRequestStop(); i++ {
Expand Down
24 changes: 24 additions & 0 deletions virtualization_view.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,11 +165,30 @@ - (instancetype)init

@end

API_AVAILABLE(macos(12.0))
@interface VMStateObserver : NSObject
- (void)observeValueForKeyPath:(NSString *)keyPath ofObject:(id)object change:(NSDictionary *)change context:(void *)context;
@end

@implementation VMStateObserver
- (void)observeValueForKeyPath:(NSString *)keyPath ofObject:(id)object change:(NSDictionary *)change context:(void *)context;
{
if ([keyPath isEqualToString:@"state"]) {
int newState = (int)[change[NSKeyValueChangeNewKey] integerValue];
if (newState == VZVirtualMachineStateStopped || newState == VZVirtualMachineStateError) {
[NSApp performSelectorOnMainThread:@selector(terminate:) withObject:context waitUntilDone:NO];
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not sure why is not enough code lines at 228 and 234?

Copy link
Owner

Choose a reason for hiding this comment

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

Ah, maybe We don't need state for error...?

Copy link
Owner

@Code-Hex Code-Hex Jan 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think this API is better than observing state by self
https://developer.apple.com/documentation/virtualization/vzvirtualmachinedelegate/3656731-virtualmachine?language=objc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This API is already used in Code-Hex/vz

vz/virtualization_view.m

Lines 207 to 212 in 4f8ae6d

- (void)virtualMachine:(VZVirtualMachine *)virtualMachine didStopWithError:(NSError *)error
{
NSLog(@"VM %@ didStopWithError: %@", virtualMachine, error);
[NSApp performSelectorOnMainThread:@selector(terminate:) withObject:self waitUntilDone:NO];
}

but the log is never printed/the method is never called. The VM did not stop because of an error, maybe it is normal this is not called.

Copy link
Owner

@Code-Hex Code-Hex Jan 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Could you try this code with Xcode?
NSLog shows only on the Xcode console

I'm sorry my mistake. It uses Stderr

Copy link
Owner

Choose a reason for hiding this comment

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

So... current code is racing with this API and new observing code that you added.

I think better fix:

  1. Remove code in didStopWithError
  2. Add code to release observer when dealloc view

And I'm not sure call remove observer in observe handler. (I think this is anti pattern) So it's good calling together with method to release observer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestions, I'll take a look!

Copy link
Contributor Author

@cfergeau cfergeau Jan 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure why is not enough code lines at 228 and 234?

(started typing this before your last round of comments, posting it as it adds a bit of explanations)

If we call Stop from the host:

  • the guest did not stop the virtual machine, so guestDidStopVirtualMachine is not called
  • there was no error, so didStopWithError is not called

[object removeObserver:self forKeyPath:@"state"];
}
}
}
@end

@implementation AppDelegate {
VZVirtualMachine *_virtualMachine;
VZVirtualMachineView *_virtualMachineView;
CGFloat _windowWidth;
CGFloat _windowHeight;
VMStateObserver *_observer;
}

- (instancetype)initWithVirtualMachine:(VZVirtualMachine *)virtualMachine
Expand All @@ -179,6 +198,11 @@ - (instancetype)initWithVirtualMachine:(VZVirtualMachine *)virtualMachine
self = [super init];
_virtualMachine = virtualMachine;
[_virtualMachine setDelegate:self];
_observer = [[VMStateObserver alloc] init];
Copy link
Owner

Choose a reason for hiding this comment

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

Mmm, when we have chance to release this object...?

[virtualMachine addObserver:_observer
forKeyPath:@"state"
options:NSKeyValueObservingOptionNew
context:(void *)self];

// Setup virtual machine view configs
VZVirtualMachineView *view = [[[VZVirtualMachineView alloc] init] autorelease];
Expand Down
Loading