[v1] tools: console: Relax stream EOF handling

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

Posted by Roman Bolshakov 4 years, 8 months ago

An attempt to poweroff a VM from inside 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.

Fixes: 29f2b5248c6 ("tools: console: pass stream/fd errors to user")
Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
---
 tools/virsh-console.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/tools/virsh-console.c b/tools/virsh-console.c
index e16f841e57..408a745b0f 100644
--- a/tools/virsh-console.c
+++ b/tools/virsh-console.c
@@ -172,10 +172,6 @@ virConsoleEventOnStream(virStreamPtr st,
         if (got == -2)
             goto cleanup; /* blocking */
         if (got <= 0) {
-            if (got == 0)
-                virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-                               _("console stream EOF"));
-
             virConsoleShutdown(con);
             goto cleanup;
         }
-- 
2.22.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

Posted by Daniel Henrique Barboza 4 years, 8 months ago

(CCing Nikolay Shirokovskiy, author of  29f2b5248c )

On 8/7/19 8:34 AM, Roman Bolshakov wrote:
> An attempt to poweroff a VM from inside 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.
>
> Fixes: 29f2b5248c6 ("tools: console: pass stream/fd errors to user")
> Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
> ---
>   tools/virsh-console.c | 4 ----
>   1 file changed, 4 deletions(-)
>
> diff --git a/tools/virsh-console.c b/tools/virsh-console.c
> index e16f841e57..408a745b0f 100644
> --- a/tools/virsh-console.c
> +++ b/tools/virsh-console.c
> @@ -172,10 +172,6 @@ virConsoleEventOnStream(virStreamPtr st,
>           if (got == -2)
>               goto cleanup; /* blocking */
>           if (got <= 0) {
> -            if (got == 0)
> -                virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -                               _("console stream EOF"));
> -

I appreciate if we can remove the 'console stream EOF' error message
that started to appear even in regular VM shutdown, but I'm not sure
if this is the right fix.

'got' is the result of virStreamRecv(). Seeing the code documentation of
this function we have:

(src/libvirt-stream.c)

  * Returns the number of bytes read, which may be less
  * than requested.
  *
  * Returns 0 when the end of the stream is reached, at
  * which time the caller should invoke virStreamFinish()
  * to get confirmation of stream completion.
  *
  * Returns -1 upon error [....]

And this is the doc for virStreamFinish():

  * This method is a synchronization point for all asynchronous
  * errors, so if this returns a success code the application can
  * be sure that all data has been successfully processed.

In the existing code we're not calling 'virStreamFinish()' to assert whether
the got=0 from virStreamRecv is a successful shutdown or not, we're
simply reporting an internal error assuming that something went wrong.

Shouldn't we call virStreamFinish if got=0 and only report an error if
virStreamFinish returns -1? Does that suffice to fix this error message
being thrown when the VM does a regular shutdown?

Thanks,

DHB

>               virConsoleShutdown(con);
>               goto cleanup;
>           }

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

Posted by Roman Bolshakov 4 years, 8 months ago

On Thu, Aug 08, 2019 at 03:00:27PM -0300, Daniel Henrique Barboza wrote:
> (CCing Nikolay Shirokovskiy, author of  29f2b5248c )
> 
> On 8/7/19 8:34 AM, Roman Bolshakov wrote:
> > An attempt to poweroff a VM from inside 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.
> > 
> > Fixes: 29f2b5248c6 ("tools: console: pass stream/fd errors to user")
> > Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
> > ---
> >   tools/virsh-console.c | 4 ----
> >   1 file changed, 4 deletions(-)
> > 
> > diff --git a/tools/virsh-console.c b/tools/virsh-console.c
> > index e16f841e57..408a745b0f 100644
> > --- a/tools/virsh-console.c
> > +++ b/tools/virsh-console.c
> > @@ -172,10 +172,6 @@ virConsoleEventOnStream(virStreamPtr st,
> >           if (got == -2)
> >               goto cleanup; /* blocking */
> >           if (got <= 0) {
> > -            if (got == 0)
> > -                virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > -                               _("console stream EOF"));
> > -
> 
> I appreciate if we can remove the 'console stream EOF' error message
> that started to appear even in regular VM shutdown, but I'm not sure
> if this is the right fix.
> 
> 'got' is the result of virStreamRecv(). Seeing the code documentation of
> this function we have:
> 
> (src/libvirt-stream.c)
> 
>  * Returns the number of bytes read, which may be less
>  * than requested.
>  *
>  * Returns 0 when the end of the stream is reached, at
>  * which time the caller should invoke virStreamFinish()
>  * to get confirmation of stream completion.
>  *
>  * Returns -1 upon error [....]
> 
> 
> And this is the doc for virStreamFinish():
> 
>  * This method is a synchronization point for all asynchronous
>  * errors, so if this returns a success code the application can
>  * be sure that all data has been successfully processed.
> 
> 
> In the existing code we're not calling 'virStreamFinish()' to assert whether
> the got=0 from virStreamRecv is a successful shutdown or not, we're
> simply reporting an internal error assuming that something went wrong.
> 
> Shouldn't we call virStreamFinish if got=0 and only report an error if
> virStreamFinish returns -1? Does that suffice to fix this error message
> being thrown when the VM does a regular shutdown?
> 

That sounds right. virConsoleShutdown can be changed to take second
boolean parameter - needAbort and virStreamAbort will be invoked if
abort is needed on the stream. Otherwise, if EOF has been received,
virStreamFinish is called on the stream.

The need to call one of the functions is documented in virStreamNew:
   If a data stream has been used, then the application must call
   virStreamFinish or virStreamAbort before free'ing to, in order to
   notify the driver of termination.

As of now virConsoleShutdown doesn't print an error if virStreamAbort
fails. I can add them in v2 for both virStreamFinish and virStreamAbort.

Thanks,
Roman

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

Posted by Nikolay Shirokovskiy 4 years, 8 months ago


On 08.08.2019 21:00, Daniel Henrique Barboza wrote:
> (CCing Nikolay Shirokovskiy, author of  29f2b5248c )
> 
> On 8/7/19 8:34 AM, Roman Bolshakov wrote:
>> An attempt to poweroff a VM from inside 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.

Hi, all. 

Sorry for the late response, I was on a long vacation and have very limited
access to the internet.

Yes, the commit 29f2b5248c6 introduced printing this error if domain was
shutdowned. This can be unnecessary if the person who run the console
is the person who shutdowned the domain. But it is not so if these are
two different persons. Also what if domain is not shutdowned but rather
crashed? So I think it is make sense to leave the message as it is.

Nikolay


>>
>> Fixes: 29f2b5248c6 ("tools: console: pass stream/fd errors to user")
>> Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
>> ---
>>   tools/virsh-console.c | 4 ----
>>   1 file changed, 4 deletions(-)
>>
>> diff --git a/tools/virsh-console.c b/tools/virsh-console.c
>> index e16f841e57..408a745b0f 100644
>> --- a/tools/virsh-console.c
>> +++ b/tools/virsh-console.c
>> @@ -172,10 +172,6 @@ virConsoleEventOnStream(virStreamPtr st,
>>           if (got == -2)
>>               goto cleanup; /* blocking */
>>           if (got <= 0) {
>> -            if (got == 0)
>> -                virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> -                               _("console stream EOF"));
>> -
> 
> I appreciate if we can remove the 'console stream EOF' error message
> that started to appear even in regular VM shutdown, but I'm not sure
> if this is the right fix.
> 
> 'got' is the result of virStreamRecv(). Seeing the code documentation of
> this function we have:
> 
> (src/libvirt-stream.c)
> 
>  * Returns the number of bytes read, which may be less
>  * than requested.
>  *
>  * Returns 0 when the end of the stream is reached, at
>  * which time the caller should invoke virStreamFinish()
>  * to get confirmation of stream completion.
>  *
>  * Returns -1 upon error [....]
> 
> 
> And this is the doc for virStreamFinish():
> 
>  * This method is a synchronization point for all asynchronous
>  * errors, so if this returns a success code the application can
>  * be sure that all data has been successfully processed.
> 
> 
> In the existing code we're not calling 'virStreamFinish()' to assert whether
> the got=0 from virStreamRecv is a successful shutdown or not, we're
> simply reporting an internal error assuming that something went wrong.
> 
> Shouldn't we call virStreamFinish if got=0 and only report an error if
> virStreamFinish returns -1? Does that suffice to fix this error message
> being thrown when the VM does a regular shutdown?
> 
> 
> 
> Thanks,
> 
> 
> DHB
> 
> 
> 
> 
> 
>>               virConsoleShutdown(con);
>>               goto cleanup;
>>           }
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

Posted by Roman Bolshakov 4 years, 7 months ago

On Mon, Aug 26, 2019 at 07:51:18AM +0000, Nikolay Shirokovskiy wrote:
> 
> 
> On 08.08.2019 21:00, Daniel Henrique Barboza wrote:
> > (CCing Nikolay Shirokovskiy, author of  29f2b5248c )
> > 
> > On 8/7/19 8:34 AM, Roman Bolshakov wrote:
> >> An attempt to poweroff a VM from inside 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.
> 
> Hi, all. 
> 
> Sorry for the late response, I was on a long vacation and have very limited
> access to the internet.
> 
> Yes, the commit 29f2b5248c6 introduced printing this error if domain was
> shutdowned. This can be unnecessary if the person who run the console
> is the person who shutdowned the domain. But it is not so if these are
> two different persons. Also what if domain is not shutdowned but rather
> crashed? So I think it is make sense to leave the message as it is.
> 

Hi Nikolay,

virsh domstate + QEMU GA can be used to extract the information if
you're seeking to find the real termination reason. It has more states
and was designed specifically for the task.

IMO when virsh console is used in "interactive" shell scripts or
embedded in another application, bad exit code should mean there's a
problem in retrieving the serial output and thus the serial session
cannot be considered complete. And unconditional failure doesn't provive
a way to distinguish line errors from successful serial session.

Hope this helps,
Roman

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list