qemu-devel
[Top][All Lists]
Advanced

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

Re: [Qemu-devel] [PATCH v6 6/7] qemu.py: cleanup and improve launch()/sh


From: Markus Armbruster
Subject: Re: [Qemu-devel] [PATCH v6 6/7] qemu.py: cleanup and improve launch()/shutdown()
Date: Tue, 15 Aug 2017 10:45:57 +0200
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/25.2 (gnu/linux)

Amador Pahim <address@hidden> writes:

> launch() is currently taking care of a number of flows, each one if its
> own exception treatment, depending on the VM state and the files
> creation state.
>
> This patch makes launch() more resilient, off-loading the core calls to
> the new _launch() and calling shutdown() if any exception is raised by
> _launch(), making sure VM will be terminated and cleaned up.
>
> Also, the pre_launch() was changed to make sure the files that will

Imperative mood, please: "change pre_launch() to make sure..."

"Also, ..." in a commit message is often a sign that the commit should
be split.  I'm not sure in this case, because I'm not yet sure I get
what it does.  Could also be a sign that it should be split :)

> be created are not present in the system before creating them and the
> post_shutdown() will now only remove files that were created by this
> instance.
>
> Signed-off-by: Amador Pahim <address@hidden>
> ---
>  scripts/qemu.py | 84 
> ++++++++++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 59 insertions(+), 25 deletions(-)
>
> diff --git a/scripts/qemu.py b/scripts/qemu.py
> index d313c6d4db..e9a3a96d13 100644
> --- a/scripts/qemu.py
> +++ b/scripts/qemu.py
> @@ -25,6 +25,10 @@ logging.basicConfig()
>  LOG = logging.getLogger(__name__)
>  
>  
> +class QEMULaunchError(Exception):
> +    pass
> +
> +
>  class QEMUMachine(object):
>      '''A QEMU VM'''
>  
> @@ -40,6 +44,7 @@ class QEMUMachine(object):
>              monitor_address = os.path.join(test_dir, name + "-monitor.sock")
>          self._monitor_address = monitor_address
>          self._qemu_log_path = os.path.join(test_dir, name + ".log")
> +        self._qemu_log_fd = None
>          self._popen = None
>          self._binary = binary
>          self._args = list(args) # Force copy args in case we modify them
> @@ -49,6 +54,8 @@ class QEMUMachine(object):
>          self._socket_scm_helper = socket_scm_helper
>          self._debug = debug
>          self._qemu_full_args = None
> +        self._created_files = []
> +        self._pending_shutdown = False
>  
>      # This can be used to add an unused monitor instance.
>      def add_monitor_telnet(self, ip, port):
> @@ -109,8 +116,9 @@ class QEMUMachine(object):
>          return self._popen.pid
>  
>      def _load_io_log(self):
> -        with open(self._qemu_log_path, "r") as fh:
> -            self._iolog = fh.read()
> +        if os.path.exists(self._qemu_log_path):
> +            with open(self._qemu_log_path, "r") as fh:
> +                self._iolog = fh.read()

The new conditional predicts whether open() will fail with ENOENT.
Trying to predict failure is generally a bad idea.  What if some other
process removes ._qemu_log_path between .exists() and open()?  You're
better off catching the exception from open().

>  
>      def _base_args(self):
>          if isinstance(self._monitor_address, tuple):
> @@ -124,40 +132,62 @@ class QEMUMachine(object):
>                  '-display', 'none', '-vga', 'none']
>  
>      def _pre_launch(self):
> -        self._qmp = qmp.qmp.QEMUMonitorProtocol(self._monitor_address, 
> server=True,
> +        if (not isinstance(self._monitor_address, tuple) and
> +                os.path.exists(self._monitor_address)):
> +            raise QEMULaunchError('File %s exists. Please remove it.' %
> +                                  self._monitor_address)

What if some other process creates ._monitor_address between .exists()
and the bind() in QEMUMonitorProtocol.__init__()?

See, .exists() is almost always wrong.

> +
> +        self._qmp = qmp.qmp.QEMUMonitorProtocol(self._monitor_address,
> +                                                server=True,
>                                                  debug=self._debug)
> +        if not isinstance(self._monitor_address, tuple):
> +            self._created_files.append(self._monitor_address)
> +
> +        if os.path.exists(self._qemu_log_path):
> +            raise QEMULaunchError('File %s exists. Please remove it.' %
> +                                  self._qemu_log_path)

Likewise.

> +
> +        self._qemu_log_fd = open(self._qemu_log_path, 'wb')
> +        self._created_files.append(self._qemu_log_path)
>  
>      def _post_launch(self):
>          self._qmp.accept()
>  
>      def _post_shutdown(self):
> -        if not isinstance(self._monitor_address, tuple):
> -            self._remove_if_exists(self._monitor_address)
> -        self._remove_if_exists(self._qemu_log_path)
> +        while self._created_files:
> +            self._remove_if_exists(self._created_files.pop())
>  
>      def launch(self):
>          '''Launch the VM and establish a QMP connection'''
> -        devnull = open(os.path.devnull, 'rb')
> -        qemulog = open(self._qemu_log_path, 'wb')
> +
> +        if self.is_running():
> +            raise QEMULaunchError('VM already running.')

Drop the period, please.

> +
> +        if self._pending_shutdown:
> +            raise QEMULaunchError('Shutdown after the previous launch '
> +                                  'is pending. Please call shutdown() '
> +                                  'before launching again.')

Single phrase describing the error, please.

> +
>          try:
> -            self._pre_launch()
>              self._qemu_full_args = None
>              self._qemu_full_args = (self._wrapper + [self._binary] +
>                                      self._base_args() + self._args)
> -            self._popen = subprocess.Popen(self._qemu_full_args,
> -                                           stdin=devnull,
> -                                           stdout=qemulog,
> -                                           stderr=subprocess.STDOUT,
> -                                           shell=False)
> -            self._post_launch()
> +            self._launch()
> +            self._pending_shutdown = True
>          except:
> -            if self.is_running():
> -                self._popen.kill()
> -                self._popen.wait()
> -            self._load_io_log()
> -            self._post_shutdown()
> +            self.shutdown()
>              raise
>  
> +    def _launch(self):
> +        self._pre_launch()
> +        devnull = open(os.path.devnull, 'rb')
> +        self._popen = subprocess.Popen(self._qemu_full_args,
> +                                       stdin=devnull,
> +                                       stdout=self._qemu_log_fd,
> +                                       stderr=subprocess.STDOUT,
> +                                       shell=False)
> +        self._post_launch()
> +
>      def shutdown(self):
>          '''Terminate the VM and clean up'''
>          if self.is_running():
> @@ -166,14 +196,18 @@ class QEMUMachine(object):
>                  self._qmp.close()
>              except:
>                  self._popen.kill()
> +            self._popen.wait()
>  
> -            exitcode = self._popen.wait()
> -            if exitcode < 0:
> -                LOG.error('qemu received signal %i:%s', -exitcode,
> +        if self._pending_shutdown:
> +            exit_code = self.exitcode()
> +            if exit_code is not None and exit_code < 0:
> +                LOG.error('qemu received signal %i: %s', -exit_code,
>                            ' Command: %r.' % ' '.join(self._qemu_full_args)
>                            if self._qemu_full_args else '')
> -            self._load_io_log()
> -            self._post_shutdown()
> +
> +        self._load_io_log()
> +        self._post_shutdown()
> +        self._pending_shutdown = False
>  
>      underscore_to_dash = string.maketrans('_', '-')
>      def qmp(self, cmd, conv_keys=True, **args):



reply via email to

[Prev in Thread] Current Thread [Next in Thread]