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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list