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
