[libvirt] [PATCH] util, remote: Fixing the sending of stream when length is defined.

Julio Faracco posted 1 patch 6 years ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20180304192421.21893-1-jcfaracco@gmail.com
Test syntax-check passed
src/remote/remote_daemon_stream.c | 24 ++++++++++++------------
src/util/virfdstream.c            |  5 +----
2 files changed, 13 insertions(+), 16 deletions(-)
[libvirt] [PATCH] util, remote: Fixing the sending of stream when length is defined.
Posted by Julio Faracco 6 years ago
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] util, remote: Fixing the sending of stream when length is defined.
Posted by Julio Faracco 6 years ago
I'm not sure about this part.
When offset is equal of length, nbytes is 0 and the function will return 0.

Do you see any possible problems to remove this part?
I checked all ret = 0 and I'm not seeing problems until now.

> -        if (got == 0)
> -            break;
> -

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] util, remote: Fixing the sending of stream when length is defined.
Posted by Peter Krempa 6 years ago
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.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] util, remote: Fixing the sending of stream when length is defined.
Posted by Michal Privoznik 6 years ago
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list