[FFmpeg-devel] [PATCH 1/2] avutil/file: Move av_tempfile() to avutil/file_open ff_tempfile()

Hendrik Leppkes h.leppkes at gmail.com
Fri Mar 11 15:35:43 CET 2016


On Wed, Mar 9, 2016 at 4:51 PM, Hendrik Leppkes <h.leppkes at gmail.com> wrote:
> On Wed, Mar 9, 2016 at 4:35 PM, Michael Niedermayer
> <michael at niedermayer.cc> wrote:
>> On Wed, Mar 09, 2016 at 04:27:12PM +0100, Hendrik Leppkes wrote:
>>> On Wed, Mar 9, 2016 at 3:37 PM, Michael Niedermayer
>>> <michael at niedermayer.cc> wrote:
>>> > document the issue with av_tempfile()
>>> >
>>> > Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
>>> > ---
>>> >  libavcodec/libxvid.h  |    2 --
>>> >  libavutil/file.c      |   48 ++--------------------------------------
>>> >  libavutil/file.h      |    1 +
>>> >  libavutil/file_open.c |   59 +++++++++++++++++++++++++++++++++++++++++++++++++
>>> >  libavutil/internal.h  |   14 ++++++++++++
>>> >  5 files changed, 76 insertions(+), 48 deletions(-)
>>> >
>>> > diff --git a/libavcodec/libxvid.h b/libavcodec/libxvid.h
>>> > index bffe07d..ef9a5a9 100644
>>> > --- a/libavcodec/libxvid.h
>>> > +++ b/libavcodec/libxvid.h
>>> > @@ -26,6 +26,4 @@
>>> >   * common functions for use with the Xvid wrappers
>>> >   */
>>> >
>>> > -int ff_tempfile(const char *prefix, char **filename);
>>> > -
>>> >  #endif /* AVCODEC_LIBXVID_H */
>>> > diff --git a/libavutil/file.c b/libavutil/file.c
>>> > index 2a06be4..25381b1 100644
>>> > --- a/libavutil/file.c
>>> > +++ b/libavutil/file.c
>>> > @@ -137,52 +137,8 @@ void av_file_unmap(uint8_t *bufptr, size_t size)
>>> >  #endif
>>> >  }
>>> >
>>> > -int av_tempfile(const char *prefix, char **filename, int log_offset, void *log_ctx)
>>> > -{
>>> > -    FileLogContext file_log_ctx = { &file_log_ctx_class, log_offset, log_ctx };
>>> > -    int fd = -1;
>>> > -#if !HAVE_MKSTEMP
>>> > -    void *ptr= tempnam(NULL, prefix);
>>> > -    if(!ptr)
>>> > -        ptr= tempnam(".", prefix);
>>> > -    *filename = av_strdup(ptr);
>>> > -#undef free
>>> > -    free(ptr);
>>> > -#else
>>> > -    size_t len = strlen(prefix) + 12; /* room for "/tmp/" and "XXXXXX\0" */
>>> > -    *filename  = av_malloc(len);
>>> > -#endif
>>> > -    /* -----common section-----*/
>>> > -    if (!*filename) {
>>> > -        av_log(&file_log_ctx, AV_LOG_ERROR, "ff_tempfile: Cannot allocate file name\n");
>>> > -        return AVERROR(ENOMEM);
>>> > -    }
>>> > -#if !HAVE_MKSTEMP
>>> > -#   ifndef O_BINARY
>>> > -#       define O_BINARY 0
>>> > -#   endif
>>> > -#   ifndef O_EXCL
>>> > -#       define O_EXCL 0
>>> > -#   endif
>>> > -    fd = open(*filename, O_RDWR | O_BINARY | O_CREAT | O_EXCL, 0600);
>>> > -#else
>>> > -    snprintf(*filename, len, "/tmp/%sXXXXXX", prefix);
>>> > -    fd = mkstemp(*filename);
>>> > -#ifdef _WIN32
>>> > -    if (fd < 0) {
>>> > -        snprintf(*filename, len, "./%sXXXXXX", prefix);
>>> > -        fd = mkstemp(*filename);
>>> > -    }
>>> > -#endif
>>> > -#endif
>>> > -    /* -----common section-----*/
>>> > -    if (fd < 0) {
>>> > -        int err = AVERROR(errno);
>>> > -        av_log(&file_log_ctx, AV_LOG_ERROR, "ff_tempfile: Cannot open temporary file %s\n", *filename);
>>> > -        av_freep(filename);
>>> > -        return err;
>>> > -    }
>>> > -    return fd; /* success */
>>> > +int av_tempfile(const char *prefix, char **filename, int log_offset, void *log_ctx) {
>>> > +    return avpriv_tempfile(prefix, filename, log_offset, log_ctx);
>>> >  }
>>> >
>>> >  #ifdef TEST
>>> > diff --git a/libavutil/file.h b/libavutil/file.h
>>> > index e931be7..8666c7b 100644
>>> > --- a/libavutil/file.h
>>> > +++ b/libavutil/file.h
>>> > @@ -62,6 +62,7 @@ void av_file_unmap(uint8_t *bufptr, size_t size);
>>> >   * @note On very old libcs it is necessary to set a secure umask before
>>> >   *       calling this, av_tempfile() can't call umask itself as it is used in
>>> >   *       libraries and could interfere with the calling application.
>>> > + * @deprecated as fd numbers cannot be passed saftely between libs on some platforms
>>> >   */
>>> >  int av_tempfile(const char *prefix, char **filename, int log_offset, void *log_ctx);
>>> >
>>> > diff --git a/libavutil/file_open.c b/libavutil/file_open.c
>>> > index 9e76127..6e58cc1 100644
>>> > --- a/libavutil/file_open.c
>>> > +++ b/libavutil/file_open.c
>>> > @@ -92,6 +92,65 @@ int avpriv_open(const char *filename, int flags, ...)
>>> >      return fd;
>>> >  }
>>> >
>>> > +typedef struct FileLogContext {
>>> > +    const AVClass *class;
>>> > +    int   log_offset;
>>> > +    void *log_ctx;
>>> > +} FileLogContext;
>>> > +
>>> > +static const AVClass file_log_ctx_class = {
>>> > +    "TEMPFILE", av_default_item_name, NULL, LIBAVUTIL_VERSION_INT,
>>> > +    offsetof(FileLogContext, log_offset), offsetof(FileLogContext, log_ctx)
>>> > +};
>>> > +
>>> > +int avpriv_tempfile(const char *prefix, char **filename, int log_offset, void *log_ctx)
>>> > +{
>>> > +    FileLogContext file_log_ctx = { &file_log_ctx_class, log_offset, log_ctx };
>>> > +    int fd = -1;
>>> > +#if !HAVE_MKSTEMP
>>> > +    void *ptr= tempnam(NULL, prefix);
>>> > +    if(!ptr)
>>> > +        ptr= tempnam(".", prefix);
>>> > +    *filename = av_strdup(ptr);
>>> > +#undef free
>>> > +    free(ptr);
>>> > +#else
>>> > +    size_t len = strlen(prefix) + 12; /* room for "/tmp/" and "XXXXXX\0" */
>>> > +    *filename  = av_malloc(len);
>>> > +#endif
>>> > +    /* -----common section-----*/
>>> > +    if (!*filename) {
>>> > +        av_log(&file_log_ctx, AV_LOG_ERROR, "ff_tempfile: Cannot allocate file name\n");
>>> > +        return AVERROR(ENOMEM);
>>> > +    }
>>> > +#if !HAVE_MKSTEMP
>>> > +#   ifndef O_BINARY
>>> > +#       define O_BINARY 0
>>> > +#   endif
>>> > +#   ifndef O_EXCL
>>> > +#       define O_EXCL 0
>>> > +#   endif
>>> > +    fd = open(*filename, O_RDWR | O_BINARY | O_CREAT | O_EXCL, 0600);
>>> > +#else
>>> > +    snprintf(*filename, len, "/tmp/%sXXXXXX", prefix);
>>> > +    fd = mkstemp(*filename);
>>> > +#ifdef _WIN32
>>> > +    if (fd < 0) {
>>> > +        snprintf(*filename, len, "./%sXXXXXX", prefix);
>>> > +        fd = mkstemp(*filename);
>>> > +    }
>>> > +#endif
>>> > +#endif
>>> > +    /* -----common section-----*/
>>> > +    if (fd < 0) {
>>> > +        int err = AVERROR(errno);
>>> > +        av_log(&file_log_ctx, AV_LOG_ERROR, "ff_tempfile: Cannot open temporary file %s\n", *filename);
>>> > +        av_freep(filename);
>>> > +        return err;
>>> > +    }
>>> > +    return fd; /* success */
>>> > +}
>>> > +
>>> >  FILE *av_fopen_utf8(const char *path, const char *mode)
>>> >  {
>>> >      int fd;
>>> > diff --git a/libavutil/internal.h b/libavutil/internal.h
>>> > index c4bcf37..da76ca2 100644
>>> > --- a/libavutil/internal.h
>>> > +++ b/libavutil/internal.h
>>> > @@ -244,6 +244,7 @@ void avpriv_request_sample(void *avc,
>>> >  #endif
>>> >
>>> >  #define avpriv_open ff_open
>>> > +#define avpriv_tempfile ff_tempfile
>>> >  #define PTRDIFF_SPECIFIER "Id"
>>> >  #define SIZE_SPECIFIER "Iu"
>>> >  #else
>>> > @@ -319,6 +320,19 @@ static av_always_inline float ff_exp10f(float x)
>>> >  av_warn_unused_result
>>> >  int avpriv_open(const char *filename, int flags, ...);
>>> >
>>> > +/**
>>> > + * Wrapper to work around the lack of mkstemp() on mingw.
>>> > + * Also, tries to create file in /tmp first, if possible.
>>> > + * *prefix can be a character constant; *filename will be allocated internally.
>>> > + * @return file descriptor of opened file (or negative value corresponding to an
>>> > + * AVERROR code on error)
>>> > + * and opened file name in **filename.
>>> > + * @note On very old libcs it is necessary to set a secure umask before
>>> > + *       calling this, av_tempfile() can't call umask itself as it is used in
>>> > + *       libraries and could interfere with the calling application.
>>> > + */
>>> > +int avpriv_tempfile(const char *prefix, char **filename, int log_offset, void *log_ctx);
>>> > +
>>> >  int avpriv_set_systematic_pal2(uint32_t pal[256], enum AVPixelFormat pix_fmt);
>>> >
>>> >  static av_always_inline av_const int avpriv_mirror(int x, int w)
>>> > --
>>> > 1.7.9.5
>>>
>>> This seems slightly confusing. You use avpriv but then define it to
>>> ff? Why not just stick to ff everywhere? It should never be exported
>>> from shared libraries to avoid this issue.
>>> Maybe I'm missing something why this is done this way. :)
>>
>> the makefiles include file_open like this:
>> OBJS-$(HAVE_LIBC_MSVCRT)               += file_open.o
>>
>> so if we use file_open.c for *_tempfile it cant be ff_ if
>> HAVE_LIBC_MSVCRT is not set as there would be none in the local lib
>> i followed the same design as avpriv/ff_open()
>> i would have favored ff_ as well if there was no pre-existing system
>>
>>
>
> I see, it only does this for msvcrt then.
> I'll test the patch later.
>

Tested the  set on the failing configuration, and it seems to work fine, thanks.

- Hendrik


More information about the ffmpeg-devel mailing list