4:08 a.m.
Diff from v2[1]
- split first patch further into 2
[1] https://www.redhat.com/archives/libvir-list/2019-March/msg00812.html
Nikolay Shirokovskiy (5):
tools: console: make console virLockableObject
tools: console: cleanup console on errors in main thread
tools: console: add missing locks in callbacks
tools: console: check if console was shutdown in callbacks
tools: console: pass stream/fd errors to user
tools/virsh-console.c | 224 ++++++++++++++++++++++++++++++++++++++------------
1 file changed, 171 insertions(+), 53 deletions(-)
--
1.8.3.1
4:08 a.m.
New subject: [libvirt] [PATCH v3 1/5] tools: console: make console virLockableObject
We need to turn console into virObject object because stream/fd callbacks
can be called from the event loop thread after freeing console
in main thread. It is convinient to turn into virLockableObject as
we have mutex in console object.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
---
tools/virsh-console.c | 74 ++++++++++++++++++++++++++++++++++++++-------------
1 file changed, 55 insertions(+), 19 deletions(-)
diff --git a/tools/virsh-console.c b/tools/virsh-console.c
index 045a636..763b395 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);
}
@@ -288,6 +300,35 @@ virConsoleEventOnStdout(int watch ATTRIBUTE_UNUSED,
}
+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;
+}
+
+
static char
virshGetEscapeChar(const char *s)
{
@@ -324,6 +365,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 +379,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,11 +388,6 @@ virshRunConsole(vshControl *ctl,
if (virDomainOpenConsole(dom, dev_name, con->st, flags) < 0)
goto cleanup;
- if (virCondInit(&con->cond) < 0 || virMutexInit(&con->lock) < 0)
- goto cleanup;
-
- virMutexLock(&con->lock);
-
con->stdinWatch = virEventAddHandle(STDIN_FILENO,
VIR_EVENT_HANDLE_READABLE,
virConsoleEventOnStdin,
@@ -368,19 +406,17 @@ virshRunConsole(vshControl *ctl,
NULL);
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);
+ virObjectUnlock(con);
+ virObjectUnref(con);
/* Restore original signal handlers */
sigaction(SIGQUIT, &old_sigquit, NULL);
--
1.8.3.1
2:49 p.m.
New subject: [libvirt] [PATCH v3 1/5] tools: console: make console virLockableObject
On 3/18/19 5:08 AM, Nikolay Shirokovskiy wrote:
We need to turn console into virObject object because stream/fd
callbacks
can be called from the event loop thread after freeing console
in main thread. It is convinient to turn into virLockableObject as
we have mutex in console object.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
---
tools/virsh-console.c | 74 ++++++++++++++++++++++++++++++++++++++-------------
1 file changed, 55 insertions(+), 19 deletions(-)
Reviewed-by: Cole Robinson <crobinso(a)redhat.com>
- Cole
4:08 a.m.
New subject: [libvirt] [PATCH v3 2/5] tools: console: cleanup console on errors in main thread
We only check now for virObjectWait failures in virshRunConsole but
we'd better check and for other failures too. And we need to shutdown
console on error in the main thread.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
---
tools/virsh-console.c | 52 +++++++++++++++++++++++++++++++++------------------
1 file changed, 34 insertions(+), 18 deletions(-)
diff --git a/tools/virsh-console.c b/tools/virsh-console.c
index 763b395..9289221 100644
--- a/tools/virsh-console.c
+++ b/tools/virsh-console.c
@@ -112,8 +112,10 @@ virConsoleShutdown(virConsolePtr con)
virEventRemoveHandle(con->stdoutWatch);
con->stdinWatch = -1;
con->stdoutWatch = -1;
- con->quit = true;
- virCondSignal(&con->cond);
+ if (!con->quit) {
+ con->quit = true;
+ virCondSignal(&con->cond);
+ }
}
@@ -388,22 +390,35 @@ virshRunConsole(vshControl *ctl,
if (virDomainOpenConsole(dom, dev_name, con->st, flags) < 0)
goto cleanup;
- con->stdinWatch = virEventAddHandle(STDIN_FILENO,
- VIR_EVENT_HANDLE_READABLE,
- virConsoleEventOnStdin,
- con,
- NULL);
- con->stdoutWatch = virEventAddHandle(STDOUT_FILENO,
- 0,
- virConsoleEventOnStdout,
- con,
- NULL);
-
- virStreamEventAddCallback(con->st,
- VIR_STREAM_EVENT_READABLE,
- virConsoleEventOnStream,
- con,
- NULL);
+ virObjectRef(con);
+ if ((con->stdinWatch = virEventAddHandle(STDIN_FILENO,
+ VIR_EVENT_HANDLE_READABLE,
+ virConsoleEventOnStdin,
+ con,
+ virObjectFreeCallback)) < 0) {
+ virObjectUnref(con);
+ goto cleanup;
+ }
+
+ virObjectRef(con);
+ if ((con->stdoutWatch = virEventAddHandle(STDOUT_FILENO,
+ 0,
+ virConsoleEventOnStdout,
+ con,
+ virObjectFreeCallback)) < 0) {
+ virObjectUnref(con);
+ goto cleanup;
+ }
+
+ 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->parent.lock) < 0) {
@@ -415,6 +430,7 @@ virshRunConsole(vshControl *ctl,
ret = 0;
cleanup:
+ virConsoleShutdown(con);
virObjectUnlock(con);
virObjectUnref(con);
--
1.8.3.1
3:38 p.m.
New subject: [libvirt] [PATCH v3 2/5] tools: console: cleanup console on errors in main thread
On 3/18/19 5:08 AM, Nikolay Shirokovskiy wrote:
We only check now for virObjectWait failures in virshRunConsole but
we'd better check and for other failures too. And we need to shutdown
console on error in the main thread.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
---
tools/virsh-console.c | 52 +++++++++++++++++++++++++++++++++------------------
1 file changed, 34 insertions(+), 18 deletions(-)
diff --git a/tools/virsh-console.c b/tools/virsh-console.c
index 763b395..9289221 100644
--- a/tools/virsh-console.c
+++ b/tools/virsh-console.c
@@ -112,8 +112,10 @@ virConsoleShutdown(virConsolePtr con)
virEventRemoveHandle(con->stdoutWatch);
con->stdinWatch = -1;
con->stdoutWatch = -1;
- con->quit = true;
- virCondSignal(&con->cond);
+ if (!con->quit) {
+ con->quit = true;
+ virCondSignal(&con->cond);
+ }
}
@@ -388,22 +390,35 @@ virshRunConsole(vshControl *ctl,
if (virDomainOpenConsole(dom, dev_name, con->st, flags) < 0)
goto cleanup;
- con->stdinWatch = virEventAddHandle(STDIN_FILENO,
- VIR_EVENT_HANDLE_READABLE,
- virConsoleEventOnStdin,
- con,
- NULL);
- con->stdoutWatch = virEventAddHandle(STDOUT_FILENO,
- 0,
- virConsoleEventOnStdout,
- con,
- NULL);
-
- virStreamEventAddCallback(con->st,
- VIR_STREAM_EVENT_READABLE,
- virConsoleEventOnStream,
- con,
- NULL);
+ virObjectRef(con);
+ if ((con->stdinWatch = virEventAddHandle(STDIN_FILENO,
+ VIR_EVENT_HANDLE_READABLE,
+ virConsoleEventOnStdin,
+ con,
+ virObjectFreeCallback)) < 0) {
+ virObjectUnref(con);
+ goto cleanup;
+ }
+
+ virObjectRef(con);
+ if ((con->stdoutWatch = virEventAddHandle(STDOUT_FILENO,
+ 0,
+ virConsoleEventOnStdout,
+ con,
+ virObjectFreeCallback)) < 0) {
+ virObjectUnref(con);
+ goto cleanup;
+ }
+
+ virObjectRef(con);
+ if (virStreamEventAddCallback(con->st,
+ VIR_STREAM_EVENT_READABLE,
+ virConsoleEventOnStream,
+ con,
+ virObjectFreeCallback) < 0) {
+ virObjectUnref(con);
+ goto cleanup;
+ }
I didn't understand this pattern at first, but I see it's used by the
qemu agent and monitor callbacks and it makes sense after refreshing my
event loop memory: we add a reference because the loop callback carries
around 'con' as an opaque data value, which is unref'd when the event
handle is removed.
while (!con->quit) {
if (virCondWait(&con->cond, &con->parent.lock) < 0) {
@@ -415,6 +430,7 @@ virshRunConsole(vshControl *ctl,
ret = 0;
cleanup:
+ virConsoleShutdown(con);
virObjectUnlock(con);
virObjectUnref(con);
And we need this here to actually trigger the handle unregistering,
incase one of the Add* calls fails. So makes sense to me
Reviewed-by: Cole Robinson <crobinso(a)redhat.com>
- Cole
4:08 a.m.
New subject: [libvirt] [PATCH v3 3/5] tools: console: add missing locks in callbacks
Stream/fd callbacks accessing console object are called from the
event loop thread and the console object is also accessed from
the main thread so we are better add locking to handlers.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
---
tools/virsh-console.c | 35 +++++++++++++++++++++++++----------
1 file changed, 25 insertions(+), 10 deletions(-)
diff --git a/tools/virsh-console.c b/tools/virsh-console.c
index 9289221..3dae707 100644
--- a/tools/virsh-console.c
+++ b/tools/virsh-console.c
@@ -137,6 +137,8 @@ virConsoleEventOnStream(virStreamPtr st,
{
virConsolePtr con = opaque;
+ virObjectLock(con);
+
if (events & VIR_STREAM_EVENT_READABLE) {
size_t avail = con->streamToTerminal.length -
con->streamToTerminal.offset;
@@ -146,7 +148,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;
@@ -157,10 +159,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)
@@ -176,10 +178,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,
@@ -201,6 +203,9 @@ virConsoleEventOnStream(virStreamPtr st,
events & VIR_STREAM_EVENT_HANGUP) {
virConsoleShutdown(con);
}
+
+ cleanup:
+ virObjectUnlock(con);
}
@@ -212,6 +217,8 @@ virConsoleEventOnStdin(int watch ATTRIBUTE_UNUSED,
{
virConsolePtr con = opaque;
+ virObjectLock(con);
+
if (events & VIR_EVENT_HANDLE_READABLE) {
size_t avail = con->terminalToStream.length -
con->terminalToStream.offset;
@@ -221,7 +228,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;
@@ -234,15 +241,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;
@@ -256,6 +263,9 @@ virConsoleEventOnStdin(int watch ATTRIBUTE_UNUSED,
events & VIR_EVENT_HANDLE_HANGUP) {
virConsoleShutdown(con);
}
+
+ cleanup:
+ virObjectUnlock(con);
}
@@ -267,6 +277,8 @@ virConsoleEventOnStdout(int watch ATTRIBUTE_UNUSED,
{
virConsolePtr con = opaque;
+ virObjectLock(con);
+
if (events & VIR_EVENT_HANDLE_WRITABLE &&
con->streamToTerminal.offset) {
ssize_t done;
@@ -277,7 +289,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,
@@ -299,6 +311,9 @@ virConsoleEventOnStdout(int watch ATTRIBUTE_UNUSED,
events & VIR_EVENT_HANDLE_HANGUP) {
virConsoleShutdown(con);
}
+
+ cleanup:
+ virObjectUnlock(con);
}
--
1.8.3.1
3:44 p.m.
New subject: [libvirt] [PATCH v3 3/5] tools: console: add missing locks in callbacks
On 3/18/19 5:08 AM, Nikolay Shirokovskiy wrote:
Stream/fd callbacks accessing console object are called from the
event loop thread and the console object is also accessed from
the main thread so we are better add locking to handlers.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
---
tools/virsh-console.c | 35 +++++++++++++++++++++++++----------
1 file changed, 25 insertions(+), 10 deletions(-)
The qemuAgentIO and qemuMonitorIO callbacks also add have matching
Ref/Unref calls along side the lock calls, but after scratching my head
over it for a while I don't think it's necessary here: even if
virConsoleShutdown is called from one of the callbacks, the main thread
will still hold a reference until the callback releases the object lock
Reviewed-by: Cole Robinson <crobinso(a)redhat.com>
- Cole
1:57 a.m.
New subject: [libvirt] [PATCH v3 3/5] tools: console: add missing locks in callbacks
On 03.04.2019 23:44, Cole Robinson wrote:
On 3/18/19 5:08 AM, Nikolay Shirokovskiy wrote:
> Stream/fd callbacks accessing console object are called from the
> event loop thread and the console object is also accessed from
> the main thread so we are better add locking to handlers.
>
> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
> ---
> tools/virsh-console.c | 35 +++++++++++++++++++++++++----------
> 1 file changed, 25 insertions(+), 10 deletions(-)
>
The qemuAgentIO and qemuMonitorIO callbacks also add have matching
Ref/Unref calls along side the lock calls, but after scratching my head
over it for a while I don't think it's necessary here: even if
virConsoleShutdown is called from one of the callbacks, the main thread
will still hold a reference until the callback releases the object lock
We don't need even to rely on main thread. Event loop itself has a reference
which will be unrefed only after callback returns.
Nikolay
9:48 a.m.
New subject: [libvirt] [PATCH v3 3/5] tools: console: add missing locks in callbacks
On 4/4/19 2:57 AM, Nikolay Shirokovskiy wrote:
On 03.04.2019 23:44, Cole Robinson wrote:
> On 3/18/19 5:08 AM, Nikolay Shirokovskiy wrote:
>> Stream/fd callbacks accessing console object are called from the
>> event loop thread and the console object is also accessed from
>> the main thread so we are better add locking to handlers.
>>
>> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
>> ---
>> tools/virsh-console.c | 35 +++++++++++++++++++++++++----------
>> 1 file changed, 25 insertions(+), 10 deletions(-)
>>
>
> The qemuAgentIO and qemuMonitorIO callbacks also add have matching
> Ref/Unref calls along side the lock calls, but after scratching my head
> over it for a while I don't think it's necessary here: even if
> virConsoleShutdown is called from one of the callbacks, the main thread
> will still hold a reference until the callback releases the object lock
>
We don't need even to rely on main thread. Event loop itself has a reference
which will be unrefed only after callback returns.
Hmm interesting, where in the code is that exactly? I know we pass a
Free callback to virEventAddHandle but that's only called when the
handle is removed, not after the callback is invoked. Maybe I'm missing
something
Thanks,
Cole
1:29 a.m.
New subject: [libvirt] [PATCH v3 3/5] tools: console: add missing locks in callbacks
On 04.04.2019 17:48, Cole Robinson wrote:
On 4/4/19 2:57 AM, Nikolay Shirokovskiy wrote:
>
>
> On 03.04.2019 23:44, Cole Robinson wrote:
>> On 3/18/19 5:08 AM, Nikolay Shirokovskiy wrote:
>>> Stream/fd callbacks accessing console object are called from the
>>> event loop thread and the console object is also accessed from
>>> the main thread so we are better add locking to handlers.
>>>
>>> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
>>> ---
>>> tools/virsh-console.c | 35 +++++++++++++++++++++++++----------
>>> 1 file changed, 25 insertions(+), 10 deletions(-)
>>>
>>
>> The qemuAgentIO and qemuMonitorIO callbacks also add have matching
>> Ref/Unref calls along side the lock calls, but after scratching my head
>> over it for a while I don't think it's necessary here: even if
>> virConsoleShutdown is called from one of the callbacks, the main thread
>> will still hold a reference until the callback releases the object lock
>>
>
> We don't need even to rely on main thread. Event loop itself has a reference
> which will be unrefed only after callback returns.
>
Hmm interesting, where in the code is that exactly? I know we pass a
Free callback to virEventAddHandle but that's only called when the
handle is removed, not after the callback is invoked. Maybe I'm missing
something
The reason we are ok is that when we call virEventPollRemoveHandle it only
marks handle to be removed so that if this call is done from callback we
don't free opaque object immediately. Only after callback is finished
we call virEventPollCleanupHandles which actually do freeing.
Nikolay
4:08 a.m.
New subject: [libvirt] [PATCH v3 4/5] tools: console: check if console was shutdown in callbacks
On error in main thread virConsoleShutdown is called which
deletes fd watches/stream callback and yet callbacks can
be called after. Thus we can incorrectly allocate
terminalToStream.data memory and get memory leak for example.
Let's check if console was shutdown in the very beginning of
callbacks.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
---
tools/virsh-console.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/tools/virsh-console.c b/tools/virsh-console.c
index 3dae707..d109734 100644
--- a/tools/virsh-console.c
+++ b/tools/virsh-console.c
@@ -139,6 +139,10 @@ virConsoleEventOnStream(virStreamPtr st,
virObjectLock(con);
+ /* we got late event after console was shutdown */
+ if (!con->st)
+ goto cleanup;
+
if (events & VIR_STREAM_EVENT_READABLE) {
size_t avail = con->streamToTerminal.length -
con->streamToTerminal.offset;
@@ -219,6 +223,10 @@ virConsoleEventOnStdin(int watch ATTRIBUTE_UNUSED,
virObjectLock(con);
+ /* we got late event after console was shutdown */
+ if (!con->st)
+ goto cleanup;
+
if (events & VIR_EVENT_HANDLE_READABLE) {
size_t avail = con->terminalToStream.length -
con->terminalToStream.offset;
@@ -279,6 +287,10 @@ virConsoleEventOnStdout(int watch ATTRIBUTE_UNUSED,
virObjectLock(con);
+ /* we got late event after console was shutdown */
+ if (!con->st)
+ goto cleanup;
+
if (events & VIR_EVENT_HANDLE_WRITABLE &&
con->streamToTerminal.offset) {
ssize_t done;
--
1.8.3.1
3:49 p.m.
New subject: [libvirt] [PATCH v3 4/5] tools: console: check if console was shutdown in callbacks
On 3/18/19 5:08 AM, Nikolay Shirokovskiy wrote:
On error in main thread virConsoleShutdown is called which
deletes fd watches/stream callback and yet callbacks can
be called after. Thus we can incorrectly allocate
terminalToStream.data memory and get memory leak for example.
Let's check if console was shutdown in the very beginning of
callbacks.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
---
tools/virsh-console.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/tools/virsh-console.c b/tools/virsh-console.c
index 3dae707..d109734 100644
--- a/tools/virsh-console.c
+++ b/tools/virsh-console.c
@@ -139,6 +139,10 @@ virConsoleEventOnStream(virStreamPtr st,
virObjectLock(con);
+ /* we got late event after console was shutdown */
+ if (!con->st)
+ goto cleanup;
+
if (events & VIR_STREAM_EVENT_READABLE) {
size_t avail = con->streamToTerminal.length -
con->streamToTerminal.offset;
@@ -219,6 +223,10 @@ virConsoleEventOnStdin(int watch ATTRIBUTE_UNUSED,
virObjectLock(con);
+ /* we got late event after console was shutdown */
+ if (!con->st)
+ goto cleanup;
+
if (events & VIR_EVENT_HANDLE_READABLE) {
size_t avail = con->terminalToStream.length -
con->terminalToStream.offset;
@@ -279,6 +287,10 @@ virConsoleEventOnStdout(int watch ATTRIBUTE_UNUSED,
virObjectLock(con);
+ /* we got late event after console was shutdown */
+ if (!con->st)
+ goto cleanup;
+
if (events & VIR_EVENT_HANDLE_WRITABLE &&
con->streamToTerminal.offset) {
ssize_t done;
I didn't verify exactly how this case can happen but it seems sane, so:
Reviewed-by: Cole Robinson <crobinso(a)redhat.com>
- Cole
4:08 a.m.
New subject: [libvirt] [PATCH v3 5/5] tools: console: pass stream/fd errors to user
If the console was disconnected due to a connection problem or a problem on the
server side it is convinient to provide the cause to the user. If the error
come from the API then the error is saved in a virsh global variable. However,
since success is returned from virshRunConsole after we reach the waiting 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. Thus let's save root cause immediately and then set it to
virsh error after all cleanup is done.
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.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
---
tools/virsh-console.c | 55 +++++++++++++++++++++++++++++++++++++++++++--------
1 file changed, 47 insertions(+), 8 deletions(-)
diff --git a/tools/virsh-console.c b/tools/virsh-console.c
index d109734..236933a 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);
@@ -128,6 +134,7 @@ virConsoleDispose(void *obj)
virStreamFree(con->st);
virCondDestroy(&con->cond);
+ virResetError(&con->error);
}
@@ -165,6 +172,10 @@ virConsoleEventOnStream(virStreamPtr st,
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;
}
@@ -247,11 +258,14 @@ virConsoleEventOnStdin(int watch ATTRIBUTE_UNUSED,
con->terminalToStream.offset,
avail);
if (got < 0) {
- if (errno != EAGAIN)
+ if (errno != EAGAIN) {
+ virReportSystemError(errno, "%s", _("cannot read from
stdin"));
virConsoleShutdown(con);
+ }
goto cleanup;
}
if (got == 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("EOF on
stdin"));
virConsoleShutdown(con);
goto cleanup;
}
@@ -267,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:
@@ -299,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", _("cannot write to
stdout"));
virConsoleShutdown(con);
+ }
goto cleanup;
}
memmove(con->streamToTerminal.data,
@@ -319,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:
@@ -449,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);
--
1.8.3.1
3:59 p.m.
New subject: [libvirt] [PATCH v3 5/5] tools: console: pass stream/fd errors to user
On 3/18/19 5:08 AM, Nikolay Shirokovskiy wrote:
If the console was disconnected due to a connection problem or a
problem on the
server side it is convinient to provide the cause to the user. If the error
come from the API then the error is saved in a virsh global variable. However,
since success is returned from virshRunConsole after we reach the waiting 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. Thus let's save root cause immediately and then set it to
virsh error after all cleanup is done.
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.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
---
tools/virsh-console.c | 55 +++++++++++++++++++++++++++++++++++++++++++--------
1 file changed, 47 insertions(+), 8 deletions(-)
diff --git a/tools/virsh-console.c b/tools/virsh-console.c
index d109734..236933a 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);
+
virGetLastError() returns NULL if err->code == VIR_ERR_OK, so that last
check can be removed. The rest looks good to me
Reviewed-by: Cole Robinson <crobinso(a)redhat.com>
- Cole
2057
days inactive
2075
days old
13 comments
2 participants
participants (2)
-
Cole Robinson -
Nikolay Shirokovskiy