On Tue, Jan 20, 2026 at 11:34:23PM +0100, Erik Huelsmann wrote:
> > > I *do* understand the perspective of the virfdstream implementation to
> > > read chunks of 256kB (and not 256kB minus 24). I do understand that -
> > > from a simplicity perspective - it makes sense to push out one message
> > > from the transfer queue at a time, but is the intent that the average
> > > transfer then becomes 128kB minus 12 bytes?
> >
> > Ok, this is an implementation flaw in virfdstream.
> >
> > There are two scenarios (1) reading from a local file - fdstream spawns
> > a background helper thread; (1) reading from a non-file - fdstream
> > reads directly from the FD.
> >
> > When reading directly from the FD, the buffer passed to virFDStreamRead
> > will be filled with as much data as is available.
> >
> > When reading via the helper thread, the buffer passed to virFDStreamRead
> > will only be filled with the data from the 1st pending message from the
> > background thread.
>
> I've studied the code and I see what you are saying indeed. As soon as
> the data in that message runs out the function prepares to return and
> just before it returns, pops the message from the transfer queue. It
> could instead go back to the beginning (nearly) to grab the next
> message and repeat itself. I guess that's not too hard to add.
>
> > We need to fix the latter so that it will fill the buffer with data from
> > *all* pending messages from the thread, until the buffer is full.
>
> Ok. I think I understand what needs to happen here. Do you mean me by
> "we" or were you looking at someone who's part of the "regulars"?
Anyone - if you have time that's great, otherwise just file a bug.
> If it's me, I *think* I can do this, but I'll need a bit of help to
> confirm my understanding that *any* "hole" this function encounters,
> is an unexpected message; I mean: should this function - after it's
> been changed - take into account that a "STREAM_HOLE" message may come
> in and that it needs to stop processing data, but it should *not*
> raise an error (as it would do now, because STREAM_HOLE messages are
> not expected right now).
IIUC, for the *first* message on the queue, we should maintain the
current logic
/* Shortcut, if the stream is in the trailing hole,
* return 0 immediately. */
if (msg->type == VIR_FDSTREAM_MSG_TYPE_HOLE &&
msg->stream.hole.len == 0) {
ret = 0;
goto cleanup;
}
If we've already copied some data out of the *first* message, and
thus are looking at a subsequent message, we should simply return
what data we have already copied if we see a MSG_TYPE_HOLE on the
queue pending next.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
