Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better parsing of command-line options #90

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

DemiMarie
Copy link
Contributor

This makes qubes-dom0-update aware of various command-line options that
take arguments, so it can process them accordingly. It is not a perfect
solution; that would require using a C wrapper program to reject
abbreviated or unknown options. It also uses bash regex matching
instead of the echo | grep anti-pattern, replaces an == with =,
adds a missing --, and unsets a variable that will be checked for
being set later.

The same approach used here could be used to refactor
qubes-gpg-client-wrapper if it were worth the time, which it probably
isn’t unless qubes-gpg-client-wrapper will see future use.

Marking as draft because I have not tested it yet. I will test it once more time becomes available.

@marmarek
Copy link
Member

This makes qubes-dom0-update aware of various command-line options that
take arguments, so it can process them accordingly.

I don't like this part - the list can easily de-synchronize, there are also some actions that takes extra arguments (like repoquery). Having --arg value working for some but not the others is IMO worse than supporting only --arg=value.

It also uses bash regex matching
instead of the echo | grep anti-pattern, replaces an == with =,
adds a missing --, and unsets a variable that will be checked for
being set later.

This part is fine.

@DemiMarie
Copy link
Contributor Author

This makes qubes-dom0-update aware of various command-line options that
take arguments, so it can process them accordingly.

I don't like this part - the list can easily de-synchronize, there are also some actions that takes extra arguments (like repoquery). Having --arg value working for some but not the others is IMO worse than supporting only --arg=value.

What about explicitly detecting known bad cases and exiting with a an error message? Ultimately I would like to do something like split-gpg1 and only allow command-line arguments we understand, so that we know we have not made any mistakes. The DNF version in dom0 is frozen, so we should be able to do this unambiguously. What do you think?

@DemiMarie
Copy link
Contributor Author

This makes qubes-dom0-update aware of various command-line options that
take arguments, so it can process them accordingly.

I don't like this part - the list can easily de-synchronize, there are also some actions that takes extra arguments (like repoquery). Having --arg value working for some but not the others is IMO worse than supporting only --arg=value.

Changed so that --arg value never works (it results in an error).

@marmarek
Copy link
Member

What about explicitly detecting known bad cases and exiting with a an error message?

Yes. this one is ok.

The DNF version in dom0 is frozen, so we should be able to do this unambiguously.

There are several actions that are limited to the call in updatevm only (like search, or list), repoquery should also be one of them. For those, we don't have frozen set of supported options.

@qubesos-bot
Copy link

qubesos-bot commented Apr 3, 2022

OpenQA test summary

Complete test suite and dependencies: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.1&build=2022040304-4.1&flavor=pull-requests

New failures, excluding unstable

Compared to: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.1&build=2022031706-4.1&flavor=update

  • system_tests_basic_vm_qrexec_gui

    • TC_30_Gui_daemon: test_000_clipboard (failure)
      self.assertEqual(clipboard_content, ... AssertionError: '' != 'test19'
  • system_tests_guivm_gui_interactive

    • update_guivm: Failed (test died)
      # Test died: command '(set -o pipefail; qubesctl --all --show-outpu...
  • system_tests_basic_vm_qrexec_gui_btrfs

    • switch_pool: wait_serial (wait serial expected)
      # wait_serial expected: qr/neGn9-\d+-/u...

    • switch_pool: Failed (test died + timed out)
      # Test died: command 'qvm-start sys-firewall sys-usb' timed out at ...

  • system_tests_basic_vm_qrexec_gui_ext4

    • switch_pool: Failed (test died)
      # Test died: command '! qvm-check sys-whonix || qvm-start sys-whoni...
  • system_tests_network_updates

    • TC_00_Dom0Upgrade_debian-11: test_000_update (failure)
      AssertionError: qubes-dom0-update failed: Using test-inst-updatevm ...

    • TC_00_Dom0Upgrade_debian-11: test_005_update_flag_clear (failure)
      AssertionError: qubes-dom0-update failed: Using test-inst-updatevm ...

    • TC_00_Dom0Upgrade_debian-11: test_006_update_flag_clear (failure)
      AssertionError: qubes-dom0-update failed: Using test-inst-updatevm ...

    • TC_00_Dom0Upgrade_debian-11: test_010_instal (failure)
      AssertionError: qubes-dom0-update failed: Using test-inst-updatevm ...

    • TC_00_Dom0Upgrade_fedora-34: test_000_update (failure)
      AssertionError: qubes-dom0-update failed: Using test-inst-updatevm ...

    • TC_00_Dom0Upgrade_fedora-34: test_005_update_flag_clear (failure)
      AssertionError: qubes-dom0-update failed: Using test-inst-updatevm ...

    • TC_00_Dom0Upgrade_fedora-34: test_006_update_flag_clear (failure)
      AssertionError: qubes-dom0-update failed: Using test-inst-updatevm ...

    • TC_00_Dom0Upgrade_fedora-34: test_010_instal (failure)
      AssertionError: qubes-dom0-update failed: Using test-inst-updatevm ...

  • system_tests_basic_vm_qrexec_gui_xfs

    • switch_pool: Failed (test died)
      # Test died: command 'qubes-dom0-update -y xfsprogs' failed at qube...

Failed tests

33 failures
  • system_tests_basic_vm_qrexec_gui

    • TC_30_Gui_daemon: test_000_clipboard (failure)
      self.assertEqual(clipboard_content, ... AssertionError: '' != 'test19'
  • system_tests_guivm_gui_interactive

    • update_guivm: Failed (test died)
      # Test died: command '(set -o pipefail; qubesctl --all --show-outpu...
  • system_tests_basic_vm_qrexec_gui_btrfs

    • switch_pool: wait_serial (wait serial expected)
      # wait_serial expected: qr/neGn9-\d+-/u...

    • switch_pool: Failed (test died + timed out)
      # Test died: command 'qvm-start sys-firewall sys-usb' timed out at ...

  • system_tests_basic_vm_qrexec_gui_ext4

    • switch_pool: Failed (test died)
      # Test died: command '! qvm-check sys-whonix || qvm-start sys-whoni...
  • system_tests_pvgrub_salt_storage

    • [unstable] TC_40_PVGrub_debian-11: test_010_template_based_vm (error + timed out)
      qubes.exc.QubesVMShutdownTimeoutError: Domain shutdown timed out: '...
  • system_tests_suspend@hw1

    • suspend: wait_serial (wait serial expected)
      # wait_serial expected: qr/p5~T5-\d+-/u...

    • suspend: Failed (test died + timed out)
      # Test died: command 'true' timed out at qubesos/tests/suspend.pm l...

    • suspend: wait_serial (wait serial expected)
      # wait_serial expected: "xl info; echo 8Ye1l-\$?-"...

    • suspend: wait_serial (wait serial expected)
      # wait_serial expected: qr/8Ye1l-\d+-/u...

    • suspend: wait_serial (wait serial expected)
      # wait_serial expected: "# "...

    • suspend: wait_serial (wait serial expected)
      # wait_serial expected: "xl list; echo MfjdI-\$?-"...

    • suspend: wait_serial (wait serial expected)
      # wait_serial expected: qr/MfjdI-\d+-/u...

    • suspend: wait_serial (wait serial expected)
      # wait_serial expected: "# "...

    • suspend: wait_serial (wait serial expected)
      # wait_serial expected: "xl dmesg; echo hNz_G-\$?-"...

    • suspend: wait_serial (wait serial expected)
      # wait_serial expected: qr/hNz_G-\d+-/u...

    • suspend: wait_serial (wait serial expected)
      # wait_serial expected: "# "...

    • suspend: wait_serial (wait serial expected)
      # wait_serial expected: "journalctl -b|tail -n 10000; echo Adw2Q-\$...

    • suspend: wait_serial (wait serial expected)
      # wait_serial expected: qr/Adw2Q-\d+-/u...

    • suspend: wait_serial (wait serial expected)
      # wait_serial expected: "# "...

  • system_tests_network_updates

    • TC_00_Dom0Upgrade_debian-11: test_000_update (failure)
      AssertionError: qubes-dom0-update failed: Using test-inst-updatevm ...

    • TC_00_Dom0Upgrade_debian-11: test_005_update_flag_clear (failure)
      AssertionError: qubes-dom0-update failed: Using test-inst-updatevm ...

    • TC_00_Dom0Upgrade_debian-11: test_006_update_flag_clear (failure)
      AssertionError: qubes-dom0-update failed: Using test-inst-updatevm ...

    • TC_00_Dom0Upgrade_debian-11: test_010_instal (failure)
      AssertionError: qubes-dom0-update failed: Using test-inst-updatevm ...

    • TC_00_Dom0Upgrade_fedora-34: test_000_update (failure)
      AssertionError: qubes-dom0-update failed: Using test-inst-updatevm ...

    • TC_00_Dom0Upgrade_fedora-34: test_005_update_flag_clear (failure)
      AssertionError: qubes-dom0-update failed: Using test-inst-updatevm ...

    • TC_00_Dom0Upgrade_fedora-34: test_006_update_flag_clear (failure)
      AssertionError: qubes-dom0-update failed: Using test-inst-updatevm ...

    • TC_00_Dom0Upgrade_fedora-34: test_010_instal (failure)
      AssertionError: qubes-dom0-update failed: Using test-inst-updatevm ...

  • system_tests_basic_vm_qrexec_gui_xfs

    • switch_pool: Failed (test died)
      # Test died: command 'qubes-dom0-update -y xfsprogs' failed at qube...
  • system_tests_dispvm

    • [unstable] TC_20_DispVM_whonix-ws-16: test_100_open_in_dispvm (failure + cleanup)
      assert len(self.loop._selector.get_map()) \... AssertionError
  • system_tests_splitgpg

Fixed failures

Compared to: https://openqa.qubes-os.org/tests/36922#dependencies

4 fixed
  • system_tests_basic_vm_qrexec_gui_ext4

    • TC_03_QvmRevertTemplateChanges: test_000_revert_linux (error)
      NotImplementedError: FileVolume supports maximum 1 volume revision ...
  • system_tests_network_updates

  • system_tests_pvgrub_salt_storage

    • TC_40_PVGrub_debian-11: test_000_standalone_vm (error + timed out)
      qubes.exc.QubesVMShutdownTimeoutError: Domain shutdown timed out: '...
  • system_tests_basic_vm_qrexec_gui@hw1

Unstable tests

  • system_tests_network

    VmNetworking_debian-11/test_040_inter_vm (1/5 times with errors)
    • job 36341 qubes.exc.QubesVMNotStartedError: Cannot dynamically attach to stop...
  • system_tests_extra

    TC_00_ImgConverter_fedora-34/test_000_png (1/5 times with errors)
    • job 36492 Cannot process volume group qubes_dom0...
    TC_00_PDFConverter_debian-11/test_001_two_pages (1/5 times with errors)
    • job 37287 qubes.exc.QubesMemoryError: Not enough memory to start domain 'test...
  • system_tests_manager

    GlobalSettingsTest/test_00_settings_started (1/5 times with errors)
    • job 36199 Exception: Cannot detect enabled ITL template update repositories
    GlobalSettingsTest/test_01_load_correct_defs (1/5 times with errors)
    • job 36199 Exception: Cannot detect enabled ITL template update repositories
    GlobalSettingsTest/test_02_dom0_updates_load (1/5 times with errors)
    • job 36199 Exception: Cannot detect enabled ITL template update repositories
    GlobalSettingsTest/test_03_nothing_changed_ok (1/5 times with errors)
    • job 36199 Exception: Cannot detect enabled ITL template update repositories
    GlobalSettingsTest/test_04_nothing_changed_cancel (1/5 times with errors)
    • job 36199 Exception: Cannot detect enabled ITL template update repositories
    GlobalSettingsTest/test_10_set_update_vm (1/5 times with errors)
    • job 36199 Exception: Cannot detect enabled ITL template update repositories
    GlobalSettingsTest/test_11_set_update_vm_to_none (1/5 times with errors)
    • job 36199 Exception: Cannot detect enabled ITL template update repositories
    GlobalSettingsTest/test_20_set_clock_vm (1/5 times with errors)
    • job 36199 Exception: Cannot detect enabled ITL template update repositories
    GlobalSettingsTest/test_21_set_clock_vm_to_none (1/5 times with errors)
    • job 36199 Exception: Cannot detect enabled ITL template update repositories
    GlobalSettingsTest/test_30_set_default_netvm (1/5 times with errors)
    • job 36199 Exception: Cannot detect enabled ITL template update repositories
    GlobalSettingsTest/test_31_set_default_netvm_to_none (1/5 times with errors)
    • job 36199 Exception: Cannot detect enabled ITL template update repositories
    GlobalSettingsTest/test_40_set_default_template (1/5 times with errors)
    • job 36199 Exception: Cannot detect enabled ITL template update repositories
    GlobalSettingsTest/test_50_set_default_kernel (1/5 times with errors)
    • job 36199 Exception: Cannot detect enabled ITL template update repositories
    GlobalSettingsTest/test_51_set_default_kernel_to_none (1/5 times with errors)
    • job 36199 Exception: Cannot detect enabled ITL template update repositories
    GlobalSettingsTest/test_60_set_dom0_updates_true (1/5 times with errors)
    • job 36199 Exception: Cannot detect enabled ITL template update repositories
    GlobalSettingsTest/test_70_change_vm_updates (1/5 times with errors)
    • job 36199 Exception: Cannot detect enabled ITL template update repositories
    GlobalSettingsTest/test_72_set_all_vms_true (1/5 times with errors)
    • job 36199 Exception: Cannot detect enabled ITL template update repositories
    GlobalSettingsTest/test_73_set_all_vms_false (1/5 times with errors)
    • job 36199 Exception: Cannot detect enabled ITL template update repositories
    GlobalSettingsTest/test_80_set_default_dispvm (1/5 times with errors)
    • job 36199 Exception: Cannot detect enabled ITL template update repositories
    GlobalSettingsTest/test_81_set_default_dispvm_to_none (1/5 times with errors)
    • job 36199 Exception: Cannot detect enabled ITL template update repositories
    GlobalSettingsTest/test_90_test_all_set_none (1/5 times with errors)
    • job 36199 Exception: Cannot detect enabled ITL template update repositories
  • system_tests_qrexec

    TC_00_Qrexec_debian-11/test_070_qrexec_vm_simultaneous_write (1/5 times with errors)
    • job 37527 AssertionError: Timeout, probably deadlock
  • system_tests_basic_vm_qrexec_gui_btrfs

    TC_30_Gui_daemon/test_000_clipboard (1/5 times with errors)
    • job 36221 self.assertEqual(clipboard_content, ... AssertionError: '' != 'test23'
  • system_tests_basic_vm_qrexec_gui_ext4

    TC_30_Gui_daemon/test_000_clipboard (1/5 times with errors)
    • job 37280 self.assertEqual(clipboard_content, ... AssertionError: '' != 'test22'
    TC_00_AppVM_whonix-gw-16-pool/test_000_start_shutdown (1/5 times with errors)
    • job 36329 qubes.exc.QubesVMShutdownTimeoutError: Domain shutdown timed out: '...
    TC_00_AppVM_whonix-gw-16-pool/test_010_run_xterm (1/5 times with errors)
    • job 36329 OSError: Volume /var/lib/qubes-pool/appvms/test-inst-vm1/private.im...
  • system_tests_pvgrub_salt_storage

    TC_40_PVGrub_debian-11/test_000_standalone_vm (3/5 times with errors)
    • job 36203 qubes.exc.QubesVMShutdownTimeoutError: Domain shutdown timed out: '...
    • job 36344 qubes.exc.QubesVMShutdownTimeoutError: Domain shutdown timed out: '...
    • job 36944 qubes.exc.QubesVMShutdownTimeoutError: Domain shutdown timed out: '...
    TC_40_PVGrub_debian-11/test_010_template_based_vm (3/5 times with errors)
    • job 36203 qubes.exc.QubesVMShutdownTimeoutError: Domain shutdown timed out: '...
    • job 36483 qubes.exc.QubesVMShutdownTimeoutError: Domain shutdown timed out: '...
    • job 36944 qubes.exc.QubesVMShutdownTimeoutError: Domain shutdown timed out: '...
  • system_tests_network_updates

    TC_10_QvmTemplate_whonix-gw-16/test_010_template_install (1/5 times with errors)
    • job 36943 AssertionError: libvirt event impl drain timeout
    TC_11_QvmTemplateMgmtVM_whonix-gw-16/test_010_template_install (2/5 times with errors)
    • job 36222 AssertionError: libvirt event impl drain timeout
    • job 36343 qvm-template: error: Template 'debian-11-minimal' not found.
  • system_tests_basic_vm_qrexec_gui_xfs

    TC_30_Gui_daemon/test_000_clipboard (1/5 times with errors)
    • job 36469 self.assertEqual(clipboard_content, ... AssertionError: '' != 'test23'
  • system_tests_network_ipv6

    VmIPv6Networking_debian-11/test_020_simple_proxyvm_nm (1/5 times with errors)
    • job 36481 AssertionError: 1 != 0 : nm-applet window not found
    VmIPv6Networking_whonix-ws-16/test_020_simple_proxyvm_nm (1/5 times with errors)
    VmIPv6Networking_debian-11/test_520_ipv6_simple_proxyvm_nm (1/5 times with errors)
    • job 36342 AssertionError: 1 != 0 : nm-applet window not found
    VmIPv6Networking_debian-11/test_540_ipv6_inter_vm (1/5 times with errors)
    • job 36342 raise exceptions.TimeoutError()... asyncio.exceptions.TimeoutError
  • system_tests_dispvm

    TC_04_DispVM/test_003_cleanup_destroyed (1/5 times with errors)
    • job 36325 raise exceptions.TimeoutError()... asyncio.exceptions.TimeoutError
    TC_20_DispVM_fedora-34/test_010_simple_dvm_run (1/5 times with errors)
    • job 36325 assert len(self.loop._selector.get_map()) \... AssertionError
    TC_20_DispVM_whonix-gw-16/test_010_simple_dvm_run (1/5 times with errors)
    TC_20_DispVM_whonix-ws-16/test_010_simple_dvm_run (1/5 times with errors)
    • job 36325 assert len(self.loop._selector.get_map()) \... AssertionError
    TC_20_DispVM_whonix-gw-16/test_020_gui_app (1/5 times with errors)
    TC_20_DispVM_debian-11/test_030_edit_file (1/5 times with errors)
    • job 36325 AssertionError: Timeout while waiting for disp[0-9]* window to show
    TC_20_DispVM_fedora-34/test_030_edit_file (1/5 times with errors)
    • job 36325 AssertionError: Timeout while waiting for disp[0-9]* window to show
    TC_20_DispVM_whonix-gw-16/test_030_edit_file (1/5 times with errors)
    TC_20_DispVM_whonix-ws-16/test_030_edit_file (1/5 times with errors)
    • job 36325 AssertionError: Timeout while waiting for disp[0-9]* window to show
    TC_20_DispVM_debian-11/test_100_open_in_dispvm (3/5 times with errors)
    • job 36185 AssertionError: './open-file test.txt' failed with ./open-file test...
    • job 36325 AssertionError: './open-file test.txt' failed with ./open-file test...
    • job 37312 AssertionError: './open-file test.txt' failed with ./open-file test...
    TC_20_DispVM_fedora-34/test_100_open_in_dispvm (1/5 times with errors)
    • job 36325 AssertionError: './open-file test.txt' failed with ./open-file test...
    TC_20_DispVM_whonix-gw-16/test_100_open_in_dispvm (1/5 times with errors)
    TC_20_DispVM_whonix-ws-16/test_100_open_in_dispvm (4/5 times with errors)
    • job 36185 AssertionError: libvirt event impl drain timeout
    • job 36325 AssertionError: './open-file test.txt' failed with ./open-file test...
    • job 36464 AssertionError: libvirt event impl drain timeout
    • job 37312 AssertionError: libvirt event impl drain timeout
  • system_tests_splitgpg

    TC_10_Thunderbird_debian-11/test_000_send_receive_default (1/5 times with errors)
    • job 36345 Exception: Failed to send message with error 'Status:'
    TC_10_Thunderbird_fedora-34/test_000_send_receive_default (1/5 times with errors)
    • job 36345 dogtail.tree.SearchError: child of [desktop frame | main]: "Thunder...
    TC_10_Thunderbird_whonix-ws-16/test_000_send_receive_default (1/5 times with errors)
    • job 36345 Exception: Failed to send message with error 'Status:'
    TC_10_Thunderbird_debian-11/test_010_send_receive_inline_signed_only (1/5 times with errors)
    • job 37296 dogtail.tree.SearchError: descendent of [application | Thunderbird]...
    TC_10_Thunderbird_fedora-34/test_010_send_receive_inline_signed_only (1/5 times with errors)
    • job 36345 dogtail.tree.SearchError: child of [desktop frame | main]: "Thunder...
    TC_10_Thunderbird_whonix-ws-16/test_010_send_receive_inline_signed_only (1/5 times with errors)
    • job 36345 Exception: Failed to send message with error 'Status:'
    TC_10_Thunderbird_debian-11/test_020_send_receive_inline_with_attachment (1/5 times with errors)
    • job 36345 Exception: Failed to send message with error 'Status:'
    TC_10_Thunderbird_fedora-34/test_020_send_receive_inline_with_attachment (1/5 times with errors)
    • job 36345 dogtail.tree.SearchError: child of [desktop frame | main]: "Thunder...
    TC_10_Thunderbird_whonix-ws-16/test_020_send_receive_inline_with_attachment (1/5 times with errors)
    • job 36345 Exception: Failed to send message with error 'Status:'

@marmarek marmarek marked this pull request as draft April 3, 2022 14:51
@marmarek
Copy link
Member

marmarek commented Apr 3, 2022

With this PR, qubes-dom0-update always fails. With rather unhelpful message: https://openqa.qubes-os.org/tests/37697#step/switch_pool/17

I tried to get more details by adding --show-output --console, but then got --console xfsprogs must be written as --consolexfsprogs.

So, please:

  1. Fix handling --console
  2. Fix error reporting on "Sending repository information to UpdateVM"
  3. Fix the actual issue it has there.
  4. Test it

And only then convert back to non-draft.

This makes qubes-dom0-update aware of various command-line options that
take arguments, so it can process them accordingly.  It is not a perfect
solution; that would require using a C wrapper program to reject
abbreviated or unknown options.  It also uses bash regex matching
instead of the `echo | grep` anti-pattern, replaces an `==` with `=`,
adds a missing `--`, and unsets a variable that will be checked for
being set later.

The same approach used here could be used to refactor
qubes-gpg-client-wrapper if it were worth the time, which it probably
isn’t unless qubes-gpg-client-wrapper will see future use.
The previous commit tried to fix up incorrectly passed options, but that
didn't always work.  Instead, try to detect known bad options (on a
best-effort basis) and print an error message.
This changes the error message to a warning and fixes up the user's
mistake.
@DemiMarie DemiMarie force-pushed the better-option-parsing branch from 1b5e47a to 0be9c7e Compare April 3, 2022 22:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants