[libvirt] [PATCH v2] tools: console: Relax stream EOF handling
Daniel Henrique Barboza
danielhb413 at gmail.com
Mon Aug 12 22:01:47 UTC 2019
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.
Thanks,
DHB
On 8/12/19 8:15 AM, Roman Bolshakov wrote:
> Regular VM shutdown triggers the error for existing session of virsh
> console and it returns with non-zero exit code:
> error: internal error: console stream EOF
>
> The message and status code are misleading because there's no real
> error. virStreamRecv returns 0 when the end of the stream is reached.
> In that case, virStreamFinish should be used to get confirmation of
> stream completion. virSteramAbort is used otherwise to terminate the
> stream early before an EOF.
>
> Fixes: 29f2b5248c6 ("tools: console: pass stream/fd errors to user")
> Signed-off-by: Roman Bolshakov <r.bolshakov at yadro.com>
> ---
> Changes since v1:
> - Added needAbort parameter to virConsoleShutdown to specify if finish
> or abort is needed
> - Use it (needAbort = false) to finish stream after EOF from
> virStreamRecv
> - Print internal error if virStreamFinish or virStreamAbort do not
> succeed
>
> tools/virsh-console.c | 44 ++++++++++++++++++++++++-------------------
> 1 file changed, 25 insertions(+), 19 deletions(-)
>
> diff --git a/tools/virsh-console.c b/tools/virsh-console.c
> index e16f841e57..ad514a99a7 100644
> --- a/tools/virsh-console.c
> +++ b/tools/virsh-console.c
> @@ -97,7 +97,7 @@ virConsoleHandleSignal(int sig ATTRIBUTE_UNUSED)
>
>
> static void
> -virConsoleShutdown(virConsolePtr con)
> +virConsoleShutdown(virConsolePtr con, bool needAbort)
> {
> virErrorPtr err = virGetLastError();
>
> @@ -105,8 +105,15 @@ virConsoleShutdown(virConsolePtr con)
> virCopyLastError(&con->error);
>
> if (con->st) {
> + int termError;
> virStreamEventRemoveCallback(con->st);
> - virStreamAbort(con->st);
> + if (needAbort)
> + termError = virStreamAbort(con->st);
> + else
> + termError = virStreamFinish(con->st);
> + if (termError < 0)
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("cannot terminate console stream"));
> virStreamFree(con->st);
> con->st = NULL;
> }
> @@ -158,7 +165,7 @@ virConsoleEventOnStream(virStreamPtr st,
> if (avail < 1024) {
> if (VIR_REALLOC_N(con->streamToTerminal.data,
> con->streamToTerminal.length + 1024) < 0) {
> - virConsoleShutdown(con);
> + virConsoleShutdown(con, true);
> goto cleanup;
> }
> con->streamToTerminal.length += 1024;
> @@ -172,11 +179,10 @@ virConsoleEventOnStream(virStreamPtr st,
> if (got == -2)
> goto cleanup; /* blocking */
> if (got <= 0) {
> + bool needAbort = true;
> if (got == 0)
> - virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> - _("console stream EOF"));
> -
> - virConsoleShutdown(con);
> + needAbort = false;
> + virConsoleShutdown(con, needAbort);
> goto cleanup;
> }
> con->streamToTerminal.offset += got;
> @@ -195,7 +201,7 @@ virConsoleEventOnStream(virStreamPtr st,
> if (done == -2)
> goto cleanup; /* blocking */
> if (done < 0) {
> - virConsoleShutdown(con);
> + virConsoleShutdown(con, true);
> goto cleanup;
> }
> memmove(con->terminalToStream.data,
> @@ -216,7 +222,7 @@ virConsoleEventOnStream(virStreamPtr st,
>
> if (events & VIR_STREAM_EVENT_ERROR ||
> events & VIR_STREAM_EVENT_HANGUP) {
> - virConsoleShutdown(con);
> + virConsoleShutdown(con, true);
> }
>
> cleanup:
> @@ -246,7 +252,7 @@ virConsoleEventOnStdin(int watch ATTRIBUTE_UNUSED,
> if (avail < 1024) {
> if (VIR_REALLOC_N(con->terminalToStream.data,
> con->terminalToStream.length + 1024) < 0) {
> - virConsoleShutdown(con);
> + virConsoleShutdown(con, true);
> goto cleanup;
> }
> con->terminalToStream.length += 1024;
> @@ -260,17 +266,17 @@ virConsoleEventOnStdin(int watch ATTRIBUTE_UNUSED,
> if (got < 0) {
> if (errno != EAGAIN) {
> virReportSystemError(errno, "%s", _("cannot read from stdin"));
> - virConsoleShutdown(con);
> + virConsoleShutdown(con, true);
> }
> goto cleanup;
> }
> if (got == 0) {
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("EOF on stdin"));
> - virConsoleShutdown(con);
> + virConsoleShutdown(con, true);
> goto cleanup;
> }
> if (con->terminalToStream.data[con->terminalToStream.offset] == con->escapeChar) {
> - virConsoleShutdown(con);
> + virConsoleShutdown(con, true);
> goto cleanup;
> }
>
> @@ -283,13 +289,13 @@ virConsoleEventOnStdin(int watch ATTRIBUTE_UNUSED,
>
> if (events & VIR_EVENT_HANDLE_ERROR) {
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("IO error on stdin"));
> - virConsoleShutdown(con);
> + virConsoleShutdown(con, true);
> goto cleanup;
> }
>
> if (events & VIR_EVENT_HANDLE_HANGUP) {
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("EOF on stdin"));
> - virConsoleShutdown(con);
> + virConsoleShutdown(con, true);
> goto cleanup;
> }
>
> @@ -322,7 +328,7 @@ virConsoleEventOnStdout(int watch ATTRIBUTE_UNUSED,
> if (done < 0) {
> if (errno != EAGAIN) {
> virReportSystemError(errno, "%s", _("cannot write to stdout"));
> - virConsoleShutdown(con);
> + virConsoleShutdown(con, true);
> }
> goto cleanup;
> }
> @@ -344,13 +350,13 @@ virConsoleEventOnStdout(int watch ATTRIBUTE_UNUSED,
>
> if (events & VIR_EVENT_HANDLE_ERROR) {
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("IO error stdout"));
> - virConsoleShutdown(con);
> + virConsoleShutdown(con, true);
> goto cleanup;
> }
>
> if (events & VIR_EVENT_HANDLE_HANGUP) {
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("EOF on stdout"));
> - virConsoleShutdown(con);
> + virConsoleShutdown(con, true);
> goto cleanup;
> }
>
> @@ -489,7 +495,7 @@ virshRunConsole(vshControl *ctl,
> ret = 0;
>
> cleanup:
> - virConsoleShutdown(con);
> + virConsoleShutdown(con, true);
>
> if (ret < 0) {
> vshResetLibvirtError();
More information about the libvir-list
mailing list