[pulseaudio-discuss] [PATCH v2 04/12] daemon: No need to check optarg, -p requires argument

[Tanu Kaskinen]

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