[PATCH 0/2] fdstream: attempt to fix EOF handling (again)
Daniel P. Berrangé (2): fdstream: don't set return value if looping to read more data fdstream: fix EOF handling when reading data src/util/virfdstream.c | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) -- 2.53.0
From: Daniel P. Berrangé <berrange@redhat.com> The 'ret' variable should only have a value assigned once we have completely finished reading data, otherwise an error on a subsequent iteration will report an error but not return a negative value. Fixes: e23fd0b7fd36c41e6db49df4f4962762d3ef6ab0 Reported-by: Roman Bogorodskiy <bogorodskiy@gmail.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/util/virfdstream.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/virfdstream.c b/src/util/virfdstream.c index 4c974ba5d7..cd2ede4501 100644 --- a/src/util/virfdstream.c +++ b/src/util/virfdstream.c @@ -961,10 +961,10 @@ static int virFDStreamRead(virStreamPtr st, char *bytes, size_t nbytes) virFDStreamMsgFree(msg); } - ret = got; if (nbytes > 0) { goto more; } + ret = got; } else { retry: ret = read(fdst->fd, bytes, nbytes); -- 2.53.0
From: Daniel P. Berrangé <berrange@redhat.com> A recent commit caused the virFDStreamRead method to loop reading data until the provided buffer is full. Unfortunately the EOF handling was not quiet correct. * When seeing a virFDStreamMsg with length zero, it would still loop trying to read more and then get an error that the thread has quit. * When seeing a virFDStreamMsg with length zero on subsequent iterations, it would discard this message, which would in turn prevent the caller from ever seeing the 'ret == 0' return value indicating EOF. The caller would then try to read again and get an error about the stream being closed. Fixes: e23fd0b7fd36c41e6db49df4f4962762d3ef6ab0 Reported-by: Roman Bogorodskiy <bogorodskiy@gmail.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/util/virfdstream.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/src/util/virfdstream.c b/src/util/virfdstream.c index cd2ede4501..9fe70be0b9 100644 --- a/src/util/virfdstream.c +++ b/src/util/virfdstream.c @@ -907,6 +907,7 @@ static int virFDStreamRead(virStreamPtr st, char *bytes, size_t nbytes) virFDStreamMsg *msg = NULL; size_t got = 0; size_t bsz = 0; + bool isEOF; more: while (!(msg = fdst->msg)) { @@ -945,6 +946,7 @@ static int virFDStreamRead(virStreamPtr st, char *bytes, size_t nbytes) goto cleanup; } + isEOF = msg->stream.data.len == 0; bsz = msg->stream.data.len - msg->stream.data.offset; if (nbytes < bsz) bsz = nbytes; @@ -956,12 +958,24 @@ static int virFDStreamRead(virStreamPtr st, char *bytes, size_t nbytes) nbytes -= bsz; msg->stream.data.offset += bsz; - if (msg->stream.data.offset == msg->stream.data.len) { + /* If we stream msg is fully consumed, then remove + * it from the queue. + * + * Exception: if this is the second time around the + * loop, and the msg indicated an EOF, we must leave + * it on the queue so a subsequent read sees the + * ret == 0 EOF condition + */ + if (msg->stream.data.offset == msg->stream.data.len && + (!isEOF || got == 0)) { virFDStreamMsgQueuePop(fdst, fdst->fd, "pipe"); virFDStreamMsgFree(msg); } - if (nbytes > 0) { + /* If we didn't just see EOF and can read more into + * 'bytes' then retry the loop + */ + if (nbytes > 0 && !isEOF) { goto more; } ret = got; -- 2.53.0
On a Tuesday in 2026, Daniel P. Berrangé via Devel wrote:
From: Daniel P. Berrangé <berrange@redhat.com>
A recent commit caused the virFDStreamRead method to loop reading data until the provided buffer is full. Unfortunately the EOF handling was not quiet correct.
*quite Jano
* When seeing a virFDStreamMsg with length zero, it would still loop trying to read more and then get an error that the thread has quit.
* When seeing a virFDStreamMsg with length zero on subsequent iterations, it would discard this message, which would in turn prevent the caller from ever seeing the 'ret == 0' return value indicating EOF. The caller would then try to read again and get an error about the stream being closed.
Fixes: e23fd0b7fd36c41e6db49df4f4962762d3ef6ab0 Reported-by: Roman Bogorodskiy <bogorodskiy@gmail.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
On Tue, Feb 24, 2026 at 06:26:04PM +0000, Daniel P. Berrangé via Devel wrote:
From: Daniel P. Berrangé <berrange@redhat.com>
A recent commit caused the virFDStreamRead method to loop reading data until the provided buffer is full. Unfortunately the EOF handling was not quiet correct.
* When seeing a virFDStreamMsg with length zero, it would still loop trying to read more and then get an error that the thread has quit.
* When seeing a virFDStreamMsg with length zero on subsequent iterations, it would discard this message, which would in turn prevent the caller from ever seeing the 'ret == 0' return value indicating EOF. The caller would then try to read again and get an error about the stream being closed.
Fixes: e23fd0b7fd36c41e6db49df4f4962762d3ef6ab0 Reported-by: Roman Bogorodskiy <bogorodskiy@gmail.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/util/virfdstream.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/src/util/virfdstream.c b/src/util/virfdstream.c index cd2ede4501..9fe70be0b9 100644 --- a/src/util/virfdstream.c +++ b/src/util/virfdstream.c @@ -907,6 +907,7 @@ static int virFDStreamRead(virStreamPtr st, char *bytes, size_t nbytes) virFDStreamMsg *msg = NULL; size_t got = 0; size_t bsz = 0; + bool isEOF;
more: while (!(msg = fdst->msg)) { @@ -945,6 +946,7 @@ static int virFDStreamRead(virStreamPtr st, char *bytes, size_t nbytes) goto cleanup; }
+ isEOF = msg->stream.data.len == 0; bsz = msg->stream.data.len - msg->stream.data.offset; if (nbytes < bsz) bsz = nbytes; @@ -956,12 +958,24 @@ static int virFDStreamRead(virStreamPtr st, char *bytes, size_t nbytes) nbytes -= bsz;
msg->stream.data.offset += bsz; - if (msg->stream.data.offset == msg->stream.data.len) { + /* If we stream msg is fully consumed, then remove
s/we/the/
+ * it from the queue. + * + * Exception: if this is the second time around the + * loop, and the msg indicated an EOF, we must leave + * it on the queue so a subsequent read sees the + * ret == 0 EOF condition + */ + if (msg->stream.data.offset == msg->stream.data.len && + (!isEOF || got == 0)) { virFDStreamMsgQueuePop(fdst, fdst->fd, "pipe"); virFDStreamMsgFree(msg); }
- if (nbytes > 0) { + /* If we didn't just see EOF and can read more into + * 'bytes' then retry the loop + */ + if (nbytes > 0 && !isEOF) { goto more; } ret = got; -- 2.53.0
In debugging this issue, I also came to the conclusion that the VIR_STREAM_NONBLOCK handling is completely broken when the background thread is in use. It was always partially broken even when we used a pipe between the foreground and backgroud threads, likely leading to busy looping AFAICT. With the switch to virFDSteamMsg queues, it looks like we hard block on reads and have unlimited memory usage on writes. I filed: https://gitlab.com/libvirt/libvirt/-/issues/855 since I can't see an immediate easy fix for this :-( On Tue, Feb 24, 2026 at 06:26:02PM +0000, Daniel P. Berrangé wrote:
Daniel P. Berrangé (2): fdstream: don't set return value if looping to read more data fdstream: fix EOF handling when reading data
src/util/virfdstream.c | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-)
-- 2.53.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 :|
Daniel P. Berrangé wrote:
Daniel P. Berrangé (2): fdstream: don't set return value if looping to read more data fdstream: fix EOF handling when reading data
src/util/virfdstream.c | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-)
Yay, this one works! Tested-by: Roman Bogorodskiy <bogorodskiy@gmail.com>
On a Tuesday in 2026, Daniel P. Berrangé via Devel wrote:
Daniel P. Berrangé (2): fdstream: don't set return value if looping to read more data fdstream: fix EOF handling when reading data
src/util/virfdstream.c | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (4)
-
Daniel P. Berrangé -
Ján Tomko -
Pavel Hrdina -
Roman Bogorodskiy