Re: [libvirt] [PATCH v2] tools: console: Relax stream EOF handling

The comment I have here is that you're changing virConsoleShutdown API for all callers, with this new boolean value 'needAbort', because of a scenario (virStreamRecv being called before) that happens only on virConsoleEventOnStream. This is what I wondered in the review of the previous version of this patch: "Shouldn't we call virStreamFinish if got=0 and only report an error if virStreamFinish returns -1?" I tried it out and it worked like a charm for me: diff --git a/tools/virsh-console.c b/tools/virsh-console.c index e16f841..998662c 100644 --- a/tools/virsh-console.c +++ b/tools/virsh-console.c @@ -172,9 +172,12 @@ virConsoleEventOnStream(virStreamPtr st,          if (got == -2)              goto cleanup; /* blocking */          if (got <= 0) { -            if (got == 0) -                virReportError(VIR_ERR_INTERNAL_ERROR, "%s", -                               _("console stream EOF")); +            if (got == 0) { +                got = virStreamFinish(st); +                if (got != 0) +                    virReportError(VIR_ERR_INTERNAL_ERROR, "%s", +                                   _("console stream EOF")); +            }              virConsoleShutdown(con);              goto cleanup; I didn't see any errors by calling virStreamFinish() before virStreamAbort() being called by virConsoleShutdown() right after (and by reading the code, it appears to be an OK usage), so I am skeptical about safeguarding the virStreamAbort() call with a boolean value like you're making here. To be clear, I'm not saying your patch is wrong - and perhaps there is a case for that safeguard inside virConsoleShutdown like you're doing here. My point here is that if the code I shown above isn't breaking anything, I'd rather go for that.