On 06/10/2013 05:29 PM, Leonardo Augusto Guimarães Garcia wrote:
> Thanks for the review! :)
>
> Comments below.
>
> On 06/08/2013 08:30 PM, Cole Robinson wrote:
>> Thanks for the patches! General idea is fine, some comments below.
>>
>> On 06/05/2013 03:40 PM, lagarcia@xxxxxxxxxxxxxxxxxx wrote:
>>> From: Leonardo Garcia <lagarcia@xxxxxxxxxx>
>>>
>>> ---
>>> ui/vmm-details.ui | 15 +++++++++++++++
>>> virtManager/baseclass.py | 5 +++--
>>> virtManager/details.py | 11 +++++++++--
>>> virtManager/engine.py | 46
>>> +++++++++++++++++++++++++++++++++++++++++++++-
>>> 4 files changed, 72 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/ui/vmm-details.ui b/ui/vmm-details.ui
>>> index ddb2a71..ea4b53e 100644
>>> --- a/ui/vmm-details.ui
>>> +++ b/ui/vmm-details.ui
>>> @@ -314,6 +314,21 @@
>>> </object>
>>> </child>
>>> <child>
>>> + <object class="GtkMenuItem" id="details-menu-delete">
>>> + <property name="visible">True</property>
>>> + <property name="can_focus">False</property>
>>> + <property name="label"
>>> translatable="yes">_Delete...</property>
>>> + <property name="use_underline">True</property>
>>> + <signal name="activate"
>>> handler="on_details_menu_delete_activate" swapped="no"/>
>>> + </object>
>>> + </child>
>>> + <child>
>>> + <object class="GtkSeparatorMenuItem" id="separator2">
>>> + <property name="visible">True</property>
>>> + <property name="can_focus">False</property>
>>> + </object>
>>> + </child>
>>> + <child>
>>> <object class="GtkMenuItem"
>>> id="details-menu-vm-screenshot">
>>> <property name="visible">True</property>
>>> <property name="can_focus">False</property>
>>> diff --git a/virtManager/baseclass.py b/virtManager/baseclass.py
>>> index 7bc7812..c3a093e 100644
>>> --- a/virtManager/baseclass.py
>>> +++ b/virtManager/baseclass.py
>>> @@ -194,8 +194,9 @@ class vmmGObjectUI(vmmGObject):
>>> self.close()
>>> vmmGObject.cleanup(self)
>>> self.builder = None
>>> - self.topwin.destroy()
>>> - self.topwin = None
>>> + if self.topwin:
>>> + self.topwin.destroy()
>>> + self.topwin = None
>>> self.uifile = None
>>> self.err = None
>>>
>> Hmm, I can see from the vmmGObjectUI code why this is needed but it shouldn't
>> be required in practice. I've pushed a cleanup now that should make this
>> unnecessary, so please drop this bit.
> I thought these pieces you commented about would require some more explanation.
>
> With the changes made in the --show* command line option, I am usually running
> virt-manager with only one window opened: the details window. If I close this
> window, the following sequence of events will happen:
>
> user action: close window -> call: vmmDetails.close -> emit: details-closed ->
> call: vmmEngine.decrement_window_counter --- as it is the last window --->
> call: vmmEngine.exit_app -> call: vmmEngine._cleanup -> call:
> vmmEngine.cleanup_conn -> call: vmmDetails.cleanup (definition in base class)
> -> call: vmmDetails.close
>
> I think it is easier to see this through the stacktrace:
>
> File "./virt-manager", line 306, in <module>
> main()
> File "./virt-manager", line 301, in main
> engine.application.run(None)
> File "/usr/lib64/python2.7/site-packages/gi/types.py", line 47, in function
> return info.invoke(*args, **kwargs)
> File "/home/laggarcia/src/git/virt-manager/virtManager/details.py", line
> 590, in close
> return self._close()
> File "/home/laggarcia/src/git/virt-manager/virtManager/details.py", line
> 615, in _close
> self.emit("details-closed")
> File "/home/laggarcia/src/git/virt-manager/virtManager/baseclass.py", line
> 157, in emit
> return GObject.GObject.emit(self, signal_name, *args)
> File "/home/laggarcia/src/git/virt-manager/virtManager/engine.py", line 336,
> in decrement_window_counter
> self.exit_app(src)
> File "/home/laggarcia/src/git/virt-manager/virtManager/engine.py", line 393,
> in exit_app
> self.cleanup()
> File "/home/laggarcia/src/git/virt-manager/virtManager/baseclass.py", line
> 66, in cleanup
> self._cleanup()
> File "/home/laggarcia/src/git/virt-manager/virtManager/engine.py", line 385,
> in _cleanup
> self.cleanup_conn(uri)
> File "/home/laggarcia/src/git/virt-manager/virtManager/engine.py", line 474,
> in cleanup_conn
> win.cleanup()
> File "/home/laggarcia/src/git/virt-manager/virtManager/baseclass.py", line
> 194, in cleanup
> self.close()
> File "/home/laggarcia/src/git/virt-manager/virtManager/details.py", line
> 590, in close
> return self._close()
>
> This sequence works fine as, eventhough vmmDetails.close is reentrant,
> vmmDetails.cleanup is called only once.
>
> However, when we have only the details window opened and we choose to delete
> the VM (the purpose of this patch proposal), something different happens:
>
> emit: vm-removed -> call: vmmEngine._do_vm_removed --- tries to cleanup the
> details window corresponding to the deleted VM ---> call vmmDetails.cleanup
> (definition in base class) -> call: vmmDetails.close -> emit: details-closed
> -> call: vmmEngine.decrement_window_counter --- as it is the last window --->
> call: vmmEngine.exit_app -> call: vmmEngine._cleanup -> call:
> vmmEngine.cleanup_conn -> call: vmmDetails.cleanup (definition in base class)
> -> call: vmmDetails.close
>
> Or, in the stack trace:
>
> File "./virt-manager", line 306, in <module>
> main()
> File "./virt-manager", line 301, in main
> engine.application.run(None)
> File "/usr/lib64/python2.7/site-packages/gi/types.py", line 47, in function
> return info.invoke(*args, **kwargs)
> File "/home/laggarcia/src/git/virt-manager/virtManager/connection.py", line
> 1330, in tick_send_signals
> self.emit("vm-removed", uuid)
> File "/home/laggarcia/src/git/virt-manager/virtManager/baseclass.py", line
> 157, in emit
> return GObject.GObject.emit(self, signal_name, *args)
> File "/home/laggarcia/src/git/virt-manager/virtManager/engine.py", line 243,
> in _do_vm_removed
> self.conns[hvuri]["windowDetails"][vmuuid].cleanup()
> File "/home/laggarcia/src/git/virt-manager/virtManager/baseclass.py", line
> 194, in cleanup
> self.close()
> File "/home/laggarcia/src/git/virt-manager/virtManager/details.py", line
> 590, in close
> return self._close()
> File "/home/laggarcia/src/git/virt-manager/virtManager/details.py", line
> 615, in _close
> self.emit("details-closed")
> File "/home/laggarcia/src/git/virt-manager/virtManager/baseclass.py", line
> 157, in emit
> return GObject.GObject.emit(self, signal_name, *args)
> File "/home/laggarcia/src/git/virt-manager/virtManager/engine.py", line 336,
> in decrement_window_counter
> self.exit_app(src)
> File "/home/laggarcia/src/git/virt-manager/virtManager/engine.py", line 393,
> in exit_app
> self.cleanup()
> File "/home/laggarcia/src/git/virt-manager/virtManager/baseclass.py", line
> 66, in cleanup
> self._cleanup()
> File "/home/laggarcia/src/git/virt-manager/virtManager/engine.py", line 385,
> in _cleanup
> self.cleanup_conn(uri)
> File "/home/laggarcia/src/git/virt-manager/virtManager/engine.py", line 474,
> in cleanup_conn
> win.cleanup()
> File "/home/laggarcia/src/git/virt-manager/virtManager/baseclass.py", line
> 194, in cleanup
> self.close()
> File "/home/laggarcia/src/git/virt-manager/virtManager/details.py", line
> 590, in close
> return self._close()
>
> The last call to vmmDetails.cleanup will destroy self.topwin. However, when
> the processing goes back to the first call to vmmDetails.cleanup, we will get
> an exception, as self.topwin has already been destroyed:
>
> 2013-06-10 16:56:55,719 (delete:74): Showing delete wizard
> 2013-06-10 16:57:00,644 (asyncjob:193): Creating async job for function
> cb=<bound method vmmDeleteDialog._async_delete of <vmmDeleteDialog object at
> 0x2fd1730 (virtManager+delete+vmmDeleteDialog at 0x321cf00)>>
> 2013-06-10 16:57:00,754 (delete:173): Threading off connection to delete vol.
> 2013-06-10 16:57:00,756 (delete:179): Deleting path:
> /var/lib/libvirt/images/Fedora18-test-clone.img
> 2013-06-10 16:57:00,927 (delete:187): Removing VM 'Fedora18-test-clone'
> 2013-06-10 16:57:02,326 (delete:83): Closing delete wizard
> 2013-06-10 16:57:02,600 (details:589): Closing VM details: <vmmDomain object
> at 0x235c870 (virtManager+domain+vmmDomain at 0x7f5894005240)>
> 2013-06-10 16:57:02,603 (engine:330): window counter decremented to 0
> 2013-06-10 16:57:02,618 (manager:211): Closing manager
> 2013-06-10 16:57:02,627 (delete:83): Closing delete wizard
> 2013-06-10 16:57:02,633 (details:589): Closing VM details: <vmmDomain object
> at 0x235c870 (virtManager+domain+vmmDomain at 0x7f5894005240)>
> 2013-06-10 16:57:02,781 (engine:408): Exiting app normally.
> 2013-06-10 16:57:02,782 (baseclass:68): Error cleaning up <vmmDetails object
> at 0x2514f50 (virtManager+details+vmmDetails at 0x2aa2320)>
> Traceback (most recent call last):
> File "/home/laggarcia/src/git/virt-manager/virtManager/baseclass.py", line
> 66, in cleanup
> self._cleanup()
> File "/home/laggarcia/src/git/virt-manager/virtManager/details.py", line
> 564, in _cleanup
> self.console.cleanup()
> AttributeError: 'NoneType' object has no attribute 'cleanup'
> 2013-06-10 16:57:02,785 (cliutils:87): Uncaught exception:
> Traceback (most recent call last):
> File "/home/laggarcia/src/git/virt-manager/virtManager/engine.py", line 243,
> in _do_vm_removed
> self.conns[hvuri]["windowDetails"][vmuuid].cleanup()
> File "/home/laggarcia/src/git/virt-manager/virtManager/baseclass.py", line
> 197, in cleanup
> self.topwin.destroy()
> AttributeError: 'NoneType' object has no attribute 'destroy'
>
> Traceback (most recent call last):
> File "/home/laggarcia/src/git/virt-manager/virtManager/engine.py", line 243,
> in _do_vm_removed
> self.conns[hvuri]["windowDetails"][vmuuid].cleanup()
> File "/home/laggarcia/src/git/virt-manager/virtManager/baseclass.py", line
> 197, in cleanup
> self.topwin.destroy()
> AttributeError: 'NoneType' object has no attribute 'destroy'
>
> The changes you made in vmmGObjectUI fixed this issue, but didn't fix the
> other two issues below.
>>
>>> diff --git a/virtManager/details.py b/virtManager/details.py
>>> index 8e927b1..1323232 100644
>>> --- a/virtManager/details.py
>>> +++ b/virtManager/details.py
>>> @@ -329,6 +329,7 @@ class vmmDetails(vmmGObjectUI):
>>> "action-exit-app": (GObject.SignalFlags.RUN_FIRST, None, []),
>>> "action-view-manager": (GObject.SignalFlags.RUN_FIRST, None, []),
>>> "action-migrate-domain": (GObject.SignalFlags.RUN_FIRST, None,
>>> [str, str]),
>>> + "action-delete-domain": (GObject.SignalFlags.RUN_FIRST, None,
>>> [str, str]),
>>> "action-clone-domain": (GObject.SignalFlags.RUN_FIRST, None,
>>> [str, str]),
>>> "details-closed": (GObject.SignalFlags.RUN_FIRST, None, []),
>>> "details-opened": (GObject.SignalFlags.RUN_FIRST, None, []),
>>> @@ -412,6 +413,7 @@ class vmmDetails(vmmGObjectUI):
>>> "on_details_menu_pause_activate": self.control_vm_pause,
>>> "on_details_menu_clone_activate": self.control_vm_clone,
>>> "on_details_menu_migrate_activate": self.control_vm_migrate,
>>> + "on_details_menu_delete_activate": self.control_vm_delete,
>>> "on_details_menu_screenshot_activate":
>>> self.control_vm_screenshot,
>>> "on_details_menu_view_toolbar_activate": self.toggle_toolbar,
>>> "on_details_menu_view_manager_activate": self.view_manager,
>>> @@ -559,8 +561,9 @@ class vmmDetails(vmmGObjectUI):
>>> for serial in self.serial_tabs:
>>> self._close_serial_tab(serial)
>>> - self.console.cleanup()
>>> - self.console = None
>>> + if self.console:
>>> + self.console.cleanup()
>>> + self.console = None
>>>
>> self.console should be unconditionally set, so I'm not sure why this is needed.
> If the previous patch (mine or yours) is applied to baseclass.py, we will hit
> another error:
>
> 2013-06-10 17:51:59,007 (engine:408): Exiting app normally.
> 2013-06-10 17:51:59,008 (baseclass:68): Error cleaning up <vmmDetails object
> at 0x330fe60 (virtManager+details+vmmDetails at 0x7f2218004620)>
> Traceback (most recent call last):
> File "/home/laggarcia/src/git/virt-manager/virtManager/baseclass.py", line
> 66, in cleanup
> self._cleanup()
> File "/home/laggarcia/src/git/virt-manager/virtManager/details.py", line
> 564, in _cleanup
> self.console.cleanup()
> AttributeError: 'NoneType' object has no attribute 'cleanup'
>
> The cause for this error is the same as the previous one: vmmDetails cleanup
> will call vmmDetails._cleanup at some point. The last call to this function
> will correctly execute self.console.cleanup(). However, the first call will
> generate above error. Hence, the need for this check.
>>
>>> self.vm = None
>>> self.conn = None
>>> @@ -1580,6 +1583,10 @@ class vmmDetails(vmmGObjectUI):
>>> self.emit("action-migrate-domain",
>>> self.vm.conn.get_uri(), self.vm.get_uuid())
>>> + def control_vm_delete(self, src_ignore):
>>> + self.emit("action-delete-domain",
>>> + self.vm.conn.get_uri(), self.vm.get_uuid())
>>> +
>>> def control_vm_screenshot(self, src_ignore):
>>> image = self.console.viewer.get_pixbuf()
>>> diff --git a/virtManager/engine.py b/virtManager/engine.py
>>> index 16ed552..7ab20e6 100644
>>> --- a/virtManager/engine.py
>>> +++ b/virtManager/engine.py
>>> @@ -48,6 +48,7 @@ from virtManager.create import vmmCreate
>>> from virtManager.host import vmmHost
>>> from virtManager.error import vmmErrorDialog
>>> from virtManager.systray import vmmSystray
>>> +from virtManager.delete import vmmDeleteDialog
>>> # Enable this to get a report of leaked objects on app shutdown
>>> # gtk3/pygobject has issues here as of Fedora 18
>>> @@ -95,6 +96,7 @@ class vmmEngine(vmmGObject):
>>> self.last_timeout = 0
>>> self.systray = None
>>> + self.delete_dialog = None
>>> self.application = Gtk.Application(
>>> application_id="com.redhat.virt-manager",
>>> flags=0)
>>> @@ -239,7 +241,9 @@ class vmmEngine(vmmGObject):
>>> return
>>> self.conns[hvuri]["windowDetails"][vmuuid].cleanup()
>>> - del(self.conns[hvuri]["windowDetails"][vmuuid])
>>> + if self.conns:
>>> + # The cleanup call above might end up emptying the conns
>>> dictionary
>>> + del(self.conns[hvuri]["windowDetails"][vmuuid])
>>>
>> How is this called after cleanup ?
> The cause here is the same as explained for the two patch hunks commented
> above. Even with the two previous patches, if I delete the VM while the
> details window is the only opened window, I got the following error:
>
> 2013-06-10 18:01:10,171 (engine:408): Exiting app normally.
> 2013-06-10 18:01:10,172 (cliutils:87): Uncaught exception:
> Traceback (most recent call last):
> File "/home/laggarcia/src/git/virt-manager/virtManager/engine.py", line 244,
> in _do_vm_removed
> del(self.conns[hvuri]["windowDetails"][vmuuid])
> KeyError: 'qemu+ssh://root@192.168.122.1/system'
>
> Traceback (most recent call last):
> File "/home/laggarcia/src/git/virt-manager/virtManager/engine.py", line 244,
> in _do_vm_removed
> del(self.conns[hvuri]["windowDetails"][vmuuid])
> KeyError: 'qemu+ssh://root@192.168.122.1/system'
>
> This happens because when vmmEngine._do_vm_removed tries to cleanup the
> details window corresponding to the deleted VM, it will end up calling
> vmmDetails.close, which will, in turn, call vmmEngine.exit_app, as the details
> window is the last one opened, (see my initial comment here). However,
> vmmEngine.exit_app calls vmmEngine._cleanup, which, in turns, has the
> following statement in its last line:
>
> self.conns = {}
>
> Hence, in the sequence of events shot by deleting a VM while the only window
> opened in virt-manager is the details window, vmmEngine.conns will be emptied
> before vmmEngine._do_vm_removed is able to delete the corresponding details
> window from the self.conns dictionary. That's why we need this additional check.
>
> The best way I found to deal with this sequence of events was to implement the
> simple checks I suggested above. However, I am open to other suggestions here
> as well.
Thanks for the thorough explanations! Indeed all these issues can be hit with
current code: have the last open window be a details window, and 'virsh
undefine' the VM behind virt-manager's back.
I pushed a patch upstream that defers vmmEngine.exit_app to an idle callback,
to let the vmmDetails cleanup bits finish and remove themselves from the
window tracking before we try and do a final cleanup pass. That should fix
these issues.
Thanks,
Cole
_______________________________________________
virt-tools-list mailing list
virt-tools-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/virt-tools-list
