Did you mean to commit this to master? http://cgit.freedesktop.org/dbus/dbus/commit/?id=6f9077ee870ad02119facf83d1293301b4535c3b

I've reverted it, because it broke the build, and introduced a tonne of warnings:

• the prototypes' arguments should be '(void)' not '()';
• the functions should return 'const char *', not 'char *';
• there's a bunch of trailing whitespace, and inconsistent spacing between function name and ( in function calls;
• dbus-sysdeps-util-unix.c:118: error: implicit declaration of ‘_dbus_config_debug_output’

I discussed this briefly with Christian Dywan on #dbus, and we're not overly convinced about this change. In his words, “in my opinion it's a purely stylistic change that hides variable names and could lead to mistake those for configuration switches”.

This patch felt a bit like it added a sin-bin, rather than putting accessors where they would logically live. Not all the environment variables are the same. The xdg directory functions, for instance, should probably live in sysdeps, mirroring GLib's g_get_user_data_dir() and friends, whereas the variables that influence the tests are a different kettle of fish.

(In reply to comment #1)
> Did you mean to commit this to master?
> http://cgit.freedesktop.org/dbus/dbus/commit/?id=6f9077ee870ad02119facf83d1293301b4535c3b
> 
> I've reverted it, because it broke the build, and introduced a tonne of
> warnings:
> 
> • the prototypes' arguments should be '(void)' not '()';
> • the functions should return 'const char *', not 'char *';
> • there's a bunch of trailing whitespace, and inconsistent spacing between
> function name and ( in function calls;
> • dbus-sysdeps-util-unix.c:118: error: implicit declaration of
> ‘_dbus_config_debug_output’
> 

> I discussed this briefly with Christian Dywan on #dbus, and we're not overly
> convinced about this change. In his words, “in my opinion it's a purely
> stylistic change that hides variable names 

> This patch felt a bit like it added a sin-bin, rather than putting accessors
> where they would logically live. 

You mean instead of 

char *
_dbus_config_starter_bus_type()

to use  

char *
_dbus_get_starter_bus_type()

and 

void 
_dbus_set_starter_bus_type(char *)


Ralf

We already have things like:
_dbus_append_keyring_directory_for_credentials()
_dbus_get_standard_system_servicedirs()
_dbus_append_system_config_file()

just add more stuff like that to sysdeps. ONLY if the implementation actually varies by platform ... don't just wrap getenvs

I don't understand this patch at all; it doesn't actually abstract anything cross-platform. It is just a 1-1 mapping with the getenv calls. Some of the env vars make no sense on Linux, some make no sense on Windows, and never will. Others may logically be cross-platform abstracted. Others are just debug stuff (so it hardly matters, but Windows can use env vars for debug stuff, afaik)

I think the sensible patch is probably to abstract _specific_ things in a logical way for those things. For example, certain file locations. But the abstraction needs to deal with "logical" locations, such as "where to put the .service files" or things like that, rather than just matching the env var names.

I don't think we need something called "dbus_config"

(In reply to comment #3)
> I think the sensible patch is probably to abstract _specific_ things in a
> logical way for those things. For example, certain file locations. But the
> abstraction needs to deal with "logical" locations, such as "where to put the
> .service files" or things like that, rather than just matching the env var
> names.

What he said.