[Merge] lp:~upstart-devel/upstart/stateful-reexec into lp:upstart

[Steve Langasek]

Review: Approve

Hi James,

Reviewing just the parts of the diff that are new since the last time I looked at the branch.

This part of init/session.c looks strange:
 
+       if (getenv ("UPSTART_TESTS"))
+               return;
+
        if (conf_source_reload (source) < 0) {
                NihError *err;
 
The commit message only says:
    - session_create_conf_source(): Don't reload conf sources when in
      testing mode.
without explaining why.

In init/tests/test_util.c:

+ * Returns 0 if strings are identical or both NULL, else 1.
+ **/
+int
+string_check (const char *a, const char *b)
+{
+       if ((a == b) && !a)
+               return 0;
+

This '&& !a' seems unnecessary - surely if a == b and are /not/ null, there's no need to call strcmp to confirm that the strings are equal.


=== modified file 'init/blocked.c'
--- init/blocked.c      2012-07-30 14:07:37 +0000
+++ init/blocked.c      2012-11-13 06:01:51 +0000
@@ -131,6 +131,9 @@ blocked_type_enum_to_str (BlockedType ty
 BlockedType
 blocked_type_str_to_enum (const char *type)
 {
+       if (! type)
+               goto error;
+
        state_str_to_enum (BLOCKED_JOB, type);
        state_str_to_enum (BLOCKED_EVENT, type);
        state_str_to_enum (BLOCKED_EMIT_METHOD, type);
@@ -141,5 +144,6 @@ blocked_type_str_to_enum (const char *ty
        state_str_to_enum (BLOCKED_INSTANCE_STOP_METHOD, type);
        state_str_to_enum (BLOCKED_INSTANCE_RESTART_METHOD, type);
 
+error:
        return -1;
 }
 

The above is documented as "Made resilient to error conditions".  But this doesn't appear to be a real-world error condition at all; the only place in the code that blocked_type_str_to_enum() is called is in state.c:

        if (! state_get_json_string_var_strict (json, "type", NULL, blocked_type_str))
                goto error;

        blocked_type = blocked_type_str_to_enum (blocked_type_str);

This means there's no possibility of the function ever being called with a null argument (and likewise for the other str_to_enum functions), so this must be here just for the test suite.  I don't think it's a good idea to complicate either the code or the test suite by handling errors that the code is written to never allow!  I've pushed a branch to address this at
lp:~vorlon/upstart/stateful-reexec-dont-shape-code-to-impossible-tests and raised an MP for your consideration.

=== modified file 'init/job_process.c'
--- init/job_process.c  2012-08-25 08:53:37 +0000
+++ init/job_process.c  2012-11-13 10:47:50 +0000
@@ -211,7 +211,7 @@ job_process_run (Job         *job,
                while (p && (*p == '\n'))
                        p++;
 
-               if ((! nl) || (! *p)) {
+               if ((! nl) || (p && ! *p)) {
                        /* Strip off the newline(s) */
                        if (nl)
                                *nl = '\0';

This change is documented as "Stop potential NULL-pointer dereference".  But here's the full context:

                p = nl = strchr (script, '\n');
                while (p && (*p == '\n'))
                        p++;

                if ((! nl) || (p && ! *p)) {
                        /* Strip off the newline(s) */
                        if (nl)
                                *nl = '\0';

So the only way p can be NULL is if 'script' contains no newlines.  In that case, nl is also null, and the first part of the if will short-circuit without ever trying to dereference p.  Why did you consider this a potential NULL dereference?  Did this come from a compiler warning?

init/log.c, log_deserialise():

-       /* Stop fd leaking to children */
-       if (log->fd != -1) {
-               if (state_toggle_cloexec (log->fd, TRUE) < 0)
-                       goto error;
-       }
+       /* re-apply CLOEXEC flag to stop log file fd being leaked to children */
+       if (log->fd != -1 && state_toggle_cloexec (log->fd, TRUE) < 0)
+               return NULL;

Neither of these is quite right.  By this point, we've already successfully deserialized the log object.  Returning NULL if we fail to set CLOEXEC doesn't help us - in fact, it means upstart will lose all track of the fd and will *never* close it, even when the associated job stops.  So we should just ignore failures to set CLOEXEC on deserialization, so that we at least have a chance to clean up the fd later.  Fix for this pushed to the branch.

--- init/session.c      2012-09-24 14:43:33 +0000
+++ init/session.c      2012-10-10 13:24:51 +0000
@@ -467,9 +467,9 @@
 static Session *
 session_deserialise (json_object *json)
 {
-       Session              *session;
+       Session              *session = NULL;
        nih_local const char *chroot = NULL;
-       uid_t                 user;
+       uid_t                 user = (uid_t)-1;
 
        nih_assert (json);
 
This is a misleading initialization.  -1 is a perfectly valid value for a uid_t; it MUST NOT be used as an error sentinel.  Fortunately nothing in the code below uses it this way, but it would be better to avoid such confusion.

+ * _state_get_json_str_array_generic:
+ *
+ * @parent: parent object for new array (may be NULL),
+ * @json: json_object pointer,
+ * @array: array to hold output,
+ * @len: length of @array,
+ * @env: TRUE if @json represents an array of environment variables,
+ *  else FALSE,

The 'env' parameter to this macro is completely unused.  Not sure what the difference is meant to be between an env array and a regular string array?

Otherwise this looks good to me.  I think it's ready to land, with or without <lp:~vorlon/upstart/stateful-reexec-dont-shape-code-to-impossible-tests>.
-- 
https://code.launchpad.net/~upstart-devel/upstart/stateful-reexec/+merge/133275
Your team Upstart Reviewers is subscribed to branch lp:upstart.