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

[Steve Langasek]

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.