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,
         )
-        output = proc.communicate()[0]
-        if output:
-            LOG.debug(output)
+        if proc.stdout:
+            LOG.debug(proc.stdout)
 
         return proc.returncode
 
-- 
2.30.2

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

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!