[libvirt] [PATCH v3 0/7] Console corruption patchset

This is the third version of this patchset, rebased, polisehd and tweaked after Eric's review. This series contains one new patch that enables reuse of code in patches later on. The qemu driver implementation of console handling is very similar to LXC's implementation, so porting this functionality to LXC should be trivial and I'll post a follow-up patch when the qemu's driver will be ok. Peter Krempa (7): pidfile: Make checking binary path in virPidFileRead optional Add flags for virDomainOpenConsole virsh: add support for VIR_DOMAIN_CONSOLE_* flags 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 | 39 ++++- include/libvirt/libvirt.h.in | 12 ++ po/POTFILES.in | 1 + src/Makefile.am | 6 +- src/conf/virconsole.c | 396 ++++++++++++++++++++++++++++++++++++++++++ src/conf/virconsole.h | 36 ++++ src/fdstream.c | 89 +++++++++- src/fdstream.h | 11 ++ src/libvirt.c | 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/virpidfile.c | 21 ++- tools/console.c | 5 +- tools/console.h | 3 +- tools/virsh.c | 24 ++- tools/virsh.pod | 8 +- 18 files changed, 660 insertions(+), 37 deletions(-) create mode 100644 src/conf/virconsole.c create mode 100644 src/conf/virconsole.h -- 1.7.3.4

This patch changes behavior of virPidFileRead to enable passing NULL as path to the binary the pid file should be checked against to skip this check. This enables using this function for reading files that have same semantics as pid files, but belong to unknown processess. --- This patch is new in the series. src/util/virpidfile.c | 21 +++++++++++++-------- 1 files changed, 13 insertions(+), 8 deletions(-) diff --git a/src/util/virpidfile.c b/src/util/virpidfile.c index 1fd6318..f1f721f 100644 --- a/src/util/virpidfile.c +++ b/src/util/virpidfile.c @@ -184,6 +184,9 @@ int virPidFileRead(const char *dir, * resolves to @binpath. This adds protection against * recycling of previously reaped pids. * + * If @binpath is NULL the check for the executable path + * is skipped. + * * Returns -errno upon error, or zero on successful * reading of the pidfile. If the PID was not still * alive, zero will be returned, but @pid will be @@ -209,16 +212,18 @@ int virPidFileReadPathIfAlive(const char *path, } #endif - if (virAsprintf(&procpath, "/proc/%d/exe", *pid) < 0) { - *pid = -1; - return -1; - } + if (binpath) { + if (virAsprintf(&procpath, "/proc/%d/exe", *pid) < 0) { + *pid = -1; + return -1; + } - if (virFileIsLink(procpath) && - virFileLinkPointsTo(procpath, binpath) == 0) - *pid = -1; + if (virFileIsLink(procpath) && + virFileLinkPointsTo(procpath, binpath) == 0) + *pid = -1; - VIR_FREE(procpath); + VIR_FREE(procpath); + } return 0; } -- 1.7.3.4

On 01/26/2012 10:16 AM, Peter Krempa wrote:
This patch changes behavior of virPidFileRead to enable passing NULL as path to the binary the pid file should be checked against to skip this check. This enables using this function for reading files that have same semantics as pid files, but belong to unknown processess.
s/processess/processes/ ACK. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

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_SAFE - specifies that the console connection should be opened only if the hypervisor supports mutualy exclusive access to console devices VIR_DOMAIN_CONSOLE_FORCE - specifies that the caller wishes to interrupt existing session and force a creation of a new one. --- Diff to v2: - added missing documentation - fixed version - added flag VIR_DOMAIN_CONSOLE_SAFE that denotes usage of safe console include/libvirt/libvirt.h.in | 12 ++++++++++++ src/libvirt.c | 11 ++++++++++- 2 files changed, 22 insertions(+), 1 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index e99cd00..10c2d8f 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -3642,7 +3642,19 @@ int virNWFilterGetUUIDString (virNWFilterPtr nwfilter, char *buf); char * virNWFilterGetXMLDesc (virNWFilterPtr nwfilter, unsigned int flags); +/** + * virDomainConsoleFlags + * + * Since 0.9.10 + */ +typedef enum { + VIR_DOMAIN_CONSOLE_FORCE = (1 << 0), /* abort a (possibly) active console + connection to force a new + connection*/ + VIR_DOMAIN_CONSOLE_SAFE = (1 << 1), /* check if the console driver supports + safe console operations */ +} virDomainConsoleFlags; int virDomainOpenConsole(virDomainPtr dom, const char *devname, diff --git a/src/libvirt.c b/src/libvirt.c index 8be4e13..0d89f1b 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -17380,7 +17380,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: extra flags; not used yet, so callers should always 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. @@ -17389,6 +17389,15 @@ 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. + * + * If flag VIR_DOMAIN_CONSOLE_SAFE the console is opened only in the + * case the hypervisor driver supports safe (mutually exclusive) + * console handling. + * * returns 0 if the console was opened, -1 on error */ int virDomainOpenConsole(virDomainPtr dom, -- 1.7.3.4

On 01/26/2012 10:16 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_SAFE - specifies that the console connection should be opened only if the hypervisor supports mutualy exclusive access to console devices
s/mutualy/mutually/
VIR_DOMAIN_CONSOLE_FORCE - specifies that the caller wishes to interrupt existing session and force a creation of a new one. --- Diff to v2: - added missing documentation - fixed version - added flag VIR_DOMAIN_CONSOLE_SAFE that denotes usage of safe console
+/** + * virDomainConsoleFlags + * + * Since 0.9.10 + */ +typedef enum {
+ VIR_DOMAIN_CONSOLE_FORCE = (1 << 0), /* abort a (possibly) active console + connection to force a new + connection*/
Space before */
+ VIR_DOMAIN_CONSOLE_SAFE = (1 << 1), /* check if the console driver supports + safe console operations */ +} virDomainConsoleFlags;
int virDomainOpenConsole(virDomainPtr dom, const char *devname, diff --git a/src/libvirt.c b/src/libvirt.c index 8be4e13..0d89f1b 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -17380,7 +17380,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: extra flags; not used yet, so callers should always pass 0 + * @flags: bit-wise OR of virDomainConsoleFlags
s/bit-wise OR/bitwise-OR/ for consistency with other lines
* * This opens the backend associated with a console, serial or * parallel port device on a guest, if the backend is supported. @@ -17389,6 +17389,15 @@ 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. + * + * If flag VIR_DOMAIN_CONSOLE_SAFE the console is opened only in the + * case the hypervisor driver supports safe (mutually exclusive)
s/case the/case where the/
+ * console handling.
Maybe add: Older servers did not support either flag, and also did not forbid simultaneous clients on a console, with potentially confusing results. When passing @flags of 0 in order to support a wider range of server versions, it is up to the client to ensure mutual exclusion. ACK. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

This patch adds support for the newly introduced VIR_DOMAIN_CONSOLE_FORCE and VIR_DOMAIN_CONSOLE_SAFE flags. 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. Flag --safe requests that the console should be opened only if the hypervisor driver supports safe console handling. The behaviour to this point was that the daemon opened 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 --- Diff to v2: - tuned spellings - broken long lines - changed extension-unfriendly code - added support for VIR_DOMAIN_CONSOLE_SAFE flag tools/console.c | 5 +++-- tools/console.h | 3 ++- tools/virsh.c | 24 +++++++++++++++++++----- tools/virsh.pod | 8 +++++++- 4 files changed, 31 insertions(+), 9 deletions(-) diff --git a/tools/console.c b/tools/console.c index 4a32522..ca226c3 100644 --- a/tools/console.c +++ b/tools/console.c @@ -299,7 +299,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; @@ -353,7 +354,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 74655c2..6d4b4f8 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -815,11 +815,17 @@ 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)")}, + {"safe", VSH_OT_BOOL, 0, + N_("only connect if safe console handling is supported")}, {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; @@ -836,7 +842,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: @@ -849,6 +855,9 @@ cmdConsole(vshControl *ctl, const vshCmd *cmd) { virDomainPtr dom; bool ret = false; + bool force = vshCommandOptBool(cmd, "force"); + bool safe = vshCommandOptBool(cmd, "safe"); + unsigned int flags = 0; const char *name = NULL; if (!vshConnectionUsability(ctl, ctl->conn)) @@ -862,7 +871,12 @@ cmdConsole(vshControl *ctl, const vshCmd *cmd) goto cleanup; } - ret = cmdRunConsole(ctl, dom, name); + if (force) + flags |= VIR_DOMAIN_CONSOLE_FORCE; + if (safe) + flags |= VIR_DOMAIN_CONSOLE_SAFE; + + ret = cmdRunConsole(ctl, dom, name, flags); cleanup: virDomainFree(dom); @@ -2222,7 +2236,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 { @@ -2725,7 +2739,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 diff --git a/tools/virsh.pod b/tools/virsh.pod index 8599f66..7859f43 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -396,13 +396,19 @@ Configure a domain to be automatically started at boot. The option I<--disable> disables autostarting. -=item B<console> I<domain-id> [I<devname>] +=item B<console> I<domain-id> [I<devname>] [I<--safe>] [I<--force>] Connect the virtual serial console for the guest. The optional I<devname> parameter refers to the device alias of an alternate console, serial or parallel device configured for the guest. If omitted, the primary console will be opened. +If the flag I<--safe> is specified, the connection is only attempted +if the driver supports safe console handling. This flag specifies, that +the server has to ensure exclusive access to console devices. Optionally +the I<--force> flag may be specified, requesting to disconnect any existing +sessions. E.g in a case of broken connection. + =item B<create> I<FILE> [I<--console>] [I<--paused>] [I<--autodestroy>] Create a domain from an XML <file>. An easy way to create the XML -- 1.7.3.4

On 01/26/2012 10:16 AM, Peter Krempa wrote:
This patch adds support for the newly introduced VIR_DOMAIN_CONSOLE_FORCE and VIR_DOMAIN_CONSOLE_SAFE flags. 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. Flag --safe requests that the console should be opened only if the hypervisor driver supports safe console handling.
The behaviour to this point was that the daemon opened 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
What you have is good, but you should also add --safe and --force to 'virsh start --console' and 'virsh create --console'. Hmm, for virsh start, naming it --force might be risky since we already have --force-boot; there, I might go --force-console. It also means that if we ever add unambiguous prefix option parsing, then --force would be ambiguous; oh well.
+++ b/tools/virsh.pod @@ -396,13 +396,19 @@ Configure a domain to be automatically started at boot.
The option I<--disable> disables autostarting.
-=item B<console> I<domain-id> [I<devname>] +=item B<console> I<domain-id> [I<devname>] [I<--safe>] [I<--force>]
Connect the virtual serial console for the guest. The optional I<devname> parameter refers to the device alias of an alternate console, serial or parallel device configured for the guest. If omitted, the primary console will be opened.
+If the flag I<--safe> is specified, the connection is only attempted +if the driver supports safe console handling. This flag specifies, that
s/specifies, that/specifies that/
+the server has to ensure exclusive access to console devices. Optionally +the I<--force> flag may be specified, requesting to disconnect any existing +sessions. E.g in a case of broken connection.
s/sessions. E.g in/sessions, such as in/ /of broken/of a broken/ ACK. We can enhance 'start' and 'create' in a separate patch. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 02/02/2012 01:43 AM, Eric Blake wrote:
On 01/26/2012 10:16 AM, Peter Krempa wrote:
This patch adds support for the newly introduced VIR_DOMAIN_CONSOLE_FORCE and VIR_DOMAIN_CONSOLE_SAFE flags. 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. Flag --safe requests that the console should be opened only if the hypervisor driver supports safe console handling.
The behaviour to this point was that the daemon opened 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
What you have is good, but you should also add --safe and --force to 'virsh start --console' and 'virsh create --console'. Hmm, for virsh start, naming it --force might be risky since we already have --force-boot; there, I might go --force-console. It also means that if we ever add unambiguous prefix option parsing, then --force would be ambiguous; oh well.
Well, I thought that for those commands the --safe and --force (especialy force) do not make sense and therefore I didn't implement them. When the domain is stopped there's only a minimal chance that someone might create a console session before the virsh command manages to open it. The --safe comand might be relevant to guarantee that the console doesn't get messed up lated. Do you think we need the --force (I'm ok with renaming it also in the console command)? Peter

On 02/02/2012 04:04 AM, Peter Krempa wrote:
What you have is good, but you should also add --safe and --force to 'virsh start --console' and 'virsh create --console'. Hmm, for virsh start, naming it --force might be risky since we already have --force-boot; there, I might go --force-console. It also means that if we ever add unambiguous prefix option parsing, then --force would be ambiguous; oh well.
Well, I thought that for those commands the --safe and --force (especialy force) do not make sense and therefore I didn't implement them.
Oh, right - now that you mention it, it makes total sense - the act of starting a domain means that no one else can be using the console, and the race window between virsh.c's two calls to start and then grab the console is probably not worth worrying about. So, as a compromise - how about documenting in the commit message your rationale for not providing the new flags for start and create, and I withdraw my request for adding those flags.
When the domain is stopped there's only a minimal chance that someone might create a console session before the virsh command manages to open it. The --safe comand might be relevant to guarantee that the console doesn't get messed up lated.
I don't think we need it. It doesn't matter whether the first console is opened with 0 or with SAFE; either way, when second attempt is tried, the results will be the same: with 0, you get safe behavior with a new server and shared console with an old server; with SAFE, you get safe behavior with a new server and rejected command with an old server. Or put another way, we aren't maintaining state on whether SAFE was used on the original open, so there's no need for a flag to request extra state. -- 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 --- Diff to v2: - fixed typos 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

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 --- Diff to v2: - removed unnecessary whitespace deletion src/fdstream.c | 39 +++++++++++++++++++++++++++++++++++++-- src/fdstream.h | 11 +++++++++++ 2 files changed, 48 insertions(+), 2 deletions(-) diff --git a/src/fdstream.c b/src/fdstream.c index 35f5135..192a7c4 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; }; @@ -232,6 +238,7 @@ cleanup: return ret; } + static int virFDStreamCloseInt(virStreamPtr st, bool streamAbort) { @@ -251,7 +258,7 @@ virFDStreamCloseInt(virStreamPtr st, bool streamAbort) fdst->cb && (fdst->events & (VIR_STREAM_EVENT_READABLE | VIR_STREAM_EVENT_WRITABLE))) { - /* don't enter this function accidentaly from the callback again */ + /* don't enter this function accidentally from the callback again */ if (fdst->abortCallbackCalled) { virMutexUnlock(&fdst->lock); return 0; @@ -261,7 +268,7 @@ virFDStreamCloseInt(virStreamPtr st, bool streamAbort) fdst->abortCallbackDispatching = true; virMutexUnlock(&fdst->lock); - /* call failure callback, poll does report nothing on closed fd */ + /* call failure callback, poll reports nothing on closed fd */ (fdst->cb)(st, VIR_STREAM_EVENT_ERROR, fdst->opaque); virMutexLock(&fdst->lock); @@ -306,6 +313,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); @@ -680,3 +695,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

This patch adds a set of functions used in creating console streams for domains using PTYs and ensures mutually exclusive access to the PTYs. If mutualy exclusive access is not used, two clients may open the same console, which results in 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 internally 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 in an error. --- Diff to v2: - moved new files to src/conf/ - tweaked configure messages and output - renamed functions to shorter and nicer variants - fixed license and year - fixed typos and grammar mistakes - added use of virPidFileReadPathIfAlive to avoid redundant code - cleaned up other parts to cope with Eric's review configure.ac | 39 ++++- po/POTFILES.in | 1 + src/Makefile.am | 6 +- src/conf/virconsole.c | 396 ++++++++++++++++++++++++++++++++++++++++++++++ src/conf/virconsole.h | 36 ++++ src/libvirt_private.syms | 6 + 6 files changed, 475 insertions(+), 9 deletions(-) create mode 100644 src/conf/virconsole.c create mode 100644 src/conf/virconsole.h diff --git a/configure.ac b/configure.ac index 9fb7bfc..3254105 100644 --- a/configure.ac +++ b/configure.ac @@ -327,6 +327,12 @@ 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) + @<:@default=disabled@:>@]), + [],[with_console_lock_files=disabled]) dnl dnl in case someone want to build static binaries @@ -1197,6 +1203,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 containing UUCP pty lock files]) +fi +AM_CONDITIONAL([VIR_PTY_LOCK_FILE_PATH], [test "$with_console_lock_files" != "disabled"]) + dnl SELinux AC_ARG_WITH([selinux], @@ -2751,14 +2773,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 locks: $with_console_lock_files]) AC_MSG_NOTICE([]) AC_MSG_NOTICE([Privileges]) AC_MSG_NOTICE([]) diff --git a/po/POTFILES.in b/po/POTFILES.in index 0126320..d2c4e21 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -17,6 +17,7 @@ src/conf/nwfilter_params.c src/conf/secret_conf.c src/conf/storage_conf.c src/conf/storage_encryption_conf.c +src/conf/virconsole.c src/cpu/cpu.c src/cpu/cpu_generic.c src/cpu/cpu_map.c diff --git a/src/Makefile.am b/src/Makefile.am index a44446f..19aca7c 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -180,6 +180,9 @@ ENCRYPTION_CONF_SOURCES = \ CPU_CONF_SOURCES = \ conf/cpu_conf.c conf/cpu_conf.h +# Safe console handling helper APIs +CONSOLE_CONF_SOURCES = \ + conf/virconsole.c conf/virconsole.h CONF_SOURCES = \ $(NETDEV_CONF_SOURCES) \ @@ -192,7 +195,8 @@ CONF_SOURCES = \ $(ENCRYPTION_CONF_SOURCES) \ $(INTERFACE_CONF_SOURCES) \ $(SECRET_CONF_SOURCES) \ - $(CPU_CONF_SOURCES) + $(CPU_CONF_SOURCES) \ + $(CONSOLE_CONF_SOURCES) # The remote RPC driver, covering domains, storage, networks, etc REMOTE_DRIVER_GENERATED = \ diff --git a/src/conf/virconsole.c b/src/conf/virconsole.c new file mode 100644 index 0000000..f411293 --- /dev/null +++ b/src/conf/virconsole.c @@ -0,0 +1,396 @@ +/** + * virconsole.c: api to guarantee mutualy exclusive + * access to domain's consoles + * + * Copyright (C) 2011-2012 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + * + * Author: Peter Krempa <pkrempa@redhat.com> + */ + +#include <config.h> + +#include <fcntl.h> +#include <unistd.h> +#include <sys/types.h> + +#include "virconsole.h" +#include "hash.h" +#include "fdstream.h" +#include "internal.h" +#include "threads.h" +#include "memory.h" +#include "virpidfile.h" +#include "logging.h" +#include "virterror_internal.h" +#include "virfile.h" + +#define VIR_FROM_THIS VIR_FROM_NONE +#define virConsoleError(code, ...) \ + virReportErrorHelper(VIR_FROM_THIS, code, __FILE__, \ + __FUNCTION__, __LINE__, __VA_ARGS__) + +/* structure holding information about consoles + * open in a given domain */ +struct _virConsoles { + virMutex lock; + virHashTablePtr hash; +}; + +typedef struct _virConsoleStreamInfo virConsoleStreamInfo; +typedef virConsoleStreamInfo *virConsoleStreamInfoPtr; +struct _virConsoleStreamInfo { + virConsolesPtr 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 *virConsoleLockFilePath(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/ anywhere in the path*/ + + /* substitute path forward slashes for underscores */ + p = filename; + 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 virConsoleLockFileCreate(const char *pty) +{ + char *path = NULL; + int ret = -1; + int lockfd = -1; + char *pidStr = NULL; + int pid; + /* build lock file path */ + if (!(path = virConsoleLockFilePath(pty))) + goto cleanup; + + + + /* check if a log file and process holding the lock still exists */ + if (virPidFileReadPathIfAlive(path, &pid, NULL) == 0 && pid >= 0) { + /* the process exists, the lockfile is valid */ + virConsoleError(VIR_ERR_OPERATION_FAILED, + _("Requested console pty '%s' is locked by " + "lock file '%s' held by process %d"), + pty, path, pid); + goto cleanup; + } else { + /* clean up the stale/corrupted/nonexistent lockfile */ + unlink(path); + } + /* lockfile doesn't (shouldn't) exist */ + + /* ensure correct format according to filesystem hierarchy standard */ + /* http://www.pathname.com/fhs/pub/fhs-2.3.html#VARLOCKLOCKFILES */ + 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 virConsoleLockFileRemove(const char *pty) +{ + char *path = virConsoleLockFilePath(pty); + if (path) + unlink(path); + VIR_FREE(path); +} +#else /* #ifdef VIR_PTY_LOCK_FILE_PATH */ +/* file locking for console devices is disabled */ +static int virConsoleLockFileCreate(const char *pty ATTRIBUTE_UNUSED) +{ + return 0; +} + +static void virConsoleLockFileRemove(const char *pty 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 information about the console + * @name Path of the pty. + */ +static void virConsoleHashEntryFree(void *data, + const void *name) +{ + const char *pty = name; + virStreamPtr st = data; + + /* free stream reference */ + virStreamFree(st); + + /* delete lock file */ + virConsoleLockFileRemove(pty); +} + +/** + * Frees opaque data provided for the stream closing callback + * + * @opaque Data to be freed. + */ +static void virConsoleFDStreamCloseCbFree(void *opaque) +{ + virConsoleStreamInfoPtr 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 virConsoleFDStreamCloseCb(virStreamPtr st ATTRIBUTE_UNUSED, + void *opaque) +{ + virConsoleStreamInfoPtr 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 + */ +virConsolesPtr virConsoleAlloc(void) +{ + virConsolesPtr 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, virConsoleHashEntryFree))) + goto error; + + if (virMutexInit(&cons->lock) < 0) + goto error; + + return cons; +error: + virConsoleFree(cons); + return NULL; +} + +/** + * Free structures for handling open console streams. + * + * @cons Pointer to the private structure. + */ +void virConsoleFree(virConsolesPtr 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 ensures 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). Returns -1 on + * error and 1 if the console stream is open and busy. + */ +int virConsoleOpen(virConsolesPtr cons, + const char *pty, + virStreamPtr st, + bool force) +{ + virConsoleStreamInfoPtr 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 = virConsoleLockFileCreate(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; + + cbdata->cons = cons; + if (!(cbdata->pty = strdup(pty))) + goto error; + + /* open the console pty */ + if (virFDStreamOpenFile(st, pty, 0, 0, O_RDWR) < 0) + goto error; + + savedStream = st; + st = NULL; + + /* add cleanup callback */ + virFDStreamSetInternalCloseCb(savedStream, + virConsoleFDStreamCloseCb, + cbdata, + virConsoleFDStreamCloseCbFree); + 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/conf/virconsole.h b/src/conf/virconsole.h new file mode 100644 index 0000000..9f7fb04 --- /dev/null +++ b/src/conf/virconsole.h @@ -0,0 +1,36 @@ +/** + * virconsole.h: api to guarantee mutualy exclusive + * access to domain's consoles + * + * Copyright (C) 2011-2012 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + * + * Author: Peter Krempa <pkrempa@redhat.com> + */ +#ifndef __VIR_CONSOLE_H__ +# define __VIR_CONSOLE_H__ + +# include "internal.h" + +typedef struct _virConsoles virConsoles; +typedef virConsoles *virConsolesPtr; + +virConsolesPtr virConsoleAlloc(void); +void virConsoleFree(virConsolesPtr cons); + +int virConsoleOpen(virConsolesPtr cons, const char *pty, + virStreamPtr st, bool force); +#endif /*__VIR_DOMAIN_CONSOLE_LOCK_H__*/ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 924ec16..ff85ace 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1165,6 +1165,12 @@ virAuditOpen; virAuditSend; +# virconsole.h +virConsoleAlloc; +virConsoleFree; +virConsoleOpen; + + # virfile.h virFileClose; virFileDirectFdClose; -- 1.7.3.4

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 recieved 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 establish the console stream connection. This function ensures 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 --- Diff to v2: - fixed for new function and file names - added support for "secure" flag - tweaked spelling and grammar 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 d56e617..06d09dc 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -192,6 +192,9 @@ static void *qemuDomainObjPrivateAlloc(void) if (qemuDomainObjInitJob(priv) < 0) goto error; + if (!(priv->cons = virConsoleAlloc())) + goto error; + priv->migMaxBandwidth = QEMU_DOMAIN_DEFAULT_MIG_BANDWIDTH_MAX; return priv; @@ -214,6 +217,8 @@ static void qemuDomainObjPrivateFree(void *data) VIR_FREE(priv->lockState); VIR_FREE(priv->origname); + virConsoleFree(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 222d80e..1333d8c 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -30,6 +30,7 @@ # include "qemu_agent.h" # include "qemu_conf.h" # include "bitmap.h" +# include "virconsole.h" # define QEMU_EXPECTED_VIRT_TYPES \ ((1 << VIR_DOMAIN_VIRT_QEMU) | \ @@ -127,6 +128,8 @@ struct _qemuDomainObjPrivate { unsigned long migMaxBandwidth; char *origname; + + virConsolesPtr cons; }; struct qemuDomainWatchdogEvent diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index ab69dca..35d5414 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -11210,8 +11210,10 @@ qemuDomainOpenConsole(virDomainPtr dom, int ret = -1; int i; virDomainChrDefPtr chr = NULL; + qemuDomainObjPrivatePtr priv; - virCheckFlags(0, -1); + virCheckFlags(VIR_DOMAIN_CONSOLE_SAFE | + VIR_DOMAIN_CONSOLE_FORCE, -1); qemuDriverLock(driver); virUUIDFormat(dom->uuid, uuidstr); @@ -11228,6 +11230,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 && @@ -11263,11 +11267,18 @@ qemuDomainOpenConsole(virDomainPtr dom, goto cleanup; } - if (virFDStreamOpenFile(st, chr->source.data.file.path, - 0, 0, O_RDWR) < 0) - goto cleanup; + /* handle mutually exclusive access to console devices */ + ret = virConsoleOpen(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 01/26/2012 06:16 PM, Peter Krempa wrote:
This is the third version of this patchset, rebased, polisehd and tweaked after Eric's review.
This series contains one new patch that enables reuse of code in patches later on.
The qemu driver implementation of console handling is very similar to LXC's implementation, so porting this functionality to LXC should be trivial and I'll post a follow-up patch when the qemu's driver will be ok.
Ping? Is there somebody brave enough to do a review? :) Peter
participants (2)
-
Eric Blake
-
Peter Krempa