Merge lp:~xnox/upstart/user-log-dir into lp:upstart

[Dimitri John Ledkov]

This branch implements writting job logs to $XDG_CACHE_HOME/upstart/ when running in --user-mode.
One can override the location with command-line arg.
Note the environment variable is not honoured.

[James Hunt (jamesodhunt)]

* init/main.c:
  - handle_logdir(): Is there a reason we are not honouring the environment variable? If not, why don't we put
    the 'dir = getenv (LOGDIR_ENV)' immediately before the 'if (user_mode)' since we can then say in the
    user_mode block: 'log_dir = dir ? dir : get_user_log_dir ()' and thus honour the env var for user mode too.
* init/xdg.c:
  - get_user_log_dir():
    - Spacing around '='.
    - 'path' is leaked.
    - We should be logging to $HOME/.cache/upstart/logs/ as per the spec: this allows us to create a
      $HOME/.cache/upstart/sessions/ also in the spec.
* init/man/init.5: This needs updating for '--user' and location of
  conf files and logfiles when running in this mode. Specifically the following sections need updating:
    - 'User Jobs'
    - 'Process environment' -> console -> log
    - 'FILES': should reflect conf directories listed in the 'User
      Session Mode' and specify location of User Session log files.
* init/man/init.8: Description of '--logdir' and '--confdir' also need updating.

review: Needs Fixing

[lp:~xnox/upstart/user-log-dir]

1425. By James Hunt on 2013-01-15

* init/log.c:
  - log_clear_unflushed(): Correct remote_closed assertion to handle
    early-job-logging scenario where a job satisfies both of the
    following conditions:
    - ends before the log directory becomes writeable.
    - has spawned one or more processes that continue to run after the
      job itself has exited and which produce output before the log
      directory becomes writeable.
    (LP: #1096531).

[James Hunt (jamesodhunt)]

The reason we don't honour the LOGDIR_ENV variable for Session Inits is to handle the scenario where the root user starts its own Session Init: if that is to happen, we don't want the root Session Init to log to the same directory as Upstart running as PID 1 should the LOGDIR_ENV variable be set when root starts the Session Init.

[Dimitri John Ledkov (xnox)]

Reading XDG specification, it sounds like instead of $XDG_CACHE_HOME/upstart/sessions, we instead should be using $XDG_RUNTIME_DIR/upstart/ to store session init's pids.

"""
$XDG_RUNTIME_DIR defines the base directory relative to which user-specific non-essential runtime files and other file objects (such as sockets, named pipes, ...) should be stored. The directory MUST be owned by the user, and he MUST be the only one having read and write access to it. Its Unix access mode MUST be 0700.

The lifetime of the directory MUST be bound to the user being logged in. It MUST be created when the user first logs in and if the user fully logs out the directory MUST be removed. If the user logs in more than once he should get pointed to the same directory, and it is mandatory that the directory continues to exist from his first login to his last logout on the system, and not removed in between. Files in the directory MUST not survive reboot or a full logout/login cycle.
"""

http://standards.freedesktop.org/basedir-spec/basedir-spec-latest.html

Currently I like how all our logs are stored in a flat directory and I'd rather not introduce subdirs.

Shall I update the spec with:
logs go to $XDG_CACHE_HOME/upstart
the address of the session goes to $XDG_RUNTIME_DIR/upstart

[James Hunt (jamesodhunt)]

Sounds good. Yes, please update the spec.

Please can you also update the doc as outlined in original comments. Also...

* init/xdg.c:
  xdg_get_cache_home(): XDG_CACHE_HOME and ".cache" should be defined in init/paths.h for consistency.

[Steve Langasek (vorlon)]

On Wed, Jan 16, 2013 at 12:02:21PM -0000, Dmitrijs Ledkovs wrote:

> Reading XDG specification, it sounds like instead of
> $XDG_CACHE_HOME/upstart/sessions, we instead should be using
> $XDG_RUNTIME_DIR/upstart/ to store session init's pids.

> """

> $XDG_RUNTIME_DIR defines the base directory relative to which
> user-specific non-essential runtime files and other file objects (such as
> sockets, named pipes, ...) should be stored. The directory MUST be owned
> by the user, and he MUST be the only one having read and write access to
> it. Its Unix access mode MUST be 0700.

> The lifetime of the directory MUST be bound to the user being logged in.
> It MUST be created when the user first logs in and if the user fully logs
> out the directory MUST be removed. If the user logs in more than once he
> should get pointed to the same directory, and it is mandatory that the
> directory continues to exist from his first login to his last logout on
> the system, and not removed in between. Files in the directory MUST not
> survive reboot or a full logout/login cycle.

This last paragraph is the problematic part, since up to this point AIUI
we've been discussing having one upstart session per login session, *not*
one upstart session per logged-in user. XDG_RUNTIME_DIR only has the
correct semantics for the latter.

[Dimitri John Ledkov (xnox)]

XDG_RUNTIME_DIR guarantees that we get the same dir no matter how many times we login.
We can manage multiple pids/sockets ourself per user.

The file we store there will be pid e.g. $XDG_RUNTIME_DIR/upstart/12345
For another session (for the same user) we will have pid e.g. $XDG_RUNTIME_DIR/upstart/15001
This is to support one upstart session per login session.

The will know the current session via environment variable.
Also we can implement `initctl list-sessions` command simply by iterating $XDG_RUNTIME_DIR/upstart/.

XDG_CACHE_HOME is not guaranteed to be cleared, while XDG_RUNTIME_DIR will always be fresh after reboots & complete logouts.

[Dimitri John Ledkov (xnox)]

> Sounds good. Yes, please update the spec.
>
> Please can you also update the doc as outlined in original comments. Also...
>
> * init/xdg.c:
> xdg_get_cache_home(): XDG_CACHE_HOME and ".cache" should be defined in
> init/paths.h for consistency.

Why? I only defined upstart specific constants in path.h, e.g. INIT_XDG_SUBDIR - the name of the subdirectory we use within all XDG_* locations.

XDG_* variables are not upstart specific and are compliant with the XDG spec.
Also their fallbacks "/etc/xdg/", "~/.config", "~/.cache" are defined in the XDG spec.
I am failing to see benefits or introducing #ifndef pointers to those names, since changing those will violate XDG specification.
And XDG specification is unlikely to change in a backwards incompatible way at this point.

Note that XDG_CONFIG_HOME, XDG_CONFIG_DIRS, "~/.config", "/etc/xdg/" are not defined in paths.h.

If libnih starts using XDG_* paths for anything, it might even make sense to move xdg_ helper functions into libnih.

I guess I can make a static array of those constants at the top of xdg.c for ease of understanding that these constants are externally specified.

[James Hunt (jamesodhunt)]

OK, you've convinced me on paths.h :-)

I've updated the spec such that we have a script which sets the appropriate XDG vars if not already set *and* creates them if they don't exist. It will then start the Session Init. However, I wonder if the Session Init should also attempt to create the directories returned by xdg_get_cache_home() and xdg_get_config_home() to handle the scenario that a user logs in multiple times and in the first session say deletes one or both of these directories.

I think that to be conformant to the XDG spec, the alluded-to script can as Dmitrijs says run 'initctl list-sessions' and if that returns nothing, remove XDG_RUNTIME_DIR before exiting.

After scrutinizing the XDG spec again, we do need to make some minor changes:

* init/xdg.c:
  - xdg_get_cache_home(): This must check that if XDG_CACHE_HOME is set that it also is an absolute path before we use it.
  - xdg_get_config_home(): Same logic as above for XDG_CONFIG_HOME.
  - get_user_upstart_dirs(): Same logic for XDG_CONFIG_DIRS.

[Dimitri John Ledkov (xnox)]

> OK, you've convinced me on paths.h :-)
>
> I've updated the spec such that we have a script which sets the appropriate
> XDG vars if not already set *and* creates them if they don't exist. It will

no, this is wrong again.
It's perfectly valid to not have XDG_HOME_* variables set, as by default the default fallbacks should be used.
It's only meant for a user to change them if they want to launch set of applications with "clean" config, cache.
The XDG_RUNTIME_DIR in ubuntu is already set by libpam-xdg-support
https://blueprints.launchpad.net/ubuntu/+spec/foundations-q-xdg-runtime-dir

So these variables should not be set, if they don't already exist.

Creating the directories if they don't yet exist is a good point. Especially brand new accounts, might not have those directories.
Oh and as per XDG spec we should create them with permissions 0700 if we need to access them.

The spec is unclear about XDG_RUNTIME_DIR, I'd like to assert if it doesn't exist. In ubuntu it exists in quantal and up. To be compliant with the spec we should gracefully fallback to some other place, but I don't know what that other place is a good to fallback to.

> then start the Session Init. However, I wonder if the Session Init should also
> attempt to create the directories returned by xdg_get_cache_home() and
> xdg_get_config_home() to handle the scenario that a user logs in multiple
> times and in the first session say deletes one or both of these directories.
>

I don't like the idea of creating empty directories, just to place inotify watch on it (for the $XDG_CONFIG_HOME/upstart).
We intend to generate logs everytime, so precreating $XDG_CACHE_HOME/upstart makes sense.

> I think that to be conformant to the XDG spec, the alluded-to script can as
> Dmitrijs says run 'initctl list-sessions' and if that returns nothing, remove
> XDG_RUNTIME_DIR before exiting.
>

I believe XDG_RUNTIME_DIR is cleaned up for us after the last logout. I'll check that. Or do you mean to clean up bits in the $XDG_RUNTIME_DIR/upstart/ that we created? E.g. remove pidfiles on exit?

(outside of this merge proposal as far as I can see)

> After scrutinizing the XDG spec again, we do need to make some minor changes:
>
> * init/xdg.c:
> - xdg_get_cache_home(): This must check that if XDG_CACHE_HOME is set that
> it also is an absolute path before we use it.
> - xdg_get_config_home(): Same logic as above for XDG_CONFIG_HOME.
> - get_user_upstart_dirs(): Same logic for XDG_CONFIG_DIRS.

All good points.

[James Hunt (jamesodhunt)]

> > OK, you've convinced me on paths.h :-)
> >
> > I've updated the spec such that we have a script which sets the appropriate
> > XDG vars if not already set *and* creates them if they don't exist. It will
>
> no, this is wrong again.
> It's perfectly valid to not have XDG_HOME_* variables set, as by default the default fallbacks should be used.
> It's only meant for a user to change them if they want to launch set of applications with "clean" config, cache.
> The XDG_RUNTIME_DIR in ubuntu is already set by libpam-xdg-support
> https://blueprints.launchpad.net/ubuntu/+spec/foundations-q-xdg-runtime-dir
>
> So these variables should not be set, if they don't already exist.
>
> Creating the directories if they don't yet exist is a good point. Especially brand new accounts, might not have those directories.

The reason for my specifying in the spec that we create the directories is that if they do not already exist, we end up with impotent inotify watches: meaning no job logging and no way for a user to create jobs *after* they create ~/.config/upstart/ manually. All very confusing from the users perspective. However, yes, seemingly we should not set them, implying Upstart needs to handle this.

> Oh and as per XDG spec we should create them with permissions 0700 if we need to access them.

Right.

>
> The spec is unclear about XDG_RUNTIME_DIR, I'd like to assert if it doesn't exist. In ubuntu it exists in quantal and up. To be compliant with the spec we should gracefully fallback to some other place, but I don't know what that other place is a good to fallback to.

Asserting is a little harsh. If it's not set, how about we display a warning and then fall back to ${TMPDIR}/${USER} and if that's not set, /tmp/${USER}? Steve?

>
> > then start the Session Init. However, I wonder if the Session Init should also
> > attempt to create the directories returned by xdg_get_cache_home() and
> > xdg_get_config_home() to handle the scenario that a user logs in multiple
> > times and in the first session say deletes one or both of these directories.

Lets go with this then and I'll revert the spec change.

> >
>
> I don't like the idea of creating empty directories, just to place inotify watch on it (for the $XDG_CONFIG_HOME/upstart).
> We intend to generate logs everytime, so precreating $XDG_CACHE_HOME/upstart makes sense.

See above: we need to do both as what is the point in creating the log directory if no jobs can be registered if the config directory does not exist? 1 inotify watch is a small price to pay for sane behaviour. Added to which there is precedent: we already create a watch for /etc/init.conf.

>
> > I think that to be conformant to the XDG spec, the alluded-to script can as
> > Dmitrijs says run 'initctl list-sessions' and if that returns nothing, remove
> > XDG_RUNTIME_DIR before exiting.
> >
>
> I believe XDG_RUNTIME_DIR is cleaned up for us after the last logout. I'll check that. Or do you mean to clean up bits in the $XDG_RUNTIME_DIR/upstart/ that we created? E.g. remove pidfiles on exit?

What is doing the cleaning up currently? And will that need to change when Upstart is in control?

[Steve Langasek (vorlon)]

On Thu, Jan 17, 2013 at 02:20:29PM -0000, James Hunt wrote:

> > The spec is unclear about XDG_RUNTIME_DIR, I'd like to assert if it
> > doesn't exist. In ubuntu it exists in quantal and up. To be compliant
> > with the spec we should gracefully fallback to some other place, but I
> > don't know what that other place is a good to fallback to.

> Asserting is a little harsh. If it's not set, how about we display a
> warning and then fall back to ${TMPDIR}/${USER} and if that's not set,
> /tmp/${USER}? Steve

I suggest consulting the desktop team, or looking at existing fd.o software,
for recommendations on a fallback path. Using ${TMPDIR:-/tmp}/${USER} has
some obvious security problems (best case: DoS) that are undesirable even in
a fallback.

I do agree that an assert() here is a bit harsh. In theory we should never
be without $XDG_RUNTIME_DIR in quantal and beyond; however, this is
implemented by way of the pam stack and relies on the local admin not
altering /etc/pam.d/common-session in an "unsupported" manner. I fear that
having upstart assert instead of opening a login session will be rather
undebuggable, and I doubt this is a good tradeoff for something that's
rather peripheral to the main functionality of upstart.

> > I believe XDG_RUNTIME_DIR is cleaned up for us after the last logout.
> > I'll check that. Or do you mean to clean up bits in the
> > $XDG_RUNTIME_DIR/upstart/ that we created? E.g. remove pidfiles on
> > exit?

> What is doing the cleaning up currently? And will that need to change when
> Upstart is in control?

pam_xdg_support, which is the same thing that creates the directory and sets
the env variable. This must not change under upstart user sessions.

[lp:~xnox/upstart/user-log-dir]

1426. By James Hunt on 2013-01-21

* init/log.c:log_clear_unflushed(): Simplify asserts.

[Dimitri John Ledkov (xnox)]

* Documentation is updated
* Checks for absolute paths added (i.e. must start with '/')
* XDG_HOME_* directories & subdirectories are created

* XDG_RUNTIME_DIR & fallback is not implemented. Need to consult desktop team on fallbacks (e.g. how other XDG compliant software handles it) and then this will be quick to add as a separate merge proposal.

I think this branch is complete for now.

[James Hunt (jamesodhunt)]

Hi Dmitrijs,

Just a few comments:

* init/xdg.c:
  - get_home_subdir(): Return NULL immediately if nih_sprintf() fails.
  - get_user_log_dir(): Return NULL immediately if nih_sprintf() fails.
  - create_dir():
    - for consistency, this should probably call
      init/system.c:int system_mkdir(const char *path, mode_t mode) passing
      @dir and 0700.
    - should assert @dir: I don't understand why it currently handles
      a NULL directory explicitly?
    - @dir param should be const.
    - Remove comment: the directory permissions must be 0700 according
      to the spec, which is an absolute value. Since we already reset
      umask(), the code will work as expected.
    - This (and/or system_mkdir()) should be documented explicitly as
      only creating the "leaf" directory (ie no recursive creation).

review: Needs Fixing

[Dimitri John Ledkov (xnox)]

On 22 January 2013 10:28, James Hunt <email address hidden> wrote:
>
> * init/xdg.c:
> - get_home_subdir(): Return NULL immediately if nih_sprintf() fails.
> - get_user_log_dir(): Return NULL immediately if nih_sprintf() fails.

Done.

> - create_dir():
> - for consistency, this should probably call
> init/system.c:int system_mkdir(const char *path, mode_t mode) passing
> @dir and 0700.
> - should assert @dir: I don't understand why it currently handles
> a NULL directory explicitly?

Well, it did so because I didn't return Null straight away when
nih_sprintfs failed in get_home_subdir() and get_user_log_dir().

> - @dir param should be const.
> - Remove comment: the directory permissions must be 0700 according
> to the spec, which is an absolute value. Since we already reset
> umask(), the code will work as expected.

Awesome. Didn't notice this.

> - This (and/or system_mkdir()) should be documented explicitly as
> only creating the "leaf" directory (ie no recursive creation).

Well... after changing get_home_subdir() & get_user_log_dir(), the
"system_mkdir()" is a wrapper with nih_assert around path argument....
I think I went overboard with this wrapper =) instead I just use mkdir
directly. I guess we could add further nih_system_error / warning
saying that we failed to create XDG paths, but imho that's just noise.

[lp:~xnox/upstart/user-log-dir]

1427. By James Hunt on 2013-01-23

* Merge of lp:~stgraber/upstart/upstart-session-socket.

1428. By Dimitri John Ledkov on 2013-01-23

Merge user-dirs

1429. By Dimitri John Ledkov on 2013-01-23

Normalise user_mode extern definitions

[James Hunt (jamesodhunt)]

Hi Dmitrijs,

Thanks - merged.

review: Approve

=== modified file 'ChangeLog'
--- ChangeLog	2013-01-21 16:24:48 +0000
+++ ChangeLog	2013-01-23 13:00:29 +0000
@@ -1,3 +1,15 @@
[phone]  Dmitrijs Ledkovs  <[e-mail]>
+
+	* init/xdg.[ch]: add xdg_get_cache_home and get_user_log_dir
+	  functions. These retrieve XDG_CACHE_HOME and a subdir inside it
+	  for upstart.
+	* init/tests/test_xdg.c: reuse test_get_config_home to test both
+	  xdg_get_cache_home and xdg_get_config_home. Add test for
+	  get_user_log_dir.
+	* init/main.c: use get_user_log_dir to setup logging
+	  directory in user_mode. For now, command line argument is
+	  honoured, while the environment override is not.
+
 2013-01-21  James Hunt  <[e-mail]>
 
 	* init/log.c:log_clear_unflushed(): Simplify asserts.
 
=== modified file 'init/Makefile.am'
--- init/Makefile.am	2012-12-19 12:46:46 +0000
+++ init/Makefile.am	2013-01-23 13:00:29 +0000
@@ -177,7 +177,7 @@
 	system.o environ.o process.o \
 	job_class.o job_process.o job.o event.o event_operator.o blocked.o \
 	parse_job.o parse_conf.o conf.o control.o \
-	session.o log.o state.o \
+	session.o log.o state.o xdg.o \
 	com.ubuntu.Upstart.o \
 	com.ubuntu.Upstart.Job.o com.ubuntu.Upstart.Instance.o \
 	$(NIH_LIBS) \
@@ -191,7 +191,7 @@
 	system.o environ.o process.o \
 	job_class.o job_process.o job.o event.o event_operator.o blocked.o \
 	parse_job.o parse_conf.o conf.o control.o \
-	session.o log.o state.o \
+	session.o log.o state.o xdg.o \
 	com.ubuntu.Upstart.o \
 	com.ubuntu.Upstart.Job.o com.ubuntu.Upstart.Instance.o \
 	$(NIH_LIBS) \
@@ -205,7 +205,7 @@
 	system.o environ.o process.o \
 	job_class.o job_process.o job.o event.o event_operator.o blocked.o \
 	parse_job.o parse_conf.o conf.o control.o \
-	session.o log.o state.o \
+	session.o log.o state.o xdg.o \
 	com.ubuntu.Upstart.o \
 	com.ubuntu.Upstart.Job.o com.ubuntu.Upstart.Instance.o \
 	$(NIH_LIBS) \
@@ -219,7 +219,7 @@
 	system.o environ.o process.o \
 	job_class.o job_process.o job.o event.o event_operator.o blocked.o \
 	parse_job.o parse_conf.o conf.o control.o \
-	session.o log.o state.o \
+	session.o log.o state.o xdg.o \
 	com.ubuntu.Upstart.o \
 	com.ubuntu.Upstart.Job.o com.ubuntu.Upstart.Instance.o \
 	$(NIH_LIBS) \
@@ -233,7 +233,7 @@
 	system.o environ.o process.o \
 	job_class.o job_process.o job.o event.o event_operator.o blocked.o \
 	parse_job.o parse_conf.o conf.o control.o \
-	session.o log.o state.o \
+	session.o log.o state.o xdg.o \
 	com.ubuntu.Upstart.o \
 	com.ubuntu.Upstart.Job.o com.ubuntu.Upstart.Instance.o \
 	$(NIH_LIBS) \
@@ -247,7 +247,7 @@
 	system.o environ.o process.o \
 	job_class.o job_process.o job.o event.o event_operator.o blocked.o \
 	parse_job.o parse_conf.o conf.o control.o \
-	session.o log.o state.o \
+	session.o log.o state.o xdg.o \
 	com.ubuntu.Upstart.o \
 	com.ubuntu.Upstart.Job.o com.ubuntu.Upstart.Instance.o \
 	$(NIH_LIBS) \
@@ -261,7 +261,7 @@
 	system.o environ.o process.o \
 	job_class.o job_process.o job.o event.o event_operator.o blocked.o \
 	parse_job.o parse_conf.o conf.o control.o \
-	session.o log.o state.o \
+	session.o log.o state.o xdg.o \
 	com.ubuntu.Upstart.o \
 	com.ubuntu.Upstart.Job.o com.ubuntu.Upstart.Instance.o \
 	$(NIH_LIBS) \
@@ -275,7 +275,7 @@
 	system.o environ.o process.o \
 	job_class.o job_process.o job.o event.o event_operator.o blocked.o \
 	parse_job.o parse_conf.o conf.o control.o \
-	session.o log.o state.o \
+	session.o log.o state.o xdg.o \
 	com.ubuntu.Upstart.o \
 	com.ubuntu.Upstart.Job.o com.ubuntu.Upstart.Instance.o \
 	$(NIH_LIBS) \
@@ -289,7 +289,7 @@
 	system.o environ.o process.o \
 	job_class.o job_process.o job.o event.o event_operator.o blocked.o \
 	parse_job.o parse_conf.o conf.o control.o \
-	session.o log.o state.o \
+	session.o log.o state.o xdg.o \
 	com.ubuntu.Upstart.o \
 	com.ubuntu.Upstart.Job.o com.ubuntu.Upstart.Instance.o \
 	$(NIH_LIBS) \
@@ -303,7 +303,7 @@
 	system.o environ.o process.o \
 	job_class.o job_process.o job.o event.o event_operator.o blocked.o \
 	parse_job.o parse_conf.o conf.o control.o \
-	session.o log.o state.o \
+	session.o log.o state.o xdg.o \
 	com.ubuntu.Upstart.o \
 	com.ubuntu.Upstart.Job.o com.ubuntu.Upstart.Instance.o \
 	$(NIH_LIBS) \
@@ -317,7 +317,7 @@
 	system.o environ.o process.o \
 	job_class.o job_process.o job.o event.o event_operator.o blocked.o \
 	parse_job.o parse_conf.o conf.o control.o \
-	session.o log.o state.o \
+	session.o log.o state.o xdg.o \
 	com.ubuntu.Upstart.o \
 	com.ubuntu.Upstart.Job.o com.ubuntu.Upstart.Instance.o \
 	$(NIH_LIBS) \
@@ -331,7 +331,7 @@
 	system.o environ.o process.o \
 	job_class.o job_process.o job.o event.o event_operator.o blocked.o \
 	parse_job.o parse_conf.o conf.o control.o \
-	session.o log.o state.o \
+	session.o log.o state.o xdg.o \
 	com.ubuntu.Upstart.o \
 	com.ubuntu.Upstart.Job.o com.ubuntu.Upstart.Instance.o \
 	$(NIH_LIBS) \
@@ -345,7 +345,7 @@
 	system.o environ.o process.o \
 	job_class.o job_process.o job.o event.o event_operator.o blocked.o \
 	parse_job.o parse_conf.o control.o \
-	session.o log.o state.o \
+	session.o log.o state.o xdg.o \
 	com.ubuntu.Upstart.o \
 	com.ubuntu.Upstart.Job.o com.ubuntu.Upstart.Instance.o \
 	$(NIH_LIBS) \
@@ -366,7 +366,7 @@
 	system.o environ.o process.o \
 	job_class.o job_process.o job.o event.o event_operator.o blocked.o \
 	parse_job.o parse_conf.o conf.o control.o \
-	session.o log.o state.o \
+	session.o log.o state.o xdg.o \
 	com.ubuntu.Upstart.o \
 	com.ubuntu.Upstart.Job.o com.ubuntu.Upstart.Instance.o \
 	$(NIH_LIBS) \
 
=== modified file 'init/control.c'
--- init/control.c	2013-01-22 15:51:01 +0000
+++ init/control.c	2013-01-23 13:00:29 +0000
@@ -76,13 +76,6 @@
 int use_session_bus = FALSE;
 
 /**
- * user_mode:
- *
- * If TRUE, upstart runs in user session mode.
- **/
-int user_mode = FALSE;
-
-/**
  * control_server_address:
  *
  * Address on which the control server may be reached.
@@ -112,6 +105,8 @@
  **/
 NihList *control_conns = NULL;
 
+/* External definitions */
+extern int user_mode;
 
 /**
  * control_init:
 
=== modified file 'init/job_process.c'
--- init/job_process.c	2013-01-21 22:44:27 +0000
+++ init/job_process.c	2013-01-23 13:00:29 +0000
@@ -66,6 +66,7 @@
 #include "job.h"
 #include "errors.h"
 #include "control.h"
+#include "xdg.h"
 
 
 /**
@@ -2197,7 +2198,7 @@
 	nih_assert (class->name);
 
 	/* Override, primarily for tests */
-	if (getenv (LOGDIR_ENV)) {
+	if (getenv (LOGDIR_ENV) && ! user_mode) {
 		dir = nih_strdup (NULL, getenv (LOGDIR_ENV));
 		nih_debug ("Using alternative directory '%s' for logs", dir);
 	} else {
 
=== modified file 'init/main.c'
--- init/main.c	2013-01-22 15:53:05 +0000
+++ init/main.c	2013-01-23 13:00:29 +0000
@@ -949,6 +949,11 @@
 	if (log_dir)
 		goto out;
 
+	if (user_mode) {
+		log_dir = get_user_log_dir ();
+		return;
+	}
+
 	log_dir = JOB_LOGDIR;
 
 	dir = getenv (LOGDIR_ENV);
 
=== modified file 'init/man/init.5'
--- init/man/init.5	2013-01-16 19:20:54 +0000
+++ init/man/init.5	2013-01-23 13:00:29 +0000
@@ -149,11 +149,8 @@
 of that job name: any subsequently searched directory that contains a
 job of the same name will be ignored. The same applies for override
 files: only the first override file found in the search order will be
-applied. Note that an override file does not have to be in the same
-directory as the job it overrides, but if not in the directory of the
-job it overrides, it must be in an
-.I earlier
-directory to that which contains the job.
+applied. Note that an override file can be in the same directory or
+earlier to that directory which contains the job file.
 
 Jobs in these locations are expected to launch the user's session.
 Upstart will try to parent all spawned process with the aid of
@@ -166,6 +163,12 @@
 When running in User Session mode, Upstart will kill all job processes
 on session logout or shutdown.
 
+All log output will be in
+.I $XDG_CACHE_HOME/upstart
+which defaults to
+.I $HOME/.cache/upstart
+.
+
 .\"
 .SS Configuration File Format
 Each line begins with a configuration stanza and continues until either
@@ -664,19 +667,23 @@
 .B log
 .RS
 .B
-Only applies to system jobs:
+Only applies to system and user session jobs:
 if specified by user jobs, the job will be considered to have specified
 the value
 .BR none "."
 
-For system jobs, if \fBlog\fR is specified, standard input is connected
+For system and user session jobs jobs, if \fBlog\fR is specified, standard input is connected
 to
 .IR /dev/null ","
 and standard output and standard error are connected to a pseudo-tty
 which logs all job output.
 
 Output is logged to file
-.IR /var/log/upstart/<job-log-file> "."
+.IR /var/log/upstart/<job-log-file>
+or
+.IR $XDG_CACHE_HOME/upstart/<job-log-file>
+for system and user session jobs respectively.
+
 If a job has specified \fBinstance\fR,
 .I <job-log-file>
 will equate to
@@ -1019,18 +1026,30 @@
 .TP
 .I $HOME/.init/*.conf
 User job configuration files.
-Note that you may make the directory
-.I $HOME/.init/
-a symbolic link to
-.BR $HOME/.config/upstart/ "."
 .
 .TP
 .I $HOME/.init/*.override
 User job override files.
 .
 .TP
+.I $XDG_CONFIG_HOME/upstart/*.conf
+User session job configuration files. See
+.B User Session Mode
+for other locations.
+.
+.TP
+.I $XDG_CONFIG_HOME/upstart/*.override
+User session job override files. See
+.B User Session Mode
+for other locations.
+.
+.TP
 .I /var/log/upstart/*.log
 Default location of system job output logs.
+.
+.TP
+.I $XDG_CACHE_HOME/upstart/*.log
+Default location of user session job output logs.
 .RE
 .\"
 .SH AUTHOR
 
=== modified file 'init/man/init.8'
--- init/man/init.8	2012-12-18 17:40:49 +0000
+++ init/man/init.8	2013-01-23 13:00:29 +0000
@@ -67,6 +67,8 @@
 .B \-\-confdir \fIdirectory\fP
 Read job configuration files from a directory other than
 \fI/etc/init\fP.
+For user session mode, read job configuration files from this
+directory at highest priority.
 .\"
 .TP
 .B \-\-default-console \fIvalue\fP
@@ -81,7 +83,8 @@
 .TP
 .B \-\-logdir \fIdirectory\fP
 Write job output log files to a directory other than
-\fI/var/log/upstart\fP.
+\fI/var/log/upstart\fP (system mode) or \fI$XDG_CACHE_HOME/upstart\fP
+(user session mode).
 .\"
 .TP
 .B \-\-no\-log
 
=== modified file 'init/tests/test_xdg.c'
--- init/tests/test_xdg.c	2012-12-18 14:16:10 +0000
+++ init/tests/test_xdg.c	2013-01-23 13:00:29 +0000
@@ -22,12 +22,26 @@
 #include <nih/string.h>
 #include <nih/test.h>
 
+#include <limits.h>
 #include <stdlib.h>
-#include <limits.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <unistd.h>
 
 #include "xdg.h"
 
 void
+_test_dir_created (char * dirname) {
+	struct stat statbuf;
+
+	TEST_EQ (lstat (dirname, &statbuf), 0);
+	TEST_TRUE (S_ISDIR (statbuf.st_mode));
+	
+	/* Check that the created directory has 0700 permissions, as per XDG spec */
+	TEST_EQ (0700, statbuf.st_mode & (S_IRWXU | S_IRWXG | S_IRWXO));
+}
+
+void
 test_get_home_subdir (void)
 {
 	char    dirname[PATH_MAX];
@@ -40,7 +54,7 @@
 	TEST_EQ (unsetenv ("HOME"), 0);
 
 	TEST_ALLOC_FAIL {
-		dir = get_home_subdir ("test");
+		dir = get_home_subdir ("test", FALSE);
 		TEST_EQ_P (dir, NULL);
 	}
 
@@ -54,7 +68,7 @@
 			expected = NIH_MUST (nih_sprintf (NULL, "%s/test", dirname));
 		}
 
-		dir = get_home_subdir ("test");
+		dir = get_home_subdir ("test", FALSE);
 
 		if (test_alloc_failed) {
 			TEST_EQ_P (dir, NULL);
@@ -69,28 +83,29 @@
 }
 
 void
-test_get_config_home (void)
+_test_get_home (char * env_var_name, char * dir_name, char * (*function)(void))
 {
 	char   dirname[PATH_MAX];
+	char   onemore[PATH_MAX];
 	char * outname;
 	char * expected;
 
-	TEST_FUNCTION ("xdg_get_config_home");
-
 	TEST_FEATURE ("with HOME set and without environment override");
 	TEST_FILENAME (dirname);
 	TEST_EQ (setenv ("HOME", dirname, 1), 0);
-	TEST_EQ (unsetenv ("XDG_CONFIG_HOME"), 0);
+	TEST_EQ (mkdir (dirname, 0755), 0);
+	TEST_EQ (unsetenv (env_var_name), 0);
 	TEST_ALLOC_FAIL {
 		TEST_ALLOC_SAFE {
-			expected = NIH_MUST (nih_sprintf (NULL, "%s/.config", dirname));
+			expected = NIH_MUST (nih_sprintf (NULL, "%s/%s", dirname, dir_name));
 		}
 
 		outname = NULL;
-		outname = xdg_get_config_home ();
+		outname = function ();
 
 		if (! test_alloc_failed) {
 			TEST_EQ_STR (outname, expected);
+			_test_dir_created (expected);
 		} else {
 			TEST_EQ_P (outname, NULL);
 		}
@@ -98,83 +113,131 @@
 		if (outname)
 			nih_free (outname);
 
-		nih_free(expected);
+		rmdir (expected);
+		nih_free (expected);
 	}
 
 	TEST_FEATURE ("with HOME set and with empty environment override");
-	TEST_EQ (setenv ("XDG_CONFIG_HOME", "", 1), 0);
-
-
-	TEST_ALLOC_FAIL {
-		TEST_ALLOC_SAFE {
-			expected = NIH_MUST (nih_sprintf (NULL, "%s/.config", dirname));
-		}
-		outname = NULL;
-		outname = xdg_get_config_home();
-
-		if (test_alloc_failed) {
-			TEST_EQ_P (outname, NULL);
-		} else {
-			TEST_EQ_STR (outname, expected);
-		}
-		if (outname)
-			nih_free (outname);
-		nih_free(expected);
+	TEST_EQ (setenv (env_var_name, "", 1), 0);
+
+
+	TEST_ALLOC_FAIL {
+		TEST_ALLOC_SAFE {
+			expected = NIH_MUST (nih_sprintf (NULL, "%s/%s", dirname, dir_name));
+		}
+		outname = NULL;
+		outname = function ();
+
+		if (test_alloc_failed) {
+			TEST_EQ_P (outname, NULL);
+		} else {
+			TEST_EQ_STR (outname, expected);
+			_test_dir_created (expected);
+		}
+		if (outname)
+			nih_free (outname);
+		rmdir (expected);
+		nih_free (expected);
+	}
+
+	TEST_FEATURE ("with HOME set and with relative environment override");
+	TEST_EQ (setenv (env_var_name, "../", 1), 0);
+
+
+	TEST_ALLOC_FAIL {
+		TEST_ALLOC_SAFE {
+			expected = NIH_MUST (nih_sprintf (NULL, "%s/%s", dirname, dir_name));
+		}
+		outname = NULL;
+		outname = function ();
+
+		if (test_alloc_failed) {
+			TEST_EQ_P (outname, NULL);
+		} else {
+			TEST_EQ_STR (outname, expected);
+			_test_dir_created (expected);
+		}
+		if (outname)
+			nih_free (outname);
+		rmdir (expected);
+		nih_free (expected);
 	}
 
 	TEST_FEATURE ("with HOME set and with environment override");
-	expected = NIH_MUST (nih_strdup (NULL, "/home/me/.config-test"));
-	TEST_EQ (setenv ("XDG_CONFIG_HOME", expected, 1), 0);
+	TEST_FILENAME (onemore);
+	expected = NIH_MUST (nih_sprintf (NULL, "%s", onemore));
+	TEST_EQ (setenv (env_var_name, expected, 1), 0);
 
 	TEST_ALLOC_FAIL {
 		outname = NULL;
-		outname = xdg_get_config_home();
+		outname = function ();
 
 		if (test_alloc_failed) {
 			TEST_EQ_P (outname, NULL);
 		} else {
 			TEST_EQ_STR (outname, expected);
+			_test_dir_created (expected);
 		}
 		if (outname)
 			nih_free (outname);
+		rmdir (expected);
 	}
 
+	TEST_EQ (rmdir (dirname), 0);
+
 	TEST_FEATURE ("without HOME set and with environment override");
 	TEST_EQ (unsetenv ("HOME"), 0);
 
 	TEST_ALLOC_FAIL {
 		outname = NULL;
-		outname = xdg_get_config_home();
+		outname = function ();
 
 		if (test_alloc_failed) {
 			TEST_EQ_P (outname, NULL);
 		} else {
 			TEST_EQ_STR (outname, expected);
+			_test_dir_created (expected);
 		}
 		if (outname)
 			nih_free (outname);
+		rmdir (expected);
 	}
 	nih_free(expected);
 
 	TEST_FEATURE ("without HOME set and with empty environment override");
-	TEST_EQ (setenv ("XDG_CONFIG_HOME", "", 1), 0);
+	TEST_EQ (setenv (env_var_name, "", 1), 0);
 
 	TEST_ALLOC_FAIL {
 		outname = NULL;
-		outname = xdg_get_config_home();
+		outname = function ();
 		TEST_EQ_P (outname, NULL);
 	}
 
 	TEST_FEATURE ("without HOME set and without environment override");
-	TEST_EQ (unsetenv ("XDG_CONFIG_HOME"), 0);
+	TEST_EQ (unsetenv (env_var_name), 0);
 	TEST_ALLOC_FAIL {
 		outname = NULL;
-		outname = xdg_get_config_home();
+		outname = function ();
 		TEST_EQ_P (outname, NULL);
 	}
 }
 
 void
+test_get_config_home (void)
+{
+	TEST_FUNCTION ("xdg_get_config_home");
+	_test_get_home ("XDG_CONFIG_HOME", ".config", &xdg_get_config_home);
+
+}
+
+void
+test_get_cache_home (void)
+{
+	TEST_FUNCTION ("xdg_get_cache_home");
+	_test_get_home ("XDG_CACHE_HOME", ".cache", &xdg_get_cache_home);
+}
+
+void
 test_get_config_dirs (void)
 {
 	char   **dirs = NULL;
@@ -260,12 +323,12 @@
 	TEST_FEATURE ("with HOME set");
 	TEST_FILENAME (dirname);
 	TEST_EQ (setenv ("HOME", dirname, 1), 0);
+	TEST_EQ (mkdir (dirname, 0755), 0);
 	TEST_EQ (unsetenv ("XDG_CONFIG_HOME"), 0);
 	TEST_EQ (unsetenv ("XDG_CONFIG_DIRS"), 0);
 
 	TEST_ALLOC_FAIL {
 		TEST_ALLOC_SAFE {
-			dirs = NULL;
 			expected = nih_str_array_new (NULL);
 			path = NIH_MUST (nih_sprintf (NULL, "%s/.config/upstart", dirname));
 			assert (nih_str_array_add (&expected, NULL, NULL, path));
@@ -282,15 +345,94 @@
 			TEST_EQ_P (dirs, NULL);
 		} else {
 			TEST_EQ_STR (dirs[0], expected[0]);
+			//_test_dir_created (expected[0]);
 			TEST_EQ_STR (dirs[1], expected[1]);
 			TEST_EQ_STR (dirs[2], "/etc/xdg/upstart");
 			TEST_EQ_STR (dirs[3], SYSTEM_USERCONFDIR);
 			TEST_EQ (dirs[4], NULL);
 			nih_free (dirs);
 		}
-		nih_free(expected);
-	}
-
+		TEST_ALLOC_SAFE {
+			rmdir (expected[0]);
+			path = nih_sprintf (NULL, "%s/.config", dirname);
+			rmdir (path);
+			nih_free (path);
+			nih_free (expected);
+		}
+	}
+
+	TEST_FEATURE ("with some invalid XDG_CONFIG_DIRS");
+	TEST_EQ (setenv ("XDG_CONFIG_DIRS", "/etc/xdg/xdg-subdir:../haha:/etc/xdg", 1), 0);
+
+	TEST_ALLOC_FAIL {
+		TEST_ALLOC_SAFE {
+			expected = nih_str_array_new (NULL);
+			path = NIH_MUST (nih_sprintf (NULL, "%s/.config/upstart", dirname));
+			assert (nih_str_array_add (&expected, NULL, NULL, path));
+			nih_free(path);
+			path = NIH_MUST (nih_sprintf (NULL, "%s/.init", dirname));
+			assert (nih_str_array_add (&expected, NULL, NULL, path));
+			nih_free(path);
+		}
+
+		dirs = NULL;
+		dirs = get_user_upstart_dirs ();
+
+		if (test_alloc_failed) {
+			TEST_EQ_P (dirs, NULL);
+		} else {
+			TEST_EQ_STR (dirs[0], expected[0]);
+			_test_dir_created (expected[0]);
+			TEST_EQ_STR (dirs[1], expected[1]);
+			TEST_EQ_STR (dirs[2], "/etc/xdg/xdg-subdir/upstart");
+			TEST_EQ_STR (dirs[3], "/etc/xdg/upstart");
+			TEST_EQ_STR (dirs[4], SYSTEM_USERCONFDIR);
+			TEST_EQ (dirs[5], NULL);
+			nih_free (dirs);
+		}
+		TEST_ALLOC_SAFE {
+			rmdir (expected[0]);
+			path = nih_sprintf (NULL, "%s/.config", dirname);
+			rmdir (path);
+			nih_free (path);
+			nih_free (expected);
+		}
+	}
+	TEST_EQ (rmdir (dirname), 0);
+}
+
+void
+test_get_user_log_dir (void)
+{
+	char dirname[PATH_MAX];
+	char         *expected;
+	char             *path;
+
+	TEST_FUNCTION ("get_user_log_dir");
+	TEST_FEATURE ("with HOME set");
+	TEST_FILENAME (dirname);
+	TEST_EQ (setenv ("HOME", dirname, 1), 0);
+	TEST_EQ (mkdir (dirname, 0755), 0);
+	TEST_EQ (unsetenv ("XDG_CACHE_HOME"), 0);
+
+	expected = nih_sprintf (NULL, "%s/.cache/upstart", dirname);
+
+	TEST_ALLOC_FAIL {
+		path = get_user_log_dir ();
+		if (test_alloc_failed) {
+			TEST_EQ_P (path, NULL);
+		} else {
+			TEST_EQ_STR (path, expected);
+			_test_dir_created (expected);
+			nih_free (path);
+		}
+	}
+	rmdir (expected);
+	nih_free (expected);
+	path = nih_sprintf (NULL, "%s/.cache", dirname);
+	rmdir (path);
+	nih_free (path);
+	rmdir (dirname);
 }
 
 int
@@ -301,6 +443,8 @@
 	test_get_config_home ();
 	test_get_config_dirs ();
 	test_get_user_upstart_dirs ();
+	test_get_cache_home ();
+	test_get_user_log_dir ();
 
 	return 0;
 }
 
=== modified file 'init/xdg.c'
--- init/xdg.c	2012-12-18 16:31:55 +0000
+++ init/xdg.c	2013-01-23 13:00:29 +0000
@@ -24,6 +24,8 @@
 #endif /* HAVE_CONFIG_H */
 
 #include <stdlib.h>
+#include <sys/stat.h>
+#include <sys/types.h>
 
 #include <nih/alloc.h>
 #include <nih/logging.h>
@@ -33,22 +35,37 @@
 #include "xdg.h"
 
 /**
+ * user_mode:
+ *
+ * If TRUE, upstart runs in user session mode.
+ **/
+int user_mode = FALSE;
+
+/**
  * get_home_subdir:
- *
- * Construct path to directory in user's HOME dir.
+ * @suffix: sub-directory name
+ * @create: flag to create sub-directory
+ * 
+ * Construct path to @suffix directory in user's HOME dir. If @create
+ * flag is TRUE, also attempt to create that directory. Errors upon
+ * directory creation are ignored.
  * 
  * Returns: newly-allocated path, or NULL on error.
- */
-
+ **/
 char *
-get_home_subdir (const char * suffix)
+get_home_subdir (const char * suffix, int create)
 {
 	char *dir;
-	nih_assert (suffix && suffix[0]);
+	nih_assert (suffix != NULL);
+	nih_assert (suffix[0]);
 	
 	dir = getenv ("HOME");
-	if (dir && dir[0]) {
+	if (dir && dir[0] == '/') {
 		dir = nih_sprintf (NULL, "%s/%s", dir, suffix);
+		if (! dir)
+			return NULL;
+		if (create)
+			mkdir (dir, 0700);
 		return dir;
 	}
 
@@ -56,6 +73,39 @@
 }
 
 /**
+ * xdg_get_cache_home:
+ *
+ * Determine an XDG compliant XDG_CACHE_HOME
+ *
+ * Returns: newly-allocated path, or NULL on error.
+ **/
+char *
+xdg_get_cache_home (void)
+{
+	nih_local char  **env = NULL;
+	char             *dir;
+
+	dir = getenv ("XDG_CACHE_HOME");
+	
+	if (dir && dir[0] == '/') {
+		mkdir (dir, 0700);
+		dir = nih_strdup (NULL, dir);
+		return dir;
+	}
+
+	/* Per XDG spec, we should only create dirs, if we are
+	 * attempting to write and the dir is not there. Here we
+	 * anticipate logging to happen really soon now, hence we
+	 * pre-create the cache dir. That does not protect us from
+	 * this directory disappering while upstart is running. =/
+	 * hence this dir should be created each time we try to write
+	 * log... */
+	dir = get_home_subdir (".cache", TRUE);
+
+	return dir;
+}
+
+/**
  * xdg_get_config_home:
  *
  * Determine an XDG compliant XDG_CONFIG_HOME
@@ -70,12 +120,17 @@
 
 	dir = getenv ("XDG_CONFIG_HOME");
 	
-	if (dir && dir[0]) {
+	if (dir && dir[0] == '/') {
+		mkdir (dir, 0700);
 		dir = nih_strdup (NULL, dir);
 		return dir;
 	}
 
-	dir = get_home_subdir (".config");
+	/* Per XDG spec, we should only create dirs, if we are
+	 * attempting to write to the dir. But we only read config
+	 * dir. But we rather create it, to place inotify watch on
+	 * it. */
+	dir = get_home_subdir (".config", TRUE);
 
 	return dir;
 }
@@ -134,6 +189,7 @@
 	if (path && path[0]) {
 	        if (! nih_strcat_sprintf (&path, NULL, "/%s", INIT_XDG_SUBDIR))
 			goto error;
+		mkdir (path, 0700);
 		if (! nih_str_array_add (&all_dirs, NULL, NULL, path))
 			goto error;
 		nih_free (path);
@@ -141,7 +197,7 @@
 	}
 
 	/* Legacy User's: ~/.init */
-	path = get_home_subdir (USERCONFDIR);
+	path = get_home_subdir (USERCONFDIR, FALSE);
 	if (! path)
 		goto error;
 
@@ -158,6 +214,8 @@
 		goto error;
 
 	for (char **p = dirs; p && *p; p++) {
+		if (*p[0] != '/')
+			continue;
 		if (! nih_strcat_sprintf (p, NULL, "/%s", INIT_XDG_SUBDIR))
 			goto error;
 		if (! nih_str_array_add (&all_dirs, NULL, NULL, *p))
@@ -185,3 +243,26 @@
 	return NULL;
 }
 
+/**
+ * get_user_log_dir:
+ *
+ * Constructs an XDG compliant path to a cache directory in the user's
+ * home directory. It can be used to store logs.
+ *
+ * Returns: newly-allocated array of paths, or NULL or error.
+ **/
+char *
+get_user_log_dir (void)
+{
+	nih_local char *path = NULL;
+	char *dir = NULL;
+	path = xdg_get_cache_home ();
+	if (path && path[0] == '/') {
+		dir = nih_sprintf (NULL, "%s/%s", path, INIT_XDG_SUBDIR);
+		if (! dir)
+			return NULL;
+		mkdir (dir, 0700);
+		return dir;
+	}
+	return NULL;
+}
 
=== modified file 'init/xdg.h'
--- init/xdg.h	2012-12-18 13:58:23 +0000
+++ init/xdg.h	2013-01-23 13:00:29 +0000
@@ -25,18 +25,24 @@
 
 NIH_BEGIN_EXTERN
 
-char *    get_home_subdir        (const char * suffix)
+char *    get_home_subdir        (const char * suffix, int create)
 	__attribute__ ((malloc, warn_unused_result));
 
 char *    xdg_get_config_home    (void)
 	__attribute__ ((malloc, warn_unused_result));
 
+char *    xdg_get_cache_home     (void)
+	__attribute__ ((malloc, warn_unused_result));
+
 char **   xdg_get_config_dirs    (void)
 	__attribute__ ((malloc, warn_unused_result));
 
 char **   get_user_upstart_dirs  (void)
 	__attribute__ ((malloc, warn_unused_result));
 
+char *    get_user_log_dir       (void)
+	__attribute__ ((malloc, warn_unused_result));
+
 NIH_END_EXTERN
 
 #endif /* INIT_XDG_H */