GNOME Bugzilla – Bug 758840
powering off live cd connected through spice leads to two main windows
Last modified: 2018-01-11 10:30:52 UTC
I am probably trying corner cases but now I have a spice box connected locally to live VM. This spice box is opened in separated window and sudo poweroff is executed. This should lead in machine poweroff and removal and the window closure as well. Not sure if Boxes can recognize live cd connected via spice. latest master (11/30/2015)
I'm not sure what you mean. What are you expecting and what happens? As for you question, no Boxes can't possibly know what kind of VM it is that you are connected to through spice.
Ah, the issue is in summary. :) Yeah, the window should be closed when the connection is closed.
Created attachment 327043 [details] [review] machine: Change a disconnected machine's state The problem is that a remote machine's separate window isn't closed when disconnected. Set a disconnected machine's state to stopped, in order for its window to be scheduled for removal.
Review of attachment 327043 [details] [review]: ::: src/machine.vala @@ +211,3 @@ return; + state = MachineState.STOPPED; This is the generic Machine class and the problem you're solving is only with RemoteMachine so you can guess what I'm getting at. :)
Created attachment 327577 [details] [review] remote-machine: Set a closed spice machine to stopped The problem is that a remote machine's separate window isn't closed when the connection is closed. Mark the machine as stopped when the conenction closes, in order for its window to be scheduled for removal.
Review of attachment 327577 [details] [review]: short log is conflicting. The change does not seem specific to SPICE. Also wording is weird. How about "remote-machine: Stop machine on display getting closed"? "connection closed" -> "display disconnected". ::: src/remote-machine.vala @@ +44,3 @@ display.connect_it (); + + display.disconnected.connect ((failed) => { you also want to disconnect from the signal in appropriate places. @@ +46,3 @@ + display.disconnected.connect ((failed) => { + if (failed == false) { // In case of closed channel + state = MachineState.STOPPED; Why only stop in case of failed disconnection?
Review of attachment 327577 [details] [review]: ::: src/remote-machine.vala @@ +44,3 @@ display.connect_it (); + + display.disconnected.connect ((failed) => { As in a more appropriate location in the code ? I can't put it in the class' constructor or in the create_display method because the display object isn't initialised yet. Other methods seem irrelevant to me. Where I have put it, we connect the callback to the signal right after instantiating the display. @@ +46,3 @@ + display.disconnected.connect ((failed) => { + if (failed == false) { // In case of closed channel + state = MachineState.STOPPED; the signal's definition: public signal void disconnected (bool connection_failed); My handled case is actually the case where the connection didn't fail because of an error, but the channel closed (the case of this bug) Yeah, it would also make sense to stop the machine if the connection failed because of an error.
Review of attachment 327577 [details] [review]: short log is conflicting. The change does not seem specific to SPICE. Also wording is weird. How about "remote-machine: Stop machine on display getting closed"? "connection closed" -> "display disconnected". ::: src/remote-machine.vala @@ +44,3 @@ display.connect_it (); + + display.disconnected.connect ((failed) => { Yes but I'm talking about disconnecting the signal. @@ +46,3 @@ + display.disconnected.connect ((failed) => { + if (failed == false) { // In case of closed channel + state = MachineState.STOPPED; Ah ok, it's not everyday I see booleans being compared (which BTW you really really don't want to do) so misread. Also you don't want to display an error if display disconnection was normal (you might want to use debug in that case though).
Created attachment 327742 [details] [review] I may have stumbled upon another bug. After the remote-machine is stopped, if I try to connect to it again and a connection can't be established, the main window closes (normally, without errors). If a connection can be established, everything is normal. The main window shouldn't close when trying to connect to a stopped machine, so it seems to me as being a different issue, which deserves a separate patch ?
Created attachment 327780 [details] [review] remote-machine: Stop machine on display getting closed Stop the machine when the spice display disconnects because of its connection closing or because of a channel error.
Created attachment 327781 [details] [review] app-window: Disconnect signals from removed window Disconnect signal callbacks from windows which are being removed from the app's windows list.
Review of attachment 327780 [details] [review]: In description, you're still claiming this is specific to SPICE (in capital letters) while patch applies to both remote machines. ::: src/machine.vala @@ +66,3 @@ } + protected ulong disconnected_remote_id; Since you couldn't figure a place in subclass for disconnection, you move this internal state of a subclass (of which parent should not know anything about) to parent class? Not impressed. :( When you need to assume something about the subclasses from a class, you make use of abstract and virtual APIs. If you look closer, this class is already handling the disconnect signal from display (and then disconnecting signal later). Why not simply modify that to our needs? ::: src/remote-machine.vala @@ +44,3 @@ display.connect_it (); + disconnected_remote_id = display.disconnected.connect ((connection_failed) => { + if (!connection_failed) // In case of closed channel This code does not know anything about channels, since it's not SPICE specific. Besides the comment isn't particularly useful. @@ +45,3 @@ + disconnected_remote_id = display.disconnected.connect ((connection_failed) => { + if (!connection_failed) // In case of closed channel + debug ("Connection to '%s' closed", name); I did ask for a debug in case of normal connection closed but I didn't ask to remove the warning for connection failed state.
Review of attachment 327781 [details] [review]: Are you sure, this change isn't all we need for this bug? ::: src/app-window.vala @@ +439,3 @@ + current_item.disconnect (machine_state_notify_id); + current_item.disconnect (machine_deleted_notify_id); + machine_state_notify_id = 0; current_item = null; would achieve the same thing I think.
Review of attachment 327780 [details] [review]: ::: src/remote-machine.vala @@ +45,3 @@ + disconnected_remote_id = display.disconnected.connect ((connection_failed) => { + if (!connection_failed) // In case of closed channel + debug ("Connection to '%s' closed", name); Actually the Machine class already handles the connection failed case and displays an error message in boxes. Is it enough ?
(In reply to Zeeshan Ali (Khattak) from comment #13) > Review of attachment 327781 [details] [review] [review]: > > Are you sure, this change isn't all we need for this bug? IMO the main window shouldn't close just because we have marked some machines as stopped (it is all we do in patch#1) . The context is the following: After the first patch applies, if a remote machines is stopped from a separate window and another random one is stopped, boxes exits. Also I think there is a need for a third patch, because VncDisplay doesn't send the disconnected signal, or any other appropriate signal for that matter.
Created attachment 328327 [details] [review] remote-machine: Stop machine on display getting closed The problem is that a remote machine's separate window isn't closed when its connection closes. Mark a remote machine as stopped when the connection closes, in order for its window to be scheduled for removal.
Created attachment 328328 [details] [review] app-window: Disconnect signals from removed window A window which was removed from the window list shouldn't still be able to send signals to boxes, so let's remove its reference.
Created attachment 328329 [details] [review] vnc-display: Send disconnect signal We need this because currently the remote machine has no way of checking if the vnc connection has closed.
Review of attachment 327780 [details] [review]: ::: src/remote-machine.vala @@ +45,3 @@ + disconnected_remote_id = display.disconnected.connect ((connection_failed) => { + if (!connection_failed) // In case of closed channel + debug ("Connection to '%s' closed", name); yeah but then yet another reason I'd think 10 more times before handling display.disconnected here an in Machine as well.
(In reply to Visarion Alexandru from comment #15) > (In reply to Zeeshan Ali (Khattak) from comment #13) > > Review of attachment 327781 [details] [review] [review] [review]: > > > > Are you sure, this change isn't all we need for this bug? > > IMO the main window shouldn't close just because we have marked some > machines as stopped (it is all we do in patch#1) > . > The context is the following: > After the first patch applies, if a remote machines is stopped from a > separate window and another random one is stopped, boxes exits. I'm not sure I follow. We should only be closing the window on machine getting stopped if machine is running in foreground of the window in question and not the last window. BTW, you didn't reply to my review in comment#12, especially this part: "If you look closer, this class is already handling the disconnect signal from display (and then disconnecting signal later). Why not simply modify that to our needs?" Do I assume you agree and hence acted on my suggestion?
Review of attachment 327780 [details] [review]: ::: src/remote-machine.vala @@ +45,3 @@ + disconnected_remote_id = display.disconnected.connect ((connection_failed) => { + if (!connection_failed) // In case of closed channel + debug ("Connection to '%s' closed", name); A solution could be to print the "connection failed" warning in the Machine class ( could be useful for libvirt machines also) and only handle the display.disconnected in a remote machine to properly change its state. The display.disconnected signal is the only sign that notifies us that we should close a remote-machine. If we aren't going to handle it in the RemoteMachine, we either stop the remote-machine from the Machine class or create another signal to notify the RemoteMachine that it must stop.
Review of attachment 327780 [details] [review]: ::: src/machine.vala @@ +66,3 @@ } + protected ulong disconnected_remote_id; That was my initial idea, actually. But, as you said, isn't the problem specific to remote machines and should be fixed in the RemoteMachine class ? Or should we check in the Machine class if the current machine is actually a remote machine and only then stop it, if necessary?
(In reply to Zeeshan Ali (Khattak) from comment #20) > (In reply to Visarion Alexandru from comment #15) > > (In reply to Zeeshan Ali (Khattak) from comment #13) > > > Review of attachment 327781 [details] [review] [review] [review] [review]: > > > > > > Are you sure, this change isn't all we need for this bug? > > > > IMO the main window shouldn't close just because we have marked some > > machines as stopped (it is all we do in patch#1) > > . > > The context is the following: > > After the first patch applies, if a remote machines is stopped from a > > separate window and another random one is stopped, boxes exits. > > I'm not sure I follow. We should only be closing the window on machine > getting stopped if machine is running in foreground of the window in > question and not the last window. Yeah, but if I apply my change to the Remote Machine ( stop it if its display disconnects), the very last window also closes on machine getting stopped, in some cases.
Review of attachment 327780 [details] [review]: ::: src/machine.vala @@ +66,3 @@ } + protected ulong disconnected_remote_id; Why do we need to check if it's RemoteMachine? ::: src/remote-machine.vala @@ +45,3 @@ + disconnected_remote_id = display.disconnected.connect ((connection_failed) => { + if (!connection_failed) // In case of closed channel + debug ("Connection to '%s' closed", name); What I'm wondering is that why you prefer (or even considering) the former solution, now that you know that Machine listens to display.disconnected already?
(In reply to Visarion Alexandru from comment #23) > (In reply to Zeeshan Ali (Khattak) from comment #20) > > (In reply to Visarion Alexandru from comment #15) > > > (In reply to Zeeshan Ali (Khattak) from comment #13) > > > > Review of attachment 327781 [details] [review] [review] [review] [review] [review]: > > > > > > > > Are you sure, this change isn't all we need for this bug? > > > > > > IMO the main window shouldn't close just because we have marked some > > > machines as stopped (it is all we do in patch#1) > > > . > > > The context is the following: > > > After the first patch applies, if a remote machines is stopped from a > > > separate window and another random one is stopped, boxes exits. > > > > I'm not sure I follow. We should only be closing the window on machine > > getting stopped if machine is running in foreground of the window in > > question and not the last window. > > > Yeah, but if I apply my change to the Remote Machine ( stop it if its > display disconnects), the very last window also closes on machine getting > stopped, in some cases. You mean, you have two windows and on display disconnection, both windows close? That would be a bug then and we need to solve it. (Review link for inline replies please).
Review of attachment 327780 [details] [review]: ::: src/machine.vala @@ +66,3 @@ } + protected ulong disconnected_remote_id; Although it still qualifies as intended behaviour, is it ok for the patch to be applied to non-remote Machines also ? ::: src/remote-machine.vala @@ +45,3 @@ + disconnected_remote_id = display.disconnected.connect ((connection_failed) => { + if (!connection_failed) // In case of closed channel + debug ("Connection to '%s' closed", name); I suppose that the former solution you are talking about is the one with the Remote Machine handling the display.disconnected signal ? If so, it is because my first patch only handled the display.disconnected in Machine class and it wasn't specific enough (the problem we are solving is only with remote machines).
Review of attachment 327780 [details] [review]: ::: src/remote-machine.vala @@ +45,3 @@ + disconnected_remote_id = display.disconnected.connect ((connection_failed) => { + if (!connection_failed) // In case of closed channel + debug ("Connection to '%s' closed", name); 1. Just because the problem is only specific to RemoteMachine, does not mean the solution must also be specific to RemoteMachine. 2. Display can get disconnected for any machine so I'm not sure this issue is specific to RemoteMachine only.
Created attachment 328660 [details] [review] machine: Stop machine on display getting closed The problem is that in some cases, a machine's separate window isn't closed when its connection closes. Let's mark a machine as stopped when its connection closes, in order for its window to be scheduled for removal.
Created attachment 328673 [details] [review] machine: Stop machine on display getting closed The problem is that in some cases, a machine's separate window isn't closed when its connection closes. Let's mark a machine as stopped when its connection closes, in order for its window to be scheduled for removal.
Review of attachment 328673 [details] [review]: * Did you test this for remote and local cases? * "it's connection closes" -> "display disconnects". Same in the next para. That's the change, and then you can add why it's more problematic for remote machines. ::: src/machine.vala @@ +212,3 @@ + if (!failed) + debug ("Connection to '%s' closed", name); Unrelated change, to this patch.
(In reply to Zeeshan Ali (Khattak) from comment #25) > (In reply to Visarion Alexandru from comment #23) > > (In reply to Zeeshan Ali (Khattak) from comment #20) > > > (In reply to Visarion Alexandru from comment #15) > > > > (In reply to Zeeshan Ali (Khattak) from comment #13) > > > > > Review of attachment 327781 [details] [review] [review] [review] [review] [review] [review]: > > > > > > > > > > Are you sure, this change isn't all we need for this bug? > > > > > > > > IMO the main window shouldn't close just because we have marked some > > > > machines as stopped (it is all we do in patch#1) > > > > . > > > > The context is the following: > > > > After the first patch applies, if a remote machines is stopped from a > > > > separate window and another random one is stopped, boxes exits. > > > > > > I'm not sure I follow. We should only be closing the window on machine > > > getting stopped if machine is running in foreground of the window in > > > question and not the last window. > > > > > > Yeah, but if I apply my change to the Remote Machine ( stop it if its > > display disconnects), the very last window also closes on machine getting > > stopped, in some cases. > > You mean, you have two windows and on display disconnection, both windows > close? That would be a bug then and we need to solve it. You never answered my question here.
Review of attachment 328328 [details] [review]: When referring to Boxes the software itself, please capitalize the first letter. ::: src/app-window.vala @@ +437,3 @@ machine.schedule_autosave (); + + current_item = null; In shortlog you are talking about disconnecting signals and in description about removing window from list, but you are only removing ref to current_item. I understand *if* those are in-directly connected but first thing commit log needs to do is to summarize the exact change and then (in description only) you can describe the implications/benefits/etc.
Review of attachment 328329 [details] [review]: * Tiny nitpick on shortlog: "Emit" is better verb for "Send". * shortlog: "disconnect" -> "disconnected". * "remote machine" -> "RemoteMachine" here cause you are explicitly talking about the class, not the remote machines that it let's us connect to. * vnc -> VNC ::: src/vnc-display.vala @@ +35,3 @@ }); display.vnc_disconnected.connect (() => { + disconnected (true); Have you made sure this does not get called on normal disconnection? I'm asking cause you are passing 'true' to 'connection_failed' param.
Review of attachment 328673 [details] [review]: ::: src/machine.vala @@ +212,3 @@ + if (!failed) + debug ("Connection to '%s' closed", name); Should I still display this debug message in another patch or not at all? . When failed is false, the connection didn't fail and it is the exact scenario of this bug.
> > > > > (In reply to Zeeshan Ali (Khattak) from comment #13) > > > > > > Review of attachment 327781 [details] [review]: > > You mean, you have two windows and on display disconnection, both windows > > close? That would be a bug then and we need to solve it. > > You never answered my question here Sorry, I thought it was rhetorical. Yes, it is exactly what happens.
Review of attachment 328329 [details] [review]: ::: src/vnc-display.vala @@ +35,3 @@ }); display.vnc_disconnected.connect (() => { + disconnected (true); This callback can be called in both cases, normal and abnormal. The only abnormal case we can know for sure about is the one with the failed authentification, because it has its own signal. I'll try to dig deeper and see if I can find more error signaling from VNC.
(In reply to Zeeshan Ali (Khattak) from comment #30) > Review of attachment 328673 [details] [review] [review]: > > * Did you test this for remote and local cases? I couldn't reply inline. I have partially tested it locally. The LibvirtMachine is already being stopped when display disconnects normally, using GVir.Domain signals. But, how do I disconnect the display of a libvirt machine abnormally ? I couldn't come up with a test case.
Review of attachment 328673 [details] [review]: ::: src/machine.vala @@ +212,3 @@ + if (!failed) + debug ("Connection to '%s' closed", name); it might be related to the scenerio of this patch, but it's not part of this patch. The hunk is unrelated to the logical change this patch aims to bring.
(In reply to Visarion Alexandru from comment #35) > > > > > > (In reply to Zeeshan Ali (Khattak) from comment #13) > > > > > > > Review of attachment 327781 [details] [review] [review]: > > > You mean, you have two windows and on display disconnection, both windows > > > close? That would be a bug then and we need to solve it. > > > > You never answered my question here > Sorry, I thought it was rhetorical. > Yes, it is exactly what happens. Is that bug independent of this one? If so, please file a new bug. If it's only happening due to this bug, we need to solve it as part of this one. If it's happening *after* applying your patches, you'll need to fix your patches. (In reply to Visarion Alexandru from comment #37) > (In reply to Zeeshan Ali (Khattak) from comment #30) > > Review of attachment 328673 [details] [review] [review] [review]: > > > > * Did you test this for remote and local cases? > I couldn't reply inline. You can't reply inline to non-inline comments. :) By "inline" comments, I mean the ones put on the code/patch lines in the Review tool. > I have partially tested it locally. Good. > The LibvirtMachine is already being > stopped when display disconnects normally, using GVir.Domain signals. > But, how do I disconnect the display of a libvirt machine abnormally ? I > couldn't come up with a test case. Isn't that what happens when you shutdown from the guest? If not, it's a rare scenario and it's not that important to test but we need to simulate it in the head.
Review of attachment 328329 [details] [review]: ::: src/vnc-display.vala @@ +35,3 @@ }); display.vnc_disconnected.connect (() => { + disconnected (true); That's a normal case, isn't it? If auth fails, we expect a disconnection. An abnormal case would be connection getting lost from the server.
(In reply to Zeeshan Ali (Khattak) from comment #39) > (In reply to Visarion Alexandru from comment #35). > Is that bug independent of this one? If so, please file a new bug. If it's > only happening due to this bug, we need to solve it as part of this one. If > it's happening *after* applying your patches, you'll need to fix your > patches. It's happening after applying my patch, but could well be caused by something not working as specified and my new patch wouldn't be the culprit. I'll dig deeper to see how come I can't reproduce the bug with libvirt machines, and maybe that will shed some light over it.
Created attachment 330377 [details] [review] machine: Stop machine on display getting disconnected The problem is that in some cases, a machine's separate window isn't closed when its display disconnects. Let's mark a machine as stopped when its display disconnects, in order for its window to be scheduled for removal.
Created attachment 330378 [details] [review] vnc-display: Send disconnect signal We need this because currently a remote machine has no way of knowing that its vnc display has disconnected.
Review of attachment 330378 [details] [review]: ::: src/vnc-display.vala @@ +35,3 @@ }); display.vnc_disconnected.connect (() => { + disconnected (false); Because of the way VNC.display is built, we currently can *not* detect an abnormal disconnection, so it will end up being managed as a normal one. From a logical point of view, the only disadvantage is that the user won't see the "Connection to <machine> failed" message.
Review of attachment 328328 [details] [review]: If bug https://bugzilla.gnome.org/show_bug.cgi?id=767214 is fixed, we probably won't need this patch anymore.
Review of attachment 330378 [details] [review]: ::: src/vnc-display.vala @@ +35,3 @@ }); display.vnc_disconnected.connect (() => { + disconnected (false); Are you answering some question of mine here? Can you please make inline comment on the obsolete patch so the conversation is in one place and easy to follow?
Review of attachment 330378 [details] [review]: ::: src/vnc-display.vala @@ +35,3 @@ }); display.vnc_disconnected.connect (() => { + disconnected (false); Not really.I was just pointing out a known issue, the fact that we can not detect an abnormal disconnection. I filed a bug on VNC-gtk. https://bugzilla.gnome.org/show_bug.cgi?id=768237
Review of attachment 330378 [details] [review]: ::: src/vnc-display.vala @@ +35,3 @@ }); display.vnc_disconnected.connect (() => { + disconnected (false); Ah ok. In that case, you want to add a note as comment in the code, with link to the bug.
Created attachment 330896 [details] [review] vnc-display: Send disconnect signal We need this because currently a remote machine has no way of knowing that its vnc display has disconnected. Known issue: we can not identify an abnormal disconnection, because of VNC-gtk's lack of such a feature.
These patches needs to be rebased on git master now that I pushed your patches from bug#744905.
Created attachment 331276 [details] [review] machine: Stop machine on display getting disconnected The problem is that in some cases, a machine's separate window isn't closed when its display disconnects. Let's mark a machine as stopped when its display disconnects, in order for its window to be scheduled for removal.
(In reply to Zeeshan Ali (Khattak) from comment #50) > These patches needs to be rebased on git master now that I pushed your > patches from bug#744905. Only the one affecting the machine needed to be rebased.
Review of attachment 328328 [details] [review]: ::: src/app-window.vala @@ +437,3 @@ machine.schedule_autosave (); + + current_item = null; This is still no addressed. If we don't need this patch, please make it obsolete.
Review of attachment 330896 [details] [review]: ack
Review of attachment 331276 [details] [review]: ack
Review of attachment 328328 [details] [review]: No longer needed, because it was reported as a separate issue https://bugzilla.gnome.org/show_bug.cgi?id=767214
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gnome-boxes/issues/77.