[libvirt] [PATCH] util, remote: Fixing the sending of stream when length is defined.
When a length of a file is defined, the responsible thread to send stream is finishing inappropriately. It is happening because there is wrong conditional which compares an offset with the length and because of that the code throws ENOSPC error. To test it: virsh# vol-upload ... --length N Signed-off-by: Julio Faracco <jcfaracco@gmail.com> --- src/remote/remote_daemon_stream.c | 24 ++++++++++++------------ src/util/virfdstream.c | 5 +---- 2 files changed, 13 insertions(+), 16 deletions(-) diff --git a/src/remote/remote_daemon_stream.c b/src/remote/remote_daemon_stream.c index 4dd3af9e0..998e82a83 100644 --- a/src/remote/remote_daemon_stream.c +++ b/src/remote/remote_daemon_stream.c @@ -549,21 +549,21 @@ daemonStreamHandleWriteData(virNetServerClientPtr client, } else if (ret == -2) { /* Blocking, so indicate we have more todo later */ return 1; - } else { - virNetMessageError rerr; + } else if (ret) { + virNetMessageError rerr; - memset(&rerr, 0, sizeof(rerr)); + memset(&rerr, 0, sizeof(rerr)); - VIR_INFO("Stream send failed"); - stream->closed = true; - virStreamEventRemoveCallback(stream->st); - virStreamAbort(stream->st); + VIR_INFO("Stream send failed"); + stream->closed = true; + virStreamEventRemoveCallback(stream->st); + virStreamAbort(stream->st); - return virNetServerProgramSendReplyError(stream->prog, - client, - msg, - &rerr, - &msg->header); + return virNetServerProgramSendReplyError(stream->prog, + client, + msg, + &rerr, + &msg->header); } return 0; diff --git a/src/util/virfdstream.c b/src/util/virfdstream.c index be40379a9..c3198c768 100644 --- a/src/util/virfdstream.c +++ b/src/util/virfdstream.c @@ -627,9 +627,6 @@ virFDStreamThread(void *opaque) if (got < 0) goto error; - if (got == 0) - break; - total += got; } @@ -783,7 +780,7 @@ static int virFDStreamWrite(virStreamPtr st, const char *bytes, size_t nbytes) virObjectLock(fdst); if (fdst->length) { - if (fdst->length == fdst->offset) { + if (fdst->offset > fdst->length) { virReportSystemError(ENOSPC, "%s", _("cannot write to stream")); virObjectUnlock(fdst); -- 2.14.1
On Sun, Mar 04, 2018 at 16:24:21 -0300, Julio Faracco wrote:
When a length of a file is defined, the responsible thread to send stream is finishing inappropriately. It is happening because there is wrong conditional which compares an offset with the length and because of that the code throws ENOSPC error.
To test it: virsh# vol-upload ... --length N
Signed-off-by: Julio Faracco <jcfaracco@gmail.com> --- src/remote/remote_daemon_stream.c | 24 ++++++++++++------------ src/util/virfdstream.c | 5 +---- 2 files changed, 13 insertions(+), 16 deletions(-)
diff --git a/src/remote/remote_daemon_stream.c b/src/remote/remote_daemon_stream.c index 4dd3af9e0..998e82a83 100644 --- a/src/remote/remote_daemon_stream.c +++ b/src/remote/remote_daemon_stream.c @@ -549,21 +549,21 @@ daemonStreamHandleWriteData(virNetServerClientPtr client, } else if (ret == -2) { /* Blocking, so indicate we have more todo later */ return 1; - } else { - virNetMessageError rerr; + } else if (ret) { + virNetMessageError rerr;
- memset(&rerr, 0, sizeof(rerr)); + memset(&rerr, 0, sizeof(rerr));
- VIR_INFO("Stream send failed"); - stream->closed = true; - virStreamEventRemoveCallback(stream->st); - virStreamAbort(stream->st); + VIR_INFO("Stream send failed"); + stream->closed = true; + virStreamEventRemoveCallback(stream->st); + virStreamAbort(stream->st);
It does not seem to be necessary to change indentation because of your change here.
On 03/04/2018 08:24 PM, Julio Faracco wrote:
When a length of a file is defined, the responsible thread to send stream is finishing inappropriately. It is happening because there is wrong conditional which compares an offset with the length and because of that the code throws ENOSPC error.
To test it: virsh# vol-upload ... --length N
Signed-off-by: Julio Faracco <jcfaracco@gmail.com> --- src/remote/remote_daemon_stream.c | 24 ++++++++++++------------ src/util/virfdstream.c | 5 +---- 2 files changed, 13 insertions(+), 16 deletions(-)
So there are couple of problems here. The first one: This patch as is (apart from points raised by Peter) makes fdstreamtest loop endlessly. Secondly, the ENOSPC error is not transferred to the client (although it is reported in the logs): # virsh vol-upload --length 1000 --pool default --vol vol.img --file /dev/random error: cannot close volume fd.img error: Library function returned error but did not set virError For this I have a patch and I'll send it soon. But the root cause is that client indeed sends more data than announced in --length (thank God for the wireshark dissector plugin). So server replies with ENOSPC. However, this always used to be the case even before I touched that part of code. So I think the proper fix is to close the stream at daemon side once fdst->offset reaches fdst->length. My recollection is that back in days of iohelper we had a pipe where we just pushed incoming data to and iohelper pulled it and wrote it into the file. However, as soon as it reached fdst->length limit it died which caused stream to get closed. Michal
participants (3)
-
Julio Faracco -
Michal Privoznik -
Peter Krempa