> @@ -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.
--
https://code.launchpad.net/~jamesodhunt/upstart/bug-1079715/+merge/135388
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
