[libvirt] [PATCH v2 0/6] Console coruption with two or more clients series
This series fixes anoying console corruption if two clients try to connect at same time to the console. The current state of this is, that two/more of threads compete for the data from the PTY. This causes that each of the consoles get scrambled and unusable. These patches add mutual exclusion for opening consoles with two different approaches and a option to terminate existing console streams. A sample implementation is done using qemu driver, but i'll add more of them if this will be OK. (They're basicaly the same as in qemu). For convinience, to review these patches: git checkout -b console_corruption 8d16201fe0e63afb5416a8eb7c6478f582ccccc0 git pull git://aeon.pipo.sk/libvirt.git console_dup (The machine should be up most of time) Peter Peter Krempa (6): Add flags for virDomainOpenConsole virsh: add support for VIR_DOMAIN_CONSOLE_FORCE flag fdstream: Emit stream abort callback even if poll() doesnt. fdstream: Add internal callback on stream close util: Add helpers for safe domain console operations qemu: Add ability to abort existing console while creating new one configure.ac | 37 +++- include/libvirt/libvirt.h.in | 12 +- src/Makefile.am | 5 +- src/fdstream.c | 95 +++++++++- src/fdstream.h | 11 + src/libvirt_private.syms | 6 + src/qemu/qemu_domain.c | 5 + src/qemu/qemu_domain.h | 3 + src/qemu/qemu_driver.c | 21 ++- src/util/domain_safe_console.c | 399 ++++++++++++++++++++++++++++++++++++++++ src/util/domain_safe_console.h | 28 +++ tools/console.c | 5 +- tools/console.h | 3 +- tools/virsh.c | 18 ++- 14 files changed, 614 insertions(+), 34 deletions(-) create mode 100644 src/util/domain_safe_console.c create mode 100644 src/util/domain_safe_console.h -- 1.7.3.4
This patch adds a set of flags to be used with the virDomainOpenConsole API call to specify if the user wishes to interrupt an existing console session or just to try open a new one. VIR_DOMAIN_CONSOLE_TRY - specifies that the caller wants to try open a new console session if no open sessions exist VIR_DOMAIN_CONSOLE_FORCE - specifies that the caller wishes to interrupt existing session and force a creation of a new one. --- include/libvirt/libvirt.h.in | 12 +++++++++++- 1 files changed, 11 insertions(+), 1 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 2480add..268326b 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -3346,7 +3346,17 @@ int virNWFilterGetUUIDString (virNWFilterPtr nwfilter, char *buf); char * virNWFilterGetXMLDesc (virNWFilterPtr nwfilter, unsigned int flags); - +/** + * virDomainConsoleFlags + * + * Since 0.9.7 + */ +typedef enum { + VIR_DOMAIN_CONSOLE_TRY = 0, /* try to open the console, don't abort active + connection */ + VIR_DOMAIN_CONSOLE_FORCE /* abort a (possibly) active console connection + to force a new connection*/ +} virDomainConsoleFlags; int virDomainOpenConsole(virDomainPtr dom, const char *devname, -- 1.7.3.4
On 12/07/2011 11:08 AM, Peter Krempa wrote:
This patch adds a set of flags to be used with the virDomainOpenConsole API call to specify if the user wishes to interrupt an existing console session or just to try open a new one.
VIR_DOMAIN_CONSOLE_TRY - specifies that the caller wants to try open a new console session if no open sessions exist
VIR_DOMAIN_CONSOLE_FORCE - specifies that the caller wishes to interrupt existing session and force a creation of a new one. --- include/libvirt/libvirt.h.in | 12 +++++++++++- 1 files changed, 11 insertions(+), 1 deletions(-)
diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 2480add..268326b 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -3346,7 +3346,17 @@ int virNWFilterGetUUIDString (virNWFilterPtr nwfilter, char *buf); char * virNWFilterGetXMLDesc (virNWFilterPtr nwfilter, unsigned int flags); - +/** + * virDomainConsoleFlags + * + * Since 0.9.7
0.9.9 (hmm, it would be really nice if we could augment http://libvirt.org/hvsupport.html to also list when a hypervisor started supporting a specific flag, rather than just the generic API).
+ */ +typedef enum { + VIR_DOMAIN_CONSOLE_TRY = 0, /* try to open the console, don't abort active + connection */
Since the default is 0, do we really need to name this flag? Then again, I guess it doesn't hurt.
+ VIR_DOMAIN_CONSOLE_FORCE /* abort a (possibly) active console connection + to force a new connection*/
I'd list this as an explicit (1 << 0), to make it obvious that we might add new bit-wise distinct flags in the future.
+} virDomainConsoleFlags;
int virDomainOpenConsole(virDomainPtr dom, const char *devname,
Missing documentation of these flags in src/libvirt.c (I'm trying to earn a reputation for being a stickler about no new features without documentation, and this is a new feature). Maybe something like: diff --git i/include/libvirt/libvirt.h.in w/include/libvirt/libvirt.h.in index 268326b..845261b 100644 --- i/include/libvirt/libvirt.h.in +++ w/include/libvirt/libvirt.h.in @@ -3349,13 +3349,11 @@ char * virNWFilterGetXMLDesc (virNWFilterPtr nwfilter, /** * virDomainConsoleFlags * - * Since 0.9.7 + * Since 0.9.9 */ typedef enum { - VIR_DOMAIN_CONSOLE_TRY = 0, /* try to open the console, don't abort active - connection */ - VIR_DOMAIN_CONSOLE_FORCE /* abort a (possibly) active console connection - to force a new connection*/ + VIR_DOMAIN_CONSOLE_FORCE = (1 << 0) /* abort a (possibly) active console, + forcing a new connection*/ } virDomainConsoleFlags; int virDomainOpenConsole(virDomainPtr dom, diff --git i/src/libvirt.c w/src/libvirt.c index 9977915..af48e13 100644 --- i/src/libvirt.c +++ w/src/libvirt.c @@ -16962,7 +16962,7 @@ virDomainSnapshotFree(virDomainSnapshotPtr snapshot) * @dom: a domain object * @dev_name: the console, serial or parallel port device alias, or NULL * @st: a stream to associate with the console - * @flags: unused, pass 0 + * @flags: bit-wise OR of virDomainConsoleFlags * * This opens the backend associated with a console, serial or * parallel port device on a guest, if the backend is supported. @@ -16971,6 +16971,11 @@ virDomainSnapshotFree(virDomainSnapshotPtr snapshot) * in @st stream, which should have been opened in non-blocking * mode for bi-directional I/O. * + * By default, when @flags is 0, the open will fail if libvirt + * detects that the console is already in use by another client; + * passing VIR_DOMAIN_CONSOLE_FORCE will cause libvirt to forcefully + * remove the other client prior to opening this console. + * * returns 0 if the console was opened, -1 on error */ int virDomainOpenConsole(virDomainPtr dom, -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Tue, Dec 13, 2011 at 05:03:05PM -0700, Eric Blake wrote:
On 12/07/2011 11:08 AM, Peter Krempa wrote:
This patch adds a set of flags to be used with the virDomainOpenConsole API call to specify if the user wishes to interrupt an existing console session or just to try open a new one.
VIR_DOMAIN_CONSOLE_TRY - specifies that the caller wants to try open a new console session if no open sessions exist
VIR_DOMAIN_CONSOLE_FORCE - specifies that the caller wishes to interrupt existing session and force a creation of a new one. --- include/libvirt/libvirt.h.in | 12 +++++++++++- 1 files changed, 11 insertions(+), 1 deletions(-)
diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 2480add..268326b 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -3346,7 +3346,17 @@ int virNWFilterGetUUIDString (virNWFilterPtr nwfilter, char *buf); char * virNWFilterGetXMLDesc (virNWFilterPtr nwfilter, unsigned int flags); - +/** + * virDomainConsoleFlags + * + * Since 0.9.7
0.9.9
(hmm, it would be really nice if we could augment http://libvirt.org/hvsupport.html to also list when a hypervisor started supporting a specific flag, rather than just the generic API).
+ */ +typedef enum { + VIR_DOMAIN_CONSOLE_TRY = 0, /* try to open the console, don't abort active + connection */
Since the default is 0, do we really need to name this flag? Then again, I guess it doesn't hurt.
In some ways it does actally hurt, because it can be confusing that 'flags & VIR_DOMAIN_CONSOLE_TRY' evaluates to false, so I would leave it out. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
This patch adds support for the newly introduced VIR_DOMAIN_CONSOLE flag. The console command now has an optional parameter --force that specifies that the user wants to forcibly interrupt an ongoing console session and create a new one. The behaviour to this point was, that the daemon openend two streams to the console, that competed for data from the pipe and the result was, that both of the consoles ended up scrambled. * tools/console.c: - add support for flag passthrough * tools/console.h: - modify function prototypes to match impl. * tools/virsh.c: - add flag --force for the console command --- tools/console.c | 5 +++-- tools/console.h | 3 ++- tools/virsh.c | 18 +++++++++++++----- 3 files changed, 18 insertions(+), 8 deletions(-) diff --git a/tools/console.c b/tools/console.c index e6118a0..475fd7b 100644 --- a/tools/console.c +++ b/tools/console.c @@ -297,7 +297,8 @@ vshGetEscapeChar(const char *s) int vshRunConsole(virDomainPtr dom, const char *dev_name, - const char *escape_seq) + const char *escape_seq, + unsigned int flags) { int ret = -1; struct termios ttyattr, rawattr; @@ -351,7 +352,7 @@ int vshRunConsole(virDomainPtr dom, if (!con->st) goto cleanup; - if (virDomainOpenConsole(dom, dev_name, con->st, 0) < 0) + if (virDomainOpenConsole(dom, dev_name, con->st, flags) < 0) goto cleanup; if (virCondInit(&con->cond) < 0 || virMutexInit(&con->lock) < 0) diff --git a/tools/console.h b/tools/console.h index 8cca08f..2b5440c 100644 --- a/tools/console.h +++ b/tools/console.h @@ -27,7 +27,8 @@ int vshRunConsole(virDomainPtr dom, const char *dev_name, - const char *escape_seq); + const char *escape_seq, + unsigned int flags); # endif /* !WIN32 */ diff --git a/tools/virsh.c b/tools/virsh.c index d02be5c..985bed0 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -787,11 +787,14 @@ static const vshCmdInfo info_console[] = { static const vshCmdOptDef opts_console[] = { {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, {"devname", VSH_OT_STRING, 0, N_("character device name")}, + {"force", VSH_OT_BOOL, 0, N_("force console connection (disconnect already connected sessions)")}, {NULL, 0, 0, NULL} }; static bool -cmdRunConsole(vshControl *ctl, virDomainPtr dom, const char *name) +cmdRunConsole(vshControl *ctl, virDomainPtr dom, + const char *name, + unsigned int flags) { bool ret = false; int state; @@ -808,7 +811,7 @@ cmdRunConsole(vshControl *ctl, virDomainPtr dom, const char *name) vshPrintExtra(ctl, _("Connected to domain %s\n"), virDomainGetName(dom)); vshPrintExtra(ctl, _("Escape character is %s\n"), ctl->escapeChar); - if (vshRunConsole(dom, name, ctl->escapeChar) == 0) + if (vshRunConsole(dom, name, ctl->escapeChar, flags) == 0) ret = true; cleanup: @@ -821,6 +824,8 @@ cmdConsole(vshControl *ctl, const vshCmd *cmd) { virDomainPtr dom; bool ret = false; + bool force = vshCommandOptBool(cmd, "force"); + unsigned int flags = 0; const char *name = NULL; if (!vshConnectionUsability(ctl, ctl->conn)) @@ -834,7 +839,10 @@ cmdConsole(vshControl *ctl, const vshCmd *cmd) goto cleanup; } - ret = cmdRunConsole(ctl, dom, name); + if (force) + flags = VIR_DOMAIN_CONSOLE_FORCE; + + ret = cmdRunConsole(ctl, dom, name, flags); cleanup: virDomainFree(dom); @@ -1851,7 +1859,7 @@ cmdCreate(vshControl *ctl, const vshCmd *cmd) virDomainGetName(dom), from); #ifndef WIN32 if (console) - cmdRunConsole(ctl, dom, NULL); + cmdRunConsole(ctl, dom, NULL, 0); #endif virDomainFree(dom); } else { @@ -2174,7 +2182,7 @@ started: vshPrint(ctl, _("Domain %s started\n"), virDomainGetName(dom)); #ifndef WIN32 - if (console && !cmdRunConsole(ctl, dom, NULL)) + if (console && !cmdRunConsole(ctl, dom, NULL, 0)) goto cleanup; #endif -- 1.7.3.4
On 12/07/2011 11:08 AM, Peter Krempa wrote:
This patch adds support for the newly introduced VIR_DOMAIN_CONSOLE
s/$/_FORCE/
flag. The console command now has an optional parameter --force that specifies that the user wants to forcibly interrupt an ongoing console session and create a new one. The behaviour to this point was, that the
s/,//
daemon openend two streams to the console, that competed for data from
s/openend/opened/
the pipe and the result was, that both of the consoles ended up
s/pipe and the result was,/pipe, and the result was/
scrambled.
* tools/console.c: - add support for flag passthrough * tools/console.h: - modify function prototypes to match impl. * tools/virsh.c: - add flag --force for the console command --- tools/console.c | 5 +++-- tools/console.h | 3 ++- tools/virsh.c | 18 +++++++++++++----- 3 files changed, 18 insertions(+), 8 deletions(-)
+++ b/tools/virsh.c @@ -787,11 +787,14 @@ static const vshCmdInfo info_console[] = { static const vshCmdOptDef opts_console[] = { {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, {"devname", VSH_OT_STRING, 0, N_("character device name")}, + {"force", VSH_OT_BOOL, 0, N_("force console connection (disconnect already connected sessions)")},
Might want to wrap this to fit 80 columns.
@@ -834,7 +839,10 @@ cmdConsole(vshControl *ctl, const vshCmd *cmd) goto cleanup; }
- ret = cmdRunConsole(ctl, dom, name); + if (force) + flags = VIR_DOMAIN_CONSOLE_FORCE;
Okay as-is, but if we add more flags in the future, this is not maintainer-friendly. I'd use |= instead of = so this line won't need future edits. ACK. Hmm, wondering about back-compat - what happens if we are talking to an older libvirt that does not know VIR_DOMAIN_CONSOLE_FORCE, but also which does not prevent multiple connections when flags==0? At first glance, it looks like we're stuck - there's no useful way to tell if a console was already in use. But maybe we could add a new API, to allow probing whether a console is in use. If the API exists, then we know whether the console is in use (and we also know whether --force will work); if the API does not exist, the we know we are talking to an older server that multiplexes connections. Maybe we should have: virsh console domain --safe => refuse to connect unless connection is safe (fails on older servers) virsh console domain --force => force connection (fails on older servers) virsh console domain => existing behavior (connects on older servers, even if it corrupts connection, and safely connects on newer servers) that is, I'm thinking it might be worth it to add a new --safe option that lets the user probe (via a new API virDomainConsoleInUse(dom, dev_name, flags)) whether the attempt will be safe. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Dňa 14.12.2011 1:14, Eric Blake wrote / napísal(a):
Hmm, wondering about back-compat - what happens if we are talking to an older libvirt that does not know VIR_DOMAIN_CONSOLE_FORCE, but also which does not prevent multiple connections when flags==0? At first glance, it looks like we're stuck - there's no useful way to tell if a console was already in use. Yes, we would end up in a state like when you try it now.
But maybe we could add a new API, to allow probing whether a console is in use. If the API exists, then we know whether the console is in use (and we also know whether --force will work); if the API does not exist, the we know we are talking to an older server that multiplexes connections. Maybe we should have:
virsh console domain --safe => refuse to connect unless connection is safe (fails on older servers) virsh console domain --force => force connection (fails on older servers) virsh console domain => existing behavior (connects on older servers, even if it corrupts connection, and safely connects on newer servers)
Yes, that's definitely a good idea.
that is, I'm thinking it might be worth it to add a new --safe option that lets the user probe (via a new API virDomainConsoleInUse(dom, dev_name, flags)) whether the attempt will be safe.
I was thinking about adding another flag. With that flag specified, the virDomainConsoleOpen function would return success without any operation, confirming, that the server is capable of handling console connections in a safe way. The client could then open the connection calling this function again. What do you think about this option? Peter
On 12/14/2011 01:46 AM, Peter Krempa wrote:
But maybe we could add a new API, to allow probing whether a console is in use. If the API exists, then we know whether the console is in use (and we also know whether --force will work); if the API does not exist, the we know we are talking to an older server that multiplexes connections. Maybe we should have:
virsh console domain --safe => refuse to connect unless connection is safe (fails on older servers) virsh console domain --force => force connection (fails on older servers) virsh console domain => existing behavior (connects on older servers, even if it corrupts connection, and safely connects on newer servers)
Yes, that's definitely a good idea.
that is, I'm thinking it might be worth it to add a new --safe option that lets the user probe (via a new API virDomainConsoleInUse(dom, dev_name, flags)) whether the attempt will be safe.
I was thinking about adding another flag. With that flag specified, the virDomainConsoleOpen function would return success without any operation, confirming, that the server is capable of handling console connections in a safe way. The client could then open the connection calling this function again.
What do you think about this option?
Good idea - using a new flag instead of a new API entry point should be sufficient. I see a couple of possibilities: virDomainConsoleOpen(,0) -> existing behavior on older servers, safe behavior on newer servers virDomainConsoleOpen(,_FORCE) -> rejected on older servers, forces behavior on newer servers virDomainConsoleOpen(,_PROBE) -> rejected on older servers, probes (but does not connect) on newer servers I'm not sure whether _PROBE should succeed or fail if it detects the server has support for safe behavior, but that the console is currently in use. Since probe does not do the open, we _must_ make Open(,0) check for other users when probe is supported, in order to avoid a TOCTTOU race. At which point, we can either have _PROBE blindly succeed when (,0) is safe (merely as a witness that can be exploited by virsh), or actually check the state of the console and return a distinct error if the console is in use at the time (but applications can't put too much trust in that return, since only the actual open attempt will tell whether the console situation changed between the probe and the open). Or maybe we do: virDomainConsoleOpen(,0) -> existing behavior on older servers, safe behavior on newer servers virDomainConsoleOpen(,_FORCE) -> rejected on older servers, forces behavior on newer servers virDomainConsoleOpen(,_SAFE) -> rejected on older servers, identical to (,0) on newer servers Either semantic works for me; and if we go with _PROBE instead of _SAFE, I'm also okay with either choice of blindly succeeding on _PROBE to indicate capability, or actually probing and returning a distinct error to indicate an in-use console. I guess at this point it's up to you in your v3, unless anyone else also expresses a preference. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
This patch causes the fdstream driver to call the stream event callback if virStreamAbort() is issued on a stream using this driver. This prohibited to abort streams from the daemon, as the daemon remote handler installs a callback to watch for stream errors as the only mean of detecting changes in the stream. * src/fdstream.c: - modify close function to call stream event callback --- src/fdstream.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++------ 1 files changed, 50 insertions(+), 6 deletions(-) diff --git a/src/fdstream.c b/src/fdstream.c index 841f979..35f5135 100644 --- a/src/fdstream.c +++ b/src/fdstream.c @@ -1,5 +1,5 @@ /* - * fdstream.h: generic streams impl for file descriptors + * fdstream.c: generic streams impl for file descriptors * * Copyright (C) 2009-2011 Red Hat, Inc. * @@ -55,6 +55,7 @@ struct virFDStreamData { unsigned long long length; int watch; + int events; /* events the stream callback is subscribed for */ bool cbRemoved; bool dispatching; bool closed; @@ -62,6 +63,10 @@ struct virFDStreamData { void *opaque; virFreeCallback ff; + /* don't call the abort callback more than once */ + bool abortCallbackCalled; + bool abortCallbackDispatching; + virMutex lock; }; @@ -92,6 +97,7 @@ static int virFDStreamRemoveCallback(virStreamPtr stream) fdst->watch = 0; fdst->ff = NULL; fdst->cb = NULL; + fdst->events = 0; fdst->opaque = NULL; ret = 0; @@ -120,6 +126,7 @@ static int virFDStreamUpdateCallback(virStreamPtr stream, int events) } virEventUpdateHandle(fdst->watch, events); + fdst->events = events; ret = 0; @@ -214,6 +221,8 @@ virFDStreamAddCallback(virStreamPtr st, fdst->cb = cb; fdst->opaque = opaque; fdst->ff = ff; + fdst->events = events; + fdst->abortCallbackCalled = false; virStreamRef(st); ret = 0; @@ -223,20 +232,43 @@ cleanup: return ret; } - static int -virFDStreamClose(virStreamPtr st) +virFDStreamCloseInt(virStreamPtr st, bool streamAbort) { - struct virFDStreamData *fdst = st->privateData; + struct virFDStreamData *fdst; int ret; VIR_DEBUG("st=%p", st); - if (!fdst) + if (!st || !(fdst = st->privateData) || fdst->abortCallbackDispatching) return 0; virMutexLock(&fdst->lock); + /* aborting the stream, ensure the callback is called if it's + * registered for stream error event */ + if (streamAbort && + fdst->cb && + (fdst->events & (VIR_STREAM_EVENT_READABLE | + VIR_STREAM_EVENT_WRITABLE))) { + /* don't enter this function accidentaly from the callback again */ + if (fdst->abortCallbackCalled) { + virMutexUnlock(&fdst->lock); + return 0; + } + + fdst->abortCallbackCalled = true; + fdst->abortCallbackDispatching = true; + virMutexUnlock(&fdst->lock); + + /* call failure callback, poll does report nothing on closed fd */ + (fdst->cb)(st, VIR_STREAM_EVENT_ERROR, fdst->opaque); + + virMutexLock(&fdst->lock); + fdst->abortCallbackDispatching = false; + } + + /* mutex locked */ ret = VIR_CLOSE(fdst->fd); if (fdst->cmd) { char buf[1024]; @@ -286,6 +318,18 @@ virFDStreamClose(virStreamPtr st) return ret; } +static int +virFDStreamClose(virStreamPtr st) +{ + return virFDStreamCloseInt(st, false); +} + +static int +virFDStreamAbort(virStreamPtr st) +{ + return virFDStreamCloseInt(st, true); +} + static int virFDStreamWrite(virStreamPtr st, const char *bytes, size_t nbytes) { struct virFDStreamData *fdst = st->privateData; @@ -392,7 +436,7 @@ static virStreamDriver virFDStreamDrv = { .streamSend = virFDStreamWrite, .streamRecv = virFDStreamRead, .streamFinish = virFDStreamClose, - .streamAbort = virFDStreamClose, + .streamAbort = virFDStreamAbort, .streamAddCallback = virFDStreamAddCallback, .streamUpdateCallback = virFDStreamUpdateCallback, .streamRemoveCallback = virFDStreamRemoveCallback -- 1.7.3.4
On 12/07/2011 11:08 AM, Peter Krempa wrote:
This patch causes the fdstream driver to call the stream event callback if virStreamAbort() is issued on a stream using this driver. This prohibited to abort streams from the daemon, as the daemon remote handler installs a callback to watch for stream errors as the only mean of detecting changes in the stream.
* src/fdstream.c: - modify close function to call stream event callback --- src/fdstream.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++------ 1 files changed, 50 insertions(+), 6 deletions(-)
+ /* aborting the stream, ensure the callback is called if it's + * registered for stream error event */ + if (streamAbort && + fdst->cb && + (fdst->events & (VIR_STREAM_EVENT_READABLE | + VIR_STREAM_EVENT_WRITABLE))) { + /* don't enter this function accidentaly from the callback again */
s/accidentaly/accidentally/
+ if (fdst->abortCallbackCalled) { + virMutexUnlock(&fdst->lock); + return 0; + } + + fdst->abortCallbackCalled = true; + fdst->abortCallbackDispatching = true; + virMutexUnlock(&fdst->lock); + + /* call failure callback, poll does report nothing on closed fd */
s/does report/reports/ I've read this for sanity, and it seems to make sense, but I'd feel more comfortable if Dan Berrange gives the final ack. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
This patch adds another callback to a FDstream object. The original callback is used by the daemon stream driver to handle events. This callback is called if and only if the stream is about to be closed. This might be used to handle cleanup steps after a fdstream exits. This will be used later on in ensuring mutualy exclusive access to consoles. * src/fdstream.c: - emit the callback, when stream is being closed - add data structures needed to handle the callback - add function to register callback * src/fdstream.h: - define function prototypes for the callback --- src/fdstream.c | 39 ++++++++++++++++++++++++++++++++++----- src/fdstream.h | 11 +++++++++++ 2 files changed, 45 insertions(+), 5 deletions(-) diff --git a/src/fdstream.c b/src/fdstream.c index 35f5135..18d7b29 100644 --- a/src/fdstream.c +++ b/src/fdstream.c @@ -67,6 +67,12 @@ struct virFDStreamData { bool abortCallbackCalled; bool abortCallbackDispatching; + /* internal callback, as the regular one (from generic streams) gets + * eaten up by the server stream driver */ + virFDStreamInternalCloseCb icbCb; + virFDStreamInternalCloseCbFreeOpaque icbFreeOpaque; + void *icbOpaque; + virMutex lock; }; @@ -183,7 +189,6 @@ static void virFDStreamCallbackFree(void *opaque) virStreamFree(st); } - static int virFDStreamAddCallback(virStreamPtr st, int events, @@ -306,6 +311,14 @@ virFDStreamCloseInt(virStreamPtr st, bool streamAbort) st->privateData = NULL; + /* call the internal stream closing callback */ + if (fdst->icbCb) { + /* the mutex is not accessible anymore, as private data are null */ + (fdst->icbCb)(st, fdst->icbOpaque); + if (fdst->icbFreeOpaque && fdst->icbOpaque) + (fdst->icbFreeOpaque)(fdst->icbOpaque); + } + if (fdst->dispatching) { fdst->closed = true; virMutexUnlock(&fdst->lock); @@ -381,7 +394,6 @@ retry: return ret; } - static int virFDStreamRead(virStreamPtr st, char *bytes, size_t nbytes) { struct virFDStreamData *fdst = st->privateData; @@ -431,7 +443,6 @@ retry: return ret; } - static virStreamDriver virFDStreamDrv = { .streamSend = virFDStreamWrite, .streamRecv = virFDStreamRead, @@ -479,14 +490,12 @@ static int virFDStreamOpenInternal(virStreamPtr st, return 0; } - int virFDStreamOpen(virStreamPtr st, int fd) { return virFDStreamOpenInternal(st, fd, NULL, -1, 0); } - #if HAVE_SYS_UN_H int virFDStreamConnectUNIX(virStreamPtr st, const char *path, @@ -680,3 +689,23 @@ int virFDStreamCreateFile(virStreamPtr st, offset, length, oflags | O_CREAT, mode); } + +int virFDStreamSetInternalCloseCb(virStreamPtr st, + virFDStreamInternalCloseCb cb, + void *opaque, + virFDStreamInternalCloseCbFreeOpaque fcb) +{ + struct virFDStreamData *fdst = st->privateData; + + virMutexLock(&fdst->lock); + + if (fdst->icbOpaque && fdst->icbFreeOpaque) + (fdst->icbFreeOpaque)(fdst->icbOpaque); + + fdst->icbCb = cb; + fdst->icbOpaque = opaque; + fdst->icbFreeOpaque = fcb; + + virMutexUnlock(&fdst->lock); + return 0; +} diff --git a/src/fdstream.h b/src/fdstream.h index f9edb23..11acb90 100644 --- a/src/fdstream.h +++ b/src/fdstream.h @@ -26,6 +26,13 @@ # include "internal.h" # include "command.h" +/* internal callback, the generic one is used up by daemon stream driver */ +/* the close callback is called with fdstream private data locked */ +typedef void (*virFDStreamInternalCloseCb)(virStreamPtr st, void *opaque); + +typedef void (*virFDStreamInternalCloseCbFreeOpaque)(void *opaque); + + int virFDStreamOpen(virStreamPtr st, int fd); @@ -45,4 +52,8 @@ int virFDStreamCreateFile(virStreamPtr st, int oflags, mode_t mode); +int virFDStreamSetInternalCloseCb(virStreamPtr st, + virFDStreamInternalCloseCb cb, + void *opaque, + virFDStreamInternalCloseCbFreeOpaque fcb); #endif /* __VIR_FDSTREAM_H_ */ -- 1.7.3.4
On 12/07/2011 11:08 AM, Peter Krempa wrote:
This patch adds another callback to a FDstream object. The original callback is used by the daemon stream driver to handle events.
This callback is called if and only if the stream is about to be closed. This might be used to handle cleanup steps after a fdstream exits. This will be used later on in ensuring mutualy exclusive access to consoles.
* src/fdstream.c: - emit the callback, when stream is being closed - add data structures needed to handle the callback - add function to register callback * src/fdstream.h: - define function prototypes for the callback --- src/fdstream.c | 39 ++++++++++++++++++++++++++++++++++----- src/fdstream.h | 11 +++++++++++ 2 files changed, 45 insertions(+), 5 deletions(-)
@@ -381,7 +394,6 @@ retry: return ret; }
- static int virFDStreamRead(virStreamPtr st, char *bytes, size_t nbytes) { struct virFDStreamData *fdst = st->privateData; @@ -431,7 +443,6 @@ retry: return ret; }
- static virStreamDriver virFDStreamDrv = { .streamSend = virFDStreamWrite, .streamRecv = virFDStreamRead, @@ -479,14 +490,12 @@ static int virFDStreamOpenInternal(virStreamPtr st, return 0; }
- int virFDStreamOpen(virStreamPtr st, int fd) { return virFDStreamOpenInternal(st, fd, NULL, -1, 0); }
-
Why all the whitespace deletion? Again, the patch looks sane, but I'd feel more comfortable with Dan's review.
+++ b/src/fdstream.h @@ -26,6 +26,13 @@ # include "internal.h" # include "command.h"
+/* internal callback, the generic one is used up by daemon stream driver */ +/* the close callback is called with fdstream private data locked */ +typedef void (*virFDStreamInternalCloseCb)(virStreamPtr st, void *opaque); + +typedef void (*virFDStreamInternalCloseCbFreeOpaque)(void *opaque);
Do we need a new typedef, or can we reuse the same signature as the generic streams one, and just make our streams callback (which we feed to the stream driver) then invoke the user's callback of the same signature?
+ + int virFDStreamOpen(virStreamPtr st, int fd);
@@ -45,4 +52,8 @@ int virFDStreamCreateFile(virStreamPtr st, int oflags, mode_t mode);
+int virFDStreamSetInternalCloseCb(virStreamPtr st, + virFDStreamInternalCloseCb cb, + void *opaque, + virFDStreamInternalCloseCbFreeOpaque fcb);
The name 'Internal' may scare users away from trying to use this. But I'm not sure of a better naming scheme. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
This patch adds a set of functions used in creating console streams for domains using PTYs and ensures mutualy exculsive access to the PTYs. If mutualy exclusive access is not used, two clients may open the same console, which results into corruption on both clients as both of them race to read data from the PTY. Two approaches are used to ensure this: 1) Internal data structure holding open PTYs. This is used internaly and enables the user to forcibly terminate another console connection eg. when somebody leaves the console open on another host. 2) UUCP style lock files: This uses UUCP lock files according to the FHS ( http://www.pathname.com/fhs/pub/fhs-2.3.html#VARLOCKLOCKFILES ) to check if other programs (like minicom) are not using the pty device of the console. This feature is disabled by default and may be enabled using configure parameter --with-console-lock-files=/path/to/lock/file/directory or --with-console-lock-files=auto (which tries to infer the location from OS used (currently only linux). On usual linux systems, normal users may not write to the /var/lock directory containing the locks. This poses problems while in session mode. If the current user has no access to the lockfile directory, check for presence of the file is still done, but no lock file is created. This does NOT result into an error. * configure.ac - add option to enable UUCP style PTY file locks * src/Makefile.am - add new files to be built with util module * src/libvirt_private.syms - add private symbol definition * src/util/domain_safe_console.c - implementation of safe console handling * src/util/domain_safe_console.h - header files --- configure.ac | 37 +++- src/Makefile.am | 5 +- src/libvirt_private.syms | 6 + src/util/domain_safe_console.c | 399 ++++++++++++++++++++++++++++++++++++++++ src/util/domain_safe_console.h | 28 +++ 5 files changed, 466 insertions(+), 9 deletions(-) create mode 100644 src/util/domain_safe_console.c create mode 100644 src/util/domain_safe_console.h diff --git a/configure.ac b/configure.ac index 77e5cc9..d427a9e 100644 --- a/configure.ac +++ b/configure.ac @@ -329,6 +329,10 @@ AC_ARG_WITH([remote], AC_HELP_STRING([--with-remote], [add remote driver support @<:@default=yes@:>@]),[],[with_remote=yes]) AC_ARG_WITH([libvirtd], AC_HELP_STRING([--with-libvirtd], [add libvirtd support @<:@default=yes@:>@]),[],[with_libvirtd=yes]) +AC_ARG_WITH([console-lock-files], + AC_HELP_STRING([--with-console-lock-files], + [location for UUCP style lock files for console PTYs (use auto for default paths on some platforms)@<:@defult=disabled@:>@]), + [],[with_console_lock_files=disabled]) dnl dnl in case someone want to build static binaries @@ -1198,6 +1202,22 @@ AM_CONDITIONAL([HAVE_AUDIT], [test "$with_audit" = "yes"]) AC_SUBST([AUDIT_CFLAGS]) AC_SUBST([AUDIT_LIBS]) +dnl UUCP style file locks for PTY consoles +if test "$with_console_lock_files" != "disabled"; then + if test "$with_console_lock_files" = "auto"; then + dnl Default locations for platforms + if test "$with_linux" = "yes"; then + with_console_lock_files=/var/lock + fi + fi + if test "$with_console_lock_files" = "auto"; then + AC_MSG_ERROR([You must specify path for the lock files on this platform]) + fi + AC_DEFINE_UNQUOTED([VIR_PTY_LOCK_FILE_PATH], "$with_console_lock_files", + [path to directory containig UUCP pty lock files ]) +fi +AM_CONDITIONAL([VIR_PTY_LOCK_FILE_PATH], [test "$with_console_lock_files" != "disabled"]) + dnl SELinux AC_ARG_WITH([selinux], @@ -2726,14 +2746,15 @@ AC_MSG_NOTICE([ Alloc OOM: $enable_oom]) AC_MSG_NOTICE([]) AC_MSG_NOTICE([Miscellaneous]) AC_MSG_NOTICE([]) -AC_MSG_NOTICE([ Debug: $enable_debug]) -AC_MSG_NOTICE([ Warnings: $enable_compile_warnings]) -AC_MSG_NOTICE([Warning Flags: $WARN_CFLAGS]) -AC_MSG_NOTICE([ Readline: $lv_use_readline]) -AC_MSG_NOTICE([ Python: $with_python]) -AC_MSG_NOTICE([ DTrace: $with_dtrace]) -AC_MSG_NOTICE([ XML Catalog: $XML_CATALOG_FILE]) -AC_MSG_NOTICE([ Init script: $with_init_script]) +AC_MSG_NOTICE([ Debug: $enable_debug]) +AC_MSG_NOTICE([ Warnings: $enable_compile_warnings]) +AC_MSG_NOTICE([ Warning Flags: $WARN_CFLAGS]) +AC_MSG_NOTICE([ Readline: $lv_use_readline]) +AC_MSG_NOTICE([ Python: $with_python]) +AC_MSG_NOTICE([ DTrace: $with_dtrace]) +AC_MSG_NOTICE([ XML Catalog: $XML_CATALOG_FILE]) +AC_MSG_NOTICE([ Init script: $with_init_script]) +AC_MSG_NOTICE([Console PTY lock path: $with_console_lock_files]) AC_MSG_NOTICE([]) AC_MSG_NOTICE([Privileges]) AC_MSG_NOTICE([]) diff --git a/src/Makefile.am b/src/Makefile.am index 93bf54c..3b04096 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -101,6 +101,9 @@ UTIL_SOURCES = \ util/virsocketaddr.h util/virsocketaddr.c \ util/virtime.h util/virtime.c +SAFE_CONSOLE_SOURCES = \ + util/domain_safe_console.c util/domain_safe_console.h + EXTRA_DIST += $(srcdir)/util/virkeymaps.h $(srcdir)/util/keymaps.csv \ $(srcdir)/util/virkeycode-mapgen.py @@ -555,7 +558,7 @@ noinst_LTLIBRARIES = libvirt_util.la libvirt_la_LIBADD = $(libvirt_la_BUILT_LIBADD) libvirt_la_BUILT_LIBADD = libvirt_util.la libvirt_util_la_SOURCES = \ - $(UTIL_SOURCES) + $(UTIL_SOURCES) $(SAFE_CONSOLE_SOURCES) libvirt_util_la_CFLAGS = $(CAPNG_CFLAGS) $(YAJL_CFLAGS) $(LIBNL_CFLAGS) \ $(AM_CFLAGS) $(AUDIT_CFLAGS) $(DEVMAPPER_CFLAGS) libvirt_util_la_LIBADD = $(CAPNG_LIBS) $(YAJL_LIBS) $(LIBNL_LIBS) \ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 99a1099..082248d 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -537,6 +537,12 @@ virDomainConfNWFilterTeardown; virDomainConfVMNWFilterTeardown; +# domain_safe_console.h +virDomainSafeConsoleAlloc; +virDomainSafeConsoleFree; +virDomainSafeConsoleOpen; + + # ebtables.h ebtablesAddForwardAllowIn; ebtablesAddForwardPolicyReject; diff --git a/src/util/domain_safe_console.c b/src/util/domain_safe_console.c new file mode 100644 index 0000000..4d5639d --- /dev/null +++ b/src/util/domain_safe_console.c @@ -0,0 +1,399 @@ +/** + * domain_safe_console.c: api to guarantee mutualy exclusive + * access to domain's consoles + * + * Copyright (C) 2011 Red Hat, Inc. + * + * See COPYING.LIB for the License of this software + * + * Author: Peter Krempa <pkrempa@redhat.com> + */ + +#include <config.h> + +#include <fcntl.h> +#include <unistd.h> +#include <signal.h> +#include <sys/types.h> + +#include "domain_safe_console.h" +#include "hash.h" +#include "fdstream.h" +#include "internal.h" +#include "threads.h" +#include "memory.h" +#include "logging.h" +#include "virterror_internal.h" +#include "virfile.h" + +#define VIR_FROM_THIS VIR_FROM_NONE +# define consoleReportError(code, ...) \ + virReportErrorHelper(VIR_FROM_THIS, code, __FILE__, \ + __FUNCTION__, __LINE__, __VA_ARGS__) + +/* structure holding information about consoles + * open in a given domain */ +struct _virDomainSafeConsoles { + virMutex lock; + virHashTablePtr hash; +}; + +typedef struct _virDomainSafeConsoleStreamInfo virDomainSafeConsoleStreamInfo; +typedef virDomainSafeConsoleStreamInfo *virDomainSafeConsoleStreamInfoPtr; +struct _virDomainSafeConsoleStreamInfo { + virDomainSafeConsolesPtr cons; + const char *pty; +}; + +#ifdef VIR_PTY_LOCK_FILE_PATH +/** + * Create a full filename with path to the lock file based on + * name/path of corresponding pty + * + * @pty path of the console device + * + * Returns a modified name, that the caller has to free or NULL + * on error. + */ +static char *virDomainSafeConsoleLockFilePath(const char *pty) +{ + char *path = NULL; + char *sanitizedPath = NULL; + char *ptyCopy; + char *filename; + char *p; + + if (!(ptyCopy = strdup(pty))) { + virReportOOMError(); + goto cleanup; + } + + if ((filename = strstr(ptyCopy, "/dev/"))) { + filename += 5; /* skip /dev/ */ + p = filename; + /* substitute path forward slashes for underscores */ + while (*p) { + if (*p == '/') + *p = '_'; + ++p; + } + } + + if (virAsprintf(&path, "%s/LCK..%s", VIR_PTY_LOCK_FILE_PATH, filename) < 0) + goto cleanup; + + sanitizedPath = virFileSanitizePath(path); + +cleanup: + VIR_FREE(path); + VIR_FREE(ptyCopy); + + return sanitizedPath; +} + +/** + * Verify and create a lock file for a console pty + * + * @pty Path of the console device + * + * Returns 0 on success, -1 on error + */ +static int virDomainSafeConsoleLockFileCreate(const char *pty) +{ + char *path = NULL; + int ret = -1; + int lockfd = -1; + char *pidStr = NULL; + int pid; + ssize_t read_n; + /* build lock file path */ + if (!(path = virDomainSafeConsoleLockFilePath(pty))) + goto cleanup; + + /* check if lock file exists */ + if ((lockfd = open(path, O_RDONLY)) >= 0) { + if ((read_n = virFileReadLimFD(lockfd, 100, &pidStr) > 0) && + (sscanf(pidStr, "%d", &pid) == 1)) { + /* lock file is valid, let's check if the pid is alive */ + if (pid > 1 && kill((pid_t) pid, 0) == 0) { + /* lock file is valid and a process with pid corresponding + * to the pid stored in the lock file exists */ + consoleReportError(VIR_ERR_OPERATION_FAILED, + _("Requested console pty '%s' is locked by " + "lock file '%s' held by process %d"), + pty, path, pid); + goto cleanup; + } + /* lock file is stale */ + VIR_DEBUG("Cleaning up stale lockfile '%s'", path); + VIR_FORCE_CLOSE(lockfd); + unlink(path); + } else { + /* lock file is corrupted */ + VIR_DEBUG("Cleaning up corrupted lockfile '%s'", path); + VIR_FORCE_CLOSE(lockfd); + unlink(path); + /* this might actualy fail, but we get an error while + * creating a new lockfile. Let the user handle the mess. */ + } + VIR_FREE(pidStr); + } + /* lockfile doesn't (shouldn't) exist */ + + /* ensure correct format according to filesystem hierarchy standard */ + if (virAsprintf(&pidStr, "%10d\n", (int) getpid()) < 0) + goto cleanup; + + /* create the lock file */ + if ((lockfd = open(path, O_WRONLY | O_CREAT | O_EXCL, 00644)) < 0) { + /* If we run in session mode, we might have no access to the lock + * file directory. We have to check for an permission denied error + * and see if we can reach it. This should cause an error only if + * we run in daemon mode and thus privileged. + */ + if (errno == EACCES && geteuid() != 0) { + VIR_DEBUG("Skipping lock file creation for pty '%s in path '%s'.", + pty, path); + ret = 0; + goto cleanup; + } + virReportSystemError(errno, + _("Couldn't create lock file for " + "pty '%s' in path '%s'"), + pty, path); + goto cleanup; + } + + /* write the pid to the file */ + if (safewrite(lockfd, pidStr, strlen(pidStr)) < 0) { + virReportSystemError(errno, + _("Couldn't write to lock file for " + "pty '%s' in path '%s'"), + pty, path); + VIR_FORCE_CLOSE(lockfd); + unlink(path); + goto cleanup; + } + + /* we hold the lock */ + ret = 0; + +cleanup: + VIR_FORCE_CLOSE(lockfd); + VIR_FREE(path); + VIR_FREE(pidStr); + + return ret; +} + +/** + * Remove a lock file for a pty + * + * @pty Path of the pty device + */ +static void virDomainSafeConsoleLockFileRemove(const char *pty) +{ + char *path = virDomainSafeConsoleLockFilePath(pty); + if (path) + unlink(path); + VIR_FREE(path); +} +#else /* #ifdef VIR_PTY_LOCK_FILE_PATH */ +/* file locking for console devices is disabled */ +static int virDomainSafeConsoleLockFileCreate(const char *pty ATTRIBUTE_UNUSED) +{ + return 0; +} + +static void virDomainSafeConsoleLockFileRemove(const char *py ATTRIBUTE_UNUSED) +{ + return; +} +#endif /* #ifdef VIR_PTY_LOCK_FILE_PATH */ + +/** + * Frees an entry from the hash containing domain's active consoles + * + * @data Opaque data, struct holding informations about the console + * @name Path of the pty. + */ +static void virDomainSafeConsoleHashEntryFree(void *data, + const void *name) +{ + const char *pty = name; + virStreamPtr st = data; + + /* free stream reference */ + virStreamFree(st); + + /* delete lock file */ + virDomainSafeConsoleLockFileRemove(pty); +} + +/** + * Frees opaque data provided for the stream closing callback + * + * @opaque Data to be freed. + */ +static void virDomainSafeConsoleFDStreamCloseCbFree(void *opaque) +{ + virDomainSafeConsoleStreamInfoPtr priv = opaque; + + VIR_FREE(priv->pty); + VIR_FREE(priv); +} + +/** + * Callback being called if a FDstream is closed. Frees console entries + * from data structures and removes lockfiles. + * + * @st Pointer to stream being closed. + * @opaque Domain's console information structure. + */ +static void virDomainSafeConsoleFDStreamCloseCb(virStreamPtr st ATTRIBUTE_UNUSED, + void *opaque) +{ + virDomainSafeConsoleStreamInfoPtr priv = opaque; + virMutexLock(&priv->cons->lock); + + /* remove entry from hash */ + virHashRemoveEntry(priv->cons->hash, priv->pty); + + virMutexUnlock(&priv->cons->lock); +} + +/** + * Allocate structures for storing information about active console streams + * in domain's private data section. + * + * Returns pointer to the allocated structure or NULL on error + */ +virDomainSafeConsolesPtr virDomainSafeConsoleAlloc(void) +{ + virDomainSafeConsolesPtr cons; + if (VIR_ALLOC(cons) < 0) + return NULL; + + /* there will hardly be any consoles most of the time, the hash + * does not have to be huge */ + if (!(cons->hash = virHashCreate(3, virDomainSafeConsoleHashEntryFree))) + goto error; + + if (virMutexInit(&cons->lock) < 0) + goto error; + + return cons; +error: + virDomainSafeConsoleFree(cons); + return NULL; +} + +/** + * Free structures for handling open console streams. + * + * @cons Pointer to the private structure. + */ +void virDomainSafeConsoleFree(virDomainSafeConsolesPtr cons) +{ + if (!cons || !cons->hash) + return; + + virMutexLock(&cons->lock); + virHashFree(cons->hash); + cons->hash = NULL; + virMutexUnlock(&cons->lock); + virMutexDestroy(&cons->lock); + + VIR_FREE(cons); +} + +/** + * Open a console stream for a domain ensuring that other streams are + * not using the console, nor any lockfiles exist. This ensuers that + * the console stream does not get corrupted due to a race on reading + * same FD by two processes. + * + * @cons Pointer to private structure holding data about console streams. + * @pty Path to the pseudo tty to be opened. + * @st Stream the client wishes to use for the console connection. + * @force On true, close active console streams for the selected console pty + * before opening this connection. + * + * Returns 0 on success and st is connected to the selected pty and + * corresponding lock file is created (if configured with). Returns -1 on + * error and 1 if the console stream is open and busy. + */ +int virDomainSafeConsoleOpen(virDomainSafeConsolesPtr cons, + const char *pty, + virStreamPtr st, + bool force) +{ + virDomainSafeConsoleStreamInfoPtr cbdata = NULL; + virStreamPtr savedStream; + int ret; + + virMutexLock(&cons->lock); + + if ((savedStream = virHashLookup(cons->hash, pty))) { + if (!force) { + /* entry found, console is busy */ + virMutexUnlock(&cons->lock); + return 1; + } else { + /* terminate existing connection */ + virFDStreamSetInternalCloseCb(savedStream, NULL, NULL, NULL); + virStreamAbort(savedStream); + virHashRemoveEntry(cons->hash, pty); + /* continue adding a new stream connection */ + } + } + + /* create the lock file */ + if ((ret = virDomainSafeConsoleLockFileCreate(pty)) < 0) { + virMutexUnlock(&cons->lock); + return ret; + } + + /* obtain a reference to the stream */ + if (virStreamRef(st) < 0) { + virMutexUnlock(&cons->lock); + return -1; + } + + if (VIR_ALLOC(cbdata) < 0) + goto error; + + if (virHashAddEntry(cons->hash, pty, st) < 0) + goto error; + + savedStream = st; + st = NULL; + + cbdata->cons = cons; + if (!(cbdata->pty = strdup(pty))) + goto error; + + /* open the console pty */ + if (virFDStreamOpenFile(savedStream, pty, 0, 0, O_RDWR) < 0) + goto error; + + + /* add cleanup callback */ + virFDStreamSetInternalCloseCb(savedStream, + virDomainSafeConsoleFDStreamCloseCb, + cbdata, + virDomainSafeConsoleFDStreamCloseCbFree); + cbdata = NULL; + + virMutexUnlock(&cons->lock); + return 0; + +error: + virStreamFree(st); + virHashRemoveEntry(cons->hash, pty); + if (cbdata) + VIR_FREE(cbdata->pty); + VIR_FREE(cbdata); + virMutexUnlock(&cons->lock); + return -1; +} diff --git a/src/util/domain_safe_console.h b/src/util/domain_safe_console.h new file mode 100644 index 0000000..39536f2 --- /dev/null +++ b/src/util/domain_safe_console.h @@ -0,0 +1,28 @@ +/** + * domain_console_lock.h: api to guarantee mutualy exclusive + * access to domain consoles + * + * Copyright (C) 2011 Red Hat, Inc. + * + * See COPYING.LIB for the License of this software + * + * Author: Peter Krempa <pkrempa@redhat.com> + */ + +#ifndef __VIR_DOMAIN_CONSOLE_LOCK_H__ +# define __VIR_DOMAIN_CONSOLE_LOCK_H__ + +# include "internal.h" + +typedef struct _virDomainSafeConsoles virDomainSafeConsoles; +typedef virDomainSafeConsoles *virDomainSafeConsolesPtr; + + +virDomainSafeConsolesPtr virDomainSafeConsoleAlloc(void); +void virDomainSafeConsoleFree(virDomainSafeConsolesPtr cons); + +int virDomainSafeConsoleOpen(virDomainSafeConsolesPtr cons, + const char *pty, + virStreamPtr st, + bool force); +#endif /*__VIR_DOMAIN_CONSOLE_LOCK_H__*/ -- 1.7.3.4
On 12/07/2011 11:08 AM, Peter Krempa wrote:
This patch adds a set of functions used in creating console streams for domains using PTYs and ensures mutualy exculsive access to the PTYs.
s/mutualy exculsive/mutually exclusive/
If mutualy exclusive access is not used, two clients may open the same
s/mutualy/mutually/
console, which results into corruption on both clients as both of them
s/into/in/
race to read data from the PTY.
Two approaches are used to ensure this: 1) Internal data structure holding open PTYs. This is used internaly and enables the user to forcibly
s/internaly/internally/
terminate another console connection eg. when somebody leaves the console open on another host.
2) UUCP style lock files: This uses UUCP lock files according to the FHS ( http://www.pathname.com/fhs/pub/fhs-2.3.html#VARLOCKLOCKFILES )
Looks like I've got some reading to do.
to check if other programs (like minicom) are not using the pty device of the console.
This feature is disabled by default and may be enabled using configure parameter --with-console-lock-files=/path/to/lock/file/directory or --with-console-lock-files=auto (which tries to infer the location from OS used (currently only linux).
On usual linux systems, normal users may not write to the /var/lock directory containing the locks. This poses problems while in session mode. If the current user has no access to the lockfile directory, check for presence of the file is still done, but no lock file is created. This does NOT result into an error.
* configure.ac - add option to enable UUCP style PTY file locks * src/Makefile.am - add new files to be built with util module * src/libvirt_private.syms - add private symbol definition * src/util/domain_safe_console.c - implementation of safe console handling * src/util/domain_safe_console.h - header files --- configure.ac | 37 +++- src/Makefile.am | 5 +- src/libvirt_private.syms | 6 + src/util/domain_safe_console.c | 399 ++++++++++++++++++++++++++++++++++++++++ src/util/domain_safe_console.h | 28 +++ 5 files changed, 466 insertions(+), 9 deletions(-) create mode 100644 src/util/domain_safe_console.c create mode 100644 src/util/domain_safe_console.h
I've run out of time to finish my review of this today, but the idea seems interesting. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 12/13/2011 05:31 PM, Eric Blake wrote:
On 12/07/2011 11:08 AM, Peter Krempa wrote:
This patch adds a set of functions used in creating console streams for domains using PTYs and ensures mutualy exculsive access to the PTYs.
s/mutualy exculsive/mutually exclusive/
--- configure.ac | 37 +++- src/Makefile.am | 5 +- src/libvirt_private.syms | 6 + src/util/domain_safe_console.c | 399 ++++++++++++++++++++++++++++++++++++++++ src/util/domain_safe_console.h | 28 +++ 5 files changed, 466 insertions(+), 9 deletions(-) create mode 100644 src/util/domain_safe_console.c create mode 100644 src/util/domain_safe_console.h
I've run out of time to finish my review of this today, but the idea seems interesting.
Still haven't finished this review, but it fails 'make syntax-check': po_check --- ./po/POTFILES.in +++ ./po/POTFILES.in @@ -108,6 +108,7 @@ src/util/command.c src/util/conf.c src/util/dnsmasq.c +src/util/domain_safe_console.c src/util/event_poll.c src/util/hash.c src/util/hooks.c maint.mk: you have changed the set of files with translatable diagnostics; apply the above patch preprocessor_indentation cppi: src/util/domain_safe_console.c: line 30: not properly indented maint.mk: incorrect preprocessor indentation -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 12/07/2011 11:08 AM, Peter Krempa wrote:
This patch adds a set of functions used in creating console streams for domains using PTYs and ensures mutualy exculsive access to the PTYs.
If mutualy exclusive access is not used, two clients may open the same console, which results into corruption on both clients as both of them race to read data from the PTY.
+++ b/configure.ac @@ -329,6 +329,10 @@ AC_ARG_WITH([remote], AC_HELP_STRING([--with-remote], [add remote driver support @<:@default=yes@:>@]),[],[with_remote=yes]) AC_ARG_WITH([libvirtd], AC_HELP_STRING([--with-libvirtd], [add libvirtd support @<:@default=yes@:>@]),[],[with_libvirtd=yes]) +AC_ARG_WITH([console-lock-files], + AC_HELP_STRING([--with-console-lock-files], + [location for UUCP style lock files for console PTYs (use auto for default paths on some platforms)@<:@defult=disabled@:>@]),
s/defult/default/ Pre-existing, but AC_HELP_STRING is deprecated per autoconf documentation; we should clean this up to use AS_HELP_STRING. Also pre-existing - this is underquoted - we should be using AC_ARG_WITH([arg], [AS_HELP_STRING([option], [description])]) but those should be a separate cleanup over all instances of this pattern. Meanwhile, in your patch, you added a rather long line. A[CS]_HELP_STRING auto-line-wraps its second argument, so you could write it like: [AC_HELP_STRING([--with-console-lock-files], [location for UUCP style lock files for console PTYs (use auto for default paths on some platforms) @<:@defulat=disables@:>@], ...
+ fi + AC_DEFINE_UNQUOTED([VIR_PTY_LOCK_FILE_PATH], "$with_console_lock_files", + [path to directory containig UUCP pty lock files ])
s/containg/containing/ s/files ]/files]/
+AC_MSG_NOTICE([ Init script: $with_init_script]) +AC_MSG_NOTICE([Console PTY lock path: $with_console_lock_files])
You'd still have to re-indent, but maybe the shorter: Console PTY locks: would be nicer.
+++ b/src/Makefile.am
Aargh - I need to quit starting the review of this at the end of my day... More tomorrow, I hope. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 12/07/2011 11:08 AM, Peter Krempa wrote:
This patch adds a set of functions used in creating console streams for domains using PTYs and ensures mutualy exculsive access to the PTYs.
Picking up on my (stalled) review...
+++ b/src/Makefile.am @@ -101,6 +101,9 @@ UTIL_SOURCES = \ util/virsocketaddr.h util/virsocketaddr.c \ util/virtime.h util/virtime.c
+SAFE_CONSOLE_SOURCES = \ + util/domain_safe_console.c util/domain_safe_console.h
Unless you are planning on including these sources into an independent library, I see no need to make a new variable. Just add these files into the right place within UTIL_SOURCES. Those are some rather long names. Also, our convention for new files has lately been to go with vir<name>.[ch], with APIs starting with vir<Name>...(). I'm thinking that this patch may be better as: virconsole.[ch] virConsoleAlloc() virConsoleFree() virConsoleOpen() That is, the name Domain doesn't add anything (we might want a console for other reasons) and the name Safe is redundant (the whole point of adding a virName API is to make the Name action safer and easier to use).
+ EXTRA_DIST += $(srcdir)/util/virkeymaps.h $(srcdir)/util/keymaps.csv \ $(srcdir)/util/virkeycode-mapgen.py
@@ -555,7 +558,7 @@ noinst_LTLIBRARIES = libvirt_util.la libvirt_la_LIBADD = $(libvirt_la_BUILT_LIBADD) libvirt_la_BUILT_LIBADD = libvirt_util.la libvirt_util_la_SOURCES = \ - $(UTIL_SOURCES) + $(UTIL_SOURCES) $(SAFE_CONSOLE_SOURCES)
If you add the new files directly to UTIL_SOURCES, then you don't need to edit this line.
+++ b/src/libvirt_private.syms @@ -537,6 +537,12 @@ virDomainConfNWFilterTeardown; virDomainConfVMNWFilterTeardown;
+# domain_safe_console.h +virDomainSafeConsoleAlloc; +virDomainSafeConsoleFree; +virDomainSafeConsoleOpen;
See above about naming ideas.
+ + # ebtables.h ebtablesAddForwardAllowIn; ebtablesAddForwardPolicyReject; diff --git a/src/util/domain_safe_console.c b/src/util/domain_safe_console.c new file mode 100644 index 0000000..4d5639d --- /dev/null +++ b/src/util/domain_safe_console.c @@ -0,0 +1,399 @@ +/** + * domain_safe_console.c: api to guarantee mutualy exclusive + * access to domain's consoles + * + * Copyright (C) 2011 Red Hat, Inc.
Shoot - I let this go so long that it is now 2012.
+ * + * See COPYING.LIB for the License of this software
We probably ought to go with the longer 3-paragraph version that specifically calls out LGPLv2+, rather than referring to COPYING.LIB.
+#ifdef VIR_PTY_LOCK_FILE_PATH +/** + * Create a full filename with path to the lock file based on + * name/path of corresponding pty + * + * @pty path of the console device + * + * Returns a modified name, that the caller has to free or NULL + * on error.
Grammar: Returns a modified name that the caller has to free, or NULL on error.
+ */ +static char *virDomainSafeConsoleLockFilePath(const char *pty) +{ + char *path = NULL; + char *sanitizedPath = NULL; + char *ptyCopy; + char *filename; + char *p; + + if (!(ptyCopy = strdup(pty))) { + virReportOOMError(); + goto cleanup; + } + + if ((filename = strstr(ptyCopy, "/dev/"))) {
Do you want to find /dev anywhere within the name, or only skip it at the beginning?
+ filename += 5; /* skip /dev/ */ + p = filename; + /* substitute path forward slashes for underscores */ + while (*p) { + if (*p == '/') + *p = '_'; + ++p;
Do you want this slash normalization to only occur if /dev/ was in the name, or shouldn't you be doing it always?
+ } + } + + if (virAsprintf(&path, "%s/LCK..%s", VIR_PTY_LOCK_FILE_PATH, filename) < 0) + goto cleanup;
That is, we converted /dev/pts/3 into /var/lock/LCK..pts_3 - and that looks correct.
+/** + * Verify and create a lock file for a console pty + * + * @pty Path of the console device + * + * Returns 0 on success, -1 on error + */ +static int virDomainSafeConsoleLockFileCreate(const char *pty) +{ + char *path = NULL; + int ret = -1; + int lockfd = -1; + char *pidStr = NULL; + int pid; + ssize_t read_n; + /* build lock file path */ + if (!(path = virDomainSafeConsoleLockFilePath(pty))) + goto cleanup; + + /* check if lock file exists */ + if ((lockfd = open(path, O_RDONLY)) >= 0) { + if ((read_n = virFileReadLimFD(lockfd, 100, &pidStr) > 0) && + (sscanf(pidStr, "%d", &pid) == 1)) {
sscanf() is too heavyweight. Use virStrToLong_i().
+ /* lock file is valid, let's check if the pid is alive */ + if (pid > 1 && kill((pid_t) pid, 0) == 0) {
Can you reuse anything from virpidfile.h here, rather than rolling your own pid file reader? In particular, kill() isn't portable to mingw, so reusing code would automatically make your code more likely to compile. Hmm - virPidFileReadPathIfAlive() is too strong - it requires that you specify which executable should own the pid. But virPidFileReadPath() is too weak - it reads the pid without checking if that pid is still alive. It might be worth adding an intermediate function, or else teaching virPidFileReadPathIfAlive to skip the probe of /proc/PID/exe if pidfile is NULL (do that as a separate pre-req patch).
+ /* lock file is valid and a process with pid corresponding + * to the pid stored in the lock file exists */ + consoleReportError(VIR_ERR_OPERATION_FAILED, + _("Requested console pty '%s' is locked by " + "lock file '%s' held by process %d"), + pty, path, pid); + goto cleanup; + } + /* lock file is stale */ + VIR_DEBUG("Cleaning up stale lockfile '%s'", path); + VIR_FORCE_CLOSE(lockfd); + unlink(path); + } else { + /* lock file is corrupted */ + VIR_DEBUG("Cleaning up corrupted lockfile '%s'", path); + VIR_FORCE_CLOSE(lockfd); + unlink(path); + /* this might actualy fail, but we get an error while
s/actualy/actually/
+ * creating a new lockfile. Let the user handle the mess. */ + } + VIR_FREE(pidStr); + } + /* lockfile doesn't (shouldn't) exist */ + + /* ensure correct format according to filesystem hierarchy standard */ + if (virAsprintf(&pidStr, "%10d\n", (int) getpid()) < 0)
Here, calling out the URL in your comment might help: http://www.pathname.com/fhs/pub/fhs-2.3.html#VARLOCKLOCKFILES
+ goto cleanup; + + /* create the lock file */ + if ((lockfd = open(path, O_WRONLY | O_CREAT | O_EXCL, 00644)) < 0) { + /* If we run in session mode, we might have no access to the lock + * file directory. We have to check for an permission denied error + * and see if we can reach it. This should cause an error only if + * we run in daemon mode and thus privileged. + */ + if (errno == EACCES && geteuid() != 0) { + VIR_DEBUG("Skipping lock file creation for pty '%s in path '%s'.", + pty, path); + ret = 0; + goto cleanup; + } + virReportSystemError(errno, + _("Couldn't create lock file for " + "pty '%s' in path '%s'"), + pty, path); + goto cleanup; + } + + /* write the pid to the file */ + if (safewrite(lockfd, pidStr, strlen(pidStr)) < 0) { + virReportSystemError(errno, + _("Couldn't write to lock file for " + "pty '%s' in path '%s'"), + pty, path); + VIR_FORCE_CLOSE(lockfd); + unlink(path); + goto cleanup; + } + + /* we hold the lock */ + ret = 0; + +cleanup: + VIR_FORCE_CLOSE(lockfd); + VIR_FREE(path); + VIR_FREE(pidStr); + + return ret;
Complex, but looks right.
+} + +/** + * Remove a lock file for a pty + * + * @pty Path of the pty device + */ +static void virDomainSafeConsoleLockFileRemove(const char *pty) +{ + char *path = virDomainSafeConsoleLockFilePath(pty); + if (path) + unlink(path); + VIR_FREE(path); +} +#else /* #ifdef VIR_PTY_LOCK_FILE_PATH */ +/* file locking for console devices is disabled */ +static int virDomainSafeConsoleLockFileCreate(const char *pty ATTRIBUTE_UNUSED) +{ + return 0;
I'm not sure whether it makes sense to log a message that we are skipping a lock file, due to no configured directory. On one hand, it's just noise, if you specifically configured things to avoid the directory. On the other hand, we warned the user about qemu:///session failing to grab a lock due to EACCES, and this is failure to grab a lock to to configuration.
+} + +static void virDomainSafeConsoleLockFileRemove(const char *py ATTRIBUTE_UNUSED)
s/py/pty/
+{ + return; +} +#endif /* #ifdef VIR_PTY_LOCK_FILE_PATH */ + +/** + * Frees an entry from the hash containing domain's active consoles + * + * @data Opaque data, struct holding informations about the console
s/informations/information/
+/** + * Open a console stream for a domain ensuring that other streams are + * not using the console, nor any lockfiles exist. This ensuers that
s/ensuers/ensures/
+ * the console stream does not get corrupted due to a race on reading + * same FD by two processes. + * + * @cons Pointer to private structure holding data about console streams. + * @pty Path to the pseudo tty to be opened. + * @st Stream the client wishes to use for the console connection. + * @force On true, close active console streams for the selected console pty + * before opening this connection. + * + * Returns 0 on success and st is connected to the selected pty and + * corresponding lock file is created (if configured with). Returns -1 on
s/if configured with/if configured/
+ * error and 1 if the console stream is open and busy. + */ +int virDomainSafeConsoleOpen(virDomainSafeConsolesPtr cons, + const char *pty, + virStreamPtr st, + bool force) +{ + virDomainSafeConsoleStreamInfoPtr cbdata = NULL; + virStreamPtr savedStream; + int ret; + + virMutexLock(&cons->lock); + + if ((savedStream = virHashLookup(cons->hash, pty))) { + if (!force) { + /* entry found, console is busy */ + virMutexUnlock(&cons->lock); + return 1; + } else { + /* terminate existing connection */ + virFDStreamSetInternalCloseCb(savedStream, NULL, NULL, NULL); + virStreamAbort(savedStream); + virHashRemoveEntry(cons->hash, pty); + /* continue adding a new stream connection */ + } + } + + /* create the lock file */ + if ((ret = virDomainSafeConsoleLockFileCreate(pty)) < 0) { + virMutexUnlock(&cons->lock); + return ret; + } + + /* obtain a reference to the stream */ + if (virStreamRef(st) < 0) { + virMutexUnlock(&cons->lock); + return -1; + }
If we get here, we added a reference to st,
+ + if (VIR_ALLOC(cbdata) < 0) + goto error; + + if (virHashAddEntry(cons->hash, pty, st) < 0) + goto error; + + savedStream = st; + st = NULL;
but here, st is set to NULL,
+ + cbdata->cons = cons; + if (!(cbdata->pty = strdup(pty))) + goto error;
and an OOM here,
+ + /* open the console pty */ + if (virFDStreamOpenFile(savedStream, pty, 0, 0, O_RDWR) < 0) + goto error; + + + /* add cleanup callback */ + virFDStreamSetInternalCloseCb(savedStream, + virDomainSafeConsoleFDStreamCloseCb, + cbdata, + virDomainSafeConsoleFDStreamCloseCbFree); + cbdata = NULL; + + virMutexUnlock(&cons->lock); + return 0; + +error: + virStreamFree(st);
means that we never remove our reference. You've created a resource leak on OOM. I'd rearrange the code to sink the swap from st over to savedStream to occur later on, when we know we are successful.
+ virHashRemoveEntry(cons->hash, pty); + if (cbdata) + VIR_FREE(cbdata->pty); + VIR_FREE(cbdata); + virMutexUnlock(&cons->lock); + return -1; +} diff --git a/src/util/domain_safe_console.h b/src/util/domain_safe_console.h new file mode 100644 index 0000000..39536f2 --- /dev/null +++ b/src/util/domain_safe_console.h @@ -0,0 +1,28 @@ +/** + * domain_console_lock.h: api to guarantee mutualy exclusive + * access to domain consoles + * + * Copyright (C) 2011 Red Hat, Inc. + * + * See COPYING.LIB for the License of this software
Same comments as for the .c file (2012, 3-paragraph form). Definitely some things to fix in v3, but looking like a decent start. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 01/18/2012 12:38 AM, Eric Blake wrote:
On 12/07/2011 11:08 AM, Peter Krempa wrote:
This patch adds a set of functions used in creating console streams for domains using PTYs and ensures mutualy exculsive access to the PTYs.
Picking up on my (stalled) review...
+++ b/src/Makefile.am @@ -101,6 +101,9 @@ UTIL_SOURCES = \ util/virsocketaddr.h util/virsocketaddr.c \ util/virtime.h util/virtime.c
+SAFE_CONSOLE_SOURCES = \ + util/domain_safe_console.c util/domain_safe_console.h
Unless you are planning on including these sources into an independent library, I see no need to make a new variable. Just add these files into the right place within UTIL_SOURCES.
This is a little bit tricky. My first approach was to add it to UTIL_SOURCES, but then I got errors about missing references (This code needs references to streams and other stuff from libvirt.[c|h].) while building the LXC helper tool. This works it around, as the lxc app does not get built with these sources, and the util lib does. Do you have any suggestions, how to work around getting errors from the lxc app? I'd rather use the UTIL_SOURCES var. if possible. Thanks, Peter
Those are some rather long names. Also, our convention for new files has lately been to go with vir<name>.[ch], with APIs starting with vir<Name>...(). I'm thinking that this patch may be better as:
virconsole.[ch] virConsoleAlloc() virConsoleFree() virConsoleOpen()
That is, the name Domain doesn't add anything (we might want a console for other reasons) and the name Safe is redundant (the whole point of adding a virName API is to make the Name action safer and easier to use).
+ EXTRA_DIST += $(srcdir)/util/virkeymaps.h $(srcdir)/util/keymaps.csv \ $(srcdir)/util/virkeycode-mapgen.py
@@ -555,7 +558,7 @@ noinst_LTLIBRARIES = libvirt_util.la libvirt_la_LIBADD = $(libvirt_la_BUILT_LIBADD) libvirt_la_BUILT_LIBADD = libvirt_util.la libvirt_util_la_SOURCES = \ - $(UTIL_SOURCES) + $(UTIL_SOURCES) $(SAFE_CONSOLE_SOURCES)
If you add the new files directly to UTIL_SOURCES, then you don't need to edit this line.
On Wed, Jan 18, 2012 at 02:47:13PM +0100, Peter Krempa wrote:
On 01/18/2012 12:38 AM, Eric Blake wrote:
On 12/07/2011 11:08 AM, Peter Krempa wrote:
This patch adds a set of functions used in creating console streams for domains using PTYs and ensures mutualy exculsive access to the PTYs.
Picking up on my (stalled) review...
+++ b/src/Makefile.am @@ -101,6 +101,9 @@ UTIL_SOURCES = \ util/virsocketaddr.h util/virsocketaddr.c \ util/virtime.h util/virtime.c
+SAFE_CONSOLE_SOURCES = \ + util/domain_safe_console.c util/domain_safe_console.h
Unless you are planning on including these sources into an independent library, I see no need to make a new variable. Just add these files into the right place within UTIL_SOURCES.
This is a little bit tricky. My first approach was to add it to UTIL_SOURCES, but then I got errors about missing references (This code needs references to streams and other stuff from libvirt.[c|h].) while building the LXC helper tool. This works it around, as the lxc app does not get built with these sources, and the util lib does.
Do you have any suggestions, how to work around getting errors from the lxc app? I'd rather use the UTIL_SOURCES var. if possible.
This is probably pushing the definition of what should go in util. I'd put it in src/conf, where we put other domain helper stuff like the events code. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
This patch fixes console corruption, that happens if two concurrent sessions are opened for a single console on a domain. Result of this corruption was, that each of the console streams did recieve just a part of the data written to the pipe so every console rendered unusable. New helper function for safe console handling is used to establis the console stream connection. This function ensurest that no other libvirt client is using the console (with the ability to disconnect consoles of libvirt clients) and that no UUCP style lockfile is placed on the PTY device. * src/qemu/qemu_domain.h - add data structure to domain's private data dealing with console connections * src/qemu/qemu_domain.c: - allocate/free domain's console data structure * src/qemu/qemu_driver.c - use the new helper function for console handling --- src/qemu/qemu_domain.c | 5 +++++ src/qemu/qemu_domain.h | 3 +++ src/qemu/qemu_driver.c | 21 ++++++++++++++++----- 3 files changed, 24 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index b28c734..08d30ab 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -217,6 +217,9 @@ static void *qemuDomainObjPrivateAlloc(void) if (qemuDomainObjInitJob(priv) < 0) goto error; + if (!(priv->cons = virDomainSafeConsoleAlloc())) + goto error; + priv->migMaxBandwidth = QEMU_DOMAIN_DEFAULT_MIG_BANDWIDTH_MAX; return priv; @@ -239,6 +242,8 @@ static void qemuDomainObjPrivateFree(void *data) VIR_FREE(priv->lockState); VIR_FREE(priv->origname); + virDomainSafeConsoleFree(priv->cons); + /* This should never be non-NULL if we get here, but just in case... */ if (priv->mon) { VIR_ERROR(_("Unexpected QEMU monitor still active during domain deletion")); diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index e8bcab3..88ddb2f 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -29,6 +29,7 @@ # include "qemu_monitor.h" # include "qemu_conf.h" # include "bitmap.h" +# include "domain_safe_console.h" # define QEMU_EXPECTED_VIRT_TYPES \ ((1 << VIR_DOMAIN_VIRT_QEMU) | \ @@ -127,6 +128,8 @@ struct _qemuDomainObjPrivate { unsigned long migMaxBandwidth; char *origname; + + virDomainSafeConsolesPtr cons; }; struct qemuDomainWatchdogEvent diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 1e5ed9a..608a2ca 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -10847,8 +10847,10 @@ qemuDomainOpenConsole(virDomainPtr dom, int ret = -1; int i; virDomainChrDefPtr chr = NULL; + qemuDomainObjPrivatePtr priv; - virCheckFlags(0, -1); + virCheckFlags(VIR_DOMAIN_CONSOLE_TRY | + VIR_DOMAIN_CONSOLE_FORCE, -1); qemuDriverLock(driver); virUUIDFormat(dom->uuid, uuidstr); @@ -10865,6 +10867,8 @@ qemuDomainOpenConsole(virDomainPtr dom, goto cleanup; } + priv = vm->privateData; + if (dev_name) { for (i = 0 ; !chr && i < vm->def->nconsoles ; i++) { if (vm->def->consoles[i]->info.alias && @@ -10900,11 +10904,18 @@ qemuDomainOpenConsole(virDomainPtr dom, goto cleanup; } - if (virFDStreamOpenFile(st, chr->source.data.file.path, - 0, 0, O_RDWR) < 0) - goto cleanup; + /* handle mutualy exclusive access to console devices */ + ret = virDomainSafeConsoleOpen(priv->cons, + chr->source.data.file.path, + st, + (flags & VIR_DOMAIN_CONSOLE_FORCE) != 0); + + if (ret == 1) { + qemuReportError(VIR_ERR_OPERATION_FAILED, + _("Active console session exists for this domain")); + ret = -1; + } - ret = 0; cleanup: if (vm) virDomainObjUnlock(vm); -- 1.7.3.4
On 12/07/2011 11:08 AM, Peter Krempa wrote:
This patch fixes console corruption, that happens if two concurrent sessions are opened for a single console on a domain. Result of this corruption was, that each of the console streams did recieve just a part
s/was, that/was that/ s/did recieve/received/
of the data written to the pipe so every console rendered unusable.
s/pipe so every console/pipe, and both sessions were/
New helper function for safe console handling is used to establis the
s/establis/establish/
console stream connection. This function ensurest that no other libvirt
s/ensurest/ensures/
client is using the console (with the ability to disconnect consoles of libvirt clients) and that no UUCP style lockfile is placed on the PTY device.
* src/qemu/qemu_domain.h - add data structure to domain's private data dealing with console connections * src/qemu/qemu_domain.c: - allocate/free domain's console data structure * src/qemu/qemu_driver.c - use the new helper function for console handling --- src/qemu/qemu_domain.c | 5 +++++ src/qemu/qemu_domain.h | 3 +++ src/qemu/qemu_driver.c | 21 ++++++++++++++++----- 3 files changed, 24 insertions(+), 5 deletions(-)
+++ b/src/qemu/qemu_driver.c @@ -10847,8 +10847,10 @@ qemuDomainOpenConsole(virDomainPtr dom, int ret = -1; int i; virDomainChrDefPtr chr = NULL; + qemuDomainObjPrivatePtr priv;
- virCheckFlags(0, -1); + virCheckFlags(VIR_DOMAIN_CONSOLE_TRY | + VIR_DOMAIN_CONSOLE_FORCE, -1);
Failed to compile, if you applied my proposed fixups to 1/6 (https://www.redhat.com/archives/libvir-list/2011-December/msg00624.html): qemu/qemu_driver.c:11284:50: error: 'VIR_DOMAIN_CONSOLE_TRY' undeclared (first use in this function)
@@ -10900,11 +10904,18 @@ qemuDomainOpenConsole(virDomainPtr dom, goto cleanup; }
- if (virFDStreamOpenFile(st, chr->source.data.file.path, - 0, 0, O_RDWR) < 0) - goto cleanup; + /* handle mutualy exclusive access to console devices */
s/mutualy/mutually/
+ ret = virDomainSafeConsoleOpen(priv->cons, + chr->source.data.file.path, + st, + (flags & VIR_DOMAIN_CONSOLE_FORCE) != 0); + + if (ret == 1) { + qemuReportError(VIR_ERR_OPERATION_FAILED, + _("Active console session exists for this domain")); + ret = -1; + }
- ret = 0;
Looks simple, compared to the bulk of the work in patch 5/6. Here's what I would squash in, as part of rebasing the series to latest. diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c index 0f358df..beffdf5 100644 --- i/src/qemu/qemu_driver.c +++ w/src/qemu/qemu_driver.c @@ -11281,8 +11281,7 @@ qemuDomainOpenConsole(virDomainPtr dom, virDomainChrDefPtr chr = NULL; qemuDomainObjPrivatePtr priv; - virCheckFlags(VIR_DOMAIN_CONSOLE_TRY | - VIR_DOMAIN_CONSOLE_FORCE, -1); + virCheckFlags(VIR_DOMAIN_CONSOLE_FORCE, -1); qemuDriverLock(driver); virUUIDFormat(dom->uuid, uuidstr); -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Wed, Dec 07, 2011 at 07:08:00PM +0100, Peter Krempa wrote:
This series fixes anoying console corruption if two clients try to connect at same time to the console. The current state of this is, that two/more of threads compete for the data from the PTY. This causes that each of the consoles get scrambled and unusable.
These patches add mutual exclusion for opening consoles with two different approaches and a option to terminate existing console streams.
A sample implementation is done using qemu driver, but i'll add more of them if this will be OK. (They're basicaly the same as in qemu).
For convinience, to review these patches:
git checkout -b console_corruption 8d16201fe0e63afb5416a8eb7c6478f582ccccc0 git pull git://aeon.pipo.sk/libvirt.git console_dup
What is the status of these patches ? Do you have any v3 pending ? In the libvirt-sandbox project, we're finding that we need this console protection you've done, to stop virt-manager intefering with the sandboxes Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 01/17/2012 02:47 PM, Daniel P. Berrange wrote:
On Wed, Dec 07, 2011 at 07:08:00PM +0100, Peter Krempa wrote:
This series fixes anoying console corruption if two clients try to connect at same time to the console. The current state of this is, that two/more of threads compete for the data from the PTY. This causes that each of the consoles get scrambled and unusable.
These patches add mutual exclusion for opening consoles with two different approaches and a option to terminate existing console streams.
A sample implementation is done using qemu driver, but i'll add more of them if this will be OK. (They're basicaly the same as in qemu).
For convinience, to review these patches:
git checkout -b console_corruption 8d16201fe0e63afb5416a8eb7c6478f582ccccc0 git pull git://aeon.pipo.sk/libvirt.git console_dup
What is the status of these patches ? Do you have any v3 pending ?
I kind of stalled on my review; I'll finish that up, and hope to see a v3 soon. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 01/17/2012 10:47 PM, Daniel P. Berrange wrote:
What is the status of these patches ? Do you have any v3 pending ?
In the libvirt-sandbox project, we're finding that we need this console protection you've done, to stop virt-manager intefering with the sandboxes
Now that the review on v2 is finished, I'll try to post a v3 ASAP. Peter
Daniel
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Peter Krempa