[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [virt-tools-list] [virt-manager][PATCH v2 2/2] Add delete VM option in console viewer.



Thanks for the patches! General idea is fine, some comments below.

On 06/05/2013 03:40 PM, lagarcia linux vnet ibm com wrote:
> From: Leonardo Garcia <lagarcia br ibm com>
> 
> ---
>  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.

> 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.

>          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 ?

>      def _do_conn_changed(self, conn):
>          if (conn.get_state() == conn.STATE_ACTIVE or
> @@ -373,6 +377,10 @@ class vmmEngine(vmmGObject):
>              self.windowMigrate.cleanup()
>              self.windowMigrate = None
>  
> +        if self.delete_dialog:
> +            self.delete_dialog.cleanup()
> +            self.delete_dialog = None
> +
>          # Do this last, so any manually 'disconnected' signals
>          # take precedence over cleanup signal removal
>          for uri in self.conns:
> @@ -594,6 +602,7 @@ class vmmEngine(vmmGObject):
>          obj.connect("action-exit-app", self.exit_app)
>          obj.connect("action-view-manager", self._do_show_manager)
>          obj.connect("action-migrate-domain", self._do_show_migrate)
> +        obj.connect("action-delete-domain", self._do_delete_domain)
>          obj.connect("action-clone-domain", self._do_show_clone)
>          obj.connect("details-opened", self.increment_window_counter)
>          obj.connect("details-closed", self.decrement_window_counter)
> @@ -984,3 +993,38 @@ class vmmEngine(vmmGObject):
>          logging.debug("Resetting vm '%s'", vm.get_name())
>          vmmAsyncJob.simple_async_noshow(vm.reset, [], src,
>                                          _("Error resetting domain"))
> +
> +    def _do_delete_domain(self, src, uri, uuid):
> +        conn = self._lookup_conn(uri)
> +        vm = conn.get_vm(uuid)
> +        details_dialog = self._get_details_dialog(uri, uuid)
> +
> +        if vm.is_active():
> +            if not util.chkbox_helper(src, self.config.get_confirm_delrunningvm,
> +                self.config.set_confirm_delrunningvm,
> +                text1=_("Are you sure you want to force poweroff '%s'?" %
> +                        vm.get_name()),
> +                text2=_("In order to delete a running VM, you first need to power "
> +                        "it off. This will immediately power off the VM without "
> +                        "shutting down the OS and may cause data loss.")):
> +                return
> +
> +            logging.debug("Forced power off of vm '%s in order to proceed with "
> +                          "its deletion'", vm.get_name())
> +            def tmpcb(job, *args, **kwargs):
> +                ignore = job
> +                vm.destroy()
> +            docb = tmpcb
> +
> +            asyncjob = vmmAsyncJob(docb, [], _("Forcing VM Power off"),
> +                _("Powering off the VM in order to proceed with its deletion."),
> +                details_dialog.topwin, async=False, show_progress=True)
> +            error, details = asyncjob.run()
> +            if error is not None:
> +                error = _("Error shutting down domain") + ": " + error
> +                src.err.show_err(error, details=details)
> +                return
> +
> +        if not self.delete_dialog:
> +            self.delete_dialog = vmmDeleteDialog()
> +        self.delete_dialog.show(vm, details_dialog.topwin)

This bit looks fine, but please also change manager.py to use this as well,
rather than instantiate its own delete dialog.

Thanks,
Cole


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]