[vlc-devel] [PATCH v2 9/13] fs: add optional dir param to vlc_MakeTmpFile()

[Lyndon Brown]

Yes I should have mentioned, I was keeping this separated as with patch
#2 for flexibility depending upon how reviewers felt about things.

Very happy to merge #9 into #7 if people are definitely happy for the
additional param to be part of the function's API.

The extra param is currently only used by the es_out_timeshift case. To
not have it means leaving the code duplication in that case.

Alternatively, we could have the function with the extra param as an
internal interface, with the external API not having it, but that's
perhaps pointlessly over complex. Or we could have a macro to avoid the
NULL in the other cases, but then what naming difference to use?

Originally I had the function as an inline helper in the header, but
decided to move it. As an inline helper, the extra param would no doubt
get optimised away in the cases not using it. There are very few uses
of the function, only 8 at the end of this patch set, with 3 being in
tests. So another consideration could be to move it back to inline...

On Tue, 2020-10-06 at 09:26 +0200, Thomas Guillem wrote:
> Should be squashed with 7/13
> 
> Or maybe, add a new function for that particular case ?
> 
> On Tue, Oct 6, 2020, at 08:00, Lyndon Brown wrote:
> > From: Lyndon Brown <jnqnfe at gmail.com>
> > Date: Tue, 6 Oct 2020 01:27:52 +0100
> > Subject: fs: add optional dir param to vlc_MakeTmpFile()
> > 
> > to avoid code duplication in cases where caller wants to possibly
> > control
> > the directory chosen rather than just rely on default temporary
> > directory.
> > 
> > diff --git a/include/vlc_fs.h b/include/vlc_fs.h
> > index 50ec52c299..07bf682c66 100644
> > --- a/include/vlc_fs.h
> > +++ b/include/vlc_fs.h
> > @@ -103,16 +103,19 @@ VLC_API int vlc_mkstemp( char * );
> >   * Temporary file creation helper
> >   *
> >   * Creates a unique temporary file, using config_GetTempPath() to 
> > determine the
> > - * directory in which to create it, and using vlc_mkstemp() for
> > the 
> > creation.
> > + * directory in which to create it (unless the caller specifies
> > an 
> > alternaive
> > + * location), and using vlc_mkstemp() for the creation.
> >   *
> >   * Note, the returned `path` pointer must be free'd with free(), 
> > except when
> >   * the returned file descriptor is -1 indicating error, where is
> > it 
> > undefined.
> >   *
> >   * @param path [OUT] full path of the created file.
> >   * @param filetemplate filename template, which must end with a 
> > sequence of 'XXXXXX'.
> > + * @param directory an optional caller chosen directory to use
> > instead 
> > of the one
> > + *                  returned by config_GetTempPath().
> >   * @return file descriptor, or -1 on error.
> >   */
> > -VLC_API int vlc_MakeTmpFile( char **path, const char *filetemplate
> > ) 
> > VLC_USED;
> > +VLC_API int vlc_MakeTmpFile( char **path, const char
> > *filetemplate, 
> > const char *directory ) VLC_USED;
> >  
> >  /**
> >   * Duplicates a file descriptor. The new file descriptor has the
> > close-on-exec
> > diff --git a/src/linux/filesystem.c b/src/linux/filesystem.c
> > index c2ec9c3a5a..d08e7a59ca 100644
> > --- a/src/linux/filesystem.c
> > +++ b/src/linux/filesystem.c
> > @@ -59,7 +59,7 @@ int vlc_memfd(void)
> >       * EISDIR, or EOPNOTSUPP, cf. man open(2). */
> >      const char *filetemplate = PACKAGE_NAME"XXXXXX";
> >      char *bufpath;
> > -    fd = vlc_MakeTmpFile(&bufpath, filetemplate);
> > +    fd = vlc_MakeTmpFile(&bufpath, filetemplate, NULL);
> >  
> >      if (fd != -1)
> >          unlink(bufpath);
> > diff --git a/src/posix/filesystem.c b/src/posix/filesystem.c
> > index 01d4aa1ab0..6e8332ebc2 100644
> > --- a/src/posix/filesystem.c
> > +++ b/src/posix/filesystem.c
> > @@ -92,7 +92,7 @@ VLC_WEAK int vlc_memfd(void)
> >  {
> >      const char *filetemplate = PACKAGE_NAME"XXXXXX";
> >      char *bufpath;
> > -    int fd = vlc_MakeTmpFile(&bufpath, filetemplate);
> > +    int fd = vlc_MakeTmpFile(&bufpath, filetemplate, NULL);
> >  
> >      if (fd != -1)
> >          unlink (bufpath);
> > diff --git a/src/text/filesystem.c b/src/text/filesystem.c
> > index fa518f6b6e..f73f16bac0 100644
> > --- a/src/text/filesystem.c
> > +++ b/src/text/filesystem.c
> > @@ -244,20 +244,25 @@ VLC_WEAK int vlc_mkstemp(char *template)
> >      return -1;
> >  }
> >  
> > -int vlc_MakeTmpFile( char **path, const char *filetemplate )
> > +int vlc_MakeTmpFile( char **path, const char *filetemplate, const
> > char 
> > *directory )
> >  {
> > -    char *tmpdir = config_GetTempPath();
> > -    if (tmpdir == NULL)
> > -        return -1;
> > +    char *dir_actual;
> > +    if (directory != NULL)
> > +        dir_actual = (char *)directory;
> > +    else {
> > +        dir_actual = config_GetTempPath();
> > +        if (dir_actual == NULL)
> > +            return -1;
> > +    }
> >  
> > -    if (asprintf(path, "%s"DIR_SEP"%s", tmpdir, filetemplate) < 0)
> > {
> > -        free(tmpdir);
> > -        return -1;
> > +    int fd = -1;
> > +    if (asprintf(path, "%s"DIR_SEP"%s", dir_actual, filetemplate)
> > >= 0) {
> > +        fd = vlc_mkstemp(*path);
> > +        if (fd == -1)
> > +            free(*path);
> >      }
> > -    free(tmpdir);
> >  
> > -    int fd = vlc_mkstemp(*path);
> > -    if (fd == -1)
> > -        free(*path);
> > +    if (directory == NULL)
> > +        free(dir_actual);
> >      return fd;
> >  }
> > 
> > _______________________________________________
> > vlc-devel mailing list
> > To unsubscribe or modify your subscription options:
> > https://mailman.videolan.org/listinfo/vlc-devel
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel