[PATCH] dir.c (II)

It has been interesting to follow the discussion following my
original patch proposal (but I can't say I understood everything).

I noticed today that dir.c was updated in the CVS archives to fix
the problem I described in my original mail. But after looking at
the change, I think it can be improved in some ways:

1) the "dir_chdir" call in "chdir_restore" is done
   even if the first call failed. I don't think this is
   "logically correct" to do: one shouldn't undo something that was
   never done. See the end of the mail for a weird example (taken
   from a testcase I added to the Rubytests on Rubyforge).

2) the memory leak that was mentioned in the previous thread doesn't
   seem to have been fixed in the change commited today.

Below is a new patch that I think solves these issues. I have also
reorganized the code somewhat, so that "chdir_yield" and
"chdir_restore" are more symmetric, and renamed some variables so
the code becomes easier to read (hopefully).

I hope the change can be useful.

/Johan Holmberg

Below follows:

     - the patch
     - a testcase fragment to demonstrate 1) above

Index: dir.c
===================================================================
RCS file: /src/ruby/dir.c,v
retrieving revision 1.126
diff -u -r1.126 dir.c
--- dir.c	17 Sep 2004 09:24:13 -0000	1.126
+++ dir.c	17 Sep 2004 21:34:44 -0000
@@ -687,26 +687,38 @@
 static VALUE chdir_thread = Qnil;

 struct chdir_data {
-    char *dist;
-    VALUE path;
+    char *old_cwd;
+    char *new_cwd;
+    VALUE yield_arg;
+    int chdir_done;
 };

 static VALUE
 chdir_yield(args)
     struct chdir_data *args;
 {
-    dir_chdir(args->dist);
-    return rb_yield(args->path);
+    dir_chdir(args->new_cwd);
+    args->chdir_done = 1;
+
+    chdir_blocking++;
+    if (chdir_thread == Qnil)
+	chdir_thread = rb_thread_current();
+
+    return rb_yield(args->yield_arg);
 }

 static VALUE
-chdir_restore(path)
-    char *path;
+chdir_restore(args)
+    struct chdir_data *args;
 {
-    chdir_blocking--;
-    if (chdir_blocking == 0)
-	chdir_thread = Qnil;
-    dir_chdir(path);
+    if (args->chdir_done) {
+	chdir_blocking--;
+	if (chdir_blocking == 0)
+	    chdir_thread = Qnil;
+
+	dir_chdir(args->old_cwd);
+    }
+    free(args->old_cwd);
     return Qnil;
 }

@@ -777,15 +789,13 @@
     }

     if (rb_block_given_p()) {
-	char *cwd = my_getcwd();
 	struct chdir_data args;
+	args.old_cwd    = my_getcwd();
+	args.new_cwd    = dist;
+	args.yield_arg  = path;
+	args.chdir_done = 0;

-	chdir_blocking++;
-	if (chdir_thread == Qnil)
-	    chdir_thread = rb_thread_current();
-	args.dist = dist;
-	args.path = path;
-	return rb_ensure(chdir_yield, (VALUE)&args, chdir_restore, (VALUE)cwd);
+	return rb_ensure(chdir_yield, (VALUE)&args, chdir_restore, (VALUE)&args);
     }
     dir_chdir(dist);

---------------------------------------------------------------------------
---------------------------------------------------------------------------

    # chdir with block to non-existing dir, and a directory above that
    # is unaccessible => we should still get Errno::ENOENT.
    Dir.chdir("_test") do
      Dir.mkdir("inaccessible")
      Dir.mkdir("inaccessible/subdir")
      Dir.chdir("inaccessible/subdir") do
        old_mode = File.stat("..").mode
        begin
          File.chmod(0, "..")  # make "inaccessible" live up to its name
          assert_raises(Errno::ENOENT) do
            Dir.chdir("non-existing") do
              # never reached
            end
          end
        ensure
          File.chmod(old_mode, "..")  # restore "inaccessible"
        end
      end
    end

---------------------------------------------------------------------------
---------------------------------------------------------------------------