[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