[2/3] Introduce a 'client_add' monitor command accepting an open FD

Allow client connections for VNC and socket based character
devices to be passed in over the monitor using SCM_RIGHTS.

One intended usage scenario is to start QEMU with VNC on a
UNIX domain socket. An unprivileged user which cannot access
the UNIX domain socket, can then connect to QEMU's VNC server
by passing an open FD to libvirt, which passes it onto QEMU.

 { "execute": "get_fd", "arguments": { "fdname": "myclient" } }
 { "return": {} }
 { "execute": "add_client", "arguments": { "protocol": "vnc",
                                           "fdname": "myclient",
                                           "skipauth": true } }
 { "return": {} }

In this case 'protocol' can be 'vnc' or 'spice', or the name
of a character device (eg from -chardev id=XXXX)

The 'skipauth' parameter can be used to skip any configured
VNC authentication scheme, which is useful if the mgmt layer
talking to the monitor has already authenticated the client
in another way.

* console.h: Define 'vnc_display_add_client' method
* monitor.c: Implement 'client_add' command
* qemu-char.c, qemu-char.h: Add 'qemu_char_add_client' method
* qerror.c, qerror.h: Add QERR_ADD_CLIENT_FAILED
* qmp-commands.hx: Declare 'client_add' command
* ui/vnc.c: Implement 'vnc_display_add_client' method
---
 console.h       |    1 +
 monitor.c       |   32 ++++++++++++++++++++++++++++++++
 qemu-char.c     |   30 ++++++++++++++++++++++++------
 qemu-char.h     |    2 ++
 qerror.c        |    4 ++++
 qerror.h        |    3 +++
 qmp-commands.hx |   27 +++++++++++++++++++++++++++
 ui/vnc.c        |    7 +++++++
 8 files changed, 100 insertions(+), 6 deletions(-)

On 23/06/2011 14:31, Daniel P. Berrange wrote:
> Allow client connections for VNC and socket based character
> devices to be passed in over the monitor using SCM_RIGHTS.
> 

Hello Daniel,

I'm a little bit late but it seems that there's a build error with this patch
when VNC is disabled.

$./configure --target-list=ppc-softmmu --disable-vnc
...
$ make
...
monitor.o: In function `add_graphics_client':
.../monitor.c:1205: undefined reference to `vnc_display_add_client'

Regards,

On Tue, Jul 26, 2011 at 05:20:16PM +0200, Fabien Chouteau wrote:
> On 23/06/2011 14:31, Daniel P. Berrange wrote:
> > Allow client connections for VNC and socket based character
> > devices to be passed in over the monitor using SCM_RIGHTS.
> > 
> 
> Hello Daniel,
> 
> I'm a little bit late but it seems that there's a build error with this patch
> when VNC is disabled.
> 
> $./configure --target-list=ppc-softmmu --disable-vnc
> ...
> $ make
> ...
> monitor.o: In function `add_graphics_client':
> .../monitor.c:1205: undefined reference to `vnc_display_add_client'

There was already a patch posted yesterday which should fix this
problem:

  Subject: [PATCH] monitor: fix build breakage with --disable-vnc

Regards,
Daniel

On 26/07/2011 17:29, Daniel P. Berrange wrote:
> On Tue, Jul 26, 2011 at 05:20:16PM +0200, Fabien Chouteau wrote:
>> On 23/06/2011 14:31, Daniel P. Berrange wrote:
>>> Allow client connections for VNC and socket based character
>>> devices to be passed in over the monitor using SCM_RIGHTS.
>>>
>>
>> Hello Daniel,
>>
>> I'm a little bit late but it seems that there's a build error with this patch
>> when VNC is disabled.
>>
>> $./configure --target-list=ppc-softmmu --disable-vnc
>> ...
>> $ make
>> ...
>> monitor.o: In function `add_graphics_client':
>> .../monitor.c:1205: undefined reference to `vnc_display_add_client'
> 
> There was already a patch posted yesterday which should fix this
> problem:
> 
>   Subject: [PATCH] monitor: fix build breakage with --disable-vnc
> 

OK, thanks.

Sorry about the noise...

On 06/23/2011 07:31 AM, Daniel P. Berrange wrote:
> Allow client connections for VNC and socket based character
> devices to be passed in over the monitor using SCM_RIGHTS.
>
> One intended usage scenario is to start QEMU with VNC on a
> UNIX domain socket. An unprivileged user which cannot access
> the UNIX domain socket, can then connect to QEMU's VNC server
> by passing an open FD to libvirt, which passes it onto QEMU.
>
>   { "execute": "get_fd", "arguments": { "fdname": "myclient" } }
>   { "return": {} }
>   { "execute": "add_client", "arguments": { "protocol": "vnc",
>                                             "fdname": "myclient",
>                                             "skipauth": true } }
>   { "return": {} }
>
> In this case 'protocol' can be 'vnc' or 'spice', or the name
> of a character device (eg from -chardev id=XXXX)
>
> The 'skipauth' parameter can be used to skip any configured
> VNC authentication scheme, which is useful if the mgmt layer
> talking to the monitor has already authenticated the client
> in another way.
>
> * console.h: Define 'vnc_display_add_client' method
> * monitor.c: Implement 'client_add' command
> * qemu-char.c, qemu-char.h: Add 'qemu_char_add_client' method

I don't feel all that good about this anymore.  The notion of adding a 
fd to an arbitrary character device is a big layering violation and 
doesn't make much sense conceptually.

A chardev cannot have multiple connections.  It seems like the use-case 
is much better suited to having an fd: protocol to create char devices with.

I'd like to partially revert this removing the qemu_chr_add_client 
interface.

Regards,

Anthony Liguori

> * qerror.c, qerror.h: Add QERR_ADD_CLIENT_FAILED
> * qmp-commands.hx: Declare 'client_add' command
> * ui/vnc.c: Implement 'vnc_display_add_client' method
> ---
>   console.h       |    1 +
>   monitor.c       |   32 ++++++++++++++++++++++++++++++++
>   qemu-char.c     |   30 ++++++++++++++++++++++++------
>   qemu-char.h     |    2 ++
>   qerror.c        |    4 ++++
>   qerror.h        |    3 +++
>   qmp-commands.hx |   27 +++++++++++++++++++++++++++
>   ui/vnc.c        |    7 +++++++
>   8 files changed, 100 insertions(+), 6 deletions(-)
>
> diff --git a/console.h b/console.h
> index 64d1f09..84d3e8d 100644
> --- a/console.h
> +++ b/console.h
> @@ -372,6 +372,7 @@ void cocoa_display_init(DisplayState *ds, int full_screen);
>   void vnc_display_init(DisplayState *ds);
>   void vnc_display_close(DisplayState *ds);
>   int vnc_display_open(DisplayState *ds, const char *display);
> +void vnc_display_add_client(DisplayState *ds, int csock, int skipauth);
>   int vnc_display_disable_login(DisplayState *ds);
>   char *vnc_display_local_addr(DisplayState *ds);
>   #ifdef CONFIG_VNC
> diff --git a/monitor.c b/monitor.c
> index 6af6a4d..23c310e 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -1185,6 +1185,38 @@ static int expire_password(Monitor *mon, const QDict *qdict, QObject **ret_data)
>       return -1;
>   }
>
> +static int add_graphics_client(Monitor *mon, const QDict *qdict, QObject **ret_data)
> +{
> +    const char *protocol  = qdict_get_str(qdict, "protocol");
> +    const char *fdname = qdict_get_str(qdict, "fdname");
> +    int skipauth = qdict_get_try_bool(qdict, "skipauth", 0);
> +    CharDriverState *s;
> +
> +    if (strcmp(protocol, "spice") == 0) {
> +        if (!using_spice) {
> +            /* correct one? spice isn't a device ,,, */
> +            qerror_report(QERR_DEVICE_NOT_ACTIVE, "spice");
> +            return -1;
> +        }
> +	qerror_report(QERR_ADD_CLIENT_FAILED);
> +	return -1;
> +    } else if (strcmp(protocol, "vnc") == 0) {
> +	int fd = monitor_get_fd(mon, fdname);
> +	vnc_display_add_client(NULL, fd, skipauth);
> +	return 0;
> +    } else if ((s = qemu_chr_find(protocol)) != NULL) {
> +	int fd = monitor_get_fd(mon, fdname);
> +	if (qemu_chr_add_client(s, fd)<  0) {
> +	    qerror_report(QERR_ADD_CLIENT_FAILED);
> +	    return -1;
> +	}
> +	return 0;
> +    }
> +
> +    qerror_report(QERR_INVALID_PARAMETER, "protocol");
> +    return -1;
> +}
> +
>   static int client_migrate_info(Monitor *mon, const QDict *qdict, QObject **ret_data)
>   {
>       const char *protocol = qdict_get_str(qdict, "protocol");
> diff --git a/qemu-char.c b/qemu-char.c
> index fb13b28..4e168ee 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -168,6 +168,11 @@ int qemu_chr_get_msgfd(CharDriverState *s)
>       return s->get_msgfd ? s->get_msgfd(s) : -1;
>   }
>
> +int qemu_chr_add_client(CharDriverState *s, int fd)
> +{
> +    return s->chr_add_client ? s->chr_add_client(s, fd) : -1;
> +}
> +
>   void qemu_chr_accept_input(CharDriverState *s)
>   {
>       if (s->chr_accept_input)
> @@ -2123,6 +2128,22 @@ static void socket_set_nodelay(int fd)
>       setsockopt(fd, IPPROTO_TCP, TCP_NODELAY, (char *)&val, sizeof(val));
>   }
>
> +static int tcp_chr_add_client(CharDriverState *chr, int fd)
> +{
> +    TCPCharDriver *s = chr->opaque;
> +    if (s->fd != -1)
> +	return -1;
> +
> +    socket_set_nonblock(fd);
> +    if (s->do_nodelay)
> +        socket_set_nodelay(fd);
> +    s->fd = fd;
> +    qemu_set_fd_handler(s->listen_fd, NULL, NULL, NULL);
> +    tcp_chr_connect(chr);
> +
> +    return 0;
> +}
> +
>   static void tcp_chr_accept(void *opaque)
>   {
>       CharDriverState *chr = opaque;
> @@ -2155,12 +2176,8 @@ static void tcp_chr_accept(void *opaque)
>               break;
>           }
>       }
> -    socket_set_nonblock(fd);
> -    if (s->do_nodelay)
> -        socket_set_nodelay(fd);
> -    s->fd = fd;
> -    qemu_set_fd_handler(s->listen_fd, NULL, NULL, NULL);
> -    tcp_chr_connect(chr);
> +    if (tcp_chr_add_client(chr, fd)<  0)
> +	close(fd);
>   }
>
>   static void tcp_chr_close(CharDriverState *chr)
> @@ -2230,6 +2247,7 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
>       chr->chr_write = tcp_chr_write;
>       chr->chr_close = tcp_chr_close;
>       chr->get_msgfd = tcp_get_msgfd;
> +    chr->chr_add_client = tcp_chr_add_client;
>
>       if (is_listen) {
>           s->listen_fd = fd;
> diff --git a/qemu-char.h b/qemu-char.h
> index 892c6da..f361c6d 100644
> --- a/qemu-char.h
> +++ b/qemu-char.h
> @@ -57,6 +57,7 @@ struct CharDriverState {
>       void (*chr_update_read_handler)(struct CharDriverState *s);
>       int (*chr_ioctl)(struct CharDriverState *s, int cmd, void *arg);
>       int (*get_msgfd)(struct CharDriverState *s);
> +    int (*chr_add_client)(struct CharDriverState *chr, int fd);
>       IOEventHandler *chr_event;
>       IOCanReadHandler *chr_can_read;
>       IOReadHandler *chr_read;
> @@ -99,6 +100,7 @@ int qemu_chr_can_read(CharDriverState *s);
>   void qemu_chr_read(CharDriverState *s, uint8_t *buf, int len);
>   int qemu_chr_get_msgfd(CharDriverState *s);
>   void qemu_chr_accept_input(CharDriverState *s);
> +int qemu_chr_add_client(CharDriverState *s, int fd);
>   void qemu_chr_info_print(Monitor *mon, const QObject *ret_data);
>   void qemu_chr_info(Monitor *mon, QObject **ret_data);
>   CharDriverState *qemu_chr_find(const char *name);
> diff --git a/qerror.c b/qerror.c
> index d7fcd93..79be1ba 100644
> --- a/qerror.c
> +++ b/qerror.c
> @@ -193,6 +193,10 @@ static const QErrorStringTable qerror_table[] = {
>           .desc      = "Could not set password",
>       },
>       {
> +        .error_fmt = QERR_ADD_CLIENT_FAILED,
> +        .desc      = "Could not add client",
> +    },
> +    {
>           .error_fmt = QERR_TOO_MANY_FILES,
>           .desc      = "Too many open files",
>       },
> diff --git a/qerror.h b/qerror.h
> index 16c830d..25773cd 100644
> --- a/qerror.h
> +++ b/qerror.h
> @@ -163,6 +163,9 @@ QError *qobject_to_qerror(const QObject *obj);
>   #define QERR_SET_PASSWD_FAILED \
>       "{ 'class': 'SetPasswdFailed', 'data': {} }"
>
> +#define QERR_ADD_CLIENT_FAILED \
> +    "{ 'class': 'AddClientFailed', 'data': {} }"
> +
>   #define QERR_TOO_MANY_FILES \
>       "{ 'class': 'TooManyFiles', 'data': {} }"
>
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 92c5c3a..1e19abc 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -885,6 +885,33 @@ Example:
>   EQMP
>
>       {
> +        .name       = "add_client",
> +        .args_type  = "protocol:s,fdname:s,skipauth:b?",
> +        .params     = "protocol fdname skipauth",
> +        .help       = "add a graphics client",
> +        .user_print = monitor_user_noop,
> +        .mhandler.cmd_new = add_graphics_client,
> +    },
> +
> +SQMP
> +add_client
> +----------
> +
> +Add a graphics client
> +
> +Arguments:
> +
> +- "protocol": protocol name (json-string)
> +- "fdname": file descriptor name (json-string)
> +
> +Example:
> +
> +->  { "execute": "add_client", "arguments": { "protocol": "vnc",
> +                                             "fdname": "myclient" } }
> +<- { "return": {} }
> +
> +EQMP
> +    {
>           .name       = "qmp_capabilities",
>           .args_type  = "",
>           .params     = "",
> diff --git a/ui/vnc.c b/ui/vnc.c
> index 39b5b51..8602adc 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -2924,3 +2924,10 @@ int vnc_display_open(DisplayState *ds, const char *display)
>       }
>       return qemu_set_fd_handler2(vs->lsock, NULL, vnc_listen_read, NULL, vs);
>   }
> +
> +void vnc_display_add_client(DisplayState *ds, int csock, int skipauth)
> +{
> +    VncDisplay *vs = ds ? (VncDisplay *)ds->opaque : vnc_display;
> +
> +    return vnc_connect(vs, csock, skipauth);
> +}

On Sat, Aug 06, 2011 at 09:38:03AM -0500, Anthony Liguori wrote:
> On 06/23/2011 07:31 AM, Daniel P. Berrange wrote:
> >Allow client connections for VNC and socket based character
> >devices to be passed in over the monitor using SCM_RIGHTS.
> >
> >One intended usage scenario is to start QEMU with VNC on a
> >UNIX domain socket. An unprivileged user which cannot access
> >the UNIX domain socket, can then connect to QEMU's VNC server
> >by passing an open FD to libvirt, which passes it onto QEMU.
> >
> >  { "execute": "get_fd", "arguments": { "fdname": "myclient" } }
> >  { "return": {} }
> >  { "execute": "add_client", "arguments": { "protocol": "vnc",
> >                                            "fdname": "myclient",
> >                                            "skipauth": true } }
> >  { "return": {} }
> >
> >In this case 'protocol' can be 'vnc' or 'spice', or the name
> >of a character device (eg from -chardev id=XXXX)
> >
> >The 'skipauth' parameter can be used to skip any configured
> >VNC authentication scheme, which is useful if the mgmt layer
> >talking to the monitor has already authenticated the client
> >in another way.
> >
> >* console.h: Define 'vnc_display_add_client' method
> >* monitor.c: Implement 'client_add' command
> >* qemu-char.c, qemu-char.h: Add 'qemu_char_add_client' method
> 
> I don't feel all that good about this anymore.  The notion of adding
> a fd to an arbitrary character device is a big layering violation
> and doesn't make much sense conceptually.
> 
> A chardev cannot have multiple connections.  It seems like the
> use-case is much better suited to having an fd: protocol to create
> char devices with.

The same idea of being able to configure a VNC server to listen
on a UNIX socket path by default, but be able to inject the client
connection via libvirt + FD passing, also applies to chardevs IMHO.
So having a fd: protocol chardev doesn't seem right to me. Even if
we don't support multiple connections, it is still useful to be able
to pass in the initial connection, for TCP/UNIX based chardevs.

> I'd like to partially revert this removing the qemu_chr_add_client
> interface.

The VNC bit was the most immediately useful part to me, so having the
chardev bits now is not critical.

Regards,
Daniel