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

[Dom Rodriguez <shymega@shymega.org.uk>]

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

On 07.09.2024 18:40, 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,socket=$PATH`, where `$DEVICE` is, in this case,
>> `gpu` or `fs`, and `$PATH` is the path to the Unix socket.
>>
>> 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..11ef6e1 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,socket=build/virtiofsd.sock:virtiofs0 \
>> +	    --vhost-user gpu,socket=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 \
>
>This is still not right, I'm afraid.
>
>When I run nix-shell --run 'make clean && make run' VMM=crosvm in
>img/app, I get this error:
>
>[2024-09-07T16:36:32.592274891+00:00 ERROR crosvm] exiting with error 1: failed to connect to vhost-user socket path build/virtiofsd.sock:virtiofs0
>
>Probably the tag should also be a comma-separated key=value option?
>
>(This doesn't have any automated test, because it's just part of the
>development environment, and setting up a test environment to resemble a
>development machine isn't trivial.  Should be possible though.)

It's bizarre. I've done some more testing, and it seems that
`--vhost-user` isn't *quite* there yet on feature parity with
`--vhost-user-fs`. Maybe I'm going wrong here, but it doesn't recognise
the tag as a k/v option.

I have managed to get the tests running with `--vhost-user-fs`, but it
does look mismatched now.

Would you prefer I revert the changes to `--vhost-user fs` =>
`--vhost-user-fs`?

>> diff --git a/release/checks/wayland/default.nix b/release/checks/wayland/default.nix
>> index d05aa88..a36dbab 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,socket=/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";
>>      };
>
>This part works now though, thanks!

Awesome. I'll leave that as-is for the next patch revision.

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 --]