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@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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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@redhat.com> - Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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@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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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@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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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@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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2026 Red Hat, Inc.