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@virtuozzo.com>
---
tools/virsh-console.c | 161 +++++++++++++++++++++++++++++++++++++-------------
1 file changed, 119 insertions(+), 42 deletions(-)
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 */
+ 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;
+ }
- 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);
+ virObjectUnlock(con);
+ virObjectUnref(con);
/* Restore original signal handlers */
sigaction(SIGQUIT, &old_sigquit, NULL);
--
1.8.3.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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
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.
>
> - 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...
John
> + virObjectUnlock(con);
> + virObjectUnref(con);
>
> /* Restore original signal handlers */
> sigaction(SIGQUIT, &old_sigquit, NULL);
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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);
>>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2026 Red Hat, Inc.