Code review comment for lp:~jamesodhunt/upstart/bug-1238078

> - "_state_deserialise_str_array(): Don't attempt to free array if type check fails." It's not at all clear to me how this relates to the rest of the code changes. If this is fixing a problem exposed by the addition of the environment serialization, it would be better if this were made more explicit - and probably kept in a commit all to its own. (This does not warrant redoing the branch, but please keep in mind for the future.)
No, I just noticed this additional issue whilst fixing the main bug. I guess it should prolly have had more prominence and a separate commit, but atleast the bug is now fixed :)

> - I don't understand the added test_initctl test case. You appear to be dynamically creating a job that has both an 'exec' and a 'script' stanza. I don't know if this is an accident or if you're relying on some particular behavior of upstart here, but if it's the latter I think this should be done some other way because the current job definition is quite opaque. If this *isn't* relying on some upstart behavior that I don't know about, then I suspect this job doesn't actually do what you intend anyway, because it talks about using a flag file to notify the job that upstart has re-execed "to allow it to move out of pre-start", but the only place the job watches for this file after creating it is in a post-stop script. So I suspect that the test succeeds but is not actually testing what it's meant to because of a broken job definition. Please take a close look at this.
Good catch - comments were misleading and indeed the auto-generated job was incorrect.

> - Finally, you've added a commented-out call to a non-existent test_reexec_job_env function, which should be dropped from the patch.
Done.