Created attachment 8078 [details] [review]
initial patch

there's no need for the #ifdef stuff here afaict, instead create an appropriate cross-platform abstraction.

I would suggest that the cross-platform abstraction returns the default config files (session and system). On UNIX, the function would just return the #defines, and on Windows it would look up dynamically the name of Program Files.

This would then be used to get the right config file in main.c when --session and --system are supplied.

(In reply to comment #2)
> there's no need for the #ifdef stuff here afaict, instead create an 
> appropriate cross-platform abstraction.
> 
> I would suggest that the cross-platform abstraction returns the default config
> files (session and system). On UNIX, the function would just return the
> #defines, and on Windows it would look up dynamically the name of Program
> Files.
> 
> This would then be used to get the right config file in main.c when --session
> and --system are supplied.
> 

This will be not enough. To be able to use relative pathes in *all* config files the working dir of the daemon has to be changed to the installation root. 

You may also need to change other places... config-loader-libxml definitely isn't the right place to be sorting this out, though, and it should not require #ifdef DBUS_WIN

(In reply to comment #4)
> You may also need to change other places... 

it is only required to change the call to bus_config_parser_new(), where should it changed else ? 

> config-loader-libxml definitely  isn't the right place to be sorting this out, 
were else ? 

bus_config_parser_new() has to be called with a fixed basedir setting and this is only possible in bus_config_load().  To be complete it should be added to config-loader-libexpat, of course.

Another approach would be to extend bus_config_load() with a basedir parameter, but this affected much more calls as the above mentioned patch. 

> though, and it should not require #ifdef DBUS_WIN

On unix there is a complete different behavior. The basedir is the dir where the config files are located. 

  if (!_dbus_string_get_dirname (file, &dirname))
    {
      _DBUS_SET_OOM (error);
      goto failed;
    }
  
  parser = bus_config_parser_new (&dirname, is_toplevel, parent);

The alternative is to provide an empty returning _dbus_get_working_dir() function call, which implies that there is no working dir on unix. Is this better ? 

Created attachment 9109 [details] [review]
 update

This approach defines a new function bus_config_get_base_dir() in which the windows specific base dir is set (comes later)

I'm missing something about this patch. Isn't the problem that the directory passed in to bus_context_new from main.c is not the right directory? So why don't we just fix the directory passed in to bus_context_new in main.c?

(In reply to comment #7)
> I'm missing something about this patch. Isn't the problem that the directory
> passed in to bus_context_new from main.c is not the right directory? So why
> don't we just fix the directory passed in to bus_context_new in main.c?
> 

See the trace call 

main 
bus_context_new (config_file,...)
  bus_config_load (config_file,...)
     if (!_dbus_string_get_dirname (config_file, &dirname)) 
        bus_config_parser_new (&dirname [1] ,...)

-> dirname is dir of config file, but it should be something else 

This is the installation structure 
dbus-root [2] 
   bin
       dbus-daemon 
       debug/dbus-daemon 
       release/dbus-daemon 
   etc [3]
       session.conf  
   data/dbus-1/services/
       debug-echo.service

The dirname in [1] should be dbus-root [2] and not the dir of session.conf [3] for example to be able to address data/dbus-1/services




   

I don't get it - the services are in /usr/share/dbus-1/services on unix, too. So they should not be located relative to the config file anyway.
How is windows different?

(In reply to comment #9)
> I don't get it - the services are in /usr/share/dbus-1/services on unix, too.
> So they should not be located relative to the config file anyway.
> How is windows different?

This path couldn't be determind on compile time only on installation time. Reread my initial comment 

>On win32 the installation dir isn't known at compile time because it depends on
>os language (german=c:\Programme\dbus, english=c:\Program Files\dbus) 

That means there could not be any hardcoded location. There is a change to use environment variables for determing pathes, but this does not help in all areas. 

Additional it may be that several application uses there own dbus package, which  makes it neccessary to hold all files of a dbus instalation in one path. 

> This path couldn't be determind on compile time only on installation time.

So I'm expecting the patch to include replacing code that hardcodes the path with a function that retrieves the path.

Why isn't the change in the place in the code where the hardcoded path is currently used?

Also, you say the problem is locating service files, but I don't see what service files have to do with config file parsing, which is what the patch changes.

Created attachment 9144 [details] [review]
completed patch 

This patch allows the <servicedir> attribute of a config file to be relative to the base dir of the installation

BTW: If this patch looks to messy, i would reask you how to implement the  required feature, which is: 

- Make is able to use relative directories in config files based on the installation root, which has to be computed at runtime *not* at compile time.
The installation root is the one level up dir of the bin dir where the installed dbus-daemon dir lives according to the following directory struture. 

installation root 
  bin\dbus-daemon.exe (used by gcc on windows named MinGW)
  bin\release\dbus-daemon.exe (used by msvc on windows)
  bin\debug\dbus-daemon.exe (used by msvc on windows)
  etc\session.config
  data\dbus-1\services\...service 

it looks like part of the patch is missing? it only has the windows part.

Created attachment 9198 [details] [review]
complete update

- added unix support
- added msvc directory support

I just don't understand this patch. If the problem is a hardcoded path on linux, then the patch should have a "minus" line that contains the hardcoded path! This just can't be changing the right place.

There are two different things:
 a) a set of install locations (on Windows, queried at runtime; on unix, 
   determined at compile time)
 b) the location of the config file, which some paths in the config file are 
   relative to

This patch appears to mix those two up. I think the patch should change the place where a) is currently provided on unix to instead call platform-dependent functions like get_servicedir, get_configdir, or whatever.

b) should require no changes for Windows that I see. The service dirs are in /usr/share on unix, they are *not* relative to the config file. So service files should be located using case a) which does not involve the libxml/expat files that I can see.

On the nitpick side, it looks like there are tabs or other whitespace problems (look at the "diff" for the attachment to the bug to see them), the variable baseDir should be "basedir" or "base_dir", and for function definitions (but not declarations) the function name should start in column 0, i.e. a line break after the return type.

the related patch is obsolate and should be closed