[PATCH] crosvm: Rename `--vhost-user-{fs,gpu}` args

[PATCH] crosvm: Rename `--vhost-user-{fs,gpu}` args

[Dom Rodriguez]

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

Re: [PATCH] crosvm: Rename `--vhost-user-{fs,gpu}` args

[Alyssa Ross]

[-- Attachment #1: Type: text/plain, Size: 890 bytes --]

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?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH] crosvm: Rename `--vhost-user-{fs,gpu}` args

[Dom Rodriguez]

[-- Attachment #1: Type: text/plain, Size: 1574 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] crosvm: Rename `--vhost-user-{fs,gpu}` args

[Alyssa Ross]

[-- Attachment #1: Type: text/plain, Size: 2080 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH] crosvm: Rename `--vhost-user-{fs,gpu}` args

[Dom Rodriguez]

[-- Attachment #1: Type: text/plain, Size: 2960 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]