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

[Tanu Kaskinen]

On Wed, 2015-05-27 at 11:38 +0200, Peter Meerwald wrote:
> On Wed, 27 May 2015, Tanu Kaskinen wrote:
> 
> > On Wed, 2015-05-27 at 00:13 +0200, Peter Meerwald wrote:
> > > > @@ -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);
> > > > +        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);
> > > 
> > > the return value is not checked;
> > > for config files included with .include, the retval is checked
> > 
> > In my opinion pa_config_parse() shouldn't even return anything. A single
> > fault in the client.conf shouldn't prevent all clients from connecting
> > to the server, and reverting to the default configuration isn't a good
> > option either. A single configuration fault should only result in a
> > single error message in the log, and the configuration parsing should
> > continue as usual after the error.
> > 
> > So, I think when config files are included with .include, the return
> > value of pa_config_parse() should not be checked.
> 
> $ rgrep pa_config_parse\( .
> ./pulsecore/conf-parser.c:        r = pa_config_parse(fn, NULL, state->item_table, state->proplist, state->userdata);
> ./pulsecore/conf-parser.c:int pa_config_parse(const char *filename, FILE *f, const pa_config_item *t, pa_proplist *proplist, void *userdata) {
> ./modules/alsa/alsa-mixer.c:    r = pa_config_parse(fn, NULL, items, p->proplist, p);
> ./modules/alsa/alsa-mixer.c:    r = pa_config_parse(fn, NULL, items, NULL, ps);
> ./modules/module-augment-properties.c:    if (pa_config_parse(fn, NULL, table, NULL, r) < 0)
> ./daemon/daemon-conf.c:    r = f ? pa_config_parse(c->config_file, f, table, NULL, NULL) : 0;
> ./pulse/client-conf.c:        pa_config_parse(fn, f, table, NULL, NULL);
> 
> client-conf does not check the retval, everywhere else it is checked
> (not saying that one way is better than the other, but probably the 
> decision to ignore the retval should be with the caller, not internally 
> in pa_config_parse())

Well, in my opinion each of the callers should choose to ignore the
return value, and if that happens, then the return value will be
redundant.

-- 
Tanu