Hi,
The provided patch allows the VNC websocket server of a qemu process to
be provided over AF_UNIX as it is already possible for standard TCP VNC
servers.
Now that many clients support websocket connections, some exclusively,
it can be useful to expose the VNC server. One such case is when a
proxy is present on a host machine, allowing it to proxy to a
deterministic location withouth having to keep track of port mappings.
Removing the condition as is done in the provided patch creates a
functional server. If there's a downside to this approach I could not
figure it out while reading the code. My hunch was that the condition
was there for a reason, but it did not jump at me.
Cheers,
- pyr
---
ui/vnc.c | 5 -----
1 file changed, 5 deletions(-)
diff --git ui/vnc.c ui/vnc.c
index 88f55cbf3c..b01a655400 100644
--- ui/vnc.c
+++ ui/vnc.c
@@ -3729,11 +3729,6 @@ static int vnc_display_get_address(const char *addrstr,
addr->type = SOCKET_ADDRESS_TYPE_UNIX;
addr->u.q_unix.path = g_strdup(addrstr + 5);
- if (websocket) {
- error_setg(errp, "UNIX sockets not supported with websock");
- goto cleanup;
- }
-
if (to) {
error_setg(errp, "Port range not support with UNIX socket");
goto cleanup;
--
2.38.1
On Fri, Dec 02, 2022 at 04:12:04PM +0100, Pierre-Yves Ritschard wrote: > Hi, > > The provided patch allows the VNC websocket server of a qemu process to > be provided over AF_UNIX as it is already possible for standard TCP VNC > servers. > > Now that many clients support websocket connections, some exclusively, > it can be useful to expose the VNC server. One such case is when a > proxy is present on a host machine, allowing it to proxy to a > deterministic location withouth having to keep track of port mappings. > > Removing the condition as is done in the provided patch creates a > functional server. If there's a downside to this approach I could not > figure it out while reading the code. My hunch was that the condition > was there for a reason, but it did not jump at me. > > Cheers, > - pyr > > --- > ui/vnc.c | 5 ----- > 1 file changed, 5 deletions(-) > > diff --git ui/vnc.c ui/vnc.c > index 88f55cbf3c..b01a655400 100644 > --- ui/vnc.c > +++ ui/vnc.c > @@ -3729,11 +3729,6 @@ static int vnc_display_get_address(const char *addrstr, > addr->type = SOCKET_ADDRESS_TYPE_UNIX; > addr->u.q_unix.path = g_strdup(addrstr + 5); > > - if (websocket) { > - error_setg(errp, "UNIX sockets not supported with websock"); > - goto cleanup; > - } > - Allowing websockets is fine, but just removing this check is not sufficient The 'websocket=XXXX' parameter for -vnc takes two formats websocket=on|off or websocket=portnum In the case of on|off, the code takes the original VNC display num and listens on 5700 + display for websockets, 590 + display for non-websockets. In the case of a explicit port, the code listens on that port. Also we fail to actually handle 'off' correctly, just treating it as a named port $ qemu-system-x86_64 -vnc :1,websocket=off qemu-system-x86_64: -vnc :1,websocket=off: address resolution failed for :off: Servname not supported for ai_socktype Anyway given an argument -vnc unix:/some/path,websocket=on this cause causes QEMU to listen on a relative path 'on'. We need to define what the semantics for websockets=on are going to be for UNIX sockets. Should it append '.ws' to the main path ? Should we just not allow websockets=on and document it must be an explicit path at all times ? We also need to document this in qemu-options.hx. > if (to) { > error_setg(errp, "Port range not support with UNIX socket"); > goto cleanup; > -- > 2.38.1 > > With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Allowing websockets is fine, but just removing this check is not > sufficient > > The 'websocket=XXXX' parameter for -vnc takes two formats > > websocket=on|off > > or > > websocket=portnum > > In the case of on|off, the code takes the original VNC display > num and listens on 5700 + display for websockets, 590 + display > for non-websockets. > > In the case of a explicit port, the code listens on that port. > > Also we fail to actually handle 'off' correctly, just treating > it as a named port > > $ qemu-system-x86_64 -vnc :1,websocket=off > qemu-system-x86_64: -vnc :1,websocket=off: address resolution failed for > :off: Servname not supported for ai_socktype > > > Anyway given an argument > > -vnc unix:/some/path,websocket=on > > this cause causes QEMU to listen on a relative path 'on'. We need > to define what the semantics for websockets=on are going to be > for UNIX sockets. Should it append '.ws' to the main path ? Should > we just not allow websockets=on and document it must be an explicit > path at all times ? > > We also need to document this in qemu-options.hx. > > > Thank you, these semantics weren't obvious to me, I will adapt accordingly and post a new patch
On Tue, 17 Jan 2023 at 13:05, Pierre-Yves Ritschard <pyr@spootnik.org> wrote: >> Allowing websockets is fine, but just removing this check is not >> sufficient > Thank you, these semantics weren't obvious to me, I will adapt accordingly and post a new patch When you do, please make sure you include a Signed-off-by: line. This is how you say you're legally OK to contribute the change and happy for it to go into QEMU, so we can't take code without one: https://www.qemu.org/docs/master/devel/submitting-a-patch.html#patch-emails-must-include-a-signed-off-by-line thanks -- PMM
Hi Gerd, If this patch is correct I can queue it via trivial branch. Thanks, Laurent Le 02/12/2022 à 16:12, Pierre-Yves Ritschard a écrit : > Hi, > > The provided patch allows the VNC websocket server of a qemu process to > be provided over AF_UNIX as it is already possible for standard TCP VNC > servers. > > Now that many clients support websocket connections, some exclusively, > it can be useful to expose the VNC server. One such case is when a > proxy is present on a host machine, allowing it to proxy to a > deterministic location withouth having to keep track of port mappings. > > Removing the condition as is done in the provided patch creates a > functional server. If there's a downside to this approach I could not > figure it out while reading the code. My hunch was that the condition > was there for a reason, but it did not jump at me. > > Cheers, > - pyr > > --- > ui/vnc.c | 5 ----- > 1 file changed, 5 deletions(-) > > diff --git ui/vnc.c ui/vnc.c > index 88f55cbf3c..b01a655400 100644 > --- ui/vnc.c > +++ ui/vnc.c > @@ -3729,11 +3729,6 @@ static int vnc_display_get_address(const char *addrstr, > addr->type = SOCKET_ADDRESS_TYPE_UNIX; > addr->u.q_unix.path = g_strdup(addrstr + 5); > > - if (websocket) { > - error_setg(errp, "UNIX sockets not supported with websock"); > - goto cleanup; > - } > - > if (to) { > error_setg(errp, "Port range not support with UNIX socket"); > goto cleanup;
On Mon, Jan 16, 2023 at 07:15:08PM +0100, Laurent Vivier wrote: > Hi Gerd, > > If this patch is correct I can queue it via trivial branch. proxying tcp websocket connections to a unix socket looks like a reasonable use case to me. Acked-by: Gerd Hoffmann <kraxel@redhat.com>
On Tue, Jan 17, 2023 at 01:43:12PM +0100, Gerd Hoffmann wrote: > On Mon, Jan 16, 2023 at 07:15:08PM +0100, Laurent Vivier wrote: > > Hi Gerd, > > > > If this patch is correct I can queue it via trivial branch. > > proxying tcp websocket connections to a unix socket looks like > a reasonable use case to me. > > Acked-by: Gerd Hoffmann <kraxel@redhat.com> Please don't queue, this patch is not correct becasue it is failing to validate the 'websocket' parameter at all. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Le 17/01/2023 à 13:53, Daniel P. Berrangé a écrit : > On Tue, Jan 17, 2023 at 01:43:12PM +0100, Gerd Hoffmann wrote: >> On Mon, Jan 16, 2023 at 07:15:08PM +0100, Laurent Vivier wrote: >>> Hi Gerd, >>> >>> If this patch is correct I can queue it via trivial branch. >> >> proxying tcp websocket connections to a unix socket looks like >> a reasonable use case to me. >> >> Acked-by: Gerd Hoffmann <kraxel@redhat.com> > > Please don't queue, this patch is not correct becasue it is failing > to validate the 'websocket' parameter at all. OK, I will not. Thank you for the comment. Laurent
> Please don't queue, this patch is not correct becasue it is failing > to validate the 'websocket' parameter at all. > > Hi, Thanks for the review! I'm happy to adapt the patch to add validation, though I am not quite sure what additional validation you would like to see occur here. Cheers, Pierre-Yves
© 2016 - 2024 Red Hat, Inc.