[Merge] lp:~jamesodhunt/upstart/bug-1238078 into lp:upstart

[Steve Langasek]

Review: Needs Fixing

Ok, finally found the time to fully review this patch.  The revised json looks good.  I have just a couple other comments:

- "_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.)

- 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.

- Finally, you've added a commented-out call to a non-existent test_reexec_job_env function, which should be dropped from the patch.
-- 
https://code.launchpad.net/~jamesodhunt/upstart/bug-1238078/+merge/190723
Your team Upstart Reviewers is subscribed to branch lp:upstart.