| Message ID | 20210512214642.2803189-4-jsnow@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Python: delint iotests, machine.py and console_socket.py | expand |
Hi, On 5/12/21 6:46 PM, John Snow wrote: > use run() instead of Popen() -- to assert to pylint that we are not > forgetting to close a long-running program. > > Signed-off-by: John Snow <jsnow@redhat.com> > --- > python/qemu/machine.py | 15 +++++++++------ > 1 file changed, 9 insertions(+), 6 deletions(-) > > diff --git a/python/qemu/machine.py b/python/qemu/machine.py > index 41f51bd27d0..c13ff9b32bf 100644 > --- a/python/qemu/machine.py > +++ b/python/qemu/machine.py > @@ -223,13 +223,16 @@ def send_fd_scm(self, fd: Optional[int] = None, > assert fd is not None > fd_param.append(str(fd)) > > - proc = subprocess.Popen( > - fd_param, stdin=subprocess.DEVNULL, stdout=subprocess.PIPE, > - stderr=subprocess.STDOUT, close_fds=False > + proc = subprocess.run( > + fd_param, > + stdin=subprocess.DEVNULL, > + stdout=subprocess.PIPE, > + stderr=subprocess.STDOUT, > + check=True, > + close_fds=False, > ) Now it might throw a CalledProcessError given that `check=True`. Shouldn't it capture the exception and (possible) re-throw as an QEMUMachineError? - Wainer > - output = proc.communicate()[0] > - if output: > - LOG.debug(output) > + if proc.stdout: > + LOG.debug(proc.stdout) > > return proc.returncode >
On 5/14/21 10:08 AM, Wainer dos Santos Moschetta wrote: > Hi, > > On 5/12/21 6:46 PM, John Snow wrote: >> use run() instead of Popen() -- to assert to pylint that we are not >> forgetting to close a long-running program. >> >> Signed-off-by: John Snow <jsnow@redhat.com> >> --- >> python/qemu/machine.py | 15 +++++++++------ >> 1 file changed, 9 insertions(+), 6 deletions(-) >> >> diff --git a/python/qemu/machine.py b/python/qemu/machine.py >> index 41f51bd27d0..c13ff9b32bf 100644 >> --- a/python/qemu/machine.py >> +++ b/python/qemu/machine.py >> @@ -223,13 +223,16 @@ def send_fd_scm(self, fd: Optional[int] = None, >> assert fd is not None >> fd_param.append(str(fd)) >> - proc = subprocess.Popen( >> - fd_param, stdin=subprocess.DEVNULL, stdout=subprocess.PIPE, >> - stderr=subprocess.STDOUT, close_fds=False >> + proc = subprocess.run( >> + fd_param, >> + stdin=subprocess.DEVNULL, >> + stdout=subprocess.PIPE, >> + stderr=subprocess.STDOUT, >> + check=True, >> + close_fds=False, >> ) > > Now it might throw a CalledProcessError given that `check=True`. > Shouldn't it capture the exception and (possible) re-throw as an > QEMUMachineError? > > - Wainer > I suppose I ought to so that it matches the other errors of this method, yes. Setting it to false and checking manually may be less code, but yeah. I'll change this. Thanks!
On 5/14/21 10:08 AM, Wainer dos Santos Moschetta wrote: > Now it might throw a CalledProcessError given that `check=True`. > Shouldn't it capture the exception and (possible) re-throw as an > QEMUMachineError? I lied to you again. The existing callers all check for failure explicitly, so in the interest of avoiding an API change, I'm just going to set check=False here. We can improve the interface separately some other time. --js
diff --git a/python/qemu/machine.py b/python/qemu/machine.py index 41f51bd27d0..c13ff9b32bf 100644 --- a/python/qemu/machine.py +++ b/python/qemu/machine.py @@ -223,13 +223,16 @@ def send_fd_scm(self, fd: Optional[int] = None, assert fd is not None fd_param.append(str(fd)) - proc = subprocess.Popen( - fd_param, stdin=subprocess.DEVNULL, stdout=subprocess.PIPE, - stderr=subprocess.STDOUT, close_fds=False + proc = subprocess.run( + fd_param, + stdin=subprocess.DEVNULL, + stdout=subprocess.PIPE, + stderr=subprocess.STDOUT, + check=True, + close_fds=False, ) - output = proc.communicate()[0] - if output: - LOG.debug(output) + if proc.stdout: + LOG.debug(proc.stdout) return proc.returncode
use run() instead of Popen() -- to assert to pylint that we are not forgetting to close a long-running program. Signed-off-by: John Snow <jsnow@redhat.com> --- python/qemu/machine.py | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-)