038b421788
This reverts commita7077b8e35
, and add comments to explain why child sources cannot be used. When a GSource is added as a child of another GSource, if its 'prepare' function indicates readiness, then the parent's 'prepare' function will never be run. The io_watch_poll_prepare absolutely *must* be run on every iteration of the main loop, to ensure that the chardev backend doesn't feed data to the frontend that it is unable to consume. At the timea7077b8e35
was made, all the child GSource impls were relying on poll'ing an FD, so their 'prepare' functions would never indicate readiness ahead of poll() being invoked. So the buggy behaviour was not noticed and lay dormant. Relatively recently the QIOChannelTLS impl introduced a level 2 child GSource, which checks with GNUTLS whether it has cached any data that was decoded but not yet consumed: commitffda5db65a
Author: Antoine Damhet <antoine.damhet@shadow.tech> Date: Tue Nov 15 15:23:29 2022 +0100 io/channel-tls: fix handling of bigger read buffers Since the TLS backend can read more data from the underlying QIOChannel we introduce a minimal child GSource to notify if we still have more data available to be read. Signed-off-by: Antoine Damhet <antoine.damhet@shadow.tech> Signed-off-by: Charles Frey <charles.frey@shadow.tech> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> With this, it is now quite common for the 'prepare' function on a QIOChannelTLS GSource to indicate immediate readiness, bypassing the parent GSource 'prepare' function. IOW, the critical 'io_watch_poll_prepare' is being skipped on some iterations of the main loop. As a result chardev frontend asserts are now being triggered as they are fed data they are not ready to consume. A reproducer is as follows: * In terminal 1 run a GNUTLS *echo* server $ gnutls-serv --echo \ --x509cafile ca-cert.pem \ --x509keyfile server-key.pem \ --x509certfile server-cert.pem \ -p 9000 * In terminal 2 run a QEMU guest $ qemu-system-s390x \ -nodefaults \ -display none \ -object tls-creds-x509,id=tls0,dir=$PWD,endpoint=client \ -chardev socket,id=con0,host=localhost,port=9000,tls-creds=tls0 \ -device sclpconsole,chardev=con0 \ -hda Fedora-Cloud-Base-39-1.5.s390x.qcow2 After the previous patch revert, but before this patch revert, this scenario will crash: qemu-system-s390x: ../hw/char/sclpconsole.c:73: chr_read: Assertion `size <= SIZE_BUFFER_VT220 - scon->iov_data_len' failed. This assert indicates that 'tcp_chr_read' was called without 'tcp_chr_read_poll' having first been checked for ability to receive more data QEMU's use of a 'prepare' function to create/delete another GSource is rather a hack and not normally the kind of thing that is expected to be done by a GSource. There is no mechanism to force GLib to always run the 'prepare' function of a parent GSource. The best option is to simply not use the child source concept, and go back to the functional approach previously relied on. Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> Reviewed-by: Thomas Huth <thuth@redhat.com> Tested-by: Thomas Huth <thuth@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
194 lines
5.7 KiB
C
194 lines
5.7 KiB
C
/*
|
|
* QEMU System Emulator
|
|
*
|
|
* Copyright (c) 2003-2008 Fabrice Bellard
|
|
*
|
|
* Permission is hereby granted, free of charge, to any person obtaining a copy
|
|
* of this software and associated documentation files (the "Software"), to deal
|
|
* in the Software without restriction, including without limitation the rights
|
|
* to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
|
|
* copies of the Software, and to permit persons to whom the Software is
|
|
* furnished to do so, subject to the following conditions:
|
|
*
|
|
* The above copyright notice and this permission notice shall be included in
|
|
* all copies or substantial portions of the Software.
|
|
*
|
|
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
|
|
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
|
|
* FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
|
|
* THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
|
|
* LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
|
|
* OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
|
|
* THE SOFTWARE.
|
|
*/
|
|
#include "qemu/osdep.h"
|
|
#include "chardev/char-io.h"
|
|
|
|
typedef struct IOWatchPoll {
|
|
GSource parent;
|
|
|
|
QIOChannel *ioc;
|
|
GSource *src;
|
|
|
|
IOCanReadHandler *fd_can_read;
|
|
GSourceFunc fd_read;
|
|
void *opaque;
|
|
GMainContext *context;
|
|
} IOWatchPoll;
|
|
|
|
static IOWatchPoll *io_watch_poll_from_source(GSource *source)
|
|
{
|
|
return container_of(source, IOWatchPoll, parent);
|
|
}
|
|
|
|
static gboolean io_watch_poll_prepare(GSource *source,
|
|
gint *timeout)
|
|
{
|
|
IOWatchPoll *iwp = io_watch_poll_from_source(source);
|
|
bool now_active = iwp->fd_can_read(iwp->opaque) > 0;
|
|
bool was_active = iwp->src != NULL;
|
|
if (was_active == now_active) {
|
|
return FALSE;
|
|
}
|
|
|
|
/*
|
|
* We do not register the QIOChannel watch as a child GSource.
|
|
* The 'prepare' function on the parent GSource will be
|
|
* skipped if a child GSource's 'prepare' function indicates
|
|
* readiness. We need this prepare function be guaranteed
|
|
* to run on *every* iteration of the main loop, because
|
|
* it is critical to ensure we remove the QIOChannel watch
|
|
* if 'fd_can_read' indicates the frontend cannot receive
|
|
* more data.
|
|
*/
|
|
if (now_active) {
|
|
iwp->src = qio_channel_create_watch(
|
|
iwp->ioc, G_IO_IN | G_IO_ERR | G_IO_HUP | G_IO_NVAL);
|
|
g_source_set_callback(iwp->src, iwp->fd_read, iwp->opaque, NULL);
|
|
g_source_attach(iwp->src, iwp->context);
|
|
} else {
|
|
g_source_destroy(iwp->src);
|
|
g_source_unref(iwp->src);
|
|
iwp->src = NULL;
|
|
}
|
|
return FALSE;
|
|
}
|
|
|
|
static gboolean io_watch_poll_check(GSource *source)
|
|
{
|
|
return FALSE;
|
|
}
|
|
|
|
static gboolean io_watch_poll_dispatch(GSource *source, GSourceFunc callback,
|
|
gpointer user_data)
|
|
{
|
|
abort();
|
|
}
|
|
|
|
static void io_watch_poll_finalize(GSource *source)
|
|
{
|
|
/*
|
|
* Due to a glib bug, removing the last reference to a source
|
|
* inside a finalize callback causes recursive locking (and a
|
|
* deadlock). This is not a problem inside other callbacks,
|
|
* including dispatch callbacks, so we call io_remove_watch_poll
|
|
* to remove this source. At this point, iwp->src must
|
|
* be NULL, or we would leak it.
|
|
*/
|
|
IOWatchPoll *iwp = io_watch_poll_from_source(source);
|
|
assert(iwp->src == NULL);
|
|
}
|
|
|
|
static GSourceFuncs io_watch_poll_funcs = {
|
|
.prepare = io_watch_poll_prepare,
|
|
.check = io_watch_poll_check,
|
|
.dispatch = io_watch_poll_dispatch,
|
|
.finalize = io_watch_poll_finalize,
|
|
};
|
|
|
|
GSource *io_add_watch_poll(Chardev *chr,
|
|
QIOChannel *ioc,
|
|
IOCanReadHandler *fd_can_read,
|
|
QIOChannelFunc fd_read,
|
|
gpointer user_data,
|
|
GMainContext *context)
|
|
{
|
|
IOWatchPoll *iwp;
|
|
char *name;
|
|
|
|
iwp = (IOWatchPoll *) g_source_new(&io_watch_poll_funcs,
|
|
sizeof(IOWatchPoll));
|
|
iwp->fd_can_read = fd_can_read;
|
|
iwp->opaque = user_data;
|
|
iwp->ioc = ioc;
|
|
iwp->fd_read = (GSourceFunc) fd_read;
|
|
iwp->src = NULL;
|
|
iwp->context = context;
|
|
|
|
name = g_strdup_printf("chardev-iowatch-%s", chr->label);
|
|
g_source_set_name((GSource *)iwp, name);
|
|
g_free(name);
|
|
|
|
g_source_attach(&iwp->parent, context);
|
|
g_source_unref(&iwp->parent);
|
|
return (GSource *)iwp;
|
|
}
|
|
|
|
static void io_remove_watch_poll(GSource *source)
|
|
{
|
|
IOWatchPoll *iwp;
|
|
|
|
iwp = io_watch_poll_from_source(source);
|
|
if (iwp->src) {
|
|
g_source_destroy(iwp->src);
|
|
g_source_unref(iwp->src);
|
|
iwp->src = NULL;
|
|
}
|
|
g_source_destroy(&iwp->parent);
|
|
}
|
|
|
|
void remove_fd_in_watch(Chardev *chr)
|
|
{
|
|
if (chr->gsource) {
|
|
io_remove_watch_poll(chr->gsource);
|
|
chr->gsource = NULL;
|
|
}
|
|
}
|
|
|
|
int io_channel_send_full(QIOChannel *ioc,
|
|
const void *buf, size_t len,
|
|
int *fds, size_t nfds)
|
|
{
|
|
size_t offset = 0;
|
|
|
|
while (offset < len) {
|
|
ssize_t ret = 0;
|
|
struct iovec iov = { .iov_base = (char *)buf + offset,
|
|
.iov_len = len - offset };
|
|
|
|
ret = qio_channel_writev_full(
|
|
ioc, &iov, 1,
|
|
fds, nfds, 0, NULL);
|
|
if (ret == QIO_CHANNEL_ERR_BLOCK) {
|
|
if (offset) {
|
|
return offset;
|
|
}
|
|
|
|
errno = EAGAIN;
|
|
return -1;
|
|
} else if (ret < 0) {
|
|
errno = EINVAL;
|
|
return -1;
|
|
}
|
|
|
|
offset += ret;
|
|
}
|
|
|
|
return offset;
|
|
}
|
|
|
|
int io_channel_send(QIOChannel *ioc, const void *buf, size_t len)
|
|
{
|
|
return io_channel_send_full(ioc, buf, len, NULL, 0);
|
|
}
|