vmm: Alphabetically sort CLI options in --help output by phip1611 · Pull Request #6988 · cloud-hypervisor/cloud-hypervisor

phip1611 · 2025-03-18T07:52:40Z

The CLI has grown to a big variety of options. clap prints them in the
help message (--help) in the order they were defined. We now are at a
point where grouping things logically together doesn't work well.
Further, there is no support by clap for logical grouping and the
current code base wasn't consistent. Therefore, this commit introduces
two changes:

a new structure do define arguments (all in an array)
an alphabetical ordering of the arguments

No other changes have been made. No options have been altered.

This significantly improves:

code maintainability and extensibility
readability of the --help output

Signed-off-by: Philipp Schuster philipp.schuster@cyberus-technology.de

CLI --help Before

Details

Launch a cloud-hypervisor VMM.

Usage: cloud-hypervisor [OPTIONS]

Options:
      --cpus <cpus>
          boot=<boot_vcpus>,max=<max_vcpus>,topology=<threads_per_core>:<cores_per_die>:<dies_per_package>:<packages>,kvm_hyperv=on|off,max_phys_bits=<maximum_number_of_physical_bits>,affinity=<list_of_vcpus_with_their_associated_cpuset>,features=<list_of_features_to_enable> [default: boot=1,max_phys_bits=46]
      --platform <platform>
          num_pci_segments=<num_pci_segments>,iommu_segments=<list_of_segments>,iommu_address_width=<bits>,serial_number=<dmi_device_serial_number>,uuid=<dmi_device_uuid>,oem_strings=<list_of_strings>
      --memory <memory>
          Memory parameters "size=<guest_memory_size>,mergeable=on|off,shared=on|off,hugepages=on|off,hugepage_size=<hugepage_size>,hotplug_method=acpi|virtio-mem,hotplug_size=<hotpluggable_memory_size>,hotplugged_size=<hotplugged_memory_size>,prefault=on|off,thp=on|off" [default: size=512M]
      --memory-zone <memory-zone>...
          User defined memory zone parameters "size=<guest_memory_region_size>,file=<backing_file>,shared=on|off,hugepages=on|off,hugepage_size=<hugepage_size>,host_numa_node=<node_id>,id=<zone_identifier>,hotplug_size=<hotpluggable_memory_size>,hotplugged_size=<hotplugged_memory_size>,prefault=on|off"
      --firmware <firmware>
          Path to firmware that is loaded in an architectural specific way
      --kernel <kernel>
          Path to kernel to load. This may be a kernel or firmware that supports a PVH entry point (e.g. vmlinux) or architecture equivalent
      --initramfs <initramfs>
          Path to initramfs image
      --cmdline <cmdline>
          Kernel command line
      --rate-limit-group <rate-limit-group>...
          Rate Limit Group parameters "bw_size=<bytes>,bw_one_time_burst=<bytes>,bw_refill_time=<ms>,ops_size=<io_ops>,ops_one_time_burst=<io_ops>,ops_refill_time=<ms>,id=<device_id>"
      --disk <disk>...
          Disk parameters "path=<disk_image_path>,readonly=on|off,direct=on|off,iommu=on|off,num_queues=<number_of_queues>,queue_size=<size_of_each_queue>,vhost_user=on|off,socket=<vhost_user_socket_path>,bw_size=<bytes>,bw_one_time_burst=<bytes>,bw_refill_time=<ms>,ops_size=<io_ops>,ops_one_time_burst=<io_ops>,ops_refill_time=<ms>,id=<device_id>,pci_segment=<segment_id>,rate_limit_group=<group_id>,queue_affinity=<list_of_queue_indices_with_their_associated_cpuset>,serial=<serial_number>
      --landlock
          enable/disable Landlock.
      --landlock-rules <landlock-rules>...
          Landlock parameters "path=<path/to/{file/dir}>,access=[rw]"
      --net <net>...
          Network parameters "tap=<if_name>,ip=<ip_addr>,mask=<net_mask>,mac=<mac_addr>,fd=<fd1,fd2...>,iommu=on|off,num_queues=<number_of_queues>,queue_size=<size_of_each_queue>,id=<device_id>,vhost_user=<vhost_user_enable>,socket=<vhost_user_socket_path>,vhost_mode=client|server,bw_size=<bytes>,bw_one_time_burst=<bytes>,bw_refill_time=<ms>,ops_size=<io_ops>,ops_one_time_burst=<io_ops>,ops_refill_time=<ms>,pci_segment=<segment_id>offload_tso=on|off,offload_ufo=on|off,offload_csum=on|off"
      --rng <rng>
          Random number generator parameters "src=<entropy_source_path>,iommu=on|off" [default: src=/dev/urandom]
      --balloon <balloon>
          Balloon parameters "size=<balloon_size>,deflate_on_oom=on|off,free_page_reporting=on|off"
      --fs <fs>...
          virtio-fs parameters "tag=<tag_name>,socket=<socket_path>,num_queues=<number_of_queues>,queue_size=<size_of_each_queue>,id=<device_id>,pci_segment=<segment_id>"
      --pmem <pmem>...
          Persistent memory parameters "file=<backing_file_path>,size=<persistent_memory_size>,iommu=on|off,discard_writes=on|off,id=<device_id>,pci_segment=<segment_id>"
      --serial <serial>
          Control serial port: off|null|pty|tty|file=</path/to/a/file>|socket=</path/to/a/file> [default: null]
      --console <console>
          Control (virtio) console: "off|null|pty|tty|file=</path/to/a/file>,iommu=on|off" [default: tty]
      --device <device>...
          Direct device assignment parameters "path=<device_path>,iommu=on|off,id=<device_id>,pci_segment=<segment_id>"
      --user-device <user-device>...
          Userspace device socket=<socket_path>,id=<device_id>,pci_segment=<segment_id>"
      --vdpa <vdpa>...
          vDPA device "path=<device_path>,num_queues=<number_of_queues>,iommu=on|off,id=<device_id>,pci_segment=<segment_id>"
      --vsock <vsock>
          Virtio VSOCK parameters "cid=<context_id>,socket=<socket_path>,iommu=on|off,id=<device_id>,pci_segment=<segment_id>"
      --pvpanic
          Enable pvpanic device
      --numa <numa>...
          Settings related to a given NUMA node "guest_numa_id=<node_id>,cpus=<cpus_id>,distances=<list_of_distances_to_destination_nodes>,memory_zones=<list_of_memory_zones>,sgx_epc_sections=<list_of_sgx_epc_sections>,pci_segments=<list_of_pci_segments>"
      --pci-segment <pci-segment>...
          PCI Segment parameters "pci_segment=<segment_id>,mmio32_aperture_weight=<scale>,mmio64_aperture_weight=<scale>"
      --watchdog
          Enable virtio-watchdog
  -v...
          Sets the level of debugging output
      --log-file <log-file>
          Log file. Standard error is used if not specified
      --api-socket <api-socket>
          HTTP API socket (UNIX domain socket): path=</path/to/a/file> or fd=<fd>.
      --event-monitor <event-monitor>
          File to report events on: path=</path/to/a/file> or fd=<fd>
      --restore <restore>
          Restore from a VM snapshot. 
          Restore parameters "source_url=<source_url>,prefault=on|off,net_fds=<list_of_net_ids_with_their_associated_fds>" 
          `source_url` should be a valid URL (e.g file:///foo/bar or tcp://192.168.1.10/foo) 
          `prefault` brings memory pages in when enabled (disabled by default) 
          `net_fds` is a list of net ids with new file descriptors. Only net devices backed by FDs directly are needed as input.
      --seccomp <seccomp>
          [default: true] [possible values: true, false, log]
      --tpm <tpm>
          TPM device "(UNIX Domain Socket from swtpm) socket=</path/to/a/socket>"
      --sgx-epc <sgx-epc>...
          SGX EPC parameters "id=<epc_section_identifier>,size=<epc_section_size>,prefault=on|off"
      --debug-console <debug-console>
          Debug console: off|pty|tty|file=</path/to/a/file>,iobase=<port in hex> [default: off,iobase=0xe9]
  -V, --version
          Print version
  -h, --help
          Print help

CLI --help After

Details

Launch a cloud-hypervisor VMM.

Usage: cloud-hypervisor [OPTIONS]

Options:
      --api-socket <api-socket>
          HTTP API socket (UNIX domain socket): path=</path/to/a/file> or fd=<fd>.
      --balloon <balloon>
          Balloon parameters "size=<balloon_size>,deflate_on_oom=on|off,free_page_reporting=on|off"
      --cmdline <cmdline>
          Kernel command line
      --console <console>
          Control (virtio) console: "off|null|pty|tty|file=</path/to/a/file>,iommu=on|off" [default: tty]
      --cpus <cpus>
          boot=<boot_vcpus>,max=<max_vcpus>,topology=<threads_per_core>:<cores_per_die>:<dies_per_package>:<packages>,kvm_hyperv=on|off,max_phys_bits=<maximum_number_of_physical_bits>,affinity=<list_of_vcpus_with_their_associated_cpuset>,features=<list_of_features_to_enable> [default: boot=1,max_phys_bits=46]
      --debug-console <debug-console>
          Debug console: off|pty|tty|file=</path/to/a/file>,iobase=<port in hex> [default: off,iobase=0xe9]
      --device <device>...
          Direct device assignment parameters "path=<device_path>,iommu=on|off,id=<device_id>,pci_segment=<segment_id>"
      --disk <disk>...
          Disk parameters "path=<disk_image_path>,readonly=on|off,direct=on|off,iommu=on|off,num_queues=<number_of_queues>,queue_size=<size_of_each_queue>,vhost_user=on|off,socket=<vhost_user_socket_path>,bw_size=<bytes>,bw_one_time_burst=<bytes>,bw_refill_time=<ms>,ops_size=<io_ops>,ops_one_time_burst=<io_ops>,ops_refill_time=<ms>,id=<device_id>,pci_segment=<segment_id>,rate_limit_group=<group_id>,queue_affinity=<list_of_queue_indices_with_their_associated_cpuset>,serial=<serial_number>
      --event-monitor <event-monitor>
          File to report events on: path=</path/to/a/file> or fd=<fd>
      --firmware <firmware>
          Path to firmware that is loaded in an architectural specific way
      --fs <fs>...
          virtio-fs parameters "tag=<tag_name>,socket=<socket_path>,num_queues=<number_of_queues>,queue_size=<size_of_each_queue>,id=<device_id>,pci_segment=<segment_id>"
      --initramfs <initramfs>
          Path to initramfs image
      --kernel <kernel>
          Path to kernel to load. This may be a kernel or firmware that supports a PVH entry point (e.g. vmlinux) or architecture equivalent
      --landlock
          enable/disable Landlock.
      --landlock-rules <landlock-rules>...
          Landlock parameters "path=<path/to/{file/dir}>,access=[rw]"
      --log-file <log-file>
          Log file. Standard error is used if not specified
      --memory <memory>
          Memory parameters "size=<guest_memory_size>,mergeable=on|off,shared=on|off,hugepages=on|off,hugepage_size=<hugepage_size>,hotplug_method=acpi|virtio-mem,hotplug_size=<hotpluggable_memory_size>,hotplugged_size=<hotplugged_memory_size>,prefault=on|off,thp=on|off" [default: size=512M]
      --memory-zone <memory-zone>...
          User defined memory zone parameters "size=<guest_memory_region_size>,file=<backing_file>,shared=on|off,hugepages=on|off,hugepage_size=<hugepage_size>,host_numa_node=<node_id>,id=<zone_identifier>,hotplug_size=<hotpluggable_memory_size>,hotplugged_size=<hotplugged_memory_size>,prefault=on|off"
      --net <net>...
          Network parameters "tap=<if_name>,ip=<ip_addr>,mask=<net_mask>,mac=<mac_addr>,fd=<fd1,fd2...>,iommu=on|off,num_queues=<number_of_queues>,queue_size=<size_of_each_queue>,id=<device_id>,vhost_user=<vhost_user_enable>,socket=<vhost_user_socket_path>,vhost_mode=client|server,bw_size=<bytes>,bw_one_time_burst=<bytes>,bw_refill_time=<ms>,ops_size=<io_ops>,ops_one_time_burst=<io_ops>,ops_refill_time=<ms>,pci_segment=<segment_id>offload_tso=on|off,offload_ufo=on|off,offload_csum=on|off"
      --numa <numa>...
          Settings related to a given NUMA node "guest_numa_id=<node_id>,cpus=<cpus_id>,distances=<list_of_distances_to_destination_nodes>,memory_zones=<list_of_memory_zones>,sgx_epc_sections=<list_of_sgx_epc_sections>,pci_segments=<list_of_pci_segments>"
      --pci-segment <pci-segment>...
          PCI Segment parameters "pci_segment=<segment_id>,mmio32_aperture_weight=<scale>,mmio64_aperture_weight=<scale>"
      --platform <platform>
          num_pci_segments=<num_pci_segments>,iommu_segments=<list_of_segments>,iommu_address_width=<bits>,serial_number=<dmi_device_serial_number>,uuid=<dmi_device_uuid>,oem_strings=<list_of_strings>
      --pmem <pmem>...
          Persistent memory parameters "file=<backing_file_path>,size=<persistent_memory_size>,iommu=on|off,discard_writes=on|off,id=<device_id>,pci_segment=<segment_id>"
      --pvpanic
          Enable pvpanic device
      --rate-limit-group <rate-limit-group>...
          Rate Limit Group parameters "bw_size=<bytes>,bw_one_time_burst=<bytes>,bw_refill_time=<ms>,ops_size=<io_ops>,ops_one_time_burst=<io_ops>,ops_refill_time=<ms>,id=<device_id>"
      --restore <restore>
          Restore from a VM snapshot. 
          Restore parameters "source_url=<source_url>,prefault=on|off,net_fds=<list_of_net_ids_with_their_associated_fds>" 
          `source_url` should be a valid URL (e.g file:///foo/bar or tcp://192.168.1.10/foo) 
          `prefault` brings memory pages in when enabled (disabled by default) 
          `net_fds` is a list of net ids with new file descriptors. Only net devices backed by FDs directly are needed as input.
      --rng <rng>
          Random number generator parameters "src=<entropy_source_path>,iommu=on|off" [default: src=/dev/urandom]
      --seccomp <seccomp>
          [default: true] [possible values: true, false, log]
      --serial <serial>
          Control serial port: off|null|pty|tty|file=</path/to/a/file>|socket=</path/to/a/file> [default: null]
      --sgx-epc <sgx-epc>...
          SGX EPC parameters "id=<epc_section_identifier>,size=<epc_section_size>,prefault=on|off"
      --tpm <tpm>
          TPM device "(UNIX Domain Socket from swtpm) socket=</path/to/a/socket>"
      --user-device <user-device>...
          Userspace device socket=<socket_path>,id=<device_id>,pci_segment=<segment_id>"
  -v...
          Sets the level of debugging output
      --vdpa <vdpa>...
          vDPA device "path=<device_path>,num_queues=<number_of_queues>,iommu=on|off,id=<device_id>,pci_segment=<segment_id>"
  -V, --version
          Print version
      --vsock <vsock>
          Virtio VSOCK parameters "cid=<context_id>,socket=<socket_path>,iommu=on|off,id=<device_id>,pci_segment=<segment_id>"
      --watchdog
          Enable virtio-watchdog
  -h, --help
          Print help

src/main.rs

RuoqingHe

Good work, the rest looks good to me, thanks @phip1611

RuoqingHe · 2025-03-19T09:50:34Z

src/main.rs

+    // Debug-only check.
+    let is_sorted = args.is_sorted_by(|a, b| a.get_id().cmp(b.get_id()) != Ordering::Greater);
+    debug_assert!(is_sorted);


IMHO we can just used assert! here, because if someone forgets to keep the order, our CI will complain about it 🤔

It feels a bit weird, as we misuse a runtime check to verify a property of compile time data. But I don't see a better solution, and I think keeping this kind of check in any form is useful

I tried something like const fn get_cli_args_sorted() but this fails for various reasons:

diff --git a/src/main.rs b/src/main.rs index 09126502..694c5c5f 100644 --- a/src/main.rs +++ b/src/main.rs @@ -177,17 +177,7 @@ fn default_rng() -> String { format!("src={}", vm_config::DEFAULT_RNG_SOURCE) } -/// Creates the CLI definition of Cloud Hypervisor. -fn create_app(default_vcpus: String, default_memory: String, default_rng: String) -> Command { - let groups = [ - ArgGroup::new("vm-config") - .multiple(true) - .requires("vm-payload"), - ArgGroup::new("vmm-config").multiple(true), - ArgGroup::new("logging").multiple(true), - ArgGroup::new("vm-payload").multiple(true), - ]; - +const fn get_cli_args_sorted(default_vcpus: String, default_memory: String, default_rng: String) -> [Arg; 37] { // All CLI arguments in alphabetical order. PLEASE keep the list of commands alphabetically // sorted as this order is taken for the `--help` output. let args = [ @@ -462,9 +452,24 @@ fn create_app(default_vcpus: String, default_memory: String, default_rng: String .group("vm-config"), ]; - // Debug-only check. let is_sorted = args.is_sorted_by(|a, b| a.get_id().cmp(b.get_id()) != Ordering::Greater); - debug_assert!(is_sorted); + // Compile time check: const function + assert!(is_sorted); + args +} + +/// Creates the CLI definition of Cloud Hypervisor. +fn create_app(default_vcpus: String, default_memory: String, default_rng: String) -> Command { + let groups = [ + ArgGroup::new("vm-config") + .multiple(true) + .requires("vm-payload"), + ArgGroup::new("vmm-config").multiple(true), + ArgGroup::new("logging").multiple(true), + ArgGroup::new("vm-payload").multiple(true), + ]; + + let args = get_cli_args_sorted(default_vcpus, default_memory, default_rng); Command::new("cloud-hypervisor") // 'BUILD_VERSION' is set by the build script 'build.rs' at

Yep, IMO the idea here is to have a alphabetically sorted help output and args layout in our code.

We could put this sorted check into our unit test then, we separate the args section and test it in unit tests

yes, probably the best solution. Thanks! Please re-check

RuoqingHe

I'm not sure this is the best way of organizing the commits, but the code looks good to me, thanks for your patience ❤️

rbradford · 2025-03-19T15:54:06Z

I'd rather not have the commit to reorder the VmConfig struct as it's not really user visible and it breaks blame, etc. Also order for many structs is important so I don't see any value in adjusting this one.

The CLI has grown to a big variety of options. clap prints them in the help message (--help) in the order they were defined. We now are at a point where grouping things logically together doesn't work well. Further, there is no support by clap for logical grouping and the current code base wasn't consistent. Therefore, this commit introduces two changes: - a new structure to define arguments (all in an array) - an alphabetical ordering of the arguments No other changes have been made. No options have been altered. This significantly improves: - code maintainability and extensibility - readability of the --help output A unit test ensures they stay sorted. A better approach to check if the list of arguments (known at build time) is sorted would be a compile time check (`const`), but this currently isn't possible in stable Rust. Signed-off-by: Philipp Schuster <philipp.schuster@cyberus-technology.de> On-behalf-of: SAP philipp.schuster@sap.com

phip1611 · 2025-03-19T16:23:26Z

I'd rather not have the commit to reorder the VmConfig struct as it's not really user visible and it breaks blame, etc. Also order for many structs is important so I don't see any value in adjusting this one.

I updated the PR. Are we good to go now?

This enables to sort them alphabetically in a next step, similar to cloud-hypervisor#6988 / c37c639. [0] cloud-hypervisor#6988 Signed-off-by: Philipp Schuster <philipp.schuster@cyberus-technology.de> On-behalf-of: SAP philipp.schuster@sap.com

This enables to sort them alphabetically in a next step, similar to #6988 / c37c639. [0] #6988 Signed-off-by: Philipp Schuster <philipp.schuster@cyberus-technology.de> On-behalf-of: SAP philipp.schuster@sap.com

phip1611 requested a review from a team as a code owner March 18, 2025 07:52

phip1611 force-pushed the cli-help-order branch from cc5408c to b6b10de Compare March 18, 2025 08:06

RuoqingHe reviewed Mar 18, 2025

View reviewed changes

src/main.rs Outdated Show resolved Hide resolved

src/main.rs Outdated Show resolved Hide resolved

phip1611 force-pushed the cli-help-order branch from b6b10de to 264b12c Compare March 18, 2025 08:14

phip1611 requested a review from RuoqingHe March 18, 2025 10:15

phip1611 force-pushed the cli-help-order branch 2 times, most recently from b3cbe31 to cc1bc1d Compare March 19, 2025 08:53

RuoqingHe reviewed Mar 19, 2025

View reviewed changes

phip1611 force-pushed the cli-help-order branch from cc1bc1d to 5f4bf55 Compare March 19, 2025 10:48

phip1611 requested a review from RuoqingHe March 19, 2025 10:49

phip1611 force-pushed the cli-help-order branch 3 times, most recently from b5c8fef to 5afef9e Compare March 19, 2025 11:58

RuoqingHe approved these changes Mar 19, 2025

View reviewed changes

RuoqingHe requested a review from rbradford March 19, 2025 12:07

phip1611 force-pushed the cli-help-order branch from 5afef9e to c37c639 Compare March 19, 2025 16:23

rbradford approved these changes Mar 20, 2025

View reviewed changes

rbradford added this pull request to the merge queue Mar 20, 2025

Merged via the queue into cloud-hypervisor:main with commit fa58b72 Mar 20, 2025
39 checks passed

phip1611 deleted the cli-help-order branch March 20, 2025 12:45

likebreath added this to Cloud Hypervisor Roadmap Mar 27, 2025

github-project-automation bot moved this to 🆕 New in Cloud Hypervisor Roadmap Mar 27, 2025

likebreath moved this from 🆕 New to ✅ Done in Cloud Hypervisor Roadmap Mar 27, 2025

phip1611 mentioned this pull request Jun 11, 2025

ch-remote: sort all commands and args alphabetically #7130

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

vmm: Alphabetically sort CLI options in --help output#6988

vmm: Alphabetically sort CLI options in --help output#6988
rbradford merged 1 commit intocloud-hypervisor:mainfrom
phip1611:cli-help-order

phip1611 commented Mar 18, 2025

Uh oh!

Uh oh!

Uh oh!

RuoqingHe left a comment

Uh oh!

RuoqingHe Mar 19, 2025

Uh oh!

phip1611 Mar 19, 2025 •

edited

Loading

Uh oh!

RuoqingHe Mar 19, 2025

Uh oh!

phip1611 Mar 19, 2025

Uh oh!

RuoqingHe left a comment

Uh oh!

rbradford commented Mar 19, 2025

Uh oh!

phip1611 commented Mar 19, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

phip1611 commented Mar 18, 2025

CLI --help Before

CLI --help After

Uh oh!

Uh oh!

Uh oh!

RuoqingHe left a comment

Choose a reason for hiding this comment

Uh oh!

RuoqingHe Mar 19, 2025

Choose a reason for hiding this comment

Uh oh!

phip1611 Mar 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RuoqingHe Mar 19, 2025

Choose a reason for hiding this comment

Uh oh!

phip1611 Mar 19, 2025

Choose a reason for hiding this comment

Uh oh!

RuoqingHe left a comment

Choose a reason for hiding this comment

Uh oh!

rbradford commented Mar 19, 2025

Uh oh!

phip1611 commented Mar 19, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

phip1611 Mar 19, 2025 •

edited

Loading