Merge lp:~jamesodhunt/upstart/bug-1096531 into lp:upstart

[James Hunt]

* init/log.c:
  - log_clear_unflushed(): Correct remote_closed assertion to handle
    early-job-logging scenario where a job satisfies both of the
    following conditions:
    - ends before the log directory becomes writeable.
    - has spawned one or more processes that continue to run after the
      job itself has exited and which produce output before the log
      directory becomes writeable.
    (LP: #1096531).

[Colin Watson (cjwatson)]

Your future self will probably thank you for separating the nih_assert (foo && bar) into two asserts, so that you can easily see which condition failed. Otherwise, looks good to me, thanks.

review: Approve

[lp:~jamesodhunt/upstart/bug-1096531]

1426. By James Hunt on 2013-01-21

* init/log.c:log_clear_unflushed(): Simplify asserts.

[James Hunt (jamesodhunt)]

Having just returned from the future (which is by the way awesome - even my toothbrush runs Ubuntu! :-), I see the wisdom of your words and have thus split that condition into two separate asserts.

Thanks for reviewing.

=== modified file 'ChangeLog'
--- ChangeLog	2013-01-08 10:27:17 +0000
+++ ChangeLog	2013-01-21 16:28:21 +0000
@@ -1,3 +1,19 @@
[phone]  James Hunt  <[e-mail]>
+
+	* init/log.c:log_clear_unflushed(): Simplify asserts.
+
[phone]  James Hunt  <[e-mail]>
+
+	* init/log.c:
+	  - log_clear_unflushed(): Correct remote_closed assertion to handle
+	    early-job-logging scenario where a job satisfies both of the
+	    following conditions:
+	    - ends before the log directory becomes writeable.
+	    - has spawned one or more processes that continue to run after the
+	      job itself has exited and which produce output before the log
+	      directory becomes writeable.
+	    (LP: #1096531).
+
 2013-01-04  Dmitrijs Ledkovs  <[e-mail]>
 
 	* init/conf.c: add ability to apply override files from higher

=== modified file 'init/log.c'
--- init/log.c	2012-12-07 21:38:17 +0000
+++ init/log.c	2013-01-21 16:28:21 +0000
@@ -793,6 +793,11 @@
 		elem = (NihListEntry *)iter;
 		log = elem->data;
 
+		/* To be added to this list, log should have been
+		 * detached from its parent job.
+		 */
+		nih_assert (log->detached);
+
 		/* We expect 'an' error (as otherwise why would the log be
 		 * in this list?), but don't assert EROFS specifically
 		 * as a precaution (since an attempt to flush the log at
@@ -800,9 +805,20 @@
 		 */
 		nih_assert (log->open_errno);
 
-		nih_assert (log->unflushed->len);
-		nih_assert (log->remote_closed);
-		nih_assert (log->detached);
+		if (log->remote_closed) {
+			/* Parent job has ended and unflushed data
+			 * exists.
+			 */
+			nih_assert (log->unflushed->len);
+			nih_assert (! log->io);
+		} else {
+			/* Parent job itself has ended, but job spawned one or
+			 * more processes that are still running and
+			 * which might still produce output (the error
+			 * handler has therefore not been called).
+			 */
+			nih_assert (log->io);
+		}
 
 		if (log_file_open (log) != 0)
 			return -1;
@@ -810,6 +826,7 @@
 		if (log_file_write (log, NULL, 0) < 0)
 			return -1;
 
+		/* This will handle any remaining unflushed log data */
 		nih_free (log);
 	}