[libvirt] [PATCH 3/4] lxc: validate container process during load config

[Daniel Veillard]

On Thu, May 29, 2008 at 03:20:15PM -0700, Dave Leskovec wrote:
> This patch adds a check that validates that the container process pid still
> exists.  This should catch cases where the container exits while libvirtd is down.

  sounds fine,

> +/**
> + * lxcCheckContainerProcess:
> + * @def: Ptr to VM definition
> + *
> + * Checks if the container process (stored at def->id is running
> + *
> + * Returns on success or -1 in case of error
> + * 0  - no process with id vm->def->id
> + * 1  - container process exists
> + * -1 - error
> + */
> +int lxcCheckContainerProcess(lxc_vm_def_t *def)
> +{
> +    int rc = -1;
> +
> +    if (1 < def->id) {
> +        if (-1 == kill(def->id, 0)) {

  hum i didn't know of that way to check for a process, cool

> +            if (ESRCH == errno) {
> +                rc = 0;
> +                DEBUG("pid %d no longer exists", def->id);
> +                goto done;
> +            }
> +
> +            lxcError(NULL, NULL, VIR_ERR_INTERNAL_ERROR,
> +                     _("error checking container process: %d %s"),
> +                     def->id, strerror(errno));
> +            goto done;
> +        }

  The problem though is that by doing just  a passive test for the PID
it feels like there is a possible race if the process counter rolled over and
another process with the same PID got create in the meantime.
  i have the feeling that a test based on the state of the file descriptors
used to communicate with the container would be more reliable. Basically if the
container disapear, then the pipe should get in a half-closed state, 
detecting the change at that level sounds like it would be more reliable,
don't you think so ?

  But as is the patch is still a good improvement, +1

Daniel

-- 
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard      | virtualization library  http://libvirt.org/
veillard at redhat.com  | libxml GNOME XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine  http://rpmfind.net/