On 02/06/2012 06:50 AM, Peter Krempa wrote: > 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 s/mutualy/mutually/ > 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). Should we also be modifying libvirt.spec.in to enable console lock files when building the RPM for Fedora? > > 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. > --- > 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]) If I say ./configure --without-console-lock-files, then that defaults $with_console_lock_files=no. For consistency, I'd rather see the default be 'no', not 'disabled'; that way, you take advantage of autoconf's automatic --without-* support. Conversely, if I say ./configure --with-console-lock-files without an argument, autoconf defaults that to yes. I think you have to handle 'auto' and 'yes' in a similar manner, except the difference is that auto can gracefully degrade to 'no', while 'yes' errors out if we can't find a default location. > > 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 So here, I would use: if test "$with_console_lock_files" != no; then if test "$with_linux" = yes; then with_console_lock_files=/var/lock elif test "$with_console_lock_files = yes; then AC_MSG_ERROR([console lock files requested, but no path given]) elif test "$with_console_lock_files" = auto; then with_console_lock_files=no fi fi if test "$with_console_lock_files" != no; then AC_DEFINE_UNQUOTED([VIR_PTY_LOCK_FILE_PATH], ...) fi > +AM_CONDITIONAL([VIR_PTY_LOCK_FILE_PATH], [test "$with_console_lock_files" != "disabled"]) and again, I'd compare to 'no', not '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]) Rather than reformat everything, why not just call the new entry AC_MSG_NOTICE([Console locks: $with_console_lock_files]) so that you line up with te remaining lines and match the configure option name. > +++ b/src/conf/virconsole.c > @@ -0,0 +1,396 @@ > +/** > + * virconsole.c: api to guarantee mutualy exclusive s/mutualy/mutually/ > + * 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 We really ought to scrub our source tree (as an independent patch) to use the FSF's preferred LGPL header (they now list a web URL instead of a street address). > +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*/ That's dangerous - you are skipping the first 5 bytes, even if /dev/ occurred somewhere other than the first 5 bytes. I think you really only want to skip things if ptyCopy starts with, rather than contains, /dev. > + > + /* substitute path forward slashes for underscores */ > + p = filename; I would consider using: p = STRSKIP(ptyCopy, "/dev/"); if (!p) p = ptyCopy; filename = p; > +static int virConsoleLockFileCreate(const char *pty) > +{ > + char *path = NULL; > + int ret = -1; > + int lockfd = -1; > + char *pidStr = NULL; > + int pid; Shouldn't this be pid_t? > + /* build lock file path */ > + if (!(path = virConsoleLockFilePath(pty))) > + goto cleanup; > + > + > + Why so many blank lines? > + /* check if a log file and process holding the lock still exists */ double space. > + 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); You must use %lld and (long long) pid when printing pid_t values, for the sake of mingw64 (hmm, that reminds me - I have pending patches that need a review: https://www.redhat.com/archives/libvir-list/2012-February/msg00559.html) > + 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) Again, %10lld, and (long long) getpid(), for mingw64 (yes, a 64-bit pid will overflow 10 bytes, but that's only on systems that aren't compliant to the FHS in the first place). > +/** > + * 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; Bug - if you fail to init a mutex, then error will call virConsoleFree, which will then attempt to lock your uninit mutex. It is only safe to call virConsoleFree() on cleanup if the mutex was created. > + > + 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; Dead assignment, since you are freeing cons a few lines later. > +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); I'm not quite sure why you want to unregister any internal callback prior to aborting the stream. I"m not saying it's wrong, just that I didn't follow it. > + 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; It looks like this function is responsible for raising an error message on all failure paths, which means you are missing virReportOOMError() here. > + > + if (virHashAddEntry(cons->hash, pty, st) < 0) > + goto error; > + > + cbdata->cons = cons; > + if (!(cbdata->pty = strdup(pty))) > + goto error; and another virReportOOMError(). > +++ b/src/conf/virconsole.h > @@ -0,0 +1,36 @@ > +/** > + * virconsole.h: api to guarantee mutualy exclusive s/mutualy/mutually/ Probably worth a v5. -- Eric Blake eblake@xxxxxxxxxx +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list