Re: [Qemu-devel] [PATCH 2/3] Introduce a 'client_add' monitor command accepting an open FD

[Anthony Liguori <anthony@codemonkey.ws>]

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);
> +}