| Message ID | 20170901112829.2571-7-apahim@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | scripts/qemu.py fixes and cleanups | expand |
On Fri, 09/01 13:28, Amador Pahim wrote: > To launch a VM, we need to create basically two files: the monitor > socket (if it's a UNIX socket) and the qemu log file. > > For the qemu log file, we currently just open the path, which will > create the file if it does not exist or overwrite the file if it does > exist. > > For the monitor socket, if it already exists, we are currently removing > it, even if it's not created by us. > > This patch moves to pre_launch() the responsibility to make sure we only > create files that are not pre-existent and to populate a list of > controlled files. This list will then be used as the reference of > files to remove during the cleanup (post_shutdown()). > > Signed-off-by: Amador Pahim <apahim@redhat.com> > --- > scripts/qemu.py | 30 +++++++++++++++++++++++------- > 1 file changed, 23 insertions(+), 7 deletions(-) > > diff --git a/scripts/qemu.py b/scripts/qemu.py > index 3ebe5ee0a4..c26e1412f9 100644 > --- a/scripts/qemu.py > +++ b/scripts/qemu.py > @@ -41,6 +41,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 > @@ -50,6 +51,7 @@ class QEMUMachine(object): > self._socket_scm_helper = socket_scm_helper > self._debug = debug > self._qemu_full_args = None > + self._created_files = [] > > # This can be used to add an unused monitor instance. > def add_monitor_telnet(self, ip, port): > @@ -128,30 +130,44 @@ class QEMUMachine(object): > '-display', 'none', '-vga', 'none'] > > def _pre_launch(self): > - self._qmp = qmp.qmp.QEMUMonitorProtocol(self._monitor_address, server=True, > - debug=self._debug) > + try: > + self._qmp = qmp.qmp.QEMUMonitorProtocol(self._monitor_address, > + server=True, > + debug=self._debug) > + except: > + raise What's the point of "except: raise"? It seems useless. > + else: > + if not isinstance(self._monitor_address, tuple): > + self._created_files.append(self._monitor_address) > + > + try: > + flags = os.O_CREAT | os.O_EXCL | os.O_WRONLY > + os.open(self._qemu_log_path, flags) Why change to os.open() instead of open()? > + except: > + raise > + else: > + self._created_files.append(self._qemu_log_path) > + self._qemu_log_fd = open(self._qemu_log_path, 'wb') > > 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''' > self._iolog = None > self._qemu_full_args = None > devnull = open(os.path.devnull, 'rb') > - qemulog = open(self._qemu_log_path, 'wb') > try: > self._pre_launch() > 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, > + stdout=self._qemu_log_fd, > stderr=subprocess.STDOUT, > shell=False) > self._post_launch() > -- > 2.13.5 > Fam
On Tue, Sep 5, 2017 at 5:18 AM, Fam Zheng <famz@redhat.com> wrote: > On Fri, 09/01 13:28, Amador Pahim wrote: >> To launch a VM, we need to create basically two files: the monitor >> socket (if it's a UNIX socket) and the qemu log file. >> >> For the qemu log file, we currently just open the path, which will >> create the file if it does not exist or overwrite the file if it does >> exist. >> >> For the monitor socket, if it already exists, we are currently removing >> it, even if it's not created by us. >> >> This patch moves to pre_launch() the responsibility to make sure we only >> create files that are not pre-existent and to populate a list of >> controlled files. This list will then be used as the reference of >> files to remove during the cleanup (post_shutdown()). >> >> Signed-off-by: Amador Pahim <apahim@redhat.com> >> --- >> scripts/qemu.py | 30 +++++++++++++++++++++++------- >> 1 file changed, 23 insertions(+), 7 deletions(-) >> >> diff --git a/scripts/qemu.py b/scripts/qemu.py >> index 3ebe5ee0a4..c26e1412f9 100644 >> --- a/scripts/qemu.py >> +++ b/scripts/qemu.py >> @@ -41,6 +41,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 >> @@ -50,6 +51,7 @@ class QEMUMachine(object): >> self._socket_scm_helper = socket_scm_helper >> self._debug = debug >> self._qemu_full_args = None >> + self._created_files = [] >> >> # This can be used to add an unused monitor instance. >> def add_monitor_telnet(self, ip, port): >> @@ -128,30 +130,44 @@ class QEMUMachine(object): >> '-display', 'none', '-vga', 'none'] >> >> def _pre_launch(self): >> - self._qmp = qmp.qmp.QEMUMonitorProtocol(self._monitor_address, server=True, >> - debug=self._debug) >> + try: >> + self._qmp = qmp.qmp.QEMUMonitorProtocol(self._monitor_address, >> + server=True, >> + debug=self._debug) >> + except: >> + raise > > What's the point of "except: raise"? It seems useless. The point is to execute the block in the else only when no exception happens. When some exception happens, I want to raise it without executing the else block. > >> + else: >> + if not isinstance(self._monitor_address, tuple): >> + self._created_files.append(self._monitor_address) >> + >> + try: >> + flags = os.O_CREAT | os.O_EXCL | os.O_WRONLY >> + os.open(self._qemu_log_path, flags) > > Why change to os.open() instead of open()? I want to create the file only if it does not exist. The open() flag 'x' is available only in python 3.3. For python <3.3, we need the os.open() to have that feature. > >> + except: >> + raise >> + else: >> + self._created_files.append(self._qemu_log_path) >> + self._qemu_log_fd = open(self._qemu_log_path, 'wb') >> >> 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''' >> self._iolog = None >> self._qemu_full_args = None >> devnull = open(os.path.devnull, 'rb') >> - qemulog = open(self._qemu_log_path, 'wb') >> try: >> self._pre_launch() >> 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, >> + stdout=self._qemu_log_fd, >> stderr=subprocess.STDOUT, >> shell=False) >> self._post_launch() >> -- >> 2.13.5 >> > > Fam
On Thu, Sep 14, 2017 at 09:38:13PM +0200, Amador Pahim wrote: > On Tue, Sep 5, 2017 at 5:18 AM, Fam Zheng <famz@redhat.com> wrote: > > On Fri, 09/01 13:28, Amador Pahim wrote: > >> To launch a VM, we need to create basically two files: the monitor > >> socket (if it's a UNIX socket) and the qemu log file. > >> > >> For the qemu log file, we currently just open the path, which will > >> create the file if it does not exist or overwrite the file if it does > >> exist. > >> > >> For the monitor socket, if it already exists, we are currently removing > >> it, even if it's not created by us. > >> > >> This patch moves to pre_launch() the responsibility to make sure we only > >> create files that are not pre-existent and to populate a list of > >> controlled files. This list will then be used as the reference of > >> files to remove during the cleanup (post_shutdown()). > >> > >> Signed-off-by: Amador Pahim <apahim@redhat.com> > >> --- > >> scripts/qemu.py | 30 +++++++++++++++++++++++------- > >> 1 file changed, 23 insertions(+), 7 deletions(-) > >> > >> diff --git a/scripts/qemu.py b/scripts/qemu.py > >> index 3ebe5ee0a4..c26e1412f9 100644 > >> --- a/scripts/qemu.py > >> +++ b/scripts/qemu.py > >> @@ -41,6 +41,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 > >> @@ -50,6 +51,7 @@ class QEMUMachine(object): > >> self._socket_scm_helper = socket_scm_helper > >> self._debug = debug > >> self._qemu_full_args = None > >> + self._created_files = [] > >> > >> # This can be used to add an unused monitor instance. > >> def add_monitor_telnet(self, ip, port): > >> @@ -128,30 +130,44 @@ class QEMUMachine(object): > >> '-display', 'none', '-vga', 'none'] > >> > >> def _pre_launch(self): > >> - self._qmp = qmp.qmp.QEMUMonitorProtocol(self._monitor_address, server=True, > >> - debug=self._debug) > >> + try: > >> + self._qmp = qmp.qmp.QEMUMonitorProtocol(self._monitor_address, > >> + server=True, > >> + debug=self._debug) > >> + except: > >> + raise > > > > What's the point of "except: raise"? It seems useless. > > The point is to execute the block in the else only when no exception > happens. When some exception happens, I want to raise it without > executing the else block. Isn't this exactly what Python does when an exception is raised with no "try" block? > > > > >> + else: > >> + if not isinstance(self._monitor_address, tuple): > >> + self._created_files.append(self._monitor_address) > >> + > >> + try: > >> + flags = os.O_CREAT | os.O_EXCL | os.O_WRONLY > >> + os.open(self._qemu_log_path, flags) > > > > Why change to os.open() instead of open()? > > I want to create the file only if it does not exist. The open() flag > 'x' is available only in python 3.3. For python <3.3, we need the > os.open() to have that feature. I'm not sure this extra complexity is really necessary. We could fix all that by using mkdtemp() and deleting the temporary directory on shutdown. > > > > >> + except: > >> + raise > >> + else: > >> + self._created_files.append(self._qemu_log_path) > >> + self._qemu_log_fd = open(self._qemu_log_path, 'wb') > >> > >> 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''' > >> self._iolog = None > >> self._qemu_full_args = None > >> devnull = open(os.path.devnull, 'rb') > >> - qemulog = open(self._qemu_log_path, 'wb') > >> try: > >> self._pre_launch() > >> 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, > >> + stdout=self._qemu_log_fd, > >> stderr=subprocess.STDOUT, > >> shell=False) > >> self._post_launch() > >> -- > >> 2.13.5 > >> > > > > Fam
On Thu, Sep 14, 2017 at 9:46 PM, Eduardo Habkost <ehabkost@redhat.com> wrote: > On Thu, Sep 14, 2017 at 09:38:13PM +0200, Amador Pahim wrote: >> On Tue, Sep 5, 2017 at 5:18 AM, Fam Zheng <famz@redhat.com> wrote: >> > On Fri, 09/01 13:28, Amador Pahim wrote: >> >> To launch a VM, we need to create basically two files: the monitor >> >> socket (if it's a UNIX socket) and the qemu log file. >> >> >> >> For the qemu log file, we currently just open the path, which will >> >> create the file if it does not exist or overwrite the file if it does >> >> exist. >> >> >> >> For the monitor socket, if it already exists, we are currently removing >> >> it, even if it's not created by us. >> >> >> >> This patch moves to pre_launch() the responsibility to make sure we only >> >> create files that are not pre-existent and to populate a list of >> >> controlled files. This list will then be used as the reference of >> >> files to remove during the cleanup (post_shutdown()). >> >> >> >> Signed-off-by: Amador Pahim <apahim@redhat.com> >> >> --- >> >> scripts/qemu.py | 30 +++++++++++++++++++++++------- >> >> 1 file changed, 23 insertions(+), 7 deletions(-) >> >> >> >> diff --git a/scripts/qemu.py b/scripts/qemu.py >> >> index 3ebe5ee0a4..c26e1412f9 100644 >> >> --- a/scripts/qemu.py >> >> +++ b/scripts/qemu.py >> >> @@ -41,6 +41,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 >> >> @@ -50,6 +51,7 @@ class QEMUMachine(object): >> >> self._socket_scm_helper = socket_scm_helper >> >> self._debug = debug >> >> self._qemu_full_args = None >> >> + self._created_files = [] >> >> >> >> # This can be used to add an unused monitor instance. >> >> def add_monitor_telnet(self, ip, port): >> >> @@ -128,30 +130,44 @@ class QEMUMachine(object): >> >> '-display', 'none', '-vga', 'none'] >> >> >> >> def _pre_launch(self): >> >> - self._qmp = qmp.qmp.QEMUMonitorProtocol(self._monitor_address, server=True, >> >> - debug=self._debug) >> >> + try: >> >> + self._qmp = qmp.qmp.QEMUMonitorProtocol(self._monitor_address, >> >> + server=True, >> >> + debug=self._debug) >> >> + except: >> >> + raise >> > >> > What's the point of "except: raise"? It seems useless. >> >> The point is to execute the block in the else only when no exception >> happens. When some exception happens, I want to raise it without >> executing the else block. > > Isn't this exactly what Python does when an exception is raised > with no "try" block? Sure, cleaning this up. > > >> >> > >> >> + else: >> >> + if not isinstance(self._monitor_address, tuple): >> >> + self._created_files.append(self._monitor_address) >> >> + >> >> + try: >> >> + flags = os.O_CREAT | os.O_EXCL | os.O_WRONLY >> >> + os.open(self._qemu_log_path, flags) >> > >> > Why change to os.open() instead of open()? >> >> I want to create the file only if it does not exist. The open() flag >> 'x' is available only in python 3.3. For python <3.3, we need the >> os.open() to have that feature. > > I'm not sure this extra complexity is really necessary. We could > fix all that by using mkdtemp() and deleting the temporary > directory on shutdown. I thought about that, but I foresee the question: hat happens if between the mkdtemp and the file creation (i.e. self._qemu_log_path) someone goes in that directory and creates a file with the same name of the self._qemu_log_path? Are we going to overwrite it? Ok, very unlikely, but possible. This extra step takes care of that. > >> >> > >> >> + except: >> >> + raise >> >> + else: >> >> + self._created_files.append(self._qemu_log_path) >> >> + self._qemu_log_fd = open(self._qemu_log_path, 'wb') >> >> >> >> 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''' >> >> self._iolog = None >> >> self._qemu_full_args = None >> >> devnull = open(os.path.devnull, 'rb') >> >> - qemulog = open(self._qemu_log_path, 'wb') >> >> try: >> >> self._pre_launch() >> >> 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, >> >> + stdout=self._qemu_log_fd, >> >> stderr=subprocess.STDOUT, >> >> shell=False) >> >> self._post_launch() >> >> -- >> >> 2.13.5 >> >> >> > >> > Fam > > -- > Eduardo
On Thu, Sep 14, 2017 at 10:05:50PM +0200, Amador Pahim wrote: > On Thu, Sep 14, 2017 at 9:46 PM, Eduardo Habkost <ehabkost@redhat.com> wrote: > > On Thu, Sep 14, 2017 at 09:38:13PM +0200, Amador Pahim wrote: > >> On Tue, Sep 5, 2017 at 5:18 AM, Fam Zheng <famz@redhat.com> wrote: > >> > On Fri, 09/01 13:28, Amador Pahim wrote: [...] > >> >> + else: > >> >> + if not isinstance(self._monitor_address, tuple): > >> >> + self._created_files.append(self._monitor_address) > >> >> + > >> >> + try: > >> >> + flags = os.O_CREAT | os.O_EXCL | os.O_WRONLY > >> >> + os.open(self._qemu_log_path, flags) > >> > > >> > Why change to os.open() instead of open()? > >> > >> I want to create the file only if it does not exist. The open() flag > >> 'x' is available only in python 3.3. For python <3.3, we need the > >> os.open() to have that feature. > > > > I'm not sure this extra complexity is really necessary. We could > > fix all that by using mkdtemp() and deleting the temporary > > directory on shutdown. > > I thought about that, but I foresee the question: hat happens if > between the mkdtemp and the file creation (i.e. self._qemu_log_path) > someone goes in that directory and creates a file with the same name > of the self._qemu_log_path? Are we going to overwrite it? Ok, very > unlikely, but possible. This extra step takes care of that. If someone creates a file inside a directory we created using mkdtemp(), we will just delete it. Why would that be a problem?
On Thu, Sep 14, 2017 at 10:18 PM, Eduardo Habkost <ehabkost@redhat.com> wrote: > On Thu, Sep 14, 2017 at 10:05:50PM +0200, Amador Pahim wrote: >> On Thu, Sep 14, 2017 at 9:46 PM, Eduardo Habkost <ehabkost@redhat.com> wrote: >> > On Thu, Sep 14, 2017 at 09:38:13PM +0200, Amador Pahim wrote: >> >> On Tue, Sep 5, 2017 at 5:18 AM, Fam Zheng <famz@redhat.com> wrote: >> >> > On Fri, 09/01 13:28, Amador Pahim wrote: > [...] >> >> >> + else: >> >> >> + if not isinstance(self._monitor_address, tuple): >> >> >> + self._created_files.append(self._monitor_address) >> >> >> + >> >> >> + try: >> >> >> + flags = os.O_CREAT | os.O_EXCL | os.O_WRONLY >> >> >> + os.open(self._qemu_log_path, flags) >> >> > >> >> > Why change to os.open() instead of open()? >> >> >> >> I want to create the file only if it does not exist. The open() flag >> >> 'x' is available only in python 3.3. For python <3.3, we need the >> >> os.open() to have that feature. >> > >> > I'm not sure this extra complexity is really necessary. We could >> > fix all that by using mkdtemp() and deleting the temporary >> > directory on shutdown. >> >> I thought about that, but I foresee the question: hat happens if >> between the mkdtemp and the file creation (i.e. self._qemu_log_path) >> someone goes in that directory and creates a file with the same name >> of the self._qemu_log_path? Are we going to overwrite it? Ok, very >> unlikely, but possible. This extra step takes care of that. > > If someone creates a file inside a directory we created using > mkdtemp(), we will just delete it. Why would that be a problem? Ok then. That simplifies the control a lot. Thanks. > > -- > Eduardo
diff --git a/scripts/qemu.py b/scripts/qemu.py index 3ebe5ee0a4..c26e1412f9 100644 --- a/scripts/qemu.py +++ b/scripts/qemu.py @@ -41,6 +41,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 @@ -50,6 +51,7 @@ class QEMUMachine(object): self._socket_scm_helper = socket_scm_helper self._debug = debug self._qemu_full_args = None + self._created_files = [] # This can be used to add an unused monitor instance. def add_monitor_telnet(self, ip, port): @@ -128,30 +130,44 @@ class QEMUMachine(object): '-display', 'none', '-vga', 'none'] def _pre_launch(self): - self._qmp = qmp.qmp.QEMUMonitorProtocol(self._monitor_address, server=True, - debug=self._debug) + try: + self._qmp = qmp.qmp.QEMUMonitorProtocol(self._monitor_address, + server=True, + debug=self._debug) + except: + raise + else: + if not isinstance(self._monitor_address, tuple): + self._created_files.append(self._monitor_address) + + try: + flags = os.O_CREAT | os.O_EXCL | os.O_WRONLY + os.open(self._qemu_log_path, flags) + except: + raise + else: + self._created_files.append(self._qemu_log_path) + self._qemu_log_fd = open(self._qemu_log_path, 'wb') 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''' self._iolog = None self._qemu_full_args = None devnull = open(os.path.devnull, 'rb') - qemulog = open(self._qemu_log_path, 'wb') try: self._pre_launch() 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, + stdout=self._qemu_log_fd, stderr=subprocess.STDOUT, shell=False) self._post_launch()
To launch a VM, we need to create basically two files: the monitor socket (if it's a UNIX socket) and the qemu log file. For the qemu log file, we currently just open the path, which will create the file if it does not exist or overwrite the file if it does exist. For the monitor socket, if it already exists, we are currently removing it, even if it's not created by us. This patch moves to pre_launch() the responsibility to make sure we only create files that are not pre-existent and to populate a list of controlled files. This list will then be used as the reference of files to remove during the cleanup (post_shutdown()). Signed-off-by: Amador Pahim <apahim@redhat.com> --- scripts/qemu.py | 30 +++++++++++++++++++++++------- 1 file changed, 23 insertions(+), 7 deletions(-)