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

ch-remote: Fix crash when run with no subcommand #6230

Merged
merged 2 commits into from
Feb 24, 2024
Merged

ch-remote: Fix crash when run with no subcommand #6230

merged 2 commits into from
Feb 24, 2024

Conversation

arachsys
Copy link
Contributor

ch-remote crashes when run with --api-socket but no subcommand:

$ target/release/ch-remote --api-socket /tmp/api
thread 'main' panicked at src/bin/ch-remote.rs:509:14:
internal error: entered unreachable code

Use clap::Command::subcommand_required(true) to yield a more friendly error in this case.

ch-remote crashes when run with --api-socket but no subcommand:

  $ target/release/ch-remote --api-socket /tmp/api
  thread 'main' panicked at src/bin/ch-remote.rs:509:14:
  internal error: entered unreachable code

Use clap::Command::subcommand_required(true) to yield a more friendly
error in this case.

Signed-off-by: Chris Webb <chris@arachsys.com>
@arachsys arachsys requested a review from a team as a code owner February 23, 2024 16:59
@arachsys
Copy link
Contributor Author

Locally, I added arg_required_else_help(true) to both cloud-hypervisor and ch-remote for better interactive convenience, but I didn't propose it in this patch as I'm not sure how you feel about that behaviour vs a 'missing subcommand' error from ch-remote?

@rbradford
Copy link
Member

Thank you so much for finding this and working on a fix. I think I .arg_required_else_help() makes sense for ch-remote and the cloud-hypervisor binary (probably also the vhost daemons that are in tree.) Running cloud-hypervisor with no arguments isn't useful (expect perhaps in the dbus case? @brkp ?).

@arachsys
Copy link
Contributor Author

arachsys commented Feb 23, 2024

Thank you so much for finding this and working on a fix. I think I .arg_required_else_help() makes sense for ch-remote and the cloud-hypervisor binary (probably also the vhost daemons that are in tree.) Running cloud-hypervisor with no arguments isn't useful (expect perhaps in the dbus case? @brkp ?).

Great, thanks! Very happy to update to add arg_required_else_help() in each place as well. I hadn't spotted vhost_user_*/src/main.rs, but looking at them, they do each have a backend argument that's required, so emitting help when run 'bare' does sound more helpful for them too.

cloud-hypervisor, ch-remote, vhost-user-block and vhost-user-net all
need at least one argument to do anything useful, so printing command
help is helpful when they are run without arguments or a subcommand.

Use clap::Command::arg_required_else_help(true) to do this.

Signed-off-by: Chris Webb <chris@arachsys.com>
@arachsys
Copy link
Contributor Author

Looks like the x86-64 (garm-jammy-amd, musl) test failed because of wget timing out. (There were quite a few strange network problems on GitHub last night: I saw some other CI stuff failing randomly on checkouts too.) I'm not sure if there's a way I can trigger it to re-run?

@rbradford rbradford merged commit 0310c57 into cloud-hypervisor:main Feb 24, 2024
29 checks passed
@brkp
Copy link
Contributor

brkp commented Feb 24, 2024

Running cloud-hypervisor with no arguments isn't useful (expect perhaps in the dbus case?).

Yeah, this applies to the D-Bus API as well, it is not started if no relevant arguments are passed to cloud-hypervisor.

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.

None yet

3 participants