[前][次][番号順一覧][スレッド一覧]

ruby-changes:40070

From: kosaki <ko1@a...>
Date: Sun, 18 Oct 2015 06:09:32 +0900 (JST)
Subject: [ruby-changes:40070] kosaki:r52151 (trunk): * ruby.c (open_load_file): reset O_NONBLOCK after open.

kosaki	2015-10-18 06:09:10 +0900 (Sun, 18 Oct 2015)

  New Revision: 52151

  http://svn.ruby-lang.org/cgi-bin/viewvc.cgi?view=revision&revision=52151

  Log:
    * ruby.c (open_load_file): reset O_NONBLOCK after open.
      Even if S_ISREG() is true, the file may be file on FUSE filesystem
      or something. We can't assume O_NONBLOCK is safe.
      Moreover, we should wait if the path is point to FIFO. That's
      FIFO semantics. GVL should be transparent from ruby script.
      Thus, just reopen without O_NONBLOCK for filling the requirements.
      [Bug #11060][Bug #11559]
    * ruby.c (loadopen_func): new for the above.
    * file.c (ruby_is_fd_loadable): new. for checks loadable file type
      of not.
    * file.c (rb_file_load_ok): use ruby_is_fd_loadble()
    * internal.h: add ruby_is_fd_loadble()
    * common.mk: now, ruby.o depend on thread.h.
    * test/ruby/test_require.rb
    (TestRequire#test_loading_fifo_threading_success): new test.
      This test successful case that loading from FIFO.
    * test/ruby/test_require.rb
    (TestRequire#test_loading_fifo_threading_raise): rename from
      test_loading_fifo_threading. You souldn't rescue an exception
      if you test raise or not.
      Moreover, this case should be caught IOError because load(FIFO)
      should be blocked until given any input.

  Modified files:
    trunk/ChangeLog
    trunk/common.mk
    trunk/file.c
    trunk/internal.h
    trunk/ruby.c
    trunk/test/ruby/test_require.rb
Index: ChangeLog
===================================================================
--- ChangeLog	(revision 52150)
+++ ChangeLog	(revision 52151)
@@ -1,3 +1,33 @@ https://github.com/ruby/ruby/blob/trunk/ChangeLog#L1
+Sun Oct 18 05:11:22 2015  KOSAKI Motohiro  <kosaki.motohiro@g...>
+
+	* ruby.c (open_load_file): reset O_NONBLOCK after open.
+	  Even if S_ISREG() is true, the file may be file on FUSE filesystem
+	  or something. We can't assume O_NONBLOCK is safe.
+	  Moreover, we should wait if the path is point to FIFO. That's
+	  FIFO semantics. GVL should be transparent from ruby script.
+	  Thus, just reopen without O_NONBLOCK for filling the requirements.
+	  [Bug #11060][Bug #11559]
+
+	* ruby.c (loadopen_func): new for the above.
+
+	* file.c (ruby_is_fd_loadable): new. for checks loadable file type
+	  of not.
+	* file.c (rb_file_load_ok): use ruby_is_fd_loadble()
+	* internal.h: add ruby_is_fd_loadble()
+
+	* common.mk: now, ruby.o depend on thread.h.
+
+	* test/ruby/test_require.rb
+	(TestRequire#test_loading_fifo_threading_success): new test.
+	  This test successful case that loading from FIFO.
+
+	* test/ruby/test_require.rb
+	(TestRequire#test_loading_fifo_threading_raise): rename from
+	  test_loading_fifo_threading. You souldn't rescue an exception
+	  if you test raise or not.
+	  Moreover, this case should be caught IOError because load(FIFO)
+	  should be blocked until given any input.
+
 Sat Oct 17 13:55:32 2015  Nobuyoshi Nakada  <nobu@r...>
 
 	* file.c (rb_file_expand_path_internal): concatenate converted
Index: common.mk
===================================================================
--- common.mk	(revision 52150)
+++ common.mk	(revision 52151)
@@ -2062,6 +2062,7 @@ ruby.$(OBJEXT): {$(VPATH)}ruby.c https://github.com/ruby/ruby/blob/trunk/common.mk#L2062
 ruby.$(OBJEXT): {$(VPATH)}ruby_atomic.h
 ruby.$(OBJEXT): {$(VPATH)}st.h
 ruby.$(OBJEXT): {$(VPATH)}subst.h
+ruby.$(OBJEXT): {$(VPATH)}thread.h
 ruby.$(OBJEXT): {$(VPATH)}thread_$(THREAD_MODEL).h
 ruby.$(OBJEXT): {$(VPATH)}thread_native.h
 ruby.$(OBJEXT): {$(VPATH)}util.h
Index: internal.h
===================================================================
--- internal.h	(revision 52150)
+++ internal.h	(revision 52151)
@@ -762,6 +762,7 @@ VALUE rb_file_expand_path_internal(VALUE https://github.com/ruby/ruby/blob/trunk/internal.h#L762
 VALUE rb_get_path_check_to_string(VALUE, int);
 VALUE rb_get_path_check_convert(VALUE, VALUE, int);
 void Init_File(void);
+int ruby_is_fd_loadable(int fd);
 
 #ifdef RUBY_FUNCTION_NAME_STRING
 # if defined __GNUC__ && __GNUC__ >= 4
Index: ruby.c
===================================================================
--- ruby.c	(revision 52150)
+++ ruby.c	(revision 52151)
@@ -16,6 +16,7 @@ https://github.com/ruby/ruby/blob/trunk/ruby.c#L16
 #include <sys/cygwin.h>
 #endif
 #include "internal.h"
+#include "ruby/thread.h"
 #include "eval_intern.h"
 #include "dln.h"
 #include <stdio.h>
@@ -1725,21 +1726,36 @@ load_file_internal(VALUE argp_v) https://github.com/ruby/ruby/blob/trunk/ruby.c#L1726
     return (VALUE)tree;
 }
 
+static void *
+loadopen_func(void *arg)
+{
+    int fd;
+    fd = rb_cloexec_open((const char *)arg, O_RDONLY, 0);
+    if (fd >= 0)
+	rb_update_max_fd(fd);
+
+    return (void *)(VALUE)fd;
+}
+
 static VALUE
 open_load_file(VALUE fname_v, int *xflag)
 {
     const char *fname = StringValueCStr(fname_v);
     VALUE f;
+    int e;
 
     if (RSTRING_LEN(fname_v) == 1 && fname[0] == '-') {
 	f = rb_stdin;
     }
     else {
-	int fd, mode = O_RDONLY;
-#if defined O_NONBLOCK && !(O_NONBLOCK & O_ACCMODE)
+	int fd;
+	/* open(2) may block if fname is point to FIFO and it's empty. Let's
+	   use O_NONBLOCK. */
+	int mode = O_RDONLY;
+#if defined O_NONBLOCK && HAVE_FCNTL && !(O_NONBLOCK & O_ACCMODE)
 	/* TODO: fix conflicting O_NONBLOCK in ruby/win32.h */
 	mode |= O_NONBLOCK;
-#elif defined O_NDELAY && !(O_NDELAY & O_ACCMODE)
+#elif defined O_NDELAY && HAVE_FCNTL && !(O_NDELAY & O_ACCMODE)
 	mod |= O_NDELAY;
 #endif
 #if defined DOSISH || defined __CYGWIN__
@@ -1751,21 +1767,44 @@ open_load_file(VALUE fname_v, int *xflag https://github.com/ruby/ruby/blob/trunk/ruby.c#L1767
 	    }
 	}
 #endif
+
 	if ((fd = rb_cloexec_open(fname, mode, 0)) < 0) {
 	    rb_load_fail(fname_v, strerror(errno));
 	}
-        rb_update_max_fd(fd);
-#if !defined DOSISH && !defined __CYGWIN__
+	rb_update_max_fd(fd);
+
+#ifdef HAVE_FCNTL
+	/* disabling O_NONBLOCK */
+	if (fcntl(fd, F_SETFL, 0) < 0) {
+	    e = errno;
+	    close(fd);
+	    rb_load_fail(fname_v, strerror(e));
+	}
+#endif
+
+#ifdef S_ISFIFO
 	{
 	    struct stat st;
-	    int e;
-	    if ((fstat(fd, &st) != 0) && (e = errno, 1) ||
-		(S_ISDIR(st.st_mode) && (e = EISDIR, 1))) {
-		(void)close(fd);
+	    if (fstat(fd, &st) != 0) {
+		e = errno;
+		close(fd);
 		rb_load_fail(fname_v, strerror(e));
 	    }
+	    if (S_ISFIFO(st.st_mode)) {
+		/* We need to wait if FIFO is empty. So, let's reopen it. */
+		close(fd);
+		fd = (int)(VALUE)rb_thread_call_without_gvl(loadopen_func,
+						(void *)fname, RUBY_UBF_IO, 0);
+		if (fd < 0)
+		    rb_load_fail(fname_v, strerror(errno));
+	    }
 	}
 #endif
+	if (!ruby_is_fd_loadable(fd)) {
+	    close(fd);
+	    rb_load_fail(fname_v, strerror(errno));
+	}
+
 	f = rb_io_fdopen(fd, mode, fname);
     }
     return f;
Index: test/ruby/test_require.rb
===================================================================
--- test/ruby/test_require.rb	(revision 52150)
+++ test/ruby/test_require.rb	(revision 52151)
@@ -694,7 +694,7 @@ class TestRequire < Test::Unit::TestCase https://github.com/ruby/ruby/blob/trunk/test/ruby/test_require.rb#L694
     }
   end
 
-  def test_loading_fifo_threading
+  def test_loading_fifo_threading_raise
     Tempfile.create(%w'fifo .rb') {|f|
       f.close
       File.unlink(f.path)
@@ -702,16 +702,39 @@ class TestRequire < Test::Unit::TestCase https://github.com/ruby/ruby/blob/trunk/test/ruby/test_require.rb#L702
       assert_separately(["-", f.path], <<-END, timeout: 3)
       th = Thread.current
       Thread.start {begin sleep(0.001) end until th.stop?; th.raise(IOError)}
-      assert_nothing_raised do
-        begin
-          load(ARGV[0])
-        rescue IOError
-        end
+      assert_raise(IOError) do
+        load(ARGV[0])
       end
       END
     }
   end unless /mswin|mingw/ =~ RUBY_PLATFORM
 
+  def test_loading_fifo_threading_success
+    Tempfile.create(%w'fifo .rb') {|f|
+      f.close
+      File.unlink(f.path)
+      File.mkfifo(f.path)
+
+      assert_separately(["-", f.path], <<-INPUT, timeout: 3)
+      path = ARGV[0]
+      th = Thread.current
+      Thread.start {
+        begin
+          sleep(0.001)
+        end until th.stop?
+        open(path, File::WRONLY | File::NONBLOCK) {|fifo_w|
+          fifo_w.puts "class C1; FOO='foo'; end"
+        }
+      }
+
+      load(path)
+      assert_equal(C1::FOO, "foo")
+    INPUT
+    }
+
+  end unless /mswin|mingw/ =~ RUBY_PLATFORM
+
+
   def test_throw_while_loading
     Tempfile.create(%w'bug-11404 .rb') do |f|
       f.puts 'sleep'
Index: file.c
===================================================================
--- file.c	(revision 52150)
+++ file.c	(revision 52151)
@@ -5670,11 +5670,35 @@ rb_path_check(const char *path) https://github.com/ruby/ruby/blob/trunk/file.c#L5670
     return 1;
 }
 
+int
+ruby_is_fd_loadable(int fd)
+{
+#ifdef _WIN32
+    return 1;
+#else
+    struct stat st;
+
+    if (fstat(fd, &st) < 0)
+	return 0;
+
+    if (S_ISREG(st.st_mode))
+	return 1;
+    if (S_ISFIFO(st.st_mode))
+	return 1;
+
+    return 0;
+#endif
+}
+
 #ifndef _WIN32
 int
 rb_file_load_ok(const char *path)
 {
     int ret = 1;
+    /*
+      open(2) may block if path is FIFO and it's empty. Let's use O_NONBLOCK.
+      FIXME: Why O_NDELAY is checked?
+    */
     int mode = (O_RDONLY |
 #if defined O_NONBLOCK
 		O_NONBLOCK |
@@ -5685,14 +5709,7 @@ rb_file_load_ok(const char *path) https://github.com/ruby/ruby/blob/trunk/file.c#L5709
     int fd = rb_cloexec_open(path, mode, 0);
     if (fd == -1) return 0;
     rb_update_max_fd(fd);
-#if !defined DOSISH
-    {
-	struct stat st;
-	if (fstat(fd, &st) || S_ISDIR(st.st_mode)) {
-	    ret = 0;
-	}
-    }
-#endif
+    ret = ruby_is_fd_loadable(fd);
     (void)close(fd);
     return ret;
 }

--
ML: ruby-changes@q...
Info: http://www.atdot.net/~ko1/quickml/

[前][次][番号順一覧][スレッド一覧]