[pulseaudio-discuss] [PATCH] conf-parser: add support for .d directories

[Tanu Kaskinen]

On Tue, 2015-05-26 at 09:35 +0200, David Henningsson wrote:
> 
> On 2015-05-22 00:21, Tanu Kaskinen wrote:
> > This allows a configuration scheme where prior to loading
> > configuration from "somefile", the parser first loads configuration
> > from files in directory "somefile.d". This feature is currently
> > enabled only for client.conf and daemon.conf.
> 
> Thanks! I'm very much in favour of this added flexibility.
> 
> There should also be some user documentation about it though. Right now 
> I'm not totally sure how the different files would override each other 
> (i e, what file takes priority) in case the same key appears in many files.

Files from client.conf.d are loaded before loading client.conf. The
system administrator's changes should override any files provided by the
distribution, and I think loading client.conf last achieves this goal
best. And yes, this should be documented. I'll do that in v2.

It occurred to me now that this patch introduces some annoying corner
cases. What if there are files in ~/.config/pulse/client.conf.d but
~/.config/pulse/client.conf doesn't exist? The current code behaves so
that if ~/.config/pulse/client.conf doesn't exist, then we'll load
configuration from /etc/pulse. That doesn't seem right, but I don't see
it as a big problem either, because this scenario is probably extremely
rare.

I think we should always load the configuration from both the system
location and from the user location (except when the PULSE_CLIENTCONFIG
environment variable is set), with the user configuration overriding the
system configuration. That's not very high priority, however, so I don't
think I'll implement that any time soon.

> > @@ -163,6 +169,38 @@ int pa_config_parse(const char *filename, FILE *f, const pa_config_item *t, pa_p
> >
> >       pa_zero(state);
> >
> > +    if (use_dot_d) {
> > +        char *dir_name;
> > +        int n;
> > +        struct dirent **entries = NULL;
> > +
> > +        dir_name = pa_sprintf_malloc("%s.d", filename);
> > +
> > +        n = scandir(dir_name, &entries, conf_filter, alphasort);
> 
> Does scandir / dirent.h exist on all supported archs?

My scandir man page says:

CONFORMING TO
       alphasort(), scandir(): 4.3BSD, POSIX.1-2008.

If it's in POSIX, I believe it's ok. Cygwin seems to support it too (the
function is not listed[1] as not-implemented).

[1] https://cygwin.com/cygwin-api/std-notimpl.html

> > +        if (n >= 0) {
> > +            int i;
> > +
> > +            for (i = 0; i < n; i++) {
> > +                char *filename2;
> > +
> > +                filename2 = pa_sprintf_malloc("%s" PA_PATH_SEP "%s", dir_name, entries[i]->d_name);
> > +                pa_config_parse(filename2, NULL, t, proplist, false, userdata);
> > +                pa_xfree(filename2);
> > +
> > +                free(entries[i]);
> > +            }
> > +
> > +            free(entries);
> > +        } else {
> > +            if (errno == ENOENT)
> > +                pa_log_debug("scandir(\"%s\") failed: %s", dir_name, pa_cstrerror(errno));
> 
> Nitpick: I suppose this debug message could be improved by just saying 
> "%s does not exist, not searching" or so.

Indeed, will fix.

-- 
Tanu