1. Patchew
  2. Libvirt
  3. View series

[PATCH resend] In virFDStreamRead(), fill buffer from this and next messages

Erik Huelsmann posted 1 patch 2 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20260211173606.3145076-1-ehuels@gmail.com
src/util/virfdstream.c | 26 +++++++++++++++++---------
1 file changed, 17 insertions(+), 9 deletions(-)
[PATCH resend] In virFDStreamRead(), fill buffer from this and next messages
Posted by Erik Huelsmann 2 months ago 
Before this change, buffers returned from virFDStreamRead() would
alternate in size (262120 and 24), because it only consumed the
bytes remaining from the current background thread message.

As the background thread reads 262144 bytes (256kB) of data in
each chunk, where the maximum size returned from virFDStreamRead()
to be transferred over the remote protocol is only 262120, 24 bytes
would be left in the buffer on each iteration. The next iteration
leaves 24 bytes, which used to be returned without considering
messages waiting in the queue.

Signed-off-by: Erik Huelsmann <ehuels@gmail.com>
---
 src/util/virfdstream.c | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

diff --git a/src/util/virfdstream.c b/src/util/virfdstream.c
index 26a1f00..4c974ba 100644
--- a/src/util/virfdstream.c
+++ b/src/util/virfdstream.c
@@ -905,7 +905,10 @@ static int virFDStreamRead(virStreamPtr st, char *bytes, size_t nbytes)
 
     if (fdst->thread) {
         virFDStreamMsg *msg = NULL;
+        size_t got = 0;
+        size_t bsz = 0;
 
+    more:
         while (!(msg = fdst->msg)) {
             if (fdst->threadQuit || fdst->threadErr) {
                 if (nbytes) {
@@ -917,7 +920,7 @@ static int virFDStreamRead(virStreamPtr st, char *bytes, size_t nbytes)
                         virReportSystemError(EBADF, "%s",
                                              _("stream is not open"));
                 } else {
-                    ret = 0;
+                    ret = got;
                 }
                 goto cleanup;
             } else {
@@ -931,7 +934,7 @@ static int virFDStreamRead(virStreamPtr st, char *bytes, size_t nbytes)
          * return 0 immediately. */
         if (msg->type == VIR_FDSTREAM_MSG_TYPE_HOLE &&
             msg->stream.hole.len == 0) {
-            ret = 0;
+            ret = got;
             goto cleanup;
         }
 
@@ -942,21 +945,26 @@ static int virFDStreamRead(virStreamPtr st, char *bytes, size_t nbytes)
             goto cleanup;
         }
 
-        if (nbytes > msg->stream.data.len - msg->stream.data.offset)
-            nbytes = msg->stream.data.len - msg->stream.data.offset;
+        bsz = msg->stream.data.len - msg->stream.data.offset;
+        if (nbytes < bsz)
+            bsz = nbytes;
 
-        memcpy(bytes,
+        memcpy(bytes + got,
                msg->stream.data.buf + msg->stream.data.offset,
-               nbytes);
+               bsz);
+        got += bsz;
+        nbytes -= bsz;
 
-        msg->stream.data.offset += nbytes;
+        msg->stream.data.offset += bsz;
         if (msg->stream.data.offset == msg->stream.data.len) {
             virFDStreamMsgQueuePop(fdst, fdst->fd, "pipe");
             virFDStreamMsgFree(msg);
         }
 
-        ret = nbytes;
-
+        ret = got;
+        if (nbytes > 0) {
+            goto more;
+        }
     } else {
      retry:
         ret = read(fdst->fd, bytes, nbytes);
-- 
2.43.0
Re: [PATCH resend] In virFDStreamRead(), fill buffer from this and next messages
Posted by Roman Bogorodskiy 1 month, 3 weeks ago 
  Erik Huelsmann wrote:

> Before this change, buffers returned from virFDStreamRead() would
> alternate in size (262120 and 24), because it only consumed the
> bytes remaining from the current background thread message.
> 
> As the background thread reads 262144 bytes (256kB) of data in
> each chunk, where the maximum size returned from virFDStreamRead()
> to be transferred over the remote protocol is only 262120, 24 bytes
> would be left in the buffer on each iteration. The next iteration
> leaves 24 bytes, which used to be returned without considering
> messages waiting in the queue.
> 
> Signed-off-by: Erik Huelsmann <ehuels@gmail.com>
> ---
>  src/util/virfdstream.c | 26 +++++++++++++++++---------
>  1 file changed, 17 insertions(+), 9 deletions(-)

Hi,

I noticed that with this change libvirt-tck tests started failing for
me on FreeBSD:

    ../scripts/storage/400-vol-download.t
    ../scripts/storage/405-vol-download-all.t
    ../scripts/storage/410-vol-download-nonblock.t

Things get back to normal when I revert this commit.

The only thing I see in the log is:

2026-02-16 18:04:27.505+0000: 75802027180048: error : virFDStreamRead:921 : stream is not open: Bad file descriptor

Is that something obvious or further debug information is necessary?

Thanks,
Roman
Re: [PATCH resend] In virFDStreamRead(), fill buffer from this and next messages
Posted by Daniel P. Berrangé via Devel 1 month, 2 weeks ago 
On Mon, Feb 16, 2026 at 07:31:16PM +0100, Roman Bogorodskiy wrote:
>   Erik Huelsmann wrote:
> 
> > Before this change, buffers returned from virFDStreamRead() would
> > alternate in size (262120 and 24), because it only consumed the
> > bytes remaining from the current background thread message.
> > 
> > As the background thread reads 262144 bytes (256kB) of data in
> > each chunk, where the maximum size returned from virFDStreamRead()
> > to be transferred over the remote protocol is only 262120, 24 bytes
> > would be left in the buffer on each iteration. The next iteration
> > leaves 24 bytes, which used to be returned without considering
> > messages waiting in the queue.
> > 
> > Signed-off-by: Erik Huelsmann <ehuels@gmail.com>
> > ---
> >  src/util/virfdstream.c | 26 +++++++++++++++++---------
> >  1 file changed, 17 insertions(+), 9 deletions(-)
> 
> Hi,
> 
> I noticed that with this change libvirt-tck tests started failing for
> me on FreeBSD:
> 
>     ../scripts/storage/400-vol-download.t
>     ../scripts/storage/405-vol-download-all.t
>     ../scripts/storage/410-vol-download-nonblock.t
> 
> Things get back to normal when I revert this commit.
> 
> The only thing I see in the log is:
> 
> 2026-02-16 18:04:27.505+0000: 75802027180048: error : virFDStreamRead:921 : stream is not open: Bad file descriptor
> 
> Is that something obvious or further debug information is necessary?

I didn't see the TCK fail, but I did strangely see that new error
message. After poking it a bit I think I found the root cause and
have CC'd you both on a patch.

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 :|
Re: [PATCH resend] In virFDStreamRead(), fill buffer from this and next messages
Posted by Erik Huelsmann 1 month, 2 weeks ago 
Hi Daniel,

> >
> > The only thing I see in the log is:
> >
> > 2026-02-16 18:04:27.505+0000: 75802027180048: error : virFDStreamRead:921 : stream is not open: Bad file descriptor
> >
> > Is that something obvious or further debug information is necessary?
>
> I didn't see the TCK fail, but I did strangely see that new error
> message. After poking it a bit I think I found the root cause and
> have CC'd you both on a patch.

Thanks for having a look! I only have Linux available, so your digging
around is very much appreciated.

I checked out your patch and I agree that's a condition I missed.
What's the process for me to indicate my approval/appreciation of the
patch (if anything is required beyond this e-mail)?


Regards,

-- 
Bye,

Erik.

http://efficito.com -- Hosted accounting and ERP.
Robust and Flexible. No vendor lock-in.
Re: [PATCH resend] In virFDStreamRead(), fill buffer from this and next messages
Posted by Daniel P. Berrangé via Devel 1 month, 2 weeks ago 
On Mon, Feb 23, 2026 at 09:29:24PM +0100, Erik Huelsmann wrote:
> Hi Daniel,
> 
> > >
> > > The only thing I see in the log is:
> > >
> > > 2026-02-16 18:04:27.505+0000: 75802027180048: error : virFDStreamRead:921 : stream is not open: Bad file descriptor
> > >
> > > Is that something obvious or further debug information is necessary?
> >
> > I didn't see the TCK fail, but I did strangely see that new error
> > message. After poking it a bit I think I found the root cause and
> > have CC'd you both on a patch.
> 
> Thanks for having a look! I only have Linux available, so your digging
> around is very much appreciated.
> 
> I checked out your patch and I agree that's a condition I missed.
> What's the process for me to indicate my approval/appreciation of the
> patch (if anything is required beyond this e-mail)?

Anyone on libvirt-list is free to send a reply to any patch
with 'Reviewed-by: NAME <EMAIL>'  if they wish to be creditted for
having done code review. Likewise for Tested-by if relevant.


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 :|
Re: [PATCH resend] In virFDStreamRead(), fill buffer from this and next messages
Posted by Erik Huelsmann 1 month, 3 weeks ago 
Hi Roman,

On Mon, Feb 16, 2026, 19:32 Roman Bogorodskiy <bogorodskiy@gmail.com> wrote:

>   Erik Huelsmann wrote:
>
> > Before this change, buffers returned from virFDStreamRead() would
> > alternate in size (262120 and 24), because it only consumed the
> > bytes remaining from the current background thread message.
> >
> > As the background thread reads 262144 bytes (256kB) of data in
> > each chunk, where the maximum size returned from virFDStreamRead()
> > to be transferred over the remote protocol is only 262120, 24 bytes
> > would be left in the buffer on each iteration. The next iteration
> > leaves 24 bytes, which used to be returned without considering
> > messages waiting in the queue.
> >
> > Signed-off-by: Erik Huelsmann <ehuels@gmail.com>
> > ---
> >  src/util/virfdstream.c | 26 +++++++++++++++++---------
> >  1 file changed, 17 insertions(+), 9 deletions(-)
>
> Hi,
>
> I noticed that with this change libvirt-tck tests started failing for
> me on FreeBSD:
>
>     ../scripts/storage/400-vol-download.t
>     ../scripts/storage/405-vol-download-all.t
>     ../scripts/storage/410-vol-download-nonblock.t
>
> Things get back to normal when I revert this commit.
>
> The only thing I see in the log is:
>
> 2026-02-16 18:04:27.505+0000: 75802027180048: error : virFDStreamRead:921
> : stream is not open: Bad file descriptor
>

I see these are Perl scripts. Can you run the tests in verboden mode?
("prove --verbose") That should provide an indication of the point in the
test where the error occurs



> Is that something obvious or further debug information is necessary?
>
> Thanks,
> Roman
>


Bye,

Erik.

http://efficito.com -- Hosted accounting and ERP.
Robust and Flexible. No vendor lock-in.
Re: [PATCH resend] In virFDStreamRead(), fill buffer from this and next messages
Posted by Roman Bogorodskiy 1 month, 3 weeks ago 
  Erik Huelsmann wrote:

> Hi Roman,
> 
> On Mon, Feb 16, 2026, 19:32 Roman Bogorodskiy <bogorodskiy@gmail.com> wrote:
> 
> >   Erik Huelsmann wrote:
> >
> > > Before this change, buffers returned from virFDStreamRead() would
> > > alternate in size (262120 and 24), because it only consumed the
> > > bytes remaining from the current background thread message.
> > >
> > > As the background thread reads 262144 bytes (256kB) of data in
> > > each chunk, where the maximum size returned from virFDStreamRead()
> > > to be transferred over the remote protocol is only 262120, 24 bytes
> > > would be left in the buffer on each iteration. The next iteration
> > > leaves 24 bytes, which used to be returned without considering
> > > messages waiting in the queue.
> > >
> > > Signed-off-by: Erik Huelsmann <ehuels@gmail.com>
> > > ---
> > >  src/util/virfdstream.c | 26 +++++++++++++++++---------
> > >  1 file changed, 17 insertions(+), 9 deletions(-)
> >
> > Hi,
> >
> > I noticed that with this change libvirt-tck tests started failing for
> > me on FreeBSD:
> >
> >     ../scripts/storage/400-vol-download.t
> >     ../scripts/storage/405-vol-download-all.t
> >     ../scripts/storage/410-vol-download-nonblock.t
> >
> > Things get back to normal when I revert this commit.
> >
> > The only thing I see in the log is:
> >
> > 2026-02-16 18:04:27.505+0000: 75802027180048: error : virFDStreamRead:921
> > : stream is not open: Bad file descriptor
> >
> 
> I see these are Perl scripts. Can you run the tests in verboden mode?
> ("prove --verbose") That should provide an indication of the point in the
> test where the error occurs

root@testrunner:~/libvirt-tck # PERL5LIB=./lib LIBVIRT_TCK_CONFIG=/etc/libvirt-tck/default.yml prove --verbose  ./scripts/storage/400-vol-download.t
./scripts/storage/400-vol-download.t .. 
1..16
# Defining transient storage pool
ok 1 - define transient storage pool
ok 2 - built storage pool
ok 3 - started storage pool
ok 4 - create raw volume
ok 5 - started download
libvirt error code: 38, message: stream is not open: Bad file descriptor
# Looks like your test exited with 255 just after 5.

By running it with '-d:Trace` it looks like it's failing on this
specific line:

https://gitlab.com/libvirt/libvirt-tck/-/blob/master/scripts/storage/400-vol-download.t?ref_type=heads#L108

>> ./scripts/storage/400-vol-download.t:108:    my $rv = $st->recv($data, $nbytes);
>> /usr/local/lib/perl5/site_perl/mach/5.42/Sys/Virt/Error.pm:52:     my $self = shift;
>> /usr/local/lib/perl5/site_perl/mach/5.42/Sys/Virt/Error.pm:54:     return "libvirt error code: " . $self->{code} . ", message: " . $self->{message} . ($self->{message} =~ /\n$/ ? "" : "\n");
libvirt error code: 38, message: stream is not open: Bad file descriptor

Also, looking at the trace, it's not the first iteration of the
`while (1) {` loop.
Re: [PATCH resend] In virFDStreamRead(), fill buffer from this and next messages
Posted by Michal Prívozník via Devel 1 month, 3 weeks ago 
On 2/17/26 20:14, Roman Bogorodskiy wrote:
>   Erik Huelsmann wrote:
> 
>> Hi Roman,
>>
>> On Mon, Feb 16, 2026, 19:32 Roman Bogorodskiy <bogorodskiy@gmail.com> wrote:
>>
>>>   Erik Huelsmann wrote:
>>>
>>>> Before this change, buffers returned from virFDStreamRead() would
>>>> alternate in size (262120 and 24), because it only consumed the
>>>> bytes remaining from the current background thread message.
>>>>
>>>> As the background thread reads 262144 bytes (256kB) of data in
>>>> each chunk, where the maximum size returned from virFDStreamRead()
>>>> to be transferred over the remote protocol is only 262120, 24 bytes
>>>> would be left in the buffer on each iteration. The next iteration
>>>> leaves 24 bytes, which used to be returned without considering
>>>> messages waiting in the queue.
>>>>
>>>> Signed-off-by: Erik Huelsmann <ehuels@gmail.com>
>>>> ---
>>>>  src/util/virfdstream.c | 26 +++++++++++++++++---------
>>>>  1 file changed, 17 insertions(+), 9 deletions(-)
>>>
>>> Hi,
>>>
>>> I noticed that with this change libvirt-tck tests started failing for
>>> me on FreeBSD:
>>>
>>>     ../scripts/storage/400-vol-download.t
>>>     ../scripts/storage/405-vol-download-all.t
>>>     ../scripts/storage/410-vol-download-nonblock.t
>>>
>>> Things get back to normal when I revert this commit.
>>>
>>> The only thing I see in the log is:
>>>
>>> 2026-02-16 18:04:27.505+0000: 75802027180048: error : virFDStreamRead:921
>>> : stream is not open: Bad file descriptor
>>>
>>
>> I see these are Perl scripts. Can you run the tests in verboden mode?
>> ("prove --verbose") That should provide an indication of the point in the
>> test where the error occurs
> 
> root@testrunner:~/libvirt-tck # PERL5LIB=./lib LIBVIRT_TCK_CONFIG=/etc/libvirt-tck/default.yml prove --verbose  ./scripts/storage/400-vol-download.t
> ./scripts/storage/400-vol-download.t .. 
> 1..16
> # Defining transient storage pool
> ok 1 - define transient storage pool
> ok 2 - built storage pool
> ok 3 - started storage pool
> ok 4 - create raw volume
> ok 5 - started download
> libvirt error code: 38, message: stream is not open: Bad file descriptor

This is weird. I tried to reproduce locally but failed to do so. Can you
perhaps rerun with LIBVIRT_DEBUG=1 and see whether there's something
that closed the stream? Or perhaps is there an error message in libvirtd
log?

Michal
Re: [PATCH resend] In virFDStreamRead(), fill buffer from this and next messages
Posted by Roman Bogorodskiy 1 month, 3 weeks ago 
  Michal Prívozník wrote:

> On 2/17/26 20:14, Roman Bogorodskiy wrote:
> >   Erik Huelsmann wrote:
> > 
> >> Hi Roman,
> >>
> >> On Mon, Feb 16, 2026, 19:32 Roman Bogorodskiy <bogorodskiy@gmail.com> wrote:
> >>
> >>>   Erik Huelsmann wrote:
> >>>
> >>>> Before this change, buffers returned from virFDStreamRead() would
> >>>> alternate in size (262120 and 24), because it only consumed the
> >>>> bytes remaining from the current background thread message.
> >>>>
> >>>> As the background thread reads 262144 bytes (256kB) of data in
> >>>> each chunk, where the maximum size returned from virFDStreamRead()
> >>>> to be transferred over the remote protocol is only 262120, 24 bytes
> >>>> would be left in the buffer on each iteration. The next iteration
> >>>> leaves 24 bytes, which used to be returned without considering
> >>>> messages waiting in the queue.
> >>>>
> >>>> Signed-off-by: Erik Huelsmann <ehuels@gmail.com>
> >>>> ---
> >>>>  src/util/virfdstream.c | 26 +++++++++++++++++---------
> >>>>  1 file changed, 17 insertions(+), 9 deletions(-)
> >>>
> >>> Hi,
> >>>
> >>> I noticed that with this change libvirt-tck tests started failing for
> >>> me on FreeBSD:
> >>>
> >>>     ../scripts/storage/400-vol-download.t
> >>>     ../scripts/storage/405-vol-download-all.t
> >>>     ../scripts/storage/410-vol-download-nonblock.t
> >>>
> >>> Things get back to normal when I revert this commit.
> >>>
> >>> The only thing I see in the log is:
> >>>
> >>> 2026-02-16 18:04:27.505+0000: 75802027180048: error : virFDStreamRead:921
> >>> : stream is not open: Bad file descriptor
> >>>
> >>
> >> I see these are Perl scripts. Can you run the tests in verboden mode?
> >> ("prove --verbose") That should provide an indication of the point in the
> >> test where the error occurs
> > 
> > root@testrunner:~/libvirt-tck # PERL5LIB=./lib LIBVIRT_TCK_CONFIG=/etc/libvirt-tck/default.yml prove --verbose  ./scripts/storage/400-vol-download.t
> > ./scripts/storage/400-vol-download.t .. 
> > 1..16
> > # Defining transient storage pool
> > ok 1 - define transient storage pool
> > ok 2 - built storage pool
> > ok 3 - started storage pool
> > ok 4 - create raw volume
> > ok 5 - started download
> > libvirt error code: 38, message: stream is not open: Bad file descriptor
> 
> This is weird. I tried to reproduce locally but failed to do so. Can you
> perhaps rerun with LIBVIRT_DEBUG=1 and see whether there's something
> that closed the stream? Or perhaps is there an error message in libvirtd
> log?
> 
> Michal

I was able to reproduce the issue without libvirt-tck.

Specifically, I created a test volume in the "dir" pool. As a test
volume I just use a text file with the "hello world!" text.

I download the volume using virsh like that:

virsh # vol-download --pool isos test.txt /dev/null

And on FreeBSD it fails with:

error: cannot receive data from volume test.txt
error: stream is not open: Bad file descriptor

However, if I manually specify the right length (13), it works fine,
e.g.:

vol-download --pool isos test.txt /dev/null --length 13

The same scenario works good for me on Fedora 43.

I have uploaded logs from both Linux and FreeBSD here:

https://people.freebsd.org/~novel/misc/libvirt-e23fd0b7fd-debug/vol-download-freebsd.txt

https://people.freebsd.org/~novel/misc/libvirt-e23fd0b7fd-debug/vol-download-linux.txt

It looks like the main difference that leads to the issue is that on
FreeBSD it is:

daemonStreamEvent:136 : st=0x5874e821ec20 events=9 EOF=0 closed=0

Where events=9 means VIR_STREAM_EVENT_READABLE +
VIR_STREAM_EVENT_HANGUP, so it goes for daemonStreamHandleRead() and
fails there.

On Linux, it is:

daemonStreamEvent:136 : st=0x7fdd240024d0 events=8 EOF=1 closed=0

so it's not VIR_STREAM_EVENT_READABLE and it sees the EOF.

I'm not yet sure why it doesn't set EOF=1 on FreeBSD.
Re: [PATCH resend] In virFDStreamRead(), fill buffer from this and next messages
Posted by Daniel P. Berrangé via Devel 2 months ago 
On Wed, Feb 11, 2026 at 06:36:06PM +0100, Erik Huelsmann wrote:
> Before this change, buffers returned from virFDStreamRead() would
> alternate in size (262120 and 24), because it only consumed the
> bytes remaining from the current background thread message.
> 
> As the background thread reads 262144 bytes (256kB) of data in
> each chunk, where the maximum size returned from virFDStreamRead()
> to be transferred over the remote protocol is only 262120, 24 bytes
> would be left in the buffer on each iteration. The next iteration
> leaves 24 bytes, which used to be returned without considering
> messages waiting in the queue.
> 
> Signed-off-by: Erik Huelsmann <ehuels@gmail.com>
> ---
>  src/util/virfdstream.c | 26 +++++++++++++++++---------
>  1 file changed, 17 insertions(+), 9 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


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 :|

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.