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@xxxxxxxxxxxxx>
>> ---
>> 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);
>>
>>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
