[vlc-devel] [PATCH v2 3/13] platform: avoid hard coded temp dir path in vlc_memfd()

[Lyndon Brown]

On Tue, 2020-10-06 at 17:49 +0300, Rémi Denis-Courmont wrote:
> Le tiistaina 6. lokakuuta 2020, 8.57.32 EEST Lyndon Brown a écrit :
> > diff --git a/src/posix/filesystem.c b/src/posix/filesystem.c
> > index 60a997d9e0..1cf5df224d 100644
> > --- a/src/posix/filesystem.c
> > +++ b/src/posix/filesystem.c
> > @@ -44,6 +44,7 @@
> > 
> >  #include <vlc_common.h>
> >  #include <vlc_fs.h>
> > +#include <vlc_configuration.h>
> > 
> >  #if !defined(HAVE_ACCEPT4)
> >  static inline void vlc_cloexec(int fd)
> > @@ -90,12 +91,22 @@ int vlc_mkstemp (char *template)
> > 
> >  VLC_WEAK int vlc_memfd(void)
> >  {
> > -    char bufpath[] = "/tmp/"PACKAGE_NAME"XXXXXX";
> > -    int fd;
> > +    char *tempdir = config_GetTempPath();
> > +    if (tempdir == NULL)
> > +        return -1;
> 
> You're adding an error case for no good reasons here.
> 
> You could just as well call getenv() directly or follow the
> config_GetSysPath() 
> pattern. Either way, it saves one strdup().

I don't like the pointless strdup() for platforms that don't really
need it either, but I didn't want to duplicate the logic (except
strdup) of config_GetTempPath here, as trivial as it may be, just to
avoid that, but if you really want I can do so. It would certainly be
nice to avoid the strdup inefficiency here.

We can't just do a getenv() call only though instead; Many linux
distros (like mine) leave $TMPDIR unset, so we'd need to grab the env
var, then replace with "/tmp" if null.

I'm not sure what you mean about "follow the config_GetSysPath()
pattern" since that also allocates. Did you mean integrate the path
fetching into that rather than have a separate function? I used a
separate function partly because I believe on some platforms the
function may return a user temp path rather than a system one, so
doesn't perfectly fit either config_GetSysPath()
or config_GetUserDir()...

> > +
> > +    const char *filetemplate = PACKAGE_NAME"XXXXXX";
> > +    char *bufpath;
> > +    if (asprintf(&bufpath, "%s/%s", tempdir, filetemplate) == -1)
> > {
> 
> Use string concatenation.

Okay. It seemed a little clearer to me to avoid it, but okay.

> > +        free(tempdir);
> > +        return -1;
> > +    }
> > +    free(tempdir);
> > 
> > -    fd = vlc_mkstemp (bufpath);
> > +    int fd = vlc_mkstemp(bufpath);
> >      if (fd != -1)
> >          unlink (bufpath);
> > +    free(bufpath);
> >      return fd;
> >  }