[Merge] lp:~jamesodhunt/upstart/initctl-list-sessions into lp:upstart

Steve Langasek steve.langasek at canonical.com
Tue Jan 29 09:40:26 UTC 2013


Review: Needs Fixing

+               p = strchr (contents, '=');
+               if (! p)
+                       continue;
+
+               /* Invalid contents */
+               if (strncmp (contents, "UPSTART_SESSION", (p - contents)))
+                       continue;
+
+               session = p + 1;
+

given that the format of this file is a fixed prefix, "UPSTART_SESSION=", I would skip the strchr() here and just use strncmp() to check the prefix.  E.g.:

               /* Invalid contents */
               if (strncmp (contents, "UPSTART_SESSION=", 16))
                       continue;

               session = contents + 16;

Maybe this next check should be moved earlier, before the file I/O?:

+               if (kill (pid, 0)) {
+                       nih_info ("%s: %s", _("Ignoring stale session file"), path);
+                       continue;
+               }


Those are both minor style comments and shouldn't block the merge; this next issue though will cause problems for users trying to build upstart on their desktop system without the use of a chroot:

+ TEST_FEATURE ("with no instances");

The 'initctl list-sessions' test appears to assume that XDG_RUNTIME_DIR is not set in the environment, and does not explicitly unset it.  This is not a safe assumption; it should be explicitly unset in the test.
-- 
https://code.launchpad.net/~jamesodhunt/upstart/initctl-list-sessions/+merge/145325
Your team Upstart Reviewers is subscribed to branch lp:upstart.



More information about the upstart-devel mailing list