On 02/23/2012 07:03 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.
Diff to v4: - fixed typos (traditionally) - fixed configure.ac to work with automaticaly used values - tweaked configure output line to line up with others without moving them - using STRSKIP to skip the start of the string instead of possibly skipping in the middle of a string - fixed whitespace problems - changed data type for pids to pid_t and tweaked printing formatters - On failure to initialise mutex in virConsoleAlloc don't skip to virConsoleFree - added comment to clarify why it's needed to unregister the callback handler - Added OOM error reporting on some error paths.
Looks better.
Note to v4 review: I changed the default value for the path of the lock files to "auto" so it will get built with "/var/lock" on linux machines, so no change to the spec file is needed.
auto should default to 'no' rather than error out on systems where we don't know.
+dnl UUCP style file locks for PTY consoles +if test "$with_console_lock_files" != "no"; then + if 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 + 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
I'd write this hunk as: if test "$with_console_lock_files" != "no"; then case $with_console_lock_files in yes | auto) dnl Default locations for platforms, or disable if unknown if test "$with_linux" = "yes"; then with_console_lock_files=/var/lock elif test "$with_console_lock_files" = "auto" with_console_lock_files=no fi;; esac if test "$with_console_lock_files" = "yes"; then AC_MSG_ERROR([You must specify path for the lock files on this platform]) fi
+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 */ + /* The internal close callback handler needs to lock cons->lock to + * remove the aborted stream from the hash. This would cause a + * deadlock as we would try to enter the lock twice from the very + * same thread. We need to unregister the callback and abort the + * stream manualy before we create a new console connection.
s/manualy/manually/ ACK with those changes. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org