| Message ID | 1403297751-10379-1-git-send-email-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
Peter Maydell <peter.maydell@linaro.org> writes: > Convert the socket char backend to the new style QAPI framework; > this allows it to return an Error ** to callers who might not > want it to print directly about socket failures. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > I'm not 100% sure I have this correct -- review from somebody > who understands the char backends appreciated! Not sure anybody really understands this mess, but I can try. Cc'ing Gerd for some more char backend expertise. > Notes: > * I haven't tested this terribly much... > * The old qemu_chr_open_socket() has an > "if (!is_waitconnect) > qemu_set_nonblock(fd); > which the QMP char-open codepath has never had. Does this matter? > Which of the two code paths was correct? Gerd? > (I needed this because I wanted to be able to programmatically > create a char backend and not have it spew the error message to > stderr if the port was in use...) > > > qemu-char.c | 113 ++++++++++++++++++++++++++++++------------------------------ > 1 file changed, 57 insertions(+), 56 deletions(-) > > diff --git a/qemu-char.c b/qemu-char.c > index b3bd3b5..39c7b9c 100644 > --- a/qemu-char.c > +++ b/qemu-char.c > @@ -2923,61 +2923,6 @@ static CharDriverState *qemu_chr_open_socket_fd(int fd, bool do_nodelay, > return chr; > } > > -static CharDriverState *qemu_chr_open_socket(QemuOpts *opts) > -{ > - CharDriverState *chr = NULL; > - Error *local_err = NULL; > - int fd = -1; > - > - bool is_listen = qemu_opt_get_bool(opts, "server", false); > - bool is_waitconnect = is_listen && qemu_opt_get_bool(opts, "wait", true); > - bool is_telnet = qemu_opt_get_bool(opts, "telnet", false); > - bool do_nodelay = !qemu_opt_get_bool(opts, "delay", true); > - bool is_unix = qemu_opt_get(opts, "path") != NULL; > - > - if (is_unix) { > - if (is_listen) { > - fd = unix_listen_opts(opts, &local_err); > - } else { > - fd = unix_connect_opts(opts, &local_err, NULL, NULL); > - } > - } else { > - if (is_listen) { > - fd = inet_listen_opts(opts, 0, &local_err); > - } else { > - fd = inet_connect_opts(opts, &local_err, NULL, NULL); > - } > - } > - if (fd < 0) { > - goto fail; > - } > - > - if (!is_waitconnect) > - qemu_set_nonblock(fd); > - > - chr = qemu_chr_open_socket_fd(fd, do_nodelay, is_listen, is_telnet, > - is_waitconnect, &local_err); > - if (local_err) { > - goto fail; > - } > - return chr; > - > - > - fail: > - if (local_err) { > - qerror_report_err(local_err); > - error_free(local_err); > - } > - if (fd >= 0) { > - closesocket(fd); > - } > - if (chr) { > - g_free(chr->opaque); > - g_free(chr); > - } > - return NULL; > -} > - > /*********************************************************/ > /* Ring buffer chardev */ > > @@ -3384,6 +3329,61 @@ static void qemu_chr_parse_mux(QemuOpts *opts, ChardevBackend *backend, > backend->mux->chardev = g_strdup(chardev); > } > > +static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend, > + Error **errp) > +{ > + bool is_listen = qemu_opt_get_bool(opts, "server", false); > + bool is_waitconnect = is_listen && qemu_opt_get_bool(opts, "wait", true); > + bool is_telnet = qemu_opt_get_bool(opts, "telnet", false); > + bool do_nodelay = !qemu_opt_get_bool(opts, "delay", true); > + const char *path = qemu_opt_get(opts, "path"); > + const char *host = qemu_opt_get(opts, "host"); > + const char *port = qemu_opt_get(opts, "port"); > + SocketAddress *addr; > + > + if (!path) { > + if (!host) { > + error_setg(errp, "chardev: socket: no host given"); > + return; > + } > + if (!port) { > + error_setg(errp, "chardev: socket: no port given"); > + return; > + } > + } > + > + backend->socket = g_new0(ChardevSocket, 1); > + > + backend->socket->has_nodelay = true; > + backend->socket->nodelay = do_nodelay; > + backend->socket->has_server = true; > + backend->socket->server = is_listen; > + backend->socket->has_telnet = true; > + backend->socket->telnet = is_telnet; > + backend->socket->has_wait = true; > + backend->socket->wait = is_waitconnect; > + > + addr = g_new0(SocketAddress, 1); > + if (path) { > + addr->kind = SOCKET_ADDRESS_KIND_UNIX; Missing: addr->q_unix = g_new(UnixSocketAddress, 1); See also socket_parse(). > + addr->q_unix->path = g_strdup(path); > + } else { > + addr->kind = SOCKET_ADDRESS_KIND_INET; > + addr->inet = g_new0(InetSocketAddress, 1); > + addr->inet->host = g_strdup(host); > + addr->inet->port = g_strdup(port); > + addr->inet->to = qemu_opt_get_number(opts, "to", 0); > + addr->inet->has_to = true; > + if (qemu_opt_get_bool(opts, "ipv4", 0)) { > + addr->inet->has_ipv4 = addr->inet->ipv4 = true; > + } > + if (qemu_opt_get_bool(opts, "ipv6", 0)) { > + addr->inet->has_ipv6 = addr->inet->ipv6 = true; > + } > + } > + backend->socket->addr = addr; > +} > + > typedef struct CharDriver { > const char *name; > /* old, pre qapi */ > @@ -4064,7 +4064,8 @@ void qmp_chardev_remove(const char *id, Error **errp) > static void register_types(void) > { > register_char_driver_qapi("null", CHARDEV_BACKEND_KIND_NULL, NULL); > - register_char_driver("socket", qemu_chr_open_socket); > + register_char_driver_qapi("socket", CHARDEV_BACKEND_KIND_SOCKET, > + qemu_chr_parse_socket); > register_char_driver("udp", qemu_chr_open_udp); > register_char_driver_qapi("ringbuf", CHARDEV_BACKEND_KIND_RINGBUF, > qemu_chr_parse_ringbuf); Only one old-style driver left: "udp". It's closely related to "socket"... would you be willing to take care of that one, too?
On 23 June 2014 13:49, Markus Armbruster <armbru@redhat.com> wrote: > Peter Maydell <peter.maydell@linaro.org> writes: > >> Convert the socket char backend to the new style QAPI framework; >> this allows it to return an Error ** to callers who might not >> want it to print directly about socket failures. >> >> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> >> --- >> I'm not 100% sure I have this correct -- review from somebody >> who understands the char backends appreciated! > > Not sure anybody really understands this mess, but I can try. Thanks. >> + addr = g_new0(SocketAddress, 1); >> + if (path) { >> + addr->kind = SOCKET_ADDRESS_KIND_UNIX; > > Missing: > > addr->q_unix = g_new(UnixSocketAddress, 1); > > See also socket_parse(). Heh, yes. You can tell I didn't test this code path ;-) >> + addr->q_unix->path = g_strdup(path); >> + } else { >> + addr->kind = SOCKET_ADDRESS_KIND_INET; >> + addr->inet = g_new0(InetSocketAddress, 1); >> + addr->inet->host = g_strdup(host); >> + addr->inet->port = g_strdup(port); >> + addr->inet->to = qemu_opt_get_number(opts, "to", 0); >> + addr->inet->has_to = true; >> + if (qemu_opt_get_bool(opts, "ipv4", 0)) { >> + addr->inet->has_ipv4 = addr->inet->ipv4 = true; >> + } >> + if (qemu_opt_get_bool(opts, "ipv6", 0)) { >> + addr->inet->has_ipv6 = addr->inet->ipv6 = true; >> + } >> + } >> + backend->socket->addr = addr; >> +} >> + >> typedef struct CharDriver { >> const char *name; >> /* old, pre qapi */ >> @@ -4064,7 +4064,8 @@ void qmp_chardev_remove(const char *id, Error **errp) >> static void register_types(void) >> { >> register_char_driver_qapi("null", CHARDEV_BACKEND_KIND_NULL, NULL); >> - register_char_driver("socket", qemu_chr_open_socket); >> + register_char_driver_qapi("socket", CHARDEV_BACKEND_KIND_SOCKET, >> + qemu_chr_parse_socket); >> register_char_driver("udp", qemu_chr_open_udp); >> register_char_driver_qapi("ringbuf", CHARDEV_BACKEND_KIND_RINGBUF, >> qemu_chr_parse_ringbuf); > > Only one old-style driver left: "udp". It's closely related to > "socket"... would you be willing to take care of that one, too? Yeah, I noticed that too; I figured I'd get this patch through review first, though. thanks -- PMM
Hi, > > * The old qemu_chr_open_socket() has an > > "if (!is_waitconnect) > > qemu_set_nonblock(fd); > > which the QMP char-open codepath has never had. Does this matter? > > Which of the two code paths was correct? > > Gerd? IIRC the socket is put into non-blocking mode anyway by the qemu socket helper functions. cheers, Gerd
On 30 June 2014 11:23, Gerd Hoffmann <kraxel@redhat.com> wrote: > Hi, > >> > * The old qemu_chr_open_socket() has an >> > "if (!is_waitconnect) >> > qemu_set_nonblock(fd); >> > which the QMP char-open codepath has never had. Does this matter? >> > Which of the two code paths was correct? >> >> Gerd? > > IIRC the socket is put into non-blocking mode anyway by the qemu socket > helper functions. In that case is qemu_chr_open_socket_fd() incorrect in marking the socket as nonblocking in the is_listen && is_waitconnect case? -- PMM
On Mo, 2014-06-30 at 11:33 +0100, Peter Maydell wrote: > On 30 June 2014 11:23, Gerd Hoffmann <kraxel@redhat.com> wrote: > > Hi, > > > >> > * The old qemu_chr_open_socket() has an > >> > "if (!is_waitconnect) > >> > qemu_set_nonblock(fd); > >> > which the QMP char-open codepath has never had. Does this matter? > >> > Which of the two code paths was correct? > >> > >> Gerd? > > > > IIRC the socket is put into non-blocking mode anyway by the qemu socket > > helper functions. > > In that case is qemu_chr_open_socket_fd() incorrect > in marking the socket as nonblocking in the > is_listen && is_waitconnect case? Honestly I think the whole waitconnect stuff should be ripped out. Obvious problem is backward compatibility. But maybe not because nobody uses it anyway. Anyone out there using chardev server sockets without 'nowait' option? waitconnect means "go into server mode, wait for a client to connect, then unpause the guest". Which handles one special case of the generic "start qemu with -S, connect everything, then sent 'cont' to monitor" libvirt is doing. IIRC "wait for client to connect" is a blocking accept() call. Which you certainly don't want do in a qmp command handler. So if we switch over chardevs created via -chardev to use the qmp init code path too we need some hackery to make '-chardev socket,wait,...' work without introducing a blocking qmp command I suspect ... cheers, Gerd
On 30 June 2014 11:57, Gerd Hoffmann <kraxel@redhat.com> wrote: > IIRC "wait for client to connect" is a blocking accept() call. Which > you certainly don't want do in a qmp command handler. So if we switch > over chardevs created via -chardev to use the qmp init code path too we > need some hackery to make '-chardev socket,wait,...' work without > introducing a blocking qmp command I suspect ... We already have blocking code paths in qmp_chardev_open_socket(): for the non-listening case we call socket_connect() with a NULL callback argument, which will result in our calling connect() in blocking mode. thanks -- PMM
Il 30/06/2014 12:57, Gerd Hoffmann ha scritto: > On Mo, 2014-06-30 at 11:33 +0100, Peter Maydell wrote: >> On 30 June 2014 11:23, Gerd Hoffmann <kraxel@redhat.com> wrote: >>> Hi, >>> >>>>> * The old qemu_chr_open_socket() has an >>>>> "if (!is_waitconnect) >>>>> qemu_set_nonblock(fd); >>>>> which the QMP char-open codepath has never had. Does this matter? >>>>> Which of the two code paths was correct? >>>> >>>> Gerd? >>> >>> IIRC the socket is put into non-blocking mode anyway by the qemu socket >>> helper functions. >> >> In that case is qemu_chr_open_socket_fd() incorrect >> in marking the socket as nonblocking in the >> is_listen && is_waitconnect case? It's unnecessary. tcp_chr_accept calls tcp_chr_add_client, which takes care of that. But it doesn't hurt either. > Honestly I think the whole waitconnect stuff should be ripped out. > Obvious problem is backward compatibility. But maybe not because nobody > uses it anyway. Anyone out there using chardev server sockets without > 'nowait' option? > > waitconnect means "go into server mode, wait for a client to connect, > then unpause the guest". Which handles one special case of the generic > "start qemu with -S, connect everything, then sent 'cont' to monitor" > libvirt is doing. > > IIRC "wait for client to connect" is a blocking accept() call. Which > you certainly don't want do in a qmp command handler. I agree you don't want to do it, but then... just don't do it. :) We can leave in waitconnect, and leave it blocking. QMP clients simply shouldn't use it, but if they do it's not our fault. In fact, ChardevSocket.wait defaults to false (unlike the command-line counterpart). Paolo
On 30 June 2014 13:29, Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 30/06/2014 12:57, Gerd Hoffmann ha scritto: >> On Mo, 2014-06-30 at 11:33 +0100, Peter Maydell wrote: >>> In that case is qemu_chr_open_socket_fd() incorrect >>> in marking the socket as nonblocking in the >>> is_listen && is_waitconnect case? > > > It's unnecessary. tcp_chr_accept calls tcp_chr_add_client, which takes care > of that. But it doesn't hurt either. I think the tcp_chr_accept->tcp_chr_add_client->set_nonblock is marking the new fd returned from accept() as nonblocking. The call in qemu_chr_open_socket_fd() is marking the listening fd as nonblocking. So those are different things... thanks -- PMM
Il 30/06/2014 14:33, Peter Maydell ha scritto: >> > >> > It's unnecessary. tcp_chr_accept calls tcp_chr_add_client, which takes care >> > of that. But it doesn't hurt either. > I think the tcp_chr_accept->tcp_chr_add_client->set_nonblock > is marking the new fd returned from accept() as nonblocking. > The call in qemu_chr_open_socket_fd() is marking the listening > fd as nonblocking. So those are different things... Uh, you're right. I think the call in qemu_chr_open_socket_fd simply means "after the currently connected disconnects, subsequent accepts will be done via select(), so the socket can now be marked as non-blocking". The qemu-char logic is right then. Paolo
diff --git a/qemu-char.c b/qemu-char.c index b3bd3b5..39c7b9c 100644 --- a/qemu-char.c +++ b/qemu-char.c @@ -2923,61 +2923,6 @@ static CharDriverState *qemu_chr_open_socket_fd(int fd, bool do_nodelay, return chr; } -static CharDriverState *qemu_chr_open_socket(QemuOpts *opts) -{ - CharDriverState *chr = NULL; - Error *local_err = NULL; - int fd = -1; - - bool is_listen = qemu_opt_get_bool(opts, "server", false); - bool is_waitconnect = is_listen && qemu_opt_get_bool(opts, "wait", true); - bool is_telnet = qemu_opt_get_bool(opts, "telnet", false); - bool do_nodelay = !qemu_opt_get_bool(opts, "delay", true); - bool is_unix = qemu_opt_get(opts, "path") != NULL; - - if (is_unix) { - if (is_listen) { - fd = unix_listen_opts(opts, &local_err); - } else { - fd = unix_connect_opts(opts, &local_err, NULL, NULL); - } - } else { - if (is_listen) { - fd = inet_listen_opts(opts, 0, &local_err); - } else { - fd = inet_connect_opts(opts, &local_err, NULL, NULL); - } - } - if (fd < 0) { - goto fail; - } - - if (!is_waitconnect) - qemu_set_nonblock(fd); - - chr = qemu_chr_open_socket_fd(fd, do_nodelay, is_listen, is_telnet, - is_waitconnect, &local_err); - if (local_err) { - goto fail; - } - return chr; - - - fail: - if (local_err) { - qerror_report_err(local_err); - error_free(local_err); - } - if (fd >= 0) { - closesocket(fd); - } - if (chr) { - g_free(chr->opaque); - g_free(chr); - } - return NULL; -} - /*********************************************************/ /* Ring buffer chardev */ @@ -3384,6 +3329,61 @@ static void qemu_chr_parse_mux(QemuOpts *opts, ChardevBackend *backend, backend->mux->chardev = g_strdup(chardev); } +static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend, + Error **errp) +{ + bool is_listen = qemu_opt_get_bool(opts, "server", false); + bool is_waitconnect = is_listen && qemu_opt_get_bool(opts, "wait", true); + bool is_telnet = qemu_opt_get_bool(opts, "telnet", false); + bool do_nodelay = !qemu_opt_get_bool(opts, "delay", true); + const char *path = qemu_opt_get(opts, "path"); + const char *host = qemu_opt_get(opts, "host"); + const char *port = qemu_opt_get(opts, "port"); + SocketAddress *addr; + + if (!path) { + if (!host) { + error_setg(errp, "chardev: socket: no host given"); + return; + } + if (!port) { + error_setg(errp, "chardev: socket: no port given"); + return; + } + } + + backend->socket = g_new0(ChardevSocket, 1); + + backend->socket->has_nodelay = true; + backend->socket->nodelay = do_nodelay; + backend->socket->has_server = true; + backend->socket->server = is_listen; + backend->socket->has_telnet = true; + backend->socket->telnet = is_telnet; + backend->socket->has_wait = true; + backend->socket->wait = is_waitconnect; + + addr = g_new0(SocketAddress, 1); + if (path) { + addr->kind = SOCKET_ADDRESS_KIND_UNIX; + addr->q_unix->path = g_strdup(path); + } else { + addr->kind = SOCKET_ADDRESS_KIND_INET; + addr->inet = g_new0(InetSocketAddress, 1); + addr->inet->host = g_strdup(host); + addr->inet->port = g_strdup(port); + addr->inet->to = qemu_opt_get_number(opts, "to", 0); + addr->inet->has_to = true; + if (qemu_opt_get_bool(opts, "ipv4", 0)) { + addr->inet->has_ipv4 = addr->inet->ipv4 = true; + } + if (qemu_opt_get_bool(opts, "ipv6", 0)) { + addr->inet->has_ipv6 = addr->inet->ipv6 = true; + } + } + backend->socket->addr = addr; +} + typedef struct CharDriver { const char *name; /* old, pre qapi */ @@ -4064,7 +4064,8 @@ void qmp_chardev_remove(const char *id, Error **errp) static void register_types(void) { register_char_driver_qapi("null", CHARDEV_BACKEND_KIND_NULL, NULL); - register_char_driver("socket", qemu_chr_open_socket); + register_char_driver_qapi("socket", CHARDEV_BACKEND_KIND_SOCKET, + qemu_chr_parse_socket); register_char_driver("udp", qemu_chr_open_udp); register_char_driver_qapi("ringbuf", CHARDEV_BACKEND_KIND_RINGBUF, qemu_chr_parse_ringbuf);
Convert the socket char backend to the new style QAPI framework; this allows it to return an Error ** to callers who might not want it to print directly about socket failures. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- I'm not 100% sure I have this correct -- review from somebody who understands the char backends appreciated! Notes: * I haven't tested this terribly much... * The old qemu_chr_open_socket() has an "if (!is_waitconnect) qemu_set_nonblock(fd); which the QMP char-open codepath has never had. Does this matter? Which of the two code paths was correct? (I needed this because I wanted to be able to programmatically create a char backend and not have it spew the error message to stderr if the port was in use...) qemu-char.c | 113 ++++++++++++++++++++++++++++++------------------------------ 1 file changed, 57 insertions(+), 56 deletions(-)