[Merge] lp:~jamesodhunt/upstart/add-session-file into lp:upstart

Steve Langasek steve.langasek at canonical.com
Tue Jan 29 08:29:22 UTC 2013


Review: Approve

get_subdir (const char *dir, const char *suffix, int create)
{
[...]
        nih_assert (dir != NULL);
[...]
        if (dir && dir[0] == '/') {
[...]

Looks like a redundant check for dir's nullness.


+char *
+xdg_get_runtime_dir (void)
+{
+       char *dir;
+
+       dir = getenv ("XDG_RUNTIME_DIR");
+
+       if (dir && dir[0] == '/') {
+               mkdir (dir, INIT_XDG_PATH_MODE);
+               dir = nih_strdup (NULL, dir);
+               return dir;
+       }
+
+       return dir;
+}

As mentioned, the session init won't have access to delete the directory... or to create it.  Given that you're not checking the return value of mkdir(), I think you should just leave that out, as a missing directory is only one possible failure scenario here and probably the least concerning (i.e., I'd be more worried about "directory exists but is not owned by the user").

There's also a superfluous 'return dir' here - ditch the one within the if block?

Looks good to me.
-- 
https://code.launchpad.net/~jamesodhunt/upstart/add-session-file/+merge/144881
Your team Upstart Reviewers is subscribed to branch lp:upstart.



More information about the upstart-devel mailing list