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. -- upstart-devel mailing list [email protected] Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/upstart-devel
