Fixed bug in zend_accel_error() and cleaned up kill_all_lockers()

1. zend_accel_error was only executing clean up if log_verbosity_level is high enough to log
2. Cleaned up kill_all_lockers function and fixed comments.

Author: mhagstrand

ext/opcache/ZendAccelerator.c

@@ -619,27 +619,42 @@ static void accel_use_shm_interned_strings(void)
 #ifndef ZEND_WIN32
 static inline void kill_all_lockers(struct flock *mem_usage_check)
 {
-	int tries = 10;
-
+	int success, tries;
 	/* so that other process won't try to force while we are busy cleaning up */
 	ZCSG(force_restart_time) = 0;
 	while (mem_usage_check->l_pid > 0) {
+		/* Clear previous errno, reset success and tries */
+		errno = 0;
+		success = 0;
+		tries = 10;
+
 		while (tries--) {
-			zend_accel_error(ACCEL_LOG_WARNING, "Killed locker %d", mem_usage_check->l_pid);
+			zend_accel_error(ACCEL_LOG_WARNING, "Attempting to kill locker %d", mem_usage_check->l_pid);
 			if (kill(mem_usage_check->l_pid, SIGKILL)) {
+				if (errno == ESRCH) {
+					/* Process died before the signal was sent */
+					success = 1;
+					zend_accel_error(ACCEL_LOG_WARNING, "Process %d died before SIGKILL was sent", mem_usage_check->l_pid);
+				}
 				break;
 			}
 			/* give it a chance to die */
 			usleep(20000);
 			if (kill(mem_usage_check->l_pid, 0)) {
-				/* can't kill it */
+				if (errno == ESRCH) {
+					/* successfully killed locker, process no longer exists  */
+					success = 1;
+					zend_accel_error(ACCEL_LOG_WARNING, "Killed locker %d", mem_usage_check->l_pid);
+				}
 				break;
 			}
 			usleep(10000);
 		}
-		if (!tries) {
-			zend_accel_error(ACCEL_LOG_WARNING, "Can't kill %d after 10 tries!", mem_usage_check->l_pid);
+		if (!success) {
+			/* errno is not ESRCH or we ran out of tries to kill the locker */
 			ZCSG(force_restart_time) = time(NULL); /* restore forced restart request */
+			/* cannot kill the locker, bail out with error */
+			zend_accel_error(ACCEL_LOG_ERROR, "Cannot kill process %d: %s!", mem_usage_check->l_pid, strerror(errno));
 		}
 
 		mem_usage_check->l_type = F_WRLCK;

ext/opcache/tests/log_verbosity_bug.phpt

@@ -0,0 +1,19 @@
+--TEST--
+Test ACCEL_LOG_FATAL will cause the process to die even if not logged
+--DESCRIPTION--
+This test forces the opcache to error by setting memory_comsumption very large.
+The resulting ACCEL_LOG_FATAL should cause php to die.
+The process should die regardless of the log_verbosity_level.
+--INI--
+opcache.enable=1
+opcache.enable_cli=1
+opcache.memory_consumption=999999999
+opcache.log_verbosity_level=-1
+--SKIPIF--
+<?php require_once('skipif.inc'); ?>
+--FILE--
+<?php
+var_dump("Script should fail");
+?>
+--EXPECTF--
+

ext/opcache/zend_accelerator_debug.c

@@ -30,22 +30,20 @@
 
 void zend_accel_error(int type, const char *format, ...)
 {
-    va_list args;
+	va_list args;
 	time_t timestamp;
 	char *time_string;
 	FILE * fLog = NULL;
 
-	if (type > ZCG(accel_directives).log_verbosity_level) {
-		return;
-	}
+	if (type <= ZCG(accel_directives).log_verbosity_level) {
 
 	timestamp = time(NULL);
 	time_string = asctime(localtime(&timestamp));
 	time_string[24] = 0;
 
 	if (!ZCG(accel_directives).error_log ||
-	    !*ZCG(accel_directives).error_log ||
-	    strcmp(ZCG(accel_directives).error_log, "stderr") == 0) {
+		!*ZCG(accel_directives).error_log ||
+		strcmp(ZCG(accel_directives).error_log, "stderr") == 0) {
 
 		fLog = stderr;
 	} else {
@@ -56,33 +54,40 @@ void zend_accel_error(int type, const char *format, ...)
 	}
 
 #ifdef ZTS
-    fprintf(fLog, "%s (" ZEND_ULONG_FMT "): ", time_string, (zend_ulong)tsrm_thread_id());
+		fprintf(fLog, "%s (" ZEND_ULONG_FMT "): ", time_string, (zend_ulong)tsrm_thread_id());
 #else
-    fprintf(fLog, "%s (%d): ", time_string, getpid());
+		fprintf(fLog, "%s (%d): ", time_string, getpid());
 #endif
 
-	switch (type) {
-		case ACCEL_LOG_FATAL:
-			fprintf(fLog, "Fatal Error ");
-			break;
-		case ACCEL_LOG_ERROR:
-			fprintf(fLog, "Error ");
-			break;
-		case ACCEL_LOG_WARNING:
-			fprintf(fLog, "Warning ");
-			break;
-		case ACCEL_LOG_INFO:
-			fprintf(fLog, "Message ");
-			break;
-		case ACCEL_LOG_DEBUG:
-			fprintf(fLog, "Debug ");
-			break;
-	}
+		switch (type) {
+			case ACCEL_LOG_FATAL:
+				fprintf(fLog, "Fatal Error ");
+				break;
+			case ACCEL_LOG_ERROR:
+				fprintf(fLog, "Error ");
+				break;
+			case ACCEL_LOG_WARNING:
+				fprintf(fLog, "Warning ");
+				break;
+			case ACCEL_LOG_INFO:
+				fprintf(fLog, "Message ");
+				break;
+			case ACCEL_LOG_DEBUG:
+				fprintf(fLog, "Debug ");
+				break;
+		}
+
+		va_start(args, format);
+		vfprintf(fLog, format, args);
+		va_end(args);
+		fprintf(fLog, "\n");
 
-    va_start(args, format);
-    vfprintf(fLog, format, args);
-    va_end(args);
-	fprintf(fLog, "\n");
+		fflush(fLog);
+		if (fLog != stderr) {
+			fclose(fLog);
+		}
+	}
+	/* perform error handling even without logging the error */
 	switch (type) {
 		case ACCEL_LOG_ERROR:
 			zend_bailout();
@@ -91,8 +96,5 @@ void zend_accel_error(int type, const char *format, ...)
 			exit(-2);
 			break;
 	}
-	fflush(fLog);
-	if (fLog != stderr) {
-		fclose(fLog);
-	}
+
 }