[Findutils-patches] Re: [PATCH 2/4] Exec predicates now store which directory they want to run in.

[Eric Blake]

On 04/10/2010 02:33 PM, James Youngman wrote:
> (starting_dir): Remove devlaration of global variable.
> (starting_desc): Remove devlaration of global variable.

s/devlaration/declaration/g

> +
> +static bool
> +record_exec_dir (struct exec_val *execp)
> +{
> +  if (!execp->wd_for_exec)
> +    {
> +      /* working directory not already known, so must be a *dir variant,
> +      and this must be the first arg we added.   However, this may
> +      be -execdir foo {} \; (i.e. not multiple).  */
> +      assert (!execp->state.todo);
> +
> +      /* Record the WD. */
> +      execp->wd_for_exec = xmalloc (sizeof (*execp->wd_for_exec));
> +      execp->wd_for_exec->name = NULL;
> +      execp->wd_for_exec->desc = openat (state.cwd_dir_fd, ".", O_RDONLY);
> +      if (execp->wd_for_exec->desc < 0)
> +     return false;
> +      set_cloexec_flag (execp->wd_for_exec->desc, true);

Yet another reason for me to spend more time on getting O_CLOEXEC into
gnulib :)

> +static void
> +cleanup_initial_cwd (void)
> +{
> +  if (0 == restore_cwd (initial_wd))
> +    {
> +      free_cwd (initial_wd);
> +      free (initial_wd);
> +      initial_wd = NULL;
> +    }
> +  else
> +    {
> +      /* Dont try to exit on failure, since we may already be in
> +      atexit. */
> +      error (0, errno,
> +          _("failed to restore initial working directory"));
> +    }

If you are already in an atexit handler, then you need to force non-zero
exit status.  It would be worth adding _exit(exit_failure) after that
error() call (see gnulib's closeout.c for an example).

> +run_in_dir (const struct saved_cwd *there,
> +         int (*callback)(void*), void *usercontext)
> +{
> +  int err, saved_errno;
> +  struct saved_cwd here;
> +  if (0 == save_cwd (&here))
> +    {
> +      if (0 == restore_cwd (there))
> +     {
> +       err = (*callback)(usercontext);

These days, it's both portable and less typing to write:

err = callback (usercontext);

Overall, it looks like a nice improvement.  I didn't closely check for
leaking fds, but then again, you recently added patches to help catch
some of those automatically.

-- 
Eric Blake   address@hidden    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature