[Qemu-devel] [PATCH] add VNC reverse connections

[Qemu-devel] [PATCH] add VNC reverse connections

[Eddie Kohler]

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

Hi all,

This patch against current CVS adds VNC reverse connections, where the server 
connects actively to a waiting client, as in "-vnc rev:5500" or "-vnc 
rev:read.cs.ucla.edu:5500".  This is quite useful if the user expects to run 
QEMU many times in succession (for example, is debugging a toy OS), and 
doesn't want to reopen a VNC client each time.

Eddie


[-- Attachment #2: qemu-cvs-vnc-rev.patch --]
[-- Type: text/x-patch, Size: 3943 bytes --]

Index: qemu-doc.texi
===================================================================
RCS file: /sources/qemu/qemu/qemu-doc.texi,v
retrieving revision 1.179
diff -u -r1.179 qemu-doc.texi
--- qemu-doc.texi	14 Jan 2008 22:09:11 -0000	1.179
+++ qemu-doc.texi	16 Jan 2008 07:25:30 -0000
@@ -429,10 +429,16 @@
 Connections will be allowed over UNIX domain sockets where @var{path} is the
 location of a unix socket to listen for connections on.
 
+@item @var{rev}:@var{interface}:@var{port}
+
+Makes a reverse connection to a VNC viewer listening on
+@var{interface} on port @var{port}. Optionally, @var{interface} can be
+omitted in which case QEMU will connect to @var{port} on the local machine.
+
 @item none
 
-VNC is initialized by not started. The monitor @code{change} command can be used
-to later start the VNC server.
+VNC is initialized but not started. The monitor @code{change} command
+can be used to later start the VNC server.
 
 @end table
 
Index: vnc.c
===================================================================
RCS file: /sources/qemu/qemu/vnc.c,v
retrieving revision 1.33
diff -u -r1.33 vnc.c
--- vnc.c	14 Jan 2008 21:45:55 -0000	1.33
+++ vnc.c	16 Jan 2008 07:25:30 -0000
@@ -1898,6 +1898,22 @@
     return 0;
 }
 
+static void vnc_connect(VncState *vs)
+{
+    VNC_DEBUG("New client on socket %d\n", vs->csock);
+    socket_set_nonblock(vs->csock);
+    qemu_set_fd_handler2(vs->csock, NULL, vnc_client_read, NULL, vs);
+    vnc_write(vs, "RFB 003.008\n", 12);
+    vnc_flush(vs);
+    vnc_read_when(vs, protocol_version, 12);
+    memset(vs->old_data, 0, vs->ds->linesize * vs->ds->height);
+    memset(vs->dirty_row, 0xFF, sizeof(vs->dirty_row));
+    vs->has_resize = 0;
+    vs->has_hextile = 0;
+    vs->ds->dpy_copy = NULL;
+    vnc_update_client(vs);
+}
+
 static void vnc_listen_read(void *opaque)
 {
     VncState *vs = opaque;
@@ -1909,18 +1925,7 @@
 
     vs->csock = accept(vs->lsock, (struct sockaddr *)&addr, &addrlen);
     if (vs->csock != -1) {
-	VNC_DEBUG("New client on socket %d\n", vs->csock);
-        socket_set_nonblock(vs->csock);
-	qemu_set_fd_handler2(vs->csock, NULL, vnc_client_read, NULL, opaque);
-	vnc_write(vs, "RFB 003.008\n", 12);
-	vnc_flush(vs);
-	vnc_read_when(vs, protocol_version, 12);
-	memset(vs->old_data, 0, vs->ds->linesize * vs->ds->height);
-	memset(vs->dirty_row, 0xFF, sizeof(vs->dirty_row));
-	vs->has_resize = 0;
-	vs->has_hextile = 0;
-	vs->ds->dpy_copy = NULL;
-        vnc_update_client(vs);
+	vnc_connect(vs);
     }
 }
 
@@ -2087,6 +2092,7 @@
     VncState *vs = ds ? (VncState *)ds->opaque : vnc_state;
     const char *options;
     int password = 0;
+    int reverse = 0;
 #if CONFIG_VNC_TLS
     int tls = 0, x509 = 0;
 #endif
@@ -2179,6 +2185,13 @@
 	}
 #endif
     }
+    if (strstart(display, "rev:", &p)) {
+	reverse = 1;
+	display = p;
+	if (!strchr(display, ':')) {
+	    display = p - 1;
+	}
+    }
 #ifndef _WIN32
     if (strstart(display, "unix:", &p)) {
 	addr = (struct sockaddr *)&uaddr;
@@ -2196,7 +2209,9 @@
 	memset(uaddr.sun_path, 0, 108);
 	snprintf(uaddr.sun_path, 108, "%s", p);
 
-	unlink(uaddr.sun_path);
+	if (!reverse) {
+	    unlink(uaddr.sun_path);
+	}
     } else
 #endif
     {
@@ -2210,7 +2225,7 @@
 	    return -1;
 	}
 
-	iaddr.sin_port = htons(ntohs(iaddr.sin_port) + 5900);
+	iaddr.sin_port = htons(ntohs(iaddr.sin_port) + (reverse ? 0 : 5900));
 
 	vs->lsock = socket(PF_INET, SOCK_STREAM, 0);
 	if (vs->lsock == -1) {
@@ -2233,6 +2248,22 @@
 	}
     }
 
+    if (reverse) {
+	if (connect(vs->lsock, addr, addrlen) == -1) {
+	    fprintf(stderr, "Connection to VNC client failed\n");
+	    close(vs->lsock);
+	    vs->lsock = -1;
+	    free(vs->display);
+	    vs->display = NULL;
+	    return -1;
+	} else {
+	    vs->csock = vs->lsock;
+	    vs->lsock = -1;
+	    vnc_connect(vs);
+	    return 0;
+	}
+    }
+
     if (bind(vs->lsock, addr, addrlen) == -1) {
 	fprintf(stderr, "bind() failed\n");
 	close(vs->lsock);

Re: [Qemu-devel] [PATCH] add VNC reverse connections

[Avi Kivity]

Eddie Kohler wrote:
> Hi all,
>
> This patch against current CVS adds VNC reverse connections, where the 
> server connects actively to a waiting client, as in "-vnc rev:5500" or 
> "-vnc rev:read.cs.ucla.edu:5500".  This is quite useful if the user 
> expects to run QEMU many times in succession (for example, is 
> debugging a toy OS), and doesn't want to reopen a VNC client each time.
>

Neat!

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] add VNC reverse connections

[Daniel P. Berrange]

On Tue, Jan 15, 2008 at 11:27:15PM -0800, Eddie Kohler wrote:
> Hi all,
> 
> This patch against current CVS adds VNC reverse connections, where the 
> server connects actively to a waiting client, as in "-vnc rev:5500" or 
> "-vnc rev:read.cs.ucla.edu:5500".  This is quite useful if the user expects 
> to run QEMU many times in succession (for example, is debugging a toy OS), 
> and doesn't want to reopen a VNC client each time.

We already have the ability to pass multiple flags / options to the VNC
driver as a post-fix to the host:port pair, so I'm not a fan of introducing
a new option as a prefix. If using existing options syntax, it could look
like:

  -vnc :5500,rev
  -vnc read.cs.ucla.edu:5500,rev

Regards,
Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 

Re: [Qemu-devel] [PATCH] add VNC reverse connections

[Anthony Liguori]

Daniel P. Berrange wrote:
> On Tue, Jan 15, 2008 at 11:27:15PM -0800, Eddie Kohler wrote:
>   
>> Hi all,
>>
>> This patch against current CVS adds VNC reverse connections, where the 
>> server connects actively to a waiting client, as in "-vnc rev:5500" or 
>> "-vnc rev:read.cs.ucla.edu:5500".  This is quite useful if the user expects 
>> to run QEMU many times in succession (for example, is debugging a toy OS), 
>> and doesn't want to reopen a VNC client each time.
>>     
>
> We already have the ability to pass multiple flags / options to the VNC
> driver as a post-fix to the host:port pair, so I'm not a fan of introducing
> a new option as a prefix. If using existing options syntax, it could look
> like:
>
>   -vnc :5500,rev
>   -vnc read.cs.ucla.edu:5500,rev
>   

Yes, but please, let's spell out "reverse".  This is a very nice patch 
to have as it's another way (than -daemonize) to reliably connect to the 
vnc server after launching a VM.

Regards,

Anthony Liguori

> Regards,
> Dan.
>   

Re: [Qemu-devel] [PATCH] add VNC reverse connections

[Eddie Kohler]

Daniel P. Berrange wrote:
> We already have the ability to pass multiple flags / options to the VNC
> driver as a post-fix to the host:port pair, so I'm not a fan of introducing
> a new option as a prefix. If using existing options syntax, it could look
> like:
> 
>   -vnc :5500,rev
>   -vnc read.cs.ucla.edu:5500,rev

This doesn't feel like an option to me, though; rather a different means of 
connecting.  Among other things, in "-vnc :0", the QEMU VNC server opens port 
5900.  But the client's listening port for reverse connections defaults to 
5500.  "-vnc :-400,rev" is clearly insane, but it seems strange for an option 
like ",rev" to change the meaning of the port field.

If you still disagree I'll produce a patch with ",reverse" as an option.

Eddie

Re: [Qemu-devel] [PATCH] add VNC reverse connections

[Anthony Liguori]

Eddie Kohler wrote:
> Daniel P. Berrange wrote:
>> We already have the ability to pass multiple flags / options to the VNC
>> driver as a post-fix to the host:port pair, so I'm not a fan of 
>> introducing
>> a new option as a prefix. If using existing options syntax, it could 
>> look
>> like:
>>
>>   -vnc :5500,rev
>>   -vnc read.cs.ucla.edu:5500,rev
>
> This doesn't feel like an option to me, though; rather a different 
> means of connecting.  Among other things, in "-vnc :0", the QEMU VNC 
> server opens port 5900.  But the client's listening port for reverse 
> connections defaults to 5500.  "-vnc :-400,rev" is clearly insane, but 
> it seems strange for an option like ",rev" to change the meaning of 
> the port field.
>
> If you still disagree I'll produce a patch with ",reverse" as an option.

I'm not a huge fan of the "option,mod,mod" syntax but Dan is right, it's 
what is used in QEMU.  Consistency is often times better than sanity :-)

Regards,

Anthony Liguori

> Eddie
>
>

Re: [Qemu-devel] [PATCH] add VNC reverse connections

[Daniel P. Berrange]

On Wed, Jan 16, 2008 at 07:09:03AM -0800, Eddie Kohler wrote:
> Daniel P. Berrange wrote:
> >We already have the ability to pass multiple flags / options to the VNC
> >driver as a post-fix to the host:port pair, so I'm not a fan of introducing
> >a new option as a prefix. If using existing options syntax, it could look
> >like:
> >
> >  -vnc :5500,rev
> >  -vnc read.cs.ucla.edu:5500,rev
> 
> This doesn't feel like an option to me, though; rather a different means of 
> connecting.  Among other things, in "-vnc :0", the QEMU VNC server opens 
> port 5900.  But the client's listening port for reverse connections 
> defaults to 5500.  "-vnc :-400,rev" is clearly insane, but it seems strange 
> for an option like ",rev" to change the meaning of the port field.

Yes that is a valid point. It is a little unfortunate we switched to using
display num instead of port num for the current VNC code. Having a syntax
which makes people use negative display nums for reverse connections would
suck. So reluctantly I think your original proposal may actually be better.

Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 

Re: [Qemu-devel] [PATCH] add VNC reverse connections

[Anthony Liguori]

Daniel P. Berrange wrote:
> On Wed, Jan 16, 2008 at 07:09:03AM -0800, Eddie Kohler wrote:
>   
>> Daniel P. Berrange wrote:
>>     
>>> We already have the ability to pass multiple flags / options to the VNC
>>> driver as a post-fix to the host:port pair, so I'm not a fan of introducing
>>> a new option as a prefix. If using existing options syntax, it could look
>>> like:
>>>
>>>  -vnc :5500,rev
>>>  -vnc read.cs.ucla.edu:5500,rev
>>>       
>> This doesn't feel like an option to me, though; rather a different means of 
>> connecting.  Among other things, in "-vnc :0", the QEMU VNC server opens 
>> port 5900.  But the client's listening port for reverse connections 
>> defaults to 5500.  "-vnc :-400,rev" is clearly insane, but it seems strange 
>> for an option like ",rev" to change the meaning of the port field.
>>     
>
> Yes that is a valid point. It is a little unfortunate we switched to using
> display num instead of port num for the current VNC code. Having a syntax
> which makes people use negative display nums for reverse connections would
> suck. So reluctantly I think your original proposal may actually be better.
>   

Yet this is the syntax we use for normal connections.  I don't see why 
the asymmetry is okay for reverse connections.

Regard,

Anthony Liguori

> Dan.
>   

Re: [Qemu-devel] [PATCH] add VNC reverse connections

[Eddie Kohler]

Anthony Liguori wrote:
>>> This doesn't feel like an option to me, though; rather a different 
>>> means of connecting.  Among other things, in "-vnc :0", the QEMU VNC 
>>> server opens port 5900.  But the client's listening port for reverse 
>>> connections defaults to 5500.  "-vnc :-400,rev" is clearly insane, 
>>> but it seems strange for an option like ",rev" to change the meaning 
>>> of the port field.
>>>     
>>
>> Yes that is a valid point. It is a little unfortunate we switched to 
>> using
>> display num instead of port num for the current VNC code. Having a syntax
>> which makes people use negative display nums for reverse connections 
>> would
>> suck. So reluctantly I think your original proposal may actually be 
>> better.
> 
> Yet this is the syntax we use for normal connections.  I don't see why 
> the asymmetry is okay for reverse connections.

Because reverse connections feel very different from normal connections, use 
ports instead of "display numbers", etc.?

Here's what the manual would look like for the "rev:" syntax.


interface:d
TCP connections will only be allowed from interface on display d. By convention 
the TCP port is 5900+d. Optionally, interface can be omitted in which case the 
server will bind to all interfaces.
...
rev:[address:]port
Connects to a VNC client listening at address:port. Optionally, address can be 
omitted in which case the server connects to localhost:port.


Here's what the manual would look like for the ",reverse" syntax.


Valid syntax for the display is

interface:d
TCP connections will only be allowed from interface on display d. By convention 
the TCP port is 5900+d. Optionally, interface can be omitted in which case the 
server will bind to all interfaces.
...
reverse
QEMU will connect to a listening VNC client, rather than waiting for a client 
connection.  If the connection has type "interface:d", then "interface" is the 
client address; if omitted, localhost is used. The display number "d" is added 
to 5900 to determine the port, so negative numbers might be necessary to 
connect to default client ports.


Or are you thinking


For reverse connections, the display number "d" is added to 5500 to determine 
the port.


in which case the interface:d definition is incorrect?

I'm not trying to make this look ugly on purpose.  The prefix syntax seems 
friendlier.

Eddie

Re: [Qemu-devel] [PATCH] add VNC reverse connections

[Eddie Kohler]

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

Well, attached is a patch with ",reverse" syntax, anyway; hopefully one of 
these syntaxes is OK!

Eddie


Anthony Liguori wrote:
> Daniel P. Berrange wrote:
>> On Wed, Jan 16, 2008 at 07:09:03AM -0800, Eddie Kohler wrote:
>>  
>>> Daniel P. Berrange wrote:
>>>    
>>>> We already have the ability to pass multiple flags / options to the VNC
>>>> driver as a post-fix to the host:port pair, so I'm not a fan of 
>>>> introducing
>>>> a new option as a prefix. If using existing options syntax, it could 
>>>> look
>>>> like:
>>>>
>>>>  -vnc :5500,rev
>>>>  -vnc read.cs.ucla.edu:5500,rev


[-- Attachment #2: qemu-cvs-vnc-reverse.patch --]
[-- Type: text/x-patch, Size: 4700 bytes --]

Index: qemu-doc.texi
===================================================================
RCS file: /sources/qemu/qemu/qemu-doc.texi,v
retrieving revision 1.179
diff -u -r1.179 qemu-doc.texi
--- qemu-doc.texi	14 Jan 2008 22:09:11 -0000	1.179
+++ qemu-doc.texi	17 Jan 2008 16:28:44 -0000
@@ -418,21 +418,21 @@
 
 @table @code
 
-@item @var{interface}:@var{d}
+@item @var{host}:@var{d}
 
-TCP connections will only be allowed from @var{interface} on display @var{d}.
-By convention the TCP port is 5900+@var{d}. Optionally, @var{interface} can
-be omitted in which case the server will bind to all interfaces.
+TCP connections will only be allowed from @var{host} on display @var{d}.
+By convention the TCP port is 5900+@var{d}. Optionally, @var{host} can
+be omitted in which case the server will accept connections from any host.
 
-@item @var{unix}:@var{path}
+@item @code{unix}:@var{path}
 
 Connections will be allowed over UNIX domain sockets where @var{path} is the
 location of a unix socket to listen for connections on.
 
 @item none
 
-VNC is initialized by not started. The monitor @code{change} command can be used
-to later start the VNC server.
+VNC is initialized but not started. The monitor @code{change} command
+can be used to later start the VNC server.
 
 @end table
 
@@ -441,6 +441,13 @@
 
 @table @code
 
+@item reverse
+
+Connect to a listening VNC client via a ``reverse'' connection. The
+client is specified by the @var{display}. For reverse network
+connections (@var{host}:@var{d},@code{reverse}), the @var{d} argument
+is a TCP port number, not a display number.
+
 @item password
 
 Require that password based authentication is used for client connections.
Index: vnc.c
===================================================================
RCS file: /sources/qemu/qemu/vnc.c,v
retrieving revision 1.33
diff -u -r1.33 vnc.c
--- vnc.c	14 Jan 2008 21:45:55 -0000	1.33
+++ vnc.c	17 Jan 2008 16:28:44 -0000
@@ -1898,6 +1898,22 @@
     return 0;
 }
 
+static void vnc_connect(VncState *vs)
+{
+    VNC_DEBUG("New client on socket %d\n", vs->csock);
+    socket_set_nonblock(vs->csock);
+    qemu_set_fd_handler2(vs->csock, NULL, vnc_client_read, NULL, vs);
+    vnc_write(vs, "RFB 003.008\n", 12);
+    vnc_flush(vs);
+    vnc_read_when(vs, protocol_version, 12);
+    memset(vs->old_data, 0, vs->ds->linesize * vs->ds->height);
+    memset(vs->dirty_row, 0xFF, sizeof(vs->dirty_row));
+    vs->has_resize = 0;
+    vs->has_hextile = 0;
+    vs->ds->dpy_copy = NULL;
+    vnc_update_client(vs);
+}
+
 static void vnc_listen_read(void *opaque)
 {
     VncState *vs = opaque;
@@ -1909,18 +1925,7 @@
 
     vs->csock = accept(vs->lsock, (struct sockaddr *)&addr, &addrlen);
     if (vs->csock != -1) {
-	VNC_DEBUG("New client on socket %d\n", vs->csock);
-        socket_set_nonblock(vs->csock);
-	qemu_set_fd_handler2(vs->csock, NULL, vnc_client_read, NULL, opaque);
-	vnc_write(vs, "RFB 003.008\n", 12);
-	vnc_flush(vs);
-	vnc_read_when(vs, protocol_version, 12);
-	memset(vs->old_data, 0, vs->ds->linesize * vs->ds->height);
-	memset(vs->dirty_row, 0xFF, sizeof(vs->dirty_row));
-	vs->has_resize = 0;
-	vs->has_hextile = 0;
-	vs->ds->dpy_copy = NULL;
-        vnc_update_client(vs);
+	vnc_connect(vs);
     }
 }
 
@@ -2087,6 +2092,7 @@
     VncState *vs = ds ? (VncState *)ds->opaque : vnc_state;
     const char *options;
     int password = 0;
+    int reverse = 0;
 #if CONFIG_VNC_TLS
     int tls = 0, x509 = 0;
 #endif
@@ -2103,6 +2109,8 @@
 	options++;
 	if (strncmp(options, "password", 8) == 0) {
 	    password = 1; /* Require password auth */
+	} else if (strncmp(options, "reverse", 7) == 0) {
+	    reverse = 1;
 #if CONFIG_VNC_TLS
 	} else if (strncmp(options, "tls", 3) == 0) {
 	    tls = 1; /* Require TLS */
@@ -2196,7 +2204,9 @@
 	memset(uaddr.sun_path, 0, 108);
 	snprintf(uaddr.sun_path, 108, "%s", p);
 
-	unlink(uaddr.sun_path);
+	if (!reverse) {
+	    unlink(uaddr.sun_path);
+	}
     } else
 #endif
     {
@@ -2210,7 +2220,7 @@
 	    return -1;
 	}
 
-	iaddr.sin_port = htons(ntohs(iaddr.sin_port) + 5900);
+	iaddr.sin_port = htons(ntohs(iaddr.sin_port) + (reverse ? 0 : 5900));
 
 	vs->lsock = socket(PF_INET, SOCK_STREAM, 0);
 	if (vs->lsock == -1) {
@@ -2233,6 +2243,22 @@
 	}
     }
 
+    if (reverse) {
+	if (connect(vs->lsock, addr, addrlen) == -1) {
+	    fprintf(stderr, "Connection to VNC client failed\n");
+	    close(vs->lsock);
+	    vs->lsock = -1;
+	    free(vs->display);
+	    vs->display = NULL;
+	    return -1;
+	} else {
+	    vs->csock = vs->lsock;
+	    vs->lsock = -1;
+	    vnc_connect(vs);
+	    return 0;
+	}
+    }
+
     if (bind(vs->lsock, addr, addrlen) == -1) {
 	fprintf(stderr, "bind() failed\n");
 	close(vs->lsock);