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@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