Merge lp://staging/~jamesodhunt/upstart/bug-1238078 into lp://staging/upstart

I need to add a few more .json test files before this is merged.

per above :)

New tests added.

Hi James,

What's the difference between the upstart-reload-signal.json test and the upstart-no-job-environ.json test? AFAICS, there isn't one. If the serialization format hasn't changed between 1.9 and 1.10, we shouldn't add a redundant test here, it's unnecessary and confusing.

Also, perhaps it would be a good idea to name these json data tests after the upstream versions they correspond to, so that it's clearer? For instance, upstart-reload-signal.json could be called 'upstart-1.9.json', and upstart-with-job-environ.json could be called 'upstart-1.11.json'.

I'm not sure if (or how) the separate session-init tests are needed. They contain different data (since they're obviously taken from a running session init's state rather than a system init), but there shouldn't be any differences in the *format*, should there? If there are no differences, then I think the tests are redundant and should be omitted. If there are differences, please call this out more clearly so that it's understood at a glance what we're testing for.

I haven't had a chance yet to review the code; will do that tomorrow and send any further comments.

Hi Steve,

> What's the difference between the upstart-reload-signal.json test and
> the upstart-no-job-environ.json test? AFAICS, there isn't one. If the
> serialization format hasn't changed between 1.9 and 1.10, we shouldn't
> add a redundant test here, it's unnecessary and confusing.
Ack - gone.

>
> Also, perhaps it would be a good idea to name these json data tests
> after the upstream versions they correspond to, so that it's clearer?
> For instance, upstart-reload-signal.json could be called
> 'upstart-1.9.json', and upstart-with-job-environ.json could be called
> 'upstart-1.11.json'.
Agreed. We just need to remember to update upstart-1.11.json if new
serialisation data is added prior to 1.11 being released.

> I'm not sure if (or how) the separate session-init tests are needed.
> They contain different data (since they're obviously taken from a
> running session init's state rather than a system init), but there
> shouldn't be any differences in the *format*, should there? If there are
> no differences, then I think the tests are redundant and should be
> omitted. If there are differences, please call this out more clearly so
> that it's understood at a glance what we're testing for.
Right - there is no difference in the format of the data in the session
init .json files: the Session Init tests were an attempt to ensure
correct deserialisation of a "non-default" job environment table (which
can only be non-default when running as a Session Init). However, I
think you are right: these tests aren't really adding much since to
test that setup. Really, we need to be able to feed the .json files
to an actual Session Init process and we don't currently have that
ability.

> I haven't had a chance yet to review the code; will do that tomorrow and
> send any further comments.
Thanks!

Branch updated.

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.

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