6:52 a.m.
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(a)virtuozzo.com>
> ---
> 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);
>