Qemu restoration by rnhmjoj · Pull Request #127933 · NixOS/nixpkgs

rnhmjoj · 2021-06-23T16:37:30Z

Motivation for this change

The NixOS QEMU module has always been in pretty degraded
state: most of the options are not type-checked or
documented (I think the reason is that the module is not
in the manual, can we fix this too?). Manipulating the
qemu.options is painful and qemu.networkingOptions
makes a very good footgun. Lots of options are hard-coded
and it's not clear how to override them1.

So, here I try to clean up things a bit and adding some
higher-level options for

Forwarding ports
Sharing directories via 9pfs
Changing screen resolution

The change is quite big but I tried to break into smaller commits.

Things done

tfc · 2021-06-24T05:48:32Z

nixos/modules/virtualisation/qemu-vm.nix

i see many changes of the kind that would also be suggested by the tool shellcheck - did you run that tool on the result and got the inspiration for the tool from there?

Maybe we could have shellcheck run over the output automatically?
(In that case it might also help if the script would be less concatenated from nix snippets)

Yes, I run shellcheck on the result/bin/run-machine-vm script and applied the changes to the Nix code.
I guess It could be added as a checkPhase to the writer functions (writeShellScript, etc.), but I'm not sure what the performance penality would be.

shellcheck is usually pretty fast but the problem I with adding that is that we probably need to disable some checks and also add a lot more ignore comments at false positives.

rnhmjoj · 2021-06-24T15:09:55Z

Fixed guest fowarding and added assertions for common mistakes.

rnhmjoj · 2021-06-30T21:30:11Z

I fixed another bug introduced with the qemu-common renaming.
I also tested nixos-rebuild build-vm and run a bunch of nixosTests: at this point I'm fairly certain it should break too many things.

rnhmjoj · 2021-07-19T20:25:31Z

Fixed a typo.

piegamesde

I tried it out and it indeed fixes #134318. Also the changes of the default values are deeply appreciated. I only reviewed the first commits in detail though.

nixos/lib/build-vms.nix

andir · 2021-09-10T13:14:50Z

@ofborg test simple

rnhmjoj · 2021-09-15T00:28:54Z

Rebased and added a release note. Also, I enabled the documentation of these options in the manual, by default.

- Fix shell quoting issues - Fix unsafe cd in run-machine-vm script

The current name is misleading: it doesn't contain cli arguments, but several constants and utility functions related to qemu. This commit also removes the use of `with import ...` for clarity.

rnhmjoj · 2021-09-18T15:04:05Z

Rebased againd a fixed a couple of conflict.
I think this PR is pretty much ready to go, can you finish your review @andir?

nrdxp

Overall looks like a very nice and welcome clean up, especially the type additions. None of my remarks are hard blockers I'd say. Good work!

nrdxp · 2021-09-28T20:38:19Z

nixos/modules/virtualisation/qemu-vm.nix

+        ${qemu}/bin/qemu-img create -f qcow2 -F qcow2 -b ${bootDisk}/disk.img "$TMPDIR/disk.img" || exit 1

-        NIX_EFI_VARS=$(readlink -f ''${NIX_EFI_VARS:-${cfg.efiVars}})
+        NIX_EFI_VARS=$(readlink -f "''${NIX_EFI_VARS:-${cfg.efiVars}}")

-        ${if cfg.useEFIBoot then ''
+        ${lib.optionalString cfg.useEFIBoot
+        ''
          # VM needs writable EFI vars
          if ! test -e "$NIX_EFI_VARS"; then
            cp ${bootDisk}/efi-vars.fd "$NIX_EFI_VARS" || exit 1
            chmod 0644 "$NIX_EFI_VARS" || exit 1
          fi
-        '' else ""}
-      '' else ""}
+        ''}
+      ''}

-      cd $TMPDIR
-      idx=0
+      cd "$TMPDIR" || exit 1


Might it be better to use a simple set -e on the script rather than all these explicit exits?

nrdxp · 2021-09-28T20:41:52Z

nixos/modules/virtualisation/qemu-vm.nix

+
+          <warning><para>
+            If the NixOS firewall on the virtual machine is enabled, you also
+            have to open the guest ports to enable the traffic between host and


could we not open the same firewall ports ourselves using a gatekeeping option that some modules have, i.e. openPorts?

rnhmjoj · 2021-09-29T07:00:57Z

Might it be better to use a simple `set -e` on the script rather than all these explicit exits?

Good idea, I've opened a new PR: #139844

could we not open the same firewall ports ourselves using a gatekeeping option that some modules have, i.e. `openPorts`?

Yes, it should be doable. However, NixOS VM are normally used to test a real setup, including the firewall configuration, so such an option should be off by default.

dasJ · 2021-09-29T12:14:18Z

Did you break unstable-small again?

From our Hydra (don't mind the weird name of nixpkgs):

in job ‘nixos.tests.boot.biosCdrom’:
error: getting status of '/nix/store/nd3fl6bqmw44adz0grd18v6i9jq21zr6-nixpkgs-patched/nixos/lib/qemu-flags.nix': No such file or directory

Probably because of this file:

nixos/tests/boot.nix:with import ../lib/qemu-flags.nix { inherit pkgs; };

rnhmjoj · 2021-09-29T13:11:09Z

It seems nixosTests.boot was changed to import qemu-flags.nix after I tested the commit that renamed qemu-flags.

The way `networkingOptions` works seems to have change following NixOS/nixpkgs#127933.

rnhmjoj requested review from aanderse, andir, basvandijk, danbst and tfc June 23, 2021 16:37

github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Jun 23, 2021

ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Jun 23, 2021

rnhmjoj force-pushed the qemu-restoration branch from c4e69df to 55c2d09 Compare June 23, 2021 17:18

tfc reviewed Jun 24, 2021

View reviewed changes

rnhmjoj force-pushed the qemu-restoration branch from 55c2d09 to 21f3338 Compare June 24, 2021 15:07

rnhmjoj force-pushed the qemu-restoration branch from 21f3338 to 9723b04 Compare June 30, 2021 19:49

rnhmjoj force-pushed the qemu-restoration branch from 9723b04 to 1eabb67 Compare July 19, 2021 20:19

rnhmjoj mentioned this pull request Aug 16, 2021

run-nixos-vm breaks with spaces in paths #134318

Closed

piegamesde approved these changes Aug 16, 2021

View reviewed changes

andir reviewed Aug 16, 2021

View reviewed changes

nixos/lib/build-vms.nix Outdated Show resolved Hide resolved

rnhmjoj force-pushed the qemu-restoration branch from 1eabb67 to c0743c9 Compare August 16, 2021 22:21

piegamesde requested a review from andir September 10, 2021 12:53

rnhmjoj force-pushed the qemu-restoration branch from c0743c9 to 9a57525 Compare September 15, 2021 00:23

github-actions bot added 8.has: changelog This PR adds or changes release notes 8.has: documentation This PR adds or changes documentation labels Sep 15, 2021

rnhmjoj added 3 commits September 18, 2021 16:46

nixos/qemu: fix several shellcheck issues

c1fd51f

- Fix shell quoting issues - Fix unsafe cd in run-machine-vm script

nixos/qemu: fix deprecation warning

65e8338

nixos/qemu: set a reasonable msize by default

2af5413

rnhmjoj added 8 commits September 18, 2021 16:47

nixos/qemu: cleaner formatting of the run-machine-vm script

f1f434c

nixos/qemu: add types to all options

30f0faa

nixos/qemu: make shared directories configurable

ec6c604

nixos/qemu: make display resolution configurable

44d95b7

nixos/qemu: add option to forward ports

b8bfc81

nixos/lib/qemu-flags: rename to qemu-common

b29c2f9

The current name is misleading: it doesn't contain cli arguments, but several constants and utility functions related to qemu. This commit also removes the use of `with import ...` for clarity.

nixos/documentation: always show qemu-vm options

e4e5d4d

nixos/docs: document new qemu-vm features

bd3cb03

rnhmjoj force-pushed the qemu-restoration branch from 9a57525 to bd3cb03 Compare September 18, 2021 15:02

nrdxp approved these changes Sep 28, 2021

View reviewed changes

rnhmjoj mentioned this pull request Sep 29, 2021

nixos/qemu: use set -e in shell script #139844

Merged

5 tasks

Artturin mentioned this pull request Sep 29, 2021

nixos/boot: qemu-flags -> qemu-common #139890

Merged

12 tasks

rnhmjoj mentioned this pull request Sep 29, 2021

nixos/tests: fix for memorySize being an integer #139907

Merged

12 tasks

Lassulus mentioned this pull request Oct 16, 2021

QEMU errors with 'Duplicate ID 'user.0' for netdev' nix-community/nixos-generators#113

Closed

chkno mentioned this pull request Oct 25, 2021

nixos/qemu-vm: Option to use squashfs Nix store closure instead of virtfs access to host's store #72354

Closed

10 tasks

noteed added a commit to hypered/curiosity that referenced this pull request Jul 30, 2022

Fix: use virtualisation.forwardPorts.

8f84f99

The way `networkingOptions` works seems to have change following NixOS/nixpkgs#127933.

rnhmjoj deleted the qemu-restoration branch July 10, 2023 14:06

Uh oh!

Conversation

rnhmjoj commented Jun 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation for this change

Things done

Uh oh!

tfc Jun 24, 2021

Choose a reason for hiding this comment

Uh oh!

rnhmjoj Jun 24, 2021

Choose a reason for hiding this comment

Uh oh!

SuperSandro2000 Jun 24, 2021

Choose a reason for hiding this comment

Uh oh!

rnhmjoj commented Jun 24, 2021

Uh oh!

rnhmjoj commented Jun 30, 2021

Uh oh!

rnhmjoj commented Jul 19, 2021

Uh oh!

piegamesde left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

andir commented Sep 10, 2021

Uh oh!

rnhmjoj commented Sep 15, 2021

Uh oh!

rnhmjoj commented Sep 18, 2021

Uh oh!

nrdxp left a comment

Choose a reason for hiding this comment

Uh oh!

nrdxp Sep 28, 2021

Choose a reason for hiding this comment

Uh oh!

nrdxp Sep 28, 2021

Choose a reason for hiding this comment

Uh oh!

rnhmjoj commented Sep 29, 2021 via email

Uh oh!

dasJ commented Sep 29, 2021

Uh oh!

rnhmjoj commented Sep 29, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

rnhmjoj commented Jun 23, 2021 •

edited

Loading