diff mbox

[FFmpeg-devel] avformat/cache - delete cache file after closing handle

Message ID e98118cf-3c65-b36f-33cd-9e3f0cbffc81@gyani.pro
State Superseded
Headers show

Commit Message

Gyan Doshi May 22, 2019, 12:53 p.m. UTC
The existing practice of unlinking path immediately after acquiring file 
handle was returning (unchecked) EPERM error on Windows. Switched to 
deletion after closing handle.
Confirmed that cache protocol temp files are deleted in Windows 7 when 
ffmpeg exits.

The cache protocol calls avutil/avpriv_tempfile(), which in turn, calls 
mkstemp (for me), so I don't see the provision to append O_TEMPORARY as 
a flag while opening. mkstemp only applies
_O_RDWR | _O_CREAT | _O_EXCL | _O_BINARY

Should be tested in other environments. If it works, supersedes 
https://patchwork.ffmpeg.org/patch/13242/

Thanks,
Gyan
From f1f17020dd96205a60195e310d8c3b53126c2e00 Mon Sep 17 00:00:00 2001
From: Gyan Doshi <ffmpeg@gyani.pro>
Date: Wed, 22 May 2019 18:05:04 +0530
Subject: [PATCH] avformat/cache - delete cache file after closing handle

Ensures cache file deletion on Windows, when compiled under MinGW.
---
 libavformat/cache.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Comments

Nicolas George May 22, 2019, 1:07 p.m. UTC | #1
Gyan (12019-05-22):
> 
> The existing practice of unlinking path immediately after acquiring file
> handle was returning (unchecked) EPERM error on Windows. Switched to
> deletion after closing handle.
> Confirmed that cache protocol temp files are deleted in Windows 7 when
> ffmpeg exits.
> 
> The cache protocol calls avutil/avpriv_tempfile(), which in turn, calls
> mkstemp (for me), so I don't see the provision to append O_TEMPORARY as a
> flag while opening. mkstemp only applies
> _O_RDWR | _O_CREAT | _O_EXCL | _O_BINARY
> 
> Should be tested in other environments. If it works, supersedes
> https://patchwork.ffmpeg.org/patch/13242/
> 
> Thanks,
> Gyan

> From f1f17020dd96205a60195e310d8c3b53126c2e00 Mon Sep 17 00:00:00 2001
> From: Gyan Doshi <ffmpeg@gyani.pro>
> Date: Wed, 22 May 2019 18:05:04 +0530
> Subject: [PATCH] avformat/cache - delete cache file after closing handle
> 
> Ensures cache file deletion on Windows, when compiled under MinGW.
> ---
>  libavformat/cache.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/libavformat/cache.c b/libavformat/cache.c
> index 66bbbf54c9..1db13ceefc 100644
> --- a/libavformat/cache.c
> +++ b/libavformat/cache.c
> @@ -54,6 +54,7 @@ typedef struct CacheEntry {
>  typedef struct Context {
>      AVClass *class;
>      int fd;
> +    char *filename;
>      struct AVTreeNode *root;
>      int64_t logical_pos;
>      int64_t cache_pos;
> @@ -83,7 +84,7 @@ static int cache_open(URLContext *h, const char *arg, int flags, AVDictionary **
>          return c->fd;
>      }
>  

> -    unlink(buffername);

On systems that allow deleting the file when it is still in use deleting
it immediately is a better idea: it reduces the risk of leaving it if
the application is interrupted prematurely.

> +    c->filename = av_strdup(buffername);

Missing error check.

>      av_freep(&buffername);
>  
>      return ffurl_open_whitelist(&c->inner, arg, flags, &h->interrupt_callback,
> @@ -292,11 +293,15 @@ static int enu_free(void *opaque, void *elem)
>  static int cache_close(URLContext *h)
>  {
>      Context *c= h->priv_data;
> +    int ret;
>  
>      av_log(h, AV_LOG_INFO, "Statistics, cache hits:%"PRId64" cache misses:%"PRId64"\n",
>             c->cache_hit, c->cache_miss);
>  
>      close(c->fd);
> +    ret = avpriv_io_delete(c->filename);
> +    if (ret < 0)
> +        av_log(h, AV_LOG_ERROR, "Could not delete %s. Error is %s\n", c->filename, av_err2str(ret));
>      ffurl_close(c->inner);
>      av_tree_enumerate(c->root, NULL, NULL, enu_free);
>      av_tree_destroy(c->root);

Regards,
diff mbox

Patch

diff --git a/libavformat/cache.c b/libavformat/cache.c
index 66bbbf54c9..1db13ceefc 100644
--- a/libavformat/cache.c
+++ b/libavformat/cache.c
@@ -54,6 +54,7 @@  typedef struct CacheEntry {
 typedef struct Context {
     AVClass *class;
     int fd;
+    char *filename;
     struct AVTreeNode *root;
     int64_t logical_pos;
     int64_t cache_pos;
@@ -83,7 +84,7 @@  static int cache_open(URLContext *h, const char *arg, int flags, AVDictionary **
         return c->fd;
     }
 
-    unlink(buffername);
+    c->filename = av_strdup(buffername);
     av_freep(&buffername);
 
     return ffurl_open_whitelist(&c->inner, arg, flags, &h->interrupt_callback,
@@ -292,11 +293,15 @@  static int enu_free(void *opaque, void *elem)
 static int cache_close(URLContext *h)
 {
     Context *c= h->priv_data;
+    int ret;
 
     av_log(h, AV_LOG_INFO, "Statistics, cache hits:%"PRId64" cache misses:%"PRId64"\n",
            c->cache_hit, c->cache_miss);
 
     close(c->fd);
+    ret = avpriv_io_delete(c->filename);
+    if (ret < 0)
+        av_log(h, AV_LOG_ERROR, "Could not delete %s. Error is %s\n", c->filename, av_err2str(ret));
     ffurl_close(c->inner);
     av_tree_enumerate(c->root, NULL, NULL, enu_free);
     av_tree_destroy(c->root);