Re: [PATCH 1/2] tools: console: cleanup console on errors in main thread

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 22.02.2019 17:55, John Ferlan wrote:
> 
> 
> On 2/15/19 9:39 AM, Nikolay Shirokovskiy wrote:
>> We only check now for virObjectWait failures in virshRunConsole but
>> we'd better check and for other failures too. Anyway if failure
>> happened we need to shutdown console to stop delivering events
>> from event loop thread or we are in trouble as console is freed
>> on virshRunConsole exit.
>>
>> And we need to add refcounter to console object because event
>> can be delivered after we remove fd handle/stream callback.
>>
>> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@xxxxxxxxxxxxx>
>> ---
>>  tools/virsh-console.c | 161 +++++++++++++++++++++++++++++++++++++-------------
>>  1 file changed, 119 insertions(+), 42 deletions(-)
>>
> 
> There's three things going on in this patch, let's split into convert
> virConsole to virObjectLockable, the usage of locks in the event
> callback methods, and finally handling the late event.
> 
> FWIW: I could see the order being
> 
> 1. Add the virMutex{Lock|Unlock} into the functions and use cleanup path
> logic since right now @con is being altered without the lock.
> 
> 2. Convert to using virObjectLockable
> 
> 3. Add logic to handle late event
> 
> 
>> diff --git a/tools/virsh-console.c b/tools/virsh-console.c
>> index 045a636..c0c3f90 100644
>> --- a/tools/virsh-console.c
>> +++ b/tools/virsh-console.c
>> @@ -60,9 +60,10 @@ struct virConsoleBuffer {
>>  typedef struct virConsole virConsole;
>>  typedef virConsole *virConsolePtr;
>>  struct virConsole {
>> +    virObjectLockable parent;
>> +
>>      virStreamPtr st;
>>      bool quit;
>> -    virMutex lock;
>>      virCond cond;
>>  
>>      int stdinWatch;
>> @@ -74,6 +75,19 @@ struct virConsole {
>>      char escapeChar;
>>  };
>>  
>> +static virClassPtr virConsoleClass;
>> +static void virConsoleDispose(void *obj);
>> +
>> +static int
>> +virConsoleOnceInit(void)
>> +{
>> +    if (!VIR_CLASS_NEW(virConsole, virClassForObjectLockable()))
>> +        return -1;
>> +
>> +    return 0;
>> +}
>> +
>> +VIR_ONCE_GLOBAL_INIT(virConsole);
>>  
>>  static void
>>  virConsoleHandleSignal(int sig ATTRIBUTE_UNUSED)
>> @@ -104,16 +118,14 @@ virConsoleShutdown(virConsolePtr con)
>>  
>>  
>>  static void
>> -virConsoleFree(virConsolePtr con)
>> +virConsoleDispose(void *obj)
>>  {
>> -    if (!con)
>> -        return;
>> +    virConsolePtr con = obj;
>>  
>>      if (con->st)
>>          virStreamFree(con->st);
>> -    virMutexDestroy(&con->lock);
>> +
>>      virCondDestroy(&con->cond);
>> -    VIR_FREE(con);
>>  }
>>  
>>  
>> @@ -123,6 +135,12 @@ virConsoleEventOnStream(virStreamPtr st,
>>  {
>>      virConsolePtr con = opaque;
>>  
>> +    virObjectLock(con);
>> +
>> +    /* we got late event after console shutdowned */
> 
> s/shutdowned/was shutdown/
> 
> (repeats)
> 
>> +    if (!con->st)
>> +        goto cleanup;
>> +
>>      if (events & VIR_STREAM_EVENT_READABLE) {
>>          size_t avail = con->streamToTerminal.length -
>>              con->streamToTerminal.offset;
>> @@ -132,7 +150,7 @@ virConsoleEventOnStream(virStreamPtr st,
>>              if (VIR_REALLOC_N(con->streamToTerminal.data,
>>                                con->streamToTerminal.length + 1024) < 0) {
>>                  virConsoleShutdown(con);
>> -                return;
>> +                goto cleanup;
>>              }
>>              con->streamToTerminal.length += 1024;
>>              avail += 1024;
>> @@ -143,10 +161,10 @@ virConsoleEventOnStream(virStreamPtr st,
>>                              con->streamToTerminal.offset,
>>                              avail);
>>          if (got == -2)
>> -            return; /* blocking */
>> +            goto cleanup; /* blocking */
>>          if (got <= 0) {
>>              virConsoleShutdown(con);
>> -            return;
>> +            goto cleanup;
>>          }
>>          con->streamToTerminal.offset += got;
>>          if (con->streamToTerminal.offset)
>> @@ -162,10 +180,10 @@ virConsoleEventOnStream(virStreamPtr st,
>>                               con->terminalToStream.data,
>>                               con->terminalToStream.offset);
>>          if (done == -2)
>> -            return; /* blocking */
>> +            goto cleanup; /* blocking */
>>          if (done < 0) {
>>              virConsoleShutdown(con);
>> -            return;
>> +            goto cleanup;
>>          }
>>          memmove(con->terminalToStream.data,
>>                  con->terminalToStream.data + done,
>> @@ -187,6 +205,9 @@ virConsoleEventOnStream(virStreamPtr st,
>>          events & VIR_STREAM_EVENT_HANGUP) {
>>          virConsoleShutdown(con);
>>      }
>> +
>> + cleanup:
>> +    virObjectUnlock(con);
>>  }
>>  
>>  
>> @@ -198,6 +219,12 @@ virConsoleEventOnStdin(int watch ATTRIBUTE_UNUSED,
>>  {
>>      virConsolePtr con = opaque;
>>  
>> +    virObjectLock(con);
>> +
>> +    /* we got late event after console shutdowned */
>> +    if (!con->st)
>> +        goto cleanup;
>> +
>>      if (events & VIR_EVENT_HANDLE_READABLE) {
>>          size_t avail = con->terminalToStream.length -
>>              con->terminalToStream.offset;
>> @@ -207,7 +234,7 @@ virConsoleEventOnStdin(int watch ATTRIBUTE_UNUSED,
>>              if (VIR_REALLOC_N(con->terminalToStream.data,
>>                                con->terminalToStream.length + 1024) < 0) {
>>                  virConsoleShutdown(con);
>> -                return;
>> +                goto cleanup;
>>              }
>>              con->terminalToStream.length += 1024;
>>              avail += 1024;
>> @@ -220,15 +247,15 @@ virConsoleEventOnStdin(int watch ATTRIBUTE_UNUSED,
>>          if (got < 0) {
>>              if (errno != EAGAIN)
>>                  virConsoleShutdown(con);
>> -            return;
>> +            goto cleanup;
>>          }
>>          if (got == 0) {
>>              virConsoleShutdown(con);
>> -            return;
>> +            goto cleanup;
>>          }
>>          if (con->terminalToStream.data[con->terminalToStream.offset] == con->escapeChar) {
>>              virConsoleShutdown(con);
>> -            return;
>> +            goto cleanup;
>>          }
>>  
>>          con->terminalToStream.offset += got;
>> @@ -242,6 +269,9 @@ virConsoleEventOnStdin(int watch ATTRIBUTE_UNUSED,
>>          events & VIR_EVENT_HANDLE_HANGUP) {
>>          virConsoleShutdown(con);
>>      }
>> +
>> + cleanup:
>> +    virObjectUnlock(con);
>>  }
>>  
>>  
>> @@ -253,6 +283,12 @@ virConsoleEventOnStdout(int watch ATTRIBUTE_UNUSED,
>>  {
>>      virConsolePtr con = opaque;
>>  
>> +    virObjectLock(con);
>> +
>> +    /* we got late event after console shutdowned */
>> +    if (!con->st)
>> +        goto cleanup;
>> +
>>      if (events & VIR_EVENT_HANDLE_WRITABLE &&
>>          con->streamToTerminal.offset) {
>>          ssize_t done;
>> @@ -263,7 +299,7 @@ virConsoleEventOnStdout(int watch ATTRIBUTE_UNUSED,
>>          if (done < 0) {
>>              if (errno != EAGAIN)
>>                  virConsoleShutdown(con);
>> -            return;
>> +            goto cleanup;
>>          }
>>          memmove(con->streamToTerminal.data,
>>                  con->streamToTerminal.data + done,
>> @@ -285,6 +321,38 @@ virConsoleEventOnStdout(int watch ATTRIBUTE_UNUSED,
>>          events & VIR_EVENT_HANDLE_HANGUP) {
>>          virConsoleShutdown(con);
>>      }
>> +
>> + cleanup:
>> +    virObjectUnlock(con);
>> +}
>> +
>> +
>> +static virConsolePtr
>> +virConsoleNew(void)
>> +{
>> +    virConsolePtr con;
>> +
>> +    if (virConsoleInitialize() < 0)
>> +        return NULL;
>> +
>> +    if (!(con = virObjectNew(virConsoleClass)))
>> +        return NULL;
>> +
>> +    if (virCondInit(&con->cond) < 0) {
>> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> +                       _("cannot initialize console condition"));
>> +
>> +        goto error;
>> +    }
>> +
>> +    con->stdinWatch = -1;
>> +    con->stdoutWatch = -1;
>> +
>> +    return con;
>> +
>> + error:
>> +    virObjectUnref(con);
>> +    return NULL;
>>  }
>>  
>>  
>> @@ -324,6 +392,11 @@ virshRunConsole(vshControl *ctl,
>>      if (vshTTYMakeRaw(ctl, true) < 0)
>>          goto resettty;
>>  
>> +    if (!(con = virConsoleNew()))
>> +        goto resettty;
>> +
>> +    virObjectLock(con);
>> +
>>      /* Trap all common signals so that we can safely restore the original
>>       * terminal settings on STDIN before the process exits - people don't like
>>       * being left with a messed up terminal ! */
>> @@ -333,9 +406,6 @@ virshRunConsole(vshControl *ctl,
>>      sigaction(SIGHUP,  &sighandler, &old_sighup);
>>      sigaction(SIGPIPE, &sighandler, &old_sigpipe);
>>  
>> -    if (VIR_ALLOC(con) < 0)
>> -        goto cleanup;
>> -
>>      con->escapeChar = virshGetEscapeChar(priv->escapeChar);
>>      con->st = virStreamNew(virDomainGetConnect(dom),
>>                             VIR_STREAM_NONBLOCK);
>> @@ -345,42 +415,49 @@ virshRunConsole(vshControl *ctl,
>>      if (virDomainOpenConsole(dom, dev_name, con->st, flags) < 0)
>>          goto cleanup;
>>  
>> -    if (virCondInit(&con->cond) < 0 || virMutexInit(&con->lock) < 0)
>> +    virObjectRef(con);
>> +    if ((con->stdinWatch = virEventAddHandle(STDIN_FILENO,
>> +                                             VIR_EVENT_HANDLE_READABLE,
>> +                                             virConsoleEventOnStdin,
>> +                                             con,
>> +                                             virObjectFreeCallback)) < 0) {
>> +        virObjectUnref(con);
>>          goto cleanup;
>> +    }
> 
> Is there a specific reason to go with:
> 
>    ObjectRef
>    if condition
>       error path
>       ObjectUnref> 
> instead of
> 
>    if condition
>    ObjectRef

We can use second construction safely here because we hold console lock and
virConsoleEventOnStdin grabs the lock too. If not we could have error
event/console shutdown/removing handle/calling free callback before we
have chance to reference console on behalf of the watch and this would
lead to use after free in virshRunConsole.

So first construction is a bit more robust. You just first refcount object
and then pass it further without extra consideration.

> 
> I guess it doesn't matter too much, but looks odd to me... If the
> virEventAddHandle fails, the virObjectFreeCallback isn't called so @con
> wouldn't be Unref'd until at least you Unlock'd @con.

That's why we unref console explicitly on virEventAddHandle failure -
because callback does not get called.

> 
>>  
>> -    virMutexLock(&con->lock);
>> -
>> -    con->stdinWatch = virEventAddHandle(STDIN_FILENO,
>> -                                        VIR_EVENT_HANDLE_READABLE,
>> -                                        virConsoleEventOnStdin,
>> -                                        con,
>> -                                        NULL);
>> -    con->stdoutWatch = virEventAddHandle(STDOUT_FILENO,
>> -                                         0,
>> -                                         virConsoleEventOnStdout,
>> -                                         con,
>> -                                         NULL);
>> +    virObjectRef(con);
>> +    if ((con->stdoutWatch = virEventAddHandle(STDOUT_FILENO,
>> +                                              0,
>> +                                              virConsoleEventOnStdout,
>> +                                              con,
>> +                                              virObjectFreeCallback)) < 0) {
>> +        virObjectUnref(con);
>> +        goto cleanup;
>> +    }
>>  
>> -    virStreamEventAddCallback(con->st,
>> -                              VIR_STREAM_EVENT_READABLE,
>> -                              virConsoleEventOnStream,
>> -                              con,
>> -                              NULL);
>> +    virObjectRef(con);
>> +    if (virStreamEventAddCallback(con->st,
>> +                                  VIR_STREAM_EVENT_READABLE,
>> +                                  virConsoleEventOnStream,
>> +                                  con,
>> +                                  virObjectFreeCallback) < 0) {
>> +        virObjectUnref(con);
>> +        goto cleanup;
>> +    }
>>  
>>      while (!con->quit) {
>> -        if (virCondWait(&con->cond, &con->lock) < 0) {
>> -            virMutexUnlock(&con->lock);
>> +        if (virCondWait(&con->cond, &con->parent.lock) < 0) {
>>              VIR_ERROR(_("unable to wait on console condition"));
>>              goto cleanup;
>>          }
>>      }
>>  
>> -    virMutexUnlock(&con->lock);
>> -
>>      ret = 0;
>>  
>>   cleanup:
>> -    virConsoleFree(con);
>> +    virConsoleShutdown(con);
> 
> Should virConsoleShutdown be adjusted to avoid virCondSignal if
> con->quit is tree or this call be adjusted to not call if con->quit is
> true... IOW: Don't call it twice...
> 

I'd better use first variant - this way we make virConsoleShutdown idempotent.

Nikolay

> 
>> +    virObjectUnlock(con);
>> +    virObjectUnref(con);
>>  
>>      /* Restore original signal handlers */
>>      sigaction(SIGQUIT, &old_sigquit, NULL);
>>

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



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux