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

[Roman Bolshakov]

On Mon, Aug 12, 2019 at 07:01:47PM -0300, 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.
> 
> 

Existing implementations of virStreamFinish and virStreamAbort for esx,
remote and fd streams behave exactly the same. They close the steam
using the same function. I haven't found any other difference. So if we
intend to minimize changes, we should use v1. It will work exactly the
same except it calls virStreamAbort to close the stream. I can also add
a check to print the error if virStreamAbort in virConsoleShutdown
fails.

However, if we want to be correct from documentation standpoint, we
should use v2.

Thanks,
Roman