[libvirt] [PATCH 2/2] tools: console: pass stream/fd errors to user

[Nikolay Shirokovskiy]

On 22.02.2019 18:07, John Ferlan wrote:
> 
> 
> On 2/15/19 9:39 AM, Nikolay Shirokovskiy wrote:
>> If console disconnected due to connection problem or problem on server
> 
> If the console was disconnected due to a connection problem or a problem
> on the server
> 
>> side for example it is convinient to provide the cause to the user.
> 
> side it is convenient
> 
>> If error comes from API then error is saved in virsh global variable
> 
> If the error come from the API, then the error is save in a virsh global
> variable.
> 
>> but as we return success from virshRunConsole if we reach waiting
> 
> However, since success is returned from virshRunConsole and we reach the
> waiting
> 
>> stage then error is never reported. Let's track for error in event loop!
> 
> stage, then the error is never reported. Let's track the error in the
> event loop.
>>
>> Next after failure we do a cleanup and this cleanup can overwrite
>> root cause. Let's save root cause and then set it to virsh error
>> after all cleanup is done.
> 
> [I think this paragraph could be dropped in favor of below]

I would like to keep it. It explains next construction in patch.

+
+    if (ret < 0) {
+        vshResetLibvirtError();
+        virSetError(&con->error);
+        vshSaveLibvirtHelperError();
+    }
+

Nikolay

> 
>>
>> Let's also add missing error reports in code.
> 
> Written this way it feels like an extra patch; however, I know it's
> not...  How about...
> 
> Since we'll be sending the error to the consumer, each failure path from
> the event handlers needs to be augmented to provide what error generated
> the failure so that can be reported properly during cleanup processing
> from virshRunConsole.
> 
> 
>>
>> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy at virtuozzo.com>
>> ---
>>  tools/virsh-console.c | 59 +++++++++++++++++++++++++++++++++++++++++++--------
>>  1 file changed, 50 insertions(+), 9 deletions(-)
>>
>> diff --git a/tools/virsh-console.c b/tools/virsh-console.c
>> index c0c3f90..f1ad076 100644
>> --- a/tools/virsh-console.c
>> +++ b/tools/virsh-console.c
>> @@ -73,6 +73,7 @@ struct virConsole {
>>      struct virConsoleBuffer terminalToStream;
>>  
>>      char escapeChar;
>> +    virError error;
>>  };
>>  
>>  static virClassPtr virConsoleClass;
>> @@ -98,6 +99,11 @@ virConsoleHandleSignal(int sig ATTRIBUTE_UNUSED)
>>  static void
>>  virConsoleShutdown(virConsolePtr con)
>>  {
>> +    virErrorPtr err = virGetLastError();
>> +
>> +    if (con->error.code == VIR_ERR_OK && err && err->code != VIR_ERR_OK)
>> +        virCopyLastError(&con->error);
>> +
>>      if (con->st) {
>>          virStreamEventRemoveCallback(con->st);
>>          virStreamAbort(con->st);
>> @@ -126,6 +132,7 @@ virConsoleDispose(void *obj)
>>          virStreamFree(con->st);
>>  
>>      virCondDestroy(&con->cond);
>> +    virResetError(&con->error);
>>  }
>>  
>>  
>> @@ -162,7 +169,13 @@ virConsoleEventOnStream(virStreamPtr st,
>>                              avail);
>>          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;
>> +        }
>> +        if (got < 0) {
> 
> if (got <= 0) {
>     if (got == 0)
>         virReportError(...)
>     virConsoleShutdown
>     goto cleanup
> }
> 
>>              virConsoleShutdown(con);
>>              goto cleanup;
>>          }
>> @@ -245,11 +258,14 @@ virConsoleEventOnStdin(int watch ATTRIBUTE_UNUSED,
>>                     con->terminalToStream.offset,
>>                     avail);
>>          if (got < 0) {
>> -            if (errno != EAGAIN)
>> +            if (errno != EAGAIN) {
>> +                virReportSystemError(errno, "%s", _("can not read from stdin"));
> 
> "cannot" is fine (similar below)
> 
> John
> 
>>                  virConsoleShutdown(con);
>> +            }
>>              goto cleanup;
>>          }
>>          if (got == 0) {
>> +            virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("EOF on stdin"));
>>              virConsoleShutdown(con);
>>              goto cleanup;
>>          }
>> @@ -265,9 +281,16 @@ virConsoleEventOnStdin(int watch ATTRIBUTE_UNUSED,
>>                                           VIR_STREAM_EVENT_WRITABLE);
>>      }
>>  
>> -    if (events & VIR_EVENT_HANDLE_ERROR ||
>> -        events & VIR_EVENT_HANDLE_HANGUP) {
>> +    if (events & VIR_EVENT_HANDLE_ERROR) {
>> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("IO error on stdin"));
>>          virConsoleShutdown(con);
>> +        goto cleanup;
>> +    }
>> +
>> +    if (events & VIR_EVENT_HANDLE_HANGUP) {
>> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("EOF on stdin"));
>> +        virConsoleShutdown(con);
>> +        goto cleanup;
>>      }
>>  
>>   cleanup:
>> @@ -297,8 +320,10 @@ virConsoleEventOnStdout(int watch ATTRIBUTE_UNUSED,
>>                       con->streamToTerminal.data,
>>                       con->streamToTerminal.offset);
>>          if (done < 0) {
>> -            if (errno != EAGAIN)
>> +            if (errno != EAGAIN) {
>> +                virReportSystemError(errno, "%s", _("can not write to stdout"));
>>                  virConsoleShutdown(con);
>> +            }
>>              goto cleanup;
>>          }
>>          memmove(con->streamToTerminal.data,
>> @@ -317,9 +342,16 @@ virConsoleEventOnStdout(int watch ATTRIBUTE_UNUSED,
>>      if (!con->streamToTerminal.offset)
>>          virEventUpdateHandle(con->stdoutWatch, 0);
>>  
>> -    if (events & VIR_EVENT_HANDLE_ERROR ||
>> -        events & VIR_EVENT_HANDLE_HANGUP) {
>> +    if (events & VIR_EVENT_HANDLE_ERROR) {
>> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("IO error stdout"));
>>          virConsoleShutdown(con);
>> +        goto cleanup;
>> +    }
>> +
>> +    if (events & VIR_EVENT_HANDLE_HANGUP) {
>> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("EOF on stdout"));
>> +        virConsoleShutdown(con);
>> +        goto cleanup;
>>      }
>>  
>>   cleanup:
>> @@ -447,15 +479,24 @@ virshRunConsole(vshControl *ctl,
>>  
>>      while (!con->quit) {
>>          if (virCondWait(&con->cond, &con->parent.lock) < 0) {
>> -            VIR_ERROR(_("unable to wait on console condition"));
>> +            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> +                           _("unable to wait on console condition"));
>>              goto cleanup;
>>          }
>>      }
>>  
>> -    ret = 0;
>> +    if (con->error.code == VIR_ERR_OK)
>> +        ret = 0;
>>  
>>   cleanup:
>>      virConsoleShutdown(con);
>> +
>> +    if (ret < 0) {
>> +        vshResetLibvirtError();
>> +        virSetError(&con->error);
>> +        vshSaveLibvirtHelperError();
>> +    }
>> +
>>      virObjectUnlock(con);
>>      virObjectUnref(con);
>>  
>>