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

[Peter Meerwald]

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)