[patch] standard_session_servicedirs (was Re: OLPC and .service files in a users home directory)

[John (J5) Palmieri]

On Wed, 2006-11-01 at 11:17 -0500, Havoc Pennington wrote:
> Hi,
> 
> John (J5) Palmieri wrote:
> > Ask and you shall receive, complete with a test case. 
> 
> Thanks! comments inline
>    	
> >    _DBUS_ASSERT_ERROR_IS_CLEAR (error);
> >      
> > -  directory = _dbus_string_get_const_data (dir);
> > +  directory = _dbus_string_get_data (dir);
> >  	
> >    if (stat (directory, &sb) < 0)
> 
> This change doesn't look right to me. "directory" is const. get_data() 
> will fail if used on a const DBusString too, which this could be.

Stray edit that made it into the diff.  This has been removed.

> > +/*
> > + * Returns the standard directories for a session bus to look for service 
> > + * activation files 
> 
> The doc comment needs "/**" to be picked up

Fixed.

> > +
> > +      if (!_dbus_string_append (&path, xdg_data_home))
> > +        {
> > +          _dbus_string_free (&path);
> > +          goto oom;
> > +        }
> > +
> > +      if (!_dbus_string_append (&path, DBUS_UNIX_STANDARD_SESSION_SERVICEDIR))
> 
> It would be good to use _dbus_concat_dir_and_file() so things work 
> whether or not there's a trailing "/"
> 
> (same for the other cases in this function)

Fixed

> > +      cpath = _dbus_strdup ("/usr/share"DBUS_UNIX_STANDARD_SESSION_SERVICEDIR);
> 
> This should be --datadir from configure, rather than /usr/share 
> hardcoded, or at least --datadir from configure should also be included

I added DBUS_DATADIR to the list separate from the XDG list.

> > +      cpath = _dbus_strdup ("/usr/local/share"DBUS_UNIX_STANDARD_SESSION_SERVICEDIR);
> 
> Does the xdg spec require this I guess?

Yes it does.

> > +#define DBUS_UNIX_STANDARD_SESSION_SERVICEDIR "/dbus-1/services"
> > +
> 
> Could just put this in the .c file to avoid misuse

Fixed.

> >  /** An opaque string type */
> >  typedef struct DBusString DBusString;
> >  
> > +/** avoid circular includes with DBusList */
> > +struct DBusList; 
> 
> Why not do this the same way as the DBusString above it (i.e. move the 
> typedef over from dbus-list.h)

Fixed.


I also took Thiago's suggestion from irc to simplify the code by
creating a colon delimited list and then parsing this with the same code
I had for parsing XDG_DATA_DIRS.  This turned out to make the code much
more readable.

-- 
John (J5) Palmieri <johnp at redhat.com>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: dbus-standard-session-servicedirs-2.patch
Type: text/x-patch
Size: 14760 bytes
Desc: not available
Url : http://lists.freedesktop.org/archives/dbus/attachments/20061101/56577521/dbus-standard-session-servicedirs-2.bin