Review: Needs Fixing
@@ -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);
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.
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.
@@ -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);
@@ -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.
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.
@@ -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.
--
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
