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 <stdio.h>
#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
regards, p.
--
Peter Meerwald
+43-664-2444418 (mobile)
