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

[Steve Langasek]

On Thu, Nov 22, 2012 at 04:36:19PM -0000, James Hunt wrote:

> Agreed - this is cleaner: fixed.

It looks like you fixed this via a push --overwrite of the branch.  The
merge proposal machinery is designed with the assumption that you will
instead push new commits to the branch on top of what is already there.
Could you please do that, in the future?

> >   nih_assert (! existing);
> > 
> >     - Fix potential invalid free if error occurs before JobClass
> >       is created.
> > 
> > class is initialized to NULL at the top of the function, so this seems
> > to be no-op churn.
> Actually, no - nih_free() has different semantics to free(3): you cannot
> legitimately pass NULL.

Ah, I was unaware.  Thanks for setting me straight. :)

> > 
> > @@ -1228,6 +1232,20 @@
> >                                                 blocked->job->class->name))
> >                                 goto error;
> > 
> > + session = blocked->job->class->session;
> > +
> > + session_index = session_get_index (session);
> > 
> > Can be written more succinctly as:
> > 
> > + session_index = session_get_index (blocked->job->class->session);
> Of course. I wrote it that way to make it clearer and to avoid
> particularly long lines. However, since we're not really adhering to any
> line-length policies these days, I've changed it as suggested.

Well, I think we should avoid overly-long lines, I just don't think creating
single-use temp variables is a good solution for oversized lines.  It would
IMHO also be fine to wrap this as:

			session_index = session_get_index
			                      (blocked->job->class->session);

> > @@ -1430,7 +1450,15 @@
> >                                                 "class", NULL, job_class_name))
> >                                 goto error;
> > 
> > - job = state_get_job (job_class_name, job_name);
> > + if (! state_get_json_int_var (json_blocked_data, "session", session_index))
> > + goto error;
> > +
> > + if (session_index < 0)
> > + goto error;
> > +
> > 
> > This is not part of the serialization format for upstart 1.6, so the
> > absence of this field must not be considered an error.

> I've retained this as an error, but changed the logic in
> state_deserialise_blocking() to ignore failures from
> state_deserialise_blocked(). This is better than pretending the job has
> a NULL session and then waiting for state_get_job() to error (maybe) and
> has parity with the way we handle JobClasses.

AIUI this means all information about blocked jobs will still be lost on
upgrade from 1.6.  I don't think that's the right trade-off; I think it's
better to assume that a missing session field means the default session
(which it will, for the vast majority of users) and dump the "blocked" state
non-fatally only if we don't find a matching job on the default session.

> > This also underscores the need for test cases that embed serialized json data as generated by different releases of upstart, to test deserialization capabilities when faced with historical data.
> Quite - there is no such thing as too much testing ;D

There definitely is such a thing as too much testing.  But when it comes to
changes to the serialization format, we definitely want to have build-time
regression tests.  And I think getting those tests in place is a blocker for
making further changes to the serialization format, now that it's published
in 1.6.

-- 
https://code.launchpad.net/~jamesodhunt/upstart/bug-1079715/+merge/135388
Your team Upstart Reviewers is subscribed to branch lp:upstart.