pcntl_rfork: following-up suggestions.

removing RFMEM constant.
treating beforehand the only case where rfork would return EINVAL.

Author: devnexen

ext/pcntl/pcntl.c

@@ -357,12 +357,6 @@ void php_register_signal_constants(INIT_FUNC_ARGS)
 #ifdef RFLINUXTHPN
 	REGISTER_LONG_CONSTANT("RFLINUXTHPN",	RFLINUXTHPN, CONST_CS | CONST_PERSISTENT);
 #endif
-#ifdef RFMEM
-	REGISTER_LONG_CONSTANT("RFMEM",	RFMEM, CONST_CS | CONST_PERSISTENT);
-#endif
-#ifdef RFSIGSHARE
-	REGISTER_LONG_CONSTANT("RFSIGSHARE",	RFSIGSHARE, CONST_CS | CONST_PERSISTENT);
-#endif
 #ifdef RFTSIGZMB
 	REGISTER_LONG_CONSTANT("RFTSIGZMB",	RFTSIGZMB, CONST_CS | CONST_PERSISTENT);
 #endif
@@ -1503,45 +1497,56 @@ PHP_FUNCTION(pcntl_unshare)
    More control over the process creation is given over fork/vfork. */
 PHP_FUNCTION(pcntl_rfork)
 {
-  zend_long flags;
-  zend_long csignal = 0;
-  pid_t pid;
-
-  ZEND_PARSE_PARAMETERS_START(1, 2)
-    Z_PARAM_LONG(flags)
-    Z_PARAM_OPTIONAL
-    Z_PARAM_LONG(csignal)
-  ZEND_PARSE_PARAMETERS_END();
-
-  /* This is a flag to use with great caution in general, preferably not within PHP */
-  if ((flags & RFMEM) != 0) {
-    flags &= ~RFMEM;
-  }
-
-  /* A new pid is required */
-  flags |= RFPROC;
-
-  if ((flags & RFTSIGZMB) != 0) {
-    flags |= RFTSIGFLAGS(csignal);
-  }
-
-  pid = rfork(flags);
-
-  if (pid == -1) {
-    PCNTL_G(last_error) = errno;
-    switch (errno) {
-      case EINVAL:
-        php_error_docref(NULL, E_WARNING, "RFFDG and RFCFDG modes are mutually exclusive");
-        break;
-      case EAGAIN:
-        php_error_docref(NULL, E_WARNING, "Maximum process creations limit reached\n");
-        break;
-      default:
-        php_error_docref(NULL, E_WARNING, "Error %d", errno);
-    }
-  }
-
-  RETURN_LONG((zend_long) pid);
+	zend_long flags;
+	zend_long csignal = 0;
+	pid_t pid;
+
+	ZEND_PARSE_PARAMETERS_START(1, 2)
+		Z_PARAM_LONG(flags)
+		Z_PARAM_OPTIONAL
+		Z_PARAM_LONG(csignal)
+	ZEND_PARSE_PARAMETERS_END();
+
+	/* This is a flag to use with great caution in general, preferably not within PHP */
+	if ((flags & RFMEM) != 0) {
+		zend_argument_value_error(1, "must not include RFMEM value, not allowed within this context");
+		RETURN_THROWS();
+	}
+
+	if ((flags & RFSIGSHARE) != 0) {
+		zend_argument_value_error(1, "must not include RFSIGSHARE value, not allowed within this context");
+		RETURN_THROWS();
+	}
+
+	if ((flags & (RFFDG | RFCFDG)) == (RFFDG | RFCFDG)) {
+		zend_argument_value_error(1, "must not include both RFFDG and RFCFDG, because these flags are mutually exclusive");
+		RETURN_THROWS();
+	}
+
+	/* A new pid is required */
+	if (!(flags & (RFPROC))) {
+		flags |= RFPROC;
+	}
+
+	if ((flags & RFTSIGZMB) != 0) {
+		flags |= RFTSIGFLAGS(csignal);
+	}
+
+	pid = rfork(flags);
+
+	if (pid == -1) {
+		PCNTL_G(last_error) = errno;
+		switch (errno) {
+			case EAGAIN:
+			php_error_docref(NULL, E_WARNING, "Maximum process creations limit reached\n");
+		break;
+
+		default:
+			php_error_docref(NULL, E_WARNING, "Error %d", errno);
+		}
+	}
+
+	RETURN_LONG((zend_long) pid);
 }
 #endif
 /* }}} */

ext/pcntl/tests/pcntl_rfork.phpt

@@ -11,14 +11,11 @@ Test function pcntl_rfork() with no flag.
 echo "*** Test with no flags ***\n";
 
 $pid = pcntl_rfork(0);
-if ($pid > 0) {
-	pcntl_wait($status);
-	var_dump($pid);
-} else {
-	var_dump($pid);
+if ($pid == 0) {
+	echo "child";
+  exit;
 }
 ?>
 --EXPECTF--
 *** Test with no flags ***
-int(0)
-int(%d)
+child

ext/pcntl/tests/pcntl_rfork_badflags.phpt

@@ -0,0 +1,45 @@
+--TEST--
+Test function pcntl_rfork() with wrong flags
+--SKIPIF--
+<?php
+	if (!extension_loaded('pcntl')) die('skip pcntl extension not available');
+	elseif (!extension_loaded('posix')) die('skip posix extension not available');
+  if (!function_exists('pcntl_rfork')) die('skip pcntl_rfork unavailable');
+?>
+--FILE--
+<?php
+echo "\n*** Test with RFMEM ***\n";
+try {
+	$pid = pcntl_rfork(32);
+} catch (ValueError $e) {
+	echo $e;
+}
+echo "\n*** Test with RFSIGSHARE ***\n";
+try {
+	$pid = pcntl_rfork(16384);
+} catch (ValueError $e) {
+	echo $e;
+}
+echo "\n*** Test with RFFDG|RFCFDG ***\n";
+try {
+	$pid = pcntl_rfork(RFFDG|RFCFDG);
+} catch (ValueError $e) {
+	echo $e;
+}
+?>
+--EXPECTF--
+*** Test with RFMEM ***
+ValueError: pcntl_rfork(): Argument #1 ($flags) must not include RFMEM value, not allowed within this context in %s
+Stack trace:
+#0 %s: pcntl_rfork(32)
+#1 {main}
+*** Test with RFSIGSHARE ***
+ValueError: pcntl_rfork(): Argument #1 ($flags) must not include RFSIGSHARE value, not allowed within this context in %s
+Stack trace:
+#0 %s: pcntl_rfork(16384)
+#1 {main}
+*** Test with RFFDG|RFCFDG ***
+ValueError: pcntl_rfork(): Argument #1 ($flags) must not include both RFFDG and RFCFDG, because these flags are mutually exclusive in %s
+Stack trace:
+#0 %s: pcntl_rfork(4100)
+#1 {main}

ext/pcntl/tests/pcntl_rfork_flags.phpt

@@ -0,0 +1,31 @@
+--TEST--
+Test function pcntl_rfork() with RFCFDG and RFFDG flags.
+--SKIPIF--
+<?php
+	if (!extension_loaded('pcntl')) die('skip pcntl extension not available');
+	elseif (!extension_loaded('posix')) die('skip posix extension not available');
+  if (!function_exists('pcntl_rfork')) die('skip pcntl_rfork unavailable');
+?>
+--FILE--
+<?php
+echo "\n*** Test with RFFDG and RFCFDG flags ***\n";
+$pid = pcntl_rfork(RFFDG);
+if ($pid > 0) {
+	pcntl_wait($status);
+  var_dump($pid);
+} else {
+	var_dump($pid);
+  exit;
+}
+
+$pid = pcntl_rfork(RFCFDG);
+if ($pid > 0) {
+  pcntl_wait($status);
+  var_dump($pid);
+}
+?>
+--EXPECTF--
+*** Test with RFFDG and RFCFDG flags ***
+int(0)
+int(%d)
+int(%d)

ext/pcntl/tests/pcntl_rfork_nowait.phpt

@@ -15,7 +15,7 @@ if ($pid > 0) {
 	var_dump($pid);
 } else {
 	var_dump($pid);
-  sleep(1);
+  sleep(2); // as the child does not wait so we see its "pid"
 }
 ?>
 --EXPECTF--