[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v5 6/7] util: Add helpers for safe domain console operations



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

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]