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

Dmitrijs Ledkovs launchpad at surgut.co.uk
Tue Jan 29 09:27:22 UTC 2013


> 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.
> 

It's a check for absolute path.

> 
> +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").
> 

Sounds like we shouldn't try to create the top level one and we are only ok to create sub-dirs inside runtimedir.

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

If the path is not absolute, it should return NULL ?!

> 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