On 8/13/19 12:01 AM, Daniel Henrique Barboza wrote:
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.
I agree that this is better solution. One way to view libvirt streams is
as a pipe. What is written on one side appears on the other. In the
analogy, if a reader gets 0 bytes from the pipe on read() they know it's
EOF and they call close() (or virStreamFinish() in this case).
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
