[PATCH] crosvm: Rename `--vhost-user-{fs,gpu}` args
crosvm was producing warnings when using `--vhost-user-gpu` and `--vhost-user-fs`. In this commit, I have adjusted the `crosvm` invocations to look something like `--vhost-user $DEVICE`, where `$DEVICE` is, in this case, `gpu` or `fs`. I have run unit tests, but would appreciate testing. Signed-off-by: Dom Rodriguez <shymega@shymega.org.uk> --- img/app/Makefile | 4 ++-- release/checks/wayland/default.nix | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/img/app/Makefile b/img/app/Makefile index 3adf8c4..fd8befe 100644 --- a/img/app/Makefile +++ b/img/app/Makefile @@ -152,8 +152,8 @@ run-crosvm: $(imgdir)/appvm/blk/root.img start-vhost-user-gpu start-virtiofsd --disk $(RUN_IMG) \ -p "console=ttyS0 root=PARTLABEL=root" \ --net tap-name=tap0 \ - --vhost-user-fs build/virtiofsd.sock:virtiofs0 \ - --vhost-user-gpu build/vhost-user-gpu.sock \ + --vhost-user fs build/virtiofsd.sock:virtiofs0 \ + --vhost-user gpu build/vhost-user-gpu.sock \ --vsock cid=3 \ --serial type=file,hardware=serial,path=build/serial.log \ --serial type=stdout,hardware=virtio-console,stdin=true \ diff --git a/release/checks/wayland/default.nix b/release/checks/wayland/default.nix index d05aa88..774c9b9 100644 --- a/release/checks/wayland/default.nix +++ b/release/checks/wayland/default.nix @@ -29,7 +29,7 @@ nixosTest ({ lib, pkgs, ... }: { systemd.services.crosvm = { after = [ "crosvm-gpu.service" "weston.service" ]; requires = [ "crosvm-gpu.service" "weston.service" ]; - serviceConfig.ExecStart = "${lib.getExe pkgs.crosvm} run -s /run/crosvm --disk ${appvm}/img/appvm/blk/root.img --disk ${run}/blk/run.img -p \"console=ttyS0 root=PARTLABEL=root\" --vhost-user-gpu /run/crosvm-gpu.sock --vsock cid=3 --serial type=stdout,hardware=virtio-console,stdin=true ${appvm}/img/appvm/vmlinux"; + serviceConfig.ExecStart = "${lib.getExe pkgs.crosvm} run -s /run/crosvm --disk ${appvm}/img/appvm/blk/root.img --disk ${run}/blk/run.img -p \"console=ttyS0 root=PARTLABEL=root\" --vhost-user gpu /run/crosvm-gpu.sock --vsock cid=3 --serial type=stdout,hardware=virtio-console,stdin=true ${appvm}/img/appvm/vmlinux"; serviceConfig.ExecStop = "${lib.getExe pkgs.crosvm} stop /run/crosvm"; }; -- 2.44.1
Dom Rodriguez <shymega@shymega.org.uk> writes:
crosvm was producing warnings when using `--vhost-user-gpu` and `--vhost-user-fs`.
In this commit, I have adjusted the `crosvm` invocations to look something like `--vhost-user $DEVICE`, where `$DEVICE` is, in this case, `gpu` or `fs`.
I have run unit tests, but would appreciate testing.
How did you run them? release/checks/wayland is failing: vm-test-run-spectrum-wayland> machine # [ 14.028205] crosvm[968]: [2024-09-06T11:19:20.132395456+00:00 ERROR crosvm] arg parsing failed: Error parsing option '--vhost-user' with value 'gpu': missing field `socket` vm-test-run-spectrum-wayland> machine # [ 14.029442] crosvm[968]: [src/main.rs:737] arg parsing failed: Error parsing option '--vhost-user' with value 'gpu': missing field `socket` Does it need to be (from memory) --vhost-user gpu,socket=path or something?
On 06.09.2024 13:22, Alyssa Ross wrote:
Dom Rodriguez <shymega@shymega.org.uk> writes:
crosvm was producing warnings when using `--vhost-user-gpu` and `--vhost-user-fs`.
In this commit, I have adjusted the `crosvm` invocations to look something like `--vhost-user $DEVICE`, where `$DEVICE` is, in this case, `gpu` or `fs`.
I have run unit tests, but would appreciate testing.
How did you run them? release/checks/wayland is failing:
vm-test-run-spectrum-wayland> machine # [ 14.028205] crosvm[968]: [2024-09-06T11:19:20.132395456+00:00 ERROR crosvm] arg parsing failed: Error parsing option '--vhost-user' with value 'gpu': missing field `socket` vm-test-run-spectrum-wayland> machine # [ 14.029442] crosvm[968]: [src/main.rs:737] arg parsing failed: Error parsing option '--vhost-user' with value 'gpu': missing field `socket`
Does it need to be (from memory) --vhost-user gpu,socket=path or something?
I thought I had tested them, but as it turned out, the command I used was only for shellcheck. I've run unit tests again, and all seems to pass. I've queued up the v2 patch for review. Just a side note from my article about Flakes - we could use the `checks` output to run on the `pre-commit` Git hook. This could then check the commit message, run tests, etc. That way we can catch these sort of mistakes before they even make it to the mailing list. But that's a digression - I think it'd be a good idea long-term though. Best wishes, -- Dom Rodriguez GPG Fingerprint: EB0D 45E6 D0DC 1BA1 A2B5 FC24 72DC F123 1E54 BD43
Dom Rodriguez <shymega@shymega.org.uk> writes:
On 06.09.2024 13:22, Alyssa Ross wrote:
Dom Rodriguez <shymega@shymega.org.uk> writes:
crosvm was producing warnings when using `--vhost-user-gpu` and `--vhost-user-fs`.
In this commit, I have adjusted the `crosvm` invocations to look something like `--vhost-user $DEVICE`, where `$DEVICE` is, in this case, `gpu` or `fs`.
I have run unit tests, but would appreciate testing.
How did you run them? release/checks/wayland is failing:
vm-test-run-spectrum-wayland> machine # [ 14.028205] crosvm[968]: [2024-09-06T11:19:20.132395456+00:00 ERROR crosvm] arg parsing failed: Error parsing option '--vhost-user' with value 'gpu': missing field `socket` vm-test-run-spectrum-wayland> machine # [ 14.029442] crosvm[968]: [src/main.rs:737] arg parsing failed: Error parsing option '--vhost-user' with value 'gpu': missing field `socket`
Does it need to be (from memory) --vhost-user gpu,socket=path or something?
I thought I had tested them, but as it turned out, the command I used was only for shellcheck. I've run unit tests again, and all seems to pass. I've queued up the v2 patch for review.
Do you think we could change anything about the documentation to avoid people making that mistake in future? It's hard to anticipate what will confuse people in advance.
Just a side note from my article about Flakes - we could use the `checks` output to run on the `pre-commit` Git hook. This could then check the commit message, run tests, etc.
We could do that already, right? There's already a command you can run to run all the checks. My concern would be that some of the checks are slow (the ones that involve building images), so I think we might want a release/checks/fast.nix or something so it's not too annoying — I find slow pre-commit hooks to be a real drag if I want to keep rewording or slightly modifying a commit. The flake eval cache would certainly help with the other slow bit (Nix eval), though.
On 08.09.2024 09:46, Alyssa Ross wrote:
Dom Rodriguez <shymega@shymega.org.uk> writes:
On 06.09.2024 13:22, Alyssa Ross wrote:
Dom Rodriguez <shymega@shymega.org.uk> writes:
crosvm was producing warnings when using `--vhost-user-gpu` and `--vhost-user-fs`.
In this commit, I have adjusted the `crosvm` invocations to look something like `--vhost-user $DEVICE`, where `$DEVICE` is, in this case, `gpu` or `fs`.
I have run unit tests, but would appreciate testing.
How did you run them? release/checks/wayland is failing:
vm-test-run-spectrum-wayland> machine # [ 14.028205] crosvm[968]: [2024-09-06T11:19:20.132395456+00:00 ERROR crosvm] arg parsing failed: Error parsing option '--vhost-user' with value 'gpu': missing field `socket` vm-test-run-spectrum-wayland> machine # [ 14.029442] crosvm[968]: [src/main.rs:737] arg parsing failed: Error parsing option '--vhost-user' with value 'gpu': missing field `socket`
Does it need to be (from memory) --vhost-user gpu,socket=path or something?
I thought I had tested them, but as it turned out, the command I used was only for shellcheck. I've run unit tests again, and all seems to pass. I've queued up the v2 patch for review.
Do you think we could change anything about the documentation to avoid people making that mistake in future? It's hard to anticipate what will confuse people in advance.
From my newcomer pespective, I assumed the command on the documentation for testing ran all checks. I just blindly copied and pasted it. That was an error on my part, and I should have known better. I think this is more of a PEBKAC problem with me, rather than the documentation.
Just a side note from my article about Flakes - we could use the `checks` output to run on the `pre-commit` Git hook. This could then check the commit message, run tests, etc.
We could do that already, right? There's already a command you can run to run all the checks. My concern would be that some of the checks are slow (the ones that involve building images), so I think we might want a release/checks/fast.nix or something so it's not too annoying — I find slow pre-commit hooks to be a real drag if I want to keep rewording or slightly modifying a commit. The flake eval cache would certainly help with the other slow bit (Nix eval), though.
I agree that the checks as a pre-commit hook would be slow. I would like to integrate a CI environment on the list for each patch. Perhaps we could use your suggestion of 'fast checks', and the CI could run all of the checks upon receiving a patch? I should have discussed this in the Flakes thread, so my suggestion is that we continue discussion there, and in Matrix/IRC if the need arises. Best wishes, -- Dom Rodriguez GPG Fingerprint: EB0D 45E6 D0DC 1BA1 A2B5 FC24 72DC F123 1E54 BD43
participants (2)
-
Alyssa Ross -
Dom Rodriguez