On Tue, Jul 28, 2026 at 04:59:20PM -0300, Fabiano Rosas wrote:
> Peter Xu <[email protected]> writes:
>
> > On Tue, Jul 28, 2026 at 05:24:36PM +0100, Daniel P. Berrangé wrote:
> >> On Tue, Jul 28, 2026 at 11:52:46AM -0400, Peter Xu wrote:
> >> > In an unlikely case, when a migration stream is attached to the
> >> > destination
> >> > QEMU and only send <4 bytes to the channel as magic, it's possible that
> >> > migration_channel_read_peek() may spin forever without yielding in the
> >> > main
> >> > thread causing two unwanted consequences:
> >> >
> >> > - CPU will spin 100% waiting for the rest bytes until it reaches 4
> >> > - (more importantly..) Main thread is stuck during this process as the
> >> > qio
> >> > operation won't really yield the coroutine
> >> >
> >> > Fix it by consuming the bytes that arrived.
> >> >
> >> > Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3889
> >> > Cc: Daniel P. Berrangé <[email protected]>
> >> > Reported-by: Feifan Qian <[email protected]>
> >> > Signed-off-by: Peter Xu <[email protected]>
> >> > ---
> >> > migration/channel.c | 26 +++++++++++++++++++++++---
> >> > 1 file changed, 23 insertions(+), 3 deletions(-)
> >> >
> >> > diff --git a/migration/channel.c b/migration/channel.c
> >> > index 1e2935f926..28fe1d2906 100644
> >> > --- a/migration/channel.c
> >> > +++ b/migration/channel.c
> >> > @@ -294,11 +294,31 @@ int migration_channel_read_peek(QIOChannel *ioc,
> >> > return -1;
> >> > }
> >> >
> >> > - if (len == buflen) {
> >> > + if (len == iov.iov_len) {
> >> > break;
> >> > - }
> >> > + } else if (len == 0) {
> >> > + qio_channel_wait_cond(ioc, G_IO_IN);
> >> > + } else {
> >> > + ssize_t received = len;
> >> >
> >> > - qio_channel_wait_cond(ioc, G_IO_IN);
> >> > + /*
> >> > + * Partially arrived, read out to make
> >> > qio_channel_wait_cond()
> >> > + * won't return immediately, causing an unwanted spin on
> >> > this
> >> > + * CPU.
> >> > + */
> >> > + iov.iov_len = len;
> >> > + len = qio_channel_readv_full(ioc, &iov, 1, NULL, NULL, 0,
> >> > errp);
> >>
> >> Sure this breaks the API behaviour that the caller is expecting to
> >> see.
> >>
> >> migration_channel_identify will call migration_channel_read_peek
> >> to match the magic bytes.
> >>
> >> But something later in the flow will actually try to read the magic
> >> bytes. By consuming them in this migration_channel_read_peek
> >> method, surely we're breaking the code that wants to read the bytes
> >> later.
> >
> > Ah right, stupid me. The best then is for partial read we apply a manual
> > wait.
> >
> > I can also revert 604bb1badc ("migration: Properly wait on G_IO_IN when
> > peeking messages"), looping with 1ms interval for data isn't too bad. But
> > the best is we only do that for partial, so:
> >
> > diff --git a/migration/channel.c b/migration/channel.c
> > index 1e2935f926..88f89521f8 100644
> > --- a/migration/channel.c
> > +++ b/migration/channel.c
> > @@ -296,9 +296,19 @@ int migration_channel_read_peek(QIOChannel *ioc,
> >
> > if (len == buflen) {
> > break;
> > + } else if (len == 0) {
> > + qio_channel_wait_cond(ioc, G_IO_IN);
> > + } else {
> > + /*
> > + * When partially ready, we can't use qio_channel_wait_cond()
> > + * because it will return immediately. Apply a manual wait.
> > + */
> > + if (qemu_in_coroutine()) {
>
> There's no coroutine at this point, could replace this with an assert.
Hmm, true.. Then it means this can stuck the main thread even if no data
arrived (len==0), hang monitors.. I hope it's not a major issue and
shouldn't easily happen in real life, because normally when src connected,
at least the headers will be dumped very soon.
I'll switch to assert for now.
>
> > + qemu_co_sleep_ns(QEMU_CLOCK_REALTIME, 1000000);
> > + } else {
> > + g_usleep(1000);
> > + }
> > }
> > -
> > - qio_channel_wait_cond(ioc, G_IO_IN);
> > }
> >
> > return 0;
> >
> > Any preference?
> >
> > Thanks,
> >
> >>
> >> > + /*
> >> > + * QIO_CHANNEL_ERR_BLOCK also shouldn't happen, due to the
> >> > + * prior peek just happened. We should be pretty sure we
> >> > will
> >> > + * read what we peeked, or the channel was broken.
> >> > + */
> >> > + if (len != received) {
> >> > + return -1;
> >> > + }
> >> > + iov.iov_base += received;
> >> > + iov.iov_len = buflen - received;
> >> > + }
> >> > }
> >> >
> >> > return 0;
> >> > --
> >> > 2.54.0
> >> >
> >>
> >> With regards,
> >> Daniel
> >> --
> >> |: https://berrange.com ~~ https://hachyderm.io/@berrange :|
> >> |: https://libvirt.org ~~ https://entangle-photo.org :|
> >> |: https://pixelfed.art/berrange ~~ https://fstop138.berrange.com :|
> >>
>
--
Peter Xu
