| Message ID | 20191112135801.854-2-wainersm@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tests/acceptance: Use QEMUMachine on tests that expect failure | expand |
On Tue, Nov 12, 2019 at 08:58:00AM -0500, Wainer dos Santos Moschetta wrote: > The QEMUMachine VM has a monitor setup on which an QMP > connection is always attempted on _post_launch() (executed > by launch()). In case the QEMU process immediatly exits > then the qmp.accept() (used to establish the connection) stalls > until it reaches timeout and consequently an exception raises. > > That behavior is undesirable when, for instance, it needs to > gather information from the QEMU binary ($ qemu -cpu list) or a > test which launches the VM expecting its failure. > > This patch adds the set_qmp_monitor() method to QEMUMachine that > allows turn off the creation of the monitor machinery on VM launch. > > Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com> > --- > python/qemu/machine.py | 68 ++++++++++++++++++++++++++++-------------- > 1 file changed, 45 insertions(+), 23 deletions(-) > > diff --git a/python/qemu/machine.py b/python/qemu/machine.py > index a4631d6934..04ee86e1ba 100644 > --- a/python/qemu/machine.py > +++ b/python/qemu/machine.py > @@ -104,6 +104,7 @@ class QEMUMachine(object): > self._events = [] > self._iolog = None > self._socket_scm_helper = socket_scm_helper > + self._qmp_set = True # Enable QMP monitor by default. > self._qmp = None > self._qemu_full_args = None > self._test_dir = test_dir > @@ -228,15 +229,16 @@ class QEMUMachine(object): > self._iolog = iolog.read() > > def _base_args(self): > - if isinstance(self._monitor_address, tuple): > - moncdev = "socket,id=mon,host=%s,port=%s" % ( > + args = ['-display', 'none', '-vga', 'none'] > + if self._qmp_set: > + if isinstance(self._monitor_address, tuple): > + moncdev = "socket,id=mon,host=%s,port=%s" % ( > self._monitor_address[0], > self._monitor_address[1]) > - else: > - moncdev = 'socket,id=mon,path=%s' % self._vm_monitor > - args = ['-chardev', moncdev, > - '-mon', 'chardev=mon,mode=control', > - '-display', 'none', '-vga', 'none'] > + else: > + moncdev = 'socket,id=mon,path=%s' % self._vm_monitor > + args.extend(['-chardev', moncdev, '-mon', > + 'chardev=mon,mode=control']) > if self._machine is not None: > args.extend(['-machine', self._machine]) > if self._console_set: > @@ -255,20 +257,21 @@ class QEMUMachine(object): > > def _pre_launch(self): > self._temp_dir = tempfile.mkdtemp(dir=self._test_dir) > - if self._monitor_address is not None: > - self._vm_monitor = self._monitor_address > - else: > - self._vm_monitor = os.path.join(self._sock_dir, > - self._name + "-monitor.sock") > - self._remove_files.append(self._vm_monitor) > self._qemu_log_path = os.path.join(self._temp_dir, self._name + ".log") > self._qemu_log_file = open(self._qemu_log_path, 'wb') > > - self._qmp = qmp.QEMUMonitorProtocol(self._vm_monitor, > - server=True) > + if self._qmp_set: > + if self._monitor_address is not None: > + self._vm_monitor = self._monitor_address > + else: > + self._vm_monitor = os.path.join(self._sock_dir, > + self._name + "-monitor.sock") > + self._remove_files.append(self._vm_monitor) > + self._qmp = qmp.QEMUMonitorProtocol(self._vm_monitor, server=True) > > def _post_launch(self): > - self._qmp.accept() > + if self._qmp: > + self._qmp.accept() > > def _post_shutdown(self): > if self._qemu_log_file is not None: > @@ -330,7 +333,8 @@ class QEMUMachine(object): > Wait for the VM to power off > """ > self._popen.wait() > - self._qmp.close() > + if self._qmp: > + self._qmp.close() > self._load_io_log() > self._post_shutdown() > > @@ -346,12 +350,13 @@ class QEMUMachine(object): > self._console_socket = None > > if self.is_running(): > - try: > - if not has_quit: > - self._qmp.cmd('quit') > - self._qmp.close() > - except: > - self._popen.kill() > + if self._qmp: > + try: > + if not has_quit: > + self._qmp.cmd('quit') > + self._qmp.close() > + except: > + self._popen.kill() > self._popen.wait() > > self._load_io_log() > @@ -368,6 +373,23 @@ class QEMUMachine(object): > > self._launched = False > > + def set_qmp_monitor(self, disabled=False, monitor_address=None): > + """ > + Set the QMP monitor. > + > + @param disabled: if True, qmp monitor options will be removed from the > + base arguments of the resulting QEMU command line. I personally tend avoid double negatives as long as I'm aware of them. Wouldn't "enabled=True" be more straightforward? Just my personal preference though. > + @param monitor_address: address for the QMP monitor. Do you have a use case for passing the monitor address here too, in addition to also being available as a parameter to __init__()? In the next patch, I don't see it being used (or here for that matter). > + @note: call this function before launch(). > + """ > + if disabled: > + self._qmp_set = False > + self._qmp = None > + else: > + self._qmp_set = True > + if monitor_address: > + self._monitor_address = monitor_address > + > def qmp(self, cmd, conv_keys=True, **args): > """ > Invoke a QMP command and return the response dict > -- > 2.18.1 > Other than those comments, it LGTM. - Cleber.
On 12/10/19 3:17 AM, Cleber Rosa wrote: > On Tue, Nov 12, 2019 at 08:58:00AM -0500, Wainer dos Santos Moschetta wrote: >> The QEMUMachine VM has a monitor setup on which an QMP >> connection is always attempted on _post_launch() (executed >> by launch()). In case the QEMU process immediatly exits >> then the qmp.accept() (used to establish the connection) stalls >> until it reaches timeout and consequently an exception raises. >> >> That behavior is undesirable when, for instance, it needs to >> gather information from the QEMU binary ($ qemu -cpu list) or a >> test which launches the VM expecting its failure. >> >> This patch adds the set_qmp_monitor() method to QEMUMachine that >> allows turn off the creation of the monitor machinery on VM launch. >> >> Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com> >> --- >> python/qemu/machine.py | 68 ++++++++++++++++++++++++++++-------------- >> 1 file changed, 45 insertions(+), 23 deletions(-) >> >> diff --git a/python/qemu/machine.py b/python/qemu/machine.py >> index a4631d6934..04ee86e1ba 100644 >> --- a/python/qemu/machine.py >> +++ b/python/qemu/machine.py >> @@ -104,6 +104,7 @@ class QEMUMachine(object): >> self._events = [] >> self._iolog = None >> self._socket_scm_helper = socket_scm_helper >> + self._qmp_set = True # Enable QMP monitor by default. >> self._qmp = None >> self._qemu_full_args = None >> self._test_dir = test_dir >> @@ -228,15 +229,16 @@ class QEMUMachine(object): >> self._iolog = iolog.read() >> >> def _base_args(self): >> - if isinstance(self._monitor_address, tuple): >> - moncdev = "socket,id=mon,host=%s,port=%s" % ( >> + args = ['-display', 'none', '-vga', 'none'] >> + if self._qmp_set: >> + if isinstance(self._monitor_address, tuple): >> + moncdev = "socket,id=mon,host=%s,port=%s" % ( >> self._monitor_address[0], >> self._monitor_address[1]) >> - else: >> - moncdev = 'socket,id=mon,path=%s' % self._vm_monitor >> - args = ['-chardev', moncdev, >> - '-mon', 'chardev=mon,mode=control', >> - '-display', 'none', '-vga', 'none'] >> + else: >> + moncdev = 'socket,id=mon,path=%s' % self._vm_monitor >> + args.extend(['-chardev', moncdev, '-mon', >> + 'chardev=mon,mode=control']) >> if self._machine is not None: >> args.extend(['-machine', self._machine]) >> if self._console_set: >> @@ -255,20 +257,21 @@ class QEMUMachine(object): >> >> def _pre_launch(self): >> self._temp_dir = tempfile.mkdtemp(dir=self._test_dir) >> - if self._monitor_address is not None: >> - self._vm_monitor = self._monitor_address >> - else: >> - self._vm_monitor = os.path.join(self._sock_dir, >> - self._name + "-monitor.sock") >> - self._remove_files.append(self._vm_monitor) >> self._qemu_log_path = os.path.join(self._temp_dir, self._name + ".log") >> self._qemu_log_file = open(self._qemu_log_path, 'wb') >> >> - self._qmp = qmp.QEMUMonitorProtocol(self._vm_monitor, >> - server=True) >> + if self._qmp_set: >> + if self._monitor_address is not None: >> + self._vm_monitor = self._monitor_address >> + else: >> + self._vm_monitor = os.path.join(self._sock_dir, >> + self._name + "-monitor.sock") >> + self._remove_files.append(self._vm_monitor) >> + self._qmp = qmp.QEMUMonitorProtocol(self._vm_monitor, server=True) >> >> def _post_launch(self): >> - self._qmp.accept() >> + if self._qmp: >> + self._qmp.accept() >> >> def _post_shutdown(self): >> if self._qemu_log_file is not None: >> @@ -330,7 +333,8 @@ class QEMUMachine(object): >> Wait for the VM to power off >> """ >> self._popen.wait() >> - self._qmp.close() >> + if self._qmp: >> + self._qmp.close() >> self._load_io_log() >> self._post_shutdown() >> >> @@ -346,12 +350,13 @@ class QEMUMachine(object): >> self._console_socket = None >> >> if self.is_running(): >> - try: >> - if not has_quit: >> - self._qmp.cmd('quit') >> - self._qmp.close() >> - except: >> - self._popen.kill() >> + if self._qmp: >> + try: >> + if not has_quit: >> + self._qmp.cmd('quit') >> + self._qmp.close() >> + except: >> + self._popen.kill() >> self._popen.wait() >> >> self._load_io_log() >> @@ -368,6 +373,23 @@ class QEMUMachine(object): >> >> self._launched = False >> >> + def set_qmp_monitor(self, disabled=False, monitor_address=None): >> + """ >> + Set the QMP monitor. >> + >> + @param disabled: if True, qmp monitor options will be removed from the >> + base arguments of the resulting QEMU command line. > I personally tend avoid double negatives as long as I'm aware of them. > Wouldn't "enabled=True" be more straightforward? Just my personal > preference though. I don't have a strong opinion about double negatives. So I'm fine with use 'enabled' instead. Besides make you happier. ;) > >> + @param monitor_address: address for the QMP monitor. > Do you have a use case for passing the monitor address here too, in > addition to also being available as a parameter to __init__()? In the > next patch, I don't see it being used (or here for that matter). I thought it would be useful in case an acceptance test needs to set the monitor address. And because avocado_qemu transparently creates the QEMUMachine instance, the test doesn't have access to the constructor. But no, I don't have a concrete case. I'm just being overcareful, so I don't mind to remove that parameter. Good catch. Thanks! - Wainer > >> + @note: call this function before launch(). >> + """ >> + if disabled: >> + self._qmp_set = False >> + self._qmp = None >> + else: >> + self._qmp_set = True >> + if monitor_address: >> + self._monitor_address = monitor_address >> + >> def qmp(self, cmd, conv_keys=True, **args): >> """ >> Invoke a QMP command and return the response dict >> -- >> 2.18.1 >> > Other than those comments, it LGTM. > > - Cleber.
diff --git a/python/qemu/machine.py b/python/qemu/machine.py index a4631d6934..04ee86e1ba 100644 --- a/python/qemu/machine.py +++ b/python/qemu/machine.py @@ -104,6 +104,7 @@ class QEMUMachine(object): self._events = [] self._iolog = None self._socket_scm_helper = socket_scm_helper + self._qmp_set = True # Enable QMP monitor by default. self._qmp = None self._qemu_full_args = None self._test_dir = test_dir @@ -228,15 +229,16 @@ class QEMUMachine(object): self._iolog = iolog.read() def _base_args(self): - if isinstance(self._monitor_address, tuple): - moncdev = "socket,id=mon,host=%s,port=%s" % ( + args = ['-display', 'none', '-vga', 'none'] + if self._qmp_set: + if isinstance(self._monitor_address, tuple): + moncdev = "socket,id=mon,host=%s,port=%s" % ( self._monitor_address[0], self._monitor_address[1]) - else: - moncdev = 'socket,id=mon,path=%s' % self._vm_monitor - args = ['-chardev', moncdev, - '-mon', 'chardev=mon,mode=control', - '-display', 'none', '-vga', 'none'] + else: + moncdev = 'socket,id=mon,path=%s' % self._vm_monitor + args.extend(['-chardev', moncdev, '-mon', + 'chardev=mon,mode=control']) if self._machine is not None: args.extend(['-machine', self._machine]) if self._console_set: @@ -255,20 +257,21 @@ class QEMUMachine(object): def _pre_launch(self): self._temp_dir = tempfile.mkdtemp(dir=self._test_dir) - if self._monitor_address is not None: - self._vm_monitor = self._monitor_address - else: - self._vm_monitor = os.path.join(self._sock_dir, - self._name + "-monitor.sock") - self._remove_files.append(self._vm_monitor) self._qemu_log_path = os.path.join(self._temp_dir, self._name + ".log") self._qemu_log_file = open(self._qemu_log_path, 'wb') - self._qmp = qmp.QEMUMonitorProtocol(self._vm_monitor, - server=True) + if self._qmp_set: + if self._monitor_address is not None: + self._vm_monitor = self._monitor_address + else: + self._vm_monitor = os.path.join(self._sock_dir, + self._name + "-monitor.sock") + self._remove_files.append(self._vm_monitor) + self._qmp = qmp.QEMUMonitorProtocol(self._vm_monitor, server=True) def _post_launch(self): - self._qmp.accept() + if self._qmp: + self._qmp.accept() def _post_shutdown(self): if self._qemu_log_file is not None: @@ -330,7 +333,8 @@ class QEMUMachine(object): Wait for the VM to power off """ self._popen.wait() - self._qmp.close() + if self._qmp: + self._qmp.close() self._load_io_log() self._post_shutdown() @@ -346,12 +350,13 @@ class QEMUMachine(object): self._console_socket = None if self.is_running(): - try: - if not has_quit: - self._qmp.cmd('quit') - self._qmp.close() - except: - self._popen.kill() + if self._qmp: + try: + if not has_quit: + self._qmp.cmd('quit') + self._qmp.close() + except: + self._popen.kill() self._popen.wait() self._load_io_log() @@ -368,6 +373,23 @@ class QEMUMachine(object): self._launched = False + def set_qmp_monitor(self, disabled=False, monitor_address=None): + """ + Set the QMP monitor. + + @param disabled: if True, qmp monitor options will be removed from the + base arguments of the resulting QEMU command line. + @param monitor_address: address for the QMP monitor. + @note: call this function before launch(). + """ + if disabled: + self._qmp_set = False + self._qmp = None + else: + self._qmp_set = True + if monitor_address: + self._monitor_address = monitor_address + def qmp(self, cmd, conv_keys=True, **args): """ Invoke a QMP command and return the response dict
The QEMUMachine VM has a monitor setup on which an QMP connection is always attempted on _post_launch() (executed by launch()). In case the QEMU process immediatly exits then the qmp.accept() (used to establish the connection) stalls until it reaches timeout and consequently an exception raises. That behavior is undesirable when, for instance, it needs to gather information from the QEMU binary ($ qemu -cpu list) or a test which launches the VM expecting its failure. This patch adds the set_qmp_monitor() method to QEMUMachine that allows turn off the creation of the monitor machinery on VM launch. Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com> --- python/qemu/machine.py | 68 ++++++++++++++++++++++++++++-------------- 1 file changed, 45 insertions(+), 23 deletions(-)