Re: [PATCH] dir.c --- Dir.chdir error handling

>Is this patch enouch?

I realized this is thread stuff, so should

  static int chdir_blocking = 0;
  static VALUE chdir_thread = Qnil;

be guarded in critical section?

(I wrote rough patch, but I don't know well how to implement
 critical section in ruby, so it's just comment now. Probably use
 rb_thread_critical?)

Index: dir.c
===================================================================
RCS file: /var/cvs/src/ruby/dir.c,v
retrieving revision 1.125
diff -u -w -b -p -r1.125 dir.c
--- dir.c	19 Aug 2004 07:33:15 -0000	1.125
+++ dir.c	11 Sep 2004 05:56:21 -0000
@@ -676,26 +676,55 @@ dir_close(dir)
     return Qnil;
 }
 
+static volatile int chdir_blocking = 0;
+static volatile VALUE chdir_thread = Qnil;
+
 static void
-dir_chdir(path)
-    const char *path;
+chdir_forever(path)
+    volatile VALUE path;
 {
-    if (chdir(path) < 0)
-	rb_sys_fail(path);
+    /* enter critical section */
+    if (chdir_blocking > 0) {
+	rb_warn("conflicting chdir during another chdir block");
+    }
+    if (chdir(RSTRING(path)->ptr) < 0) {
+	/* leave critical section before raise */
+	rb_sys_fail(RSTRING(path)->ptr);
+    }
+    /* leave critical section */
 }
 
-static int chdir_blocking = 0;
-static VALUE chdir_thread = Qnil;
+static void
+chdir_enter(path)
+    volatile VALUE path;
+{
+    /* enter critical section */
+    if (chdir_blocking > 0 && rb_thread_current() != chdir_thread) {
+	rb_warn("conflicting chdir during another chdir block");
+    }
+    if (chdir(RSTRING(path)->ptr) < 0) {
+	/* leave critical section before raise */
+	rb_sys_fail(RSTRING(path)->ptr);
+    }
+    chdir_blocking++;
+    if (chdir_thread == Qnil)
+	chdir_thread = rb_thread_current();
+    /* leave critical section */
+}
 
 static VALUE
-chdir_restore(path)
-    char *path;
+chdir_leave(path)
+    volatile VALUE path;
 {
+    /* enter critical section */
     chdir_blocking--;
     if (chdir_blocking == 0)
 	chdir_thread = Qnil;
-    dir_chdir(path);
-    free(path);
+    if (chdir(RSTRING(path)->ptr) < 0) {
+	/* leave critical section before raise */
+	rb_sys_fail(RSTRING(path)->ptr);
+    }
+    /* leave critical section */
     return Qnil;
 }
 
@@ -744,36 +773,28 @@ dir_s_chdir(argc, argv, obj)
     VALUE *argv;
     VALUE obj;
 {
-    VALUE path = Qnil;
-    char *dist = "";
+    VALUE path;
 
     rb_secure(2);
     if (rb_scan_args(argc, argv, "01", &path) == 1) {
 	FilePathValue(path);
-	dist = RSTRING(path)->ptr;
     }
     else {
-	dist = getenv("HOME");
+	char *dist = getenv("HOME");
 	if (!dist) {
 	    dist = getenv("LOGDIR");
 	    if (!dist) rb_raise(rb_eArgError, "HOME/LOGDIR not set");
 	}
-    }
-
-    if (chdir_blocking > 0) {
-	if (!rb_block_given_p() || rb_thread_current() != chdir_thread)
-	    rb_warn("conflicting chdir during another chdir block");
+	path = rb_str_new2(dist);
     }
 
     if (rb_block_given_p()) {
-	char *cwd = my_getcwd();
-	chdir_blocking++;
-	if (chdir_thread == Qnil)
-	    chdir_thread = rb_thread_current();
-	dir_chdir(dist);
-	return rb_ensure(rb_yield, path, chdir_restore, (VALUE)cwd);
+	char *tmp = my_getcwd();
+	VALUE cwd = rb_str_new2(tmp); free(tmp);
+	chdir_enter(path);
+	return rb_ensure(rb_yield, path, chdir_leave, cwd);
     }
-    dir_chdir(dist);
+    chdir_forever(path);
 
     return INT2FIX(0);
 }