Re: [PATCH] Revert "Ensure async packets never get marked for sync replies"

["Daniel P. Berrange" <berrange@xxxxxxxxxx>]

On Tue, Sep 20, 2011 at 06:20:48PM +0200, Michal Privoznik wrote:
> This partially reverts commit 984840a2c292402926ad100aeea33f8859ff31a9.
> 
> Without this patch, client don't finish a stream which makes any
> API involving virStream block indefinitely.
> ---
>  src/rpc/virnetclient.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c
> index 055361d..50f46ea 100644
> --- a/src/rpc/virnetclient.c
> +++ b/src/rpc/virnetclient.c
> @@ -627,6 +627,11 @@ static int virNetClientCallDispatchStream(virNetClientPtr client)
>      case VIR_NET_CONTINUE: {
>          if (virNetClientStreamQueuePacket(st, &client->msg) < 0)
>              return -1;
> +
> +        if (thecall && thecall->expectReply) {
> +            VIR_DEBUG("Got sync data packet completion");
> +            thecall->mode = VIR_NET_CLIENT_MODE_COMPLETE;
> +        }
>          return 0;
>      }

This doesn't work, because 'thecall' could be the RPC message associated
with a virStreamAbort() call, in which case we'd be signalling abort
too soon, and when the real VIR_NET_ERROR message for the abort later
arraives we'll not know what todo with it. This is what the quoted commit
was solving.

The problem is that with doing virStreamRecv(), we will put a fake call
on the queue. We only want to signal completion of those fake calls.
This fake call should have been set to have status VIR_NET_CONTINUE
but we forgot that. So I think we need this instead:

diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c
index 055361d..57c75c0 100644
--- a/src/rpc/virnetclient.c
+++ b/src/rpc/virnetclient.c
@@ -627,6 +627,18 @@ static int virNetClientCallDispatchStream(virNetClientPtr client)
     case VIR_NET_CONTINUE: {
         if (virNetClientStreamQueuePacket(st, &client->msg) < 0)
             return -1;
+
+        if (thecall && thecall->expectReply) {
+            if (thecall->msg->header.status == VIR_NET_CONTINUE) {
+                VIR_DEBUG("Got a synchronous confirm");
+                thecall->mode = VIR_NET_CLIENT_MODE_COMPLETE;
+            } else {
+                VIR_DEBUG("Not completing call with status %d", thecall->msg->header.status);
+            }
+        } else {
+            VIR_DEBUG("Got unexpected async stream finish confirmation");
+            return -1;
+        }
         return 0;
     }
 
@@ -1189,6 +1201,7 @@ int virNetClientSend(virNetClientPtr client,
     int ret = -1;
 
     if (expectReply &&
+        (msg->bufferLength != 0) &&
         (msg->header.status == VIR_NET_CONTINUE)) {
         virNetError(VIR_ERR_INTERNAL_ERROR, "%s",
                     _("Attempt to send an asynchronous message with a synchronous reply"));
diff --git a/src/rpc/virnetclientstream.c b/src/rpc/virnetclientstream.c
index 2cc84d4..4cd0295 100644
--- a/src/rpc/virnetclientstream.c
+++ b/src/rpc/virnetclientstream.c
@@ -400,6 +400,7 @@ int virNetClientStreamRecvPacket(virNetClientStreamPtr st,
         msg->header.type = VIR_NET_STREAM;
         msg->header.serial = st->serial;
         msg->header.proc = st->proc;
+        msg->header.status = VIR_NET_CONTINUE;
 
         VIR_DEBUG("Dummy packet to wait for stream data");
         virMutexUnlock(&st->lock);


We will need to Dave Allen to confirm that his  console program does not
get a regression with this change added.

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

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