On Tue, Sep 20, 2011 at 06:00:55PM +0100, Daniel P. Berrange wrote:

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.

Applying just this patch to the current head breaks both my program and virsh console: # ./upstream_libvirt/install/bin/virsh console foo Connected to domain foo Escape character is ^] 15:51:31.911: 28047: info : libvirt version: 0.9.5 15:51:31.911: 28047: warning : virNetClientIncomingEvent:1187 : Something went wrong during async message processing Fedora release 1 Was there some other patch required? Dave