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

nixos/qemu-guest-agent: fix start service #110927

Merged
merged 1 commit into from May 19, 2021
Merged

Conversation

Izorkin
Copy link
Contributor

@Izorkin Izorkin commented Jan 27, 2021

Motivation for this change

Fix start qemu-guest-agent service

cc @Mic92 @blitz @flokli

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

Copy link
Contributor

@blitz blitz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks ok to me from the code changes. I don't have a system to test this, though.

@flokli
Copy link
Contributor

flokli commented Jan 27, 2021

I'd prefer if we could fix this while building qemu itself, not when invoking the qemu-guest-agent binary. Using a path inside the nix store can't ever work.

We also might be able to test this in a VM test, by waiting for the systemd unit to have succeeded in starting up.

The test instrumentation sets services.qemuGuest.enable, so the unit should be configured in every test VM.

@Izorkin
Copy link
Contributor Author

Izorkin commented Jan 27, 2021

Don't know how to fix qemu build.

@flokli
Copy link
Contributor

flokli commented Jan 27, 2021

This path is assembled in qga/main.c, which calls to qemu_get_local_state_pathname (from util/oslib-posix.c).

This uses the CONFIG_QEMU_LOCALSTATEDIR define, which is set in meson.build.

For some reason, this is set to get_option('prefix') / get_option('localstatedir') (which doesn't work on NixOS).

I'm not sure how it works on other distros, but we might just want to ship a downstream patch in nixpkgs, that changes this to another path. (and do some digging upstream about why it's done that way).

@Izorkin
Copy link
Contributor Author

Izorkin commented Jan 27, 2021

@flokli with add to configureFlags option "--localstatedir=/var" error build qemu if activated nix.useSandbox = true;

@Patryk27
Copy link
Member

FWIW, I've just got bit by qemu-agent not starting, and this patch helped.

As for the approach taken - as far as I know, going with the --statedir-esque kind of parameters is a pretty common practice; maybe instead of trying to work-around this issue by patching qga/main.c (which seems kinda brittle), we could apply the easy, low-hanging-fruit as in this merge request?

@AluisioASG
Copy link
Contributor

Is this needed to avoid shipping a broken service with 21.05?

@Mic92 Mic92 merged commit 5b4915f into NixOS:master May 19, 2021
@Izorkin Izorkin deleted the fix-qemu-ga branch May 19, 2021 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants