From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:48137) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Qpi0x-0003Mi-12 for qemu-devel@nongnu.org; Sat, 06 Aug 2011 10:38:12 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Qpi0v-0007My-Fl for qemu-devel@nongnu.org; Sat, 06 Aug 2011 10:38:10 -0400 Received: from mail-gy0-f173.google.com ([209.85.160.173]:54037) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Qpi0v-0007Mu-Av for qemu-devel@nongnu.org; Sat, 06 Aug 2011 10:38:09 -0400 Received: by gyd12 with SMTP id 12so1431926gyd.4 for ; Sat, 06 Aug 2011 07:38:06 -0700 (PDT) Message-ID: <4E3D51CB.7000102@codemonkey.ws> Date: Sat, 06 Aug 2011 09:38:03 -0500 From: Anthony Liguori MIME-Version: 1.0 References: <1308832303-24205-1-git-send-email-berrange@redhat.com> <1308832303-24205-3-git-send-email-berrange@redhat.com> In-Reply-To: <1308832303-24205-3-git-send-email-berrange@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 2/3] Introduce a 'client_add' monitor command accepting an open FD List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Daniel P. Berrange" Cc: qemu-devel@nongnu.org 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); > +}