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