On Wed, 2015-09-16 at 11:57 +0200, Peter Meerwald wrote:
> Hello,
>
> > > > CID 1323589
> > > >
> > > > getopt() makes sure that we have an argument for -p
> > > > remove the broken check for optarg being set
> > > > ---
> > > > src/daemon/cmdline.c | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/src/daemon/cmdline.c b/src/daemon/cmdline.c
> > > > index a2fb6d5..68a02e3 100644
> > > > --- a/src/daemon/cmdline.c
> > > > +++ b/src/daemon/cmdline.c
> > > > @@ -312,7 +312,7 @@ int pa_cmdline_parse(pa_daemon_conf *conf, int
> > > > argc, char *const argv [], int *d
> > > > case 'p':
> > > > case ARG_DL_SEARCH_PATH:
> > > > pa_xfree(conf->dl_search_path);
> > > > - conf->dl_search_path = *optarg ? pa_xstrdup(optarg)
> > > > : NULL;
> > > > + conf->dl_search_path = pa_xstrdup(optarg);
> > >
> > > What was wrong with the check? Note that *optarg checks whether the
> > > argument is an empty string, not whether optarg is NULL. This changes
> > > the behaviour, and the commit message doesn't really explain what was
> > > wrong with the old behaviour.
> >
> > Erf, that'll teach me to be lax about my own discipline during the freeze.
> >
> > I'm not sure what the right thing to do here is. Having a pulseaudio
> > -p "" arguably isn't an error, so this patch might still be okay.
>
> it teaches me that these kind of patches are delicate and the analysis
> tool is a (too) strong bias for a change
>
> the change effectively is the handling of -p "";
> before: conf->dl_search_path = NULL;
> after: conf->dl_search_path = pa_xstrdup("");
>
> I think the patch is OK nevertheless; have a look at
> daemon-conf.c:pa_daemon_conf_env()
> if ((e = getenv(ENV_DL_SEARCH_PATH))) {
> pa_xfree(c->dl_search_path);
> c->dl_search_path = pa_xstrdup(e);
> }
>
> here there is no check for e being "" -- so -p is now consistent with
> setting ENV_DL_SEARCH_PATH
>
> the only use of conf->dl_search_path is in main.c
> if (conf->dl_search_path)
> lt_dlsetsearchpath(conf->dl_search_path);
>
> so what is the difference between NOT calling lt_dlsetsearchpath() and
> lt_dlsetsearchpath("")?
>
> running
> #include
> #include "ltdl.h"
> int main() {
> printf(" '%s'\n", lt_dlgetsearchpath());
>
> printf("%d\n", lt_dlsetsearchpath("/tmp"));
> printf(" %d\n", lt_dlsetsearchpath(""));
> printf(" '%s'\n", lt_dlgetsearchpath());
>
> printf("%d\n", lt_dlsetsearchpath("/tmp"));
> printf(" %d\n", lt_dlsetsearchpath(NULL));
> printf(" '%s'\n", lt_dlgetsearchpath());
> }
> returns
> '(null)'
> 0
> 0
> '(null)'
> 0
> 0
> '(null)'
> for me -- so the dlsearchpath is NULL initially, and it can be cleared by
> calling lt_dlsetsearchpath("")
>
> I guess there is no API guarantee what has to happen
> for lt_dlsetsearchpath("") but it looks OK here :)
>
>
> so the patch only changes behaviour if somehow dlsearchpath is NOT NULL
> initially (doesn't seem to be the case) and -p "" is given; and I think
> the patch improves the situation: the explicit -p "" leads to
> lt_dlsetsearchpath("") actually being called and not ignored
>
>
> sorry for the mess at an inappropriate time, the commit message totally
> doesn't reflect above findings (which I became aware of after the fact :)
>
> I suggest to keep the patch
I agree, no need to revert. What was the issue that Coverity was
worried about? Should I create an account at coverity.com and somehow
look up the CID?
--
Tanu
