python: misc cleanups for python code

[PATCH 13/14] iotests/151: ensure subprocesses are cleaned up

Posted by Daniel P. Berrangé 4 months ago

The iotest 151 creates a bunch of subprocesses, with their stdout
connected to a pipe but never reads any data from them and does
not gurantee the processes are killed on cleanup.

This triggers resource leak warnings from python when the
subprocess.Popen object is garbage collected.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 tests/qemu-iotests/151 | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tests/qemu-iotests/151 b/tests/qemu-iotests/151
index f2ff9c5dac..06ee3585db 100755
--- a/tests/qemu-iotests/151
+++ b/tests/qemu-iotests/151
@@ -263,6 +263,11 @@ class TestThrottledWithNbdExportBase(iotests.QMPTestCase):
                         break
                     except subprocess.TimeoutExpired:
                         self.vm.qtest(f'clock_step {1 * 1000 * 1000 * 1000}')
+                try:
+                    p.kill()
+                    p.stdout.close()
+                except:
+                    pass
         except IndexError:
             pass
 
-- 
2.49.0

Re: [PATCH 13/14] iotests/151: ensure subprocesses are cleaned up

Posted by John Snow 2 months, 3 weeks ago

On Tue, Jul 15, 2025 at 10:32 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> The iotest 151 creates a bunch of subprocesses, with their stdout
> connected to a pipe but never reads any data from them and does
> not gurantee the processes are killed on cleanup.
>
> This triggers resource leak warnings from python when the
> subprocess.Popen object is garbage collected.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  tests/qemu-iotests/151 | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/tests/qemu-iotests/151 b/tests/qemu-iotests/151
> index f2ff9c5dac..06ee3585db 100755
> --- a/tests/qemu-iotests/151
> +++ b/tests/qemu-iotests/151
> @@ -263,6 +263,11 @@ class TestThrottledWithNbdExportBase(iotests.QMPTestCase):
>                          break
>                      except subprocess.TimeoutExpired:
>                          self.vm.qtest(f'clock_step {1 * 1000 * 1000 * 1000}')
> +                try:
> +                    p.kill()
> +                    p.stdout.close()
> +                except:
> +                    pass

You can decide how much you care here, but using "except Exception:"
will allow critical exceptions to still crash the program, like
KeyboardInterrupt and SystemExit et al. It's good hygiene for things
that may or may not hang so you can still use Ctrl+C.

>          except IndexError:
>              pass
>
> --
> 2.49.0
>

Re: [PATCH 13/14] iotests/151: ensure subprocesses are cleaned up

Posted by Daniel P. Berrangé 2 months, 3 weeks ago

On Tue, Aug 19, 2025 at 05:37:04PM -0400, John Snow wrote:
> On Tue, Jul 15, 2025 at 10:32 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
> >
> > The iotest 151 creates a bunch of subprocesses, with their stdout
> > connected to a pipe but never reads any data from them and does
> > not gurantee the processes are killed on cleanup.
> >
> > This triggers resource leak warnings from python when the
> > subprocess.Popen object is garbage collected.
> >
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  tests/qemu-iotests/151 | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/tests/qemu-iotests/151 b/tests/qemu-iotests/151
> > index f2ff9c5dac..06ee3585db 100755
> > --- a/tests/qemu-iotests/151
> > +++ b/tests/qemu-iotests/151
> > @@ -263,6 +263,11 @@ class TestThrottledWithNbdExportBase(iotests.QMPTestCase):
> >                          break
> >                      except subprocess.TimeoutExpired:
> >                          self.vm.qtest(f'clock_step {1 * 1000 * 1000 * 1000}')
> > +                try:
> > +                    p.kill()
> > +                    p.stdout.close()
> > +                except:
> > +                    pass
> 
> You can decide how much you care here, but using "except Exception:"
> will allow critical exceptions to still crash the program, like
> KeyboardInterrupt and SystemExit et al. It's good hygiene for things
> that may or may not hang so you can still use Ctrl+C.

Yes, that should be changed as you describe.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|