Code review comment for lp:~jamesodhunt/upstart/bug-1079715

> @@ -378,6 +374,10 @@
>
> existing = (JobClass *)nih_hash_search (job_classes, class->name, NULL);
>
> + /* Ensure no existing class exists for the same session */
> + while (existing && existing->session != class->session)
> + existing = (JobClass *)nih_hash_search (job_classes, class->name, &existing->entry);
> +
> nih_assert (! existing);
>
> job_class_add (class);
>
> I suggest the following instead:
>
> @@ -372,11 +372,10 @@
>
> control_init ();
>
> - existing = (JobClass *)nih_hash_search (job_classes, class->name, NULL);
> -
> /* Ensure no existing class exists for the same session */
> - while (existing && existing->session != class->session)
> - existing = (JobClass *)nih_hash_search (job_classes, class->name, &existing->entry);
> + do {
> + existing = (JobClass *)nih_hash_search (job_classes, class->name, existing ? &existing->entry : NULL);
> + } while (existing && existing->session != class->session);
Agreed - this is cleaner: fixed.

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

>
> if (session_index < 0)
> - goto error;
> + goto out;
> +
> + session = session_from_index (session_index);
> +
> + /* XXX: user and chroot jobs are not currently supported
> + * due to ConfSources not currently being serialised.
> + */
> + nih_assert (session == NULL);
>
> This is a warning on serialization and you're making it a fatal error on deserialization. Please fall back to skipping deserialization of user and chroot jobs (if found) instead of breaking init in this case.
Done.

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

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

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

>
> @@ -1643,7 +1674,13 @@
> nih_assert (job_class);
> nih_assert (job_classes);
>
> - class = (JobClass *)nih_hash_lookup (job_classes, job_class);
> + class = (JobClass *)nih_hash_search (job_classes, job_class, NULL);
> + if (! class)
> + goto error;
> +
> + while (class && class->session != session)
> + class = (JobClass *)nih_hash_search (job_classes, job_class,
> +&class->entry);
> +
> if (! class)
> goto error;
>
> As above, can be expressed with less code duplication with a do { } while.
>
Fixed.