From 405464a67e49be28e07aff64f549e7f912fdedfa Mon Sep 17 00:00:00 2001
From: Pawel Marczewski <pawel@invisiblethingslab.com>
Date: Thu, 16 Jan 2020 12:31:55 +0100
Subject: [PATCH] qvm-shutdown: report errors, don't crash on DispVMs

qvm-shutdown with the --wait option checks if the machine
state is 'Halted', but a disposable VM is usually deleted by
the time of the final check, resulting in a non-zero exit code.

This change handles properly disposable VMs, and makes sure
we always output an error message when finishing with a non-zero
exit code.

Fixes QubesOS/qubes-issues#5245.
---
 qubesadmin/tests/tools/qvm_shutdown.py |  5 +++--
 qubesadmin/tools/__init__.py           |  4 ++--
 qubesadmin/tools/qvm_shutdown.py       | 23 ++++++++++++++++++++---
 3 files changed, 25 insertions(+), 7 deletions(-)

diff --git a/qubesadmin/tests/tools/qvm_shutdown.py b/qubesadmin/tests/tools/qvm_shutdown.py
index b37956b..a180a0f 100644
--- a/qubesadmin/tests/tools/qvm_shutdown.py
+++ b/qubesadmin/tests/tools/qvm_shutdown.py
@@ -293,6 +293,7 @@ class TC_00_qvm_shutdown(qubesadmin.tests.QubesTestCase):
         self.app.expected_calls[
             ('sys-net', 'admin.vm.List', None, None)] = \
             b'0\x00sys-net class=AppVM state=Halted\n'
-        qubesadmin.tools.qvm_shutdown.main(
-            ['--wait', '--all', '--timeout=1'], app=self.app)
+        with self.assertRaisesRegexp(SystemExit, '2'):
+            qubesadmin.tools.qvm_shutdown.main(
+                ['--wait', '--all', '--timeout=1'], app=self.app)
         self.assertAllCalled()
diff --git a/qubesadmin/tools/__init__.py b/qubesadmin/tools/__init__.py
index 25b9638..ef18e06 100644
--- a/qubesadmin/tools/__init__.py
+++ b/qubesadmin/tools/__init__.py
@@ -408,12 +408,12 @@ class QubesArgumentParser(argparse.ArgumentParser):
         return namespace
 
 
-    def error_runtime(self, message):
+    def error_runtime(self, message, exit_code=1):
         '''Runtime error, without showing usage.
 
         :param str message: message to show
         '''
-        self.exit(1, '{}: error: {}\n'.format(self.prog, message))
+        self.exit(exit_code, '{}: error: {}\n'.format(self.prog, message))
 
 
     @staticmethod
diff --git a/qubesadmin/tools/qvm_shutdown.py b/qubesadmin/tools/qvm_shutdown.py
index 0263af5..f9b10cf 100644
--- a/qubesadmin/tools/qvm_shutdown.py
+++ b/qubesadmin/tools/qvm_shutdown.py
@@ -73,7 +73,12 @@ def main(args=None, app=None):  # pylint: disable=missing-docstring
                 else:
                     remaining_domains.add(vm)
         if not args.wait:
-            return len(remaining_domains)
+            if remaining_domains:
+                parser.error_runtime(
+                    'Failed to shut down: ' +
+                    ', '.join(vm.name for vm in remaining_domains),
+                    len(remaining_domains))
+            return
         this_round_domains.difference_update(remaining_domains)
         if not this_round_domains:
             # no VM shutdown request succeed, no sense to try again
@@ -122,8 +127,20 @@ def main(args=None, app=None):  # pylint: disable=missing-docstring
     if args.wait:
         if have_events:
             loop.close()
-        return len([vm for vm in args.domains
-            if vm.get_power_state() != 'Halted'])
+        failed = []
+        for vm in args.domains:
+            power_state = vm.get_power_state()
+            # DispVM might have been deleted before we check them,
+            # so NA is acceptable.
+            if not (power_state == 'Halted' or
+                    (vm.klass == 'DispVM' and power_state == 'NA')):
+                failed.append(vm)
+        if failed:
+            parser.error_runtime(
+                'Failed to shut down: ' +
+                ', '.join(vm.name for vm in failed),
+                len(failed))
+
 
 if __name__ == '__main__':
     sys.exit(main())