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

Submitted by Gyan on May 22, 2019, 3:36 p.m.

Details

Message ID 2b67cafd-f865-a901-a3ab-e54836c52dfb@gyani.pro
State New
Headers show

Commit Message

Gyan May 22, 2019, 3:36 p.m.
On 22-05-2019 06:37 PM, Nicolas George wrote:
> 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);

Revised patch.

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

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

Comments

Michael Niedermayer May 23, 2019, 3:43 p.m.
On Wed, May 22, 2019 at 09:06:11PM +0530, Gyan wrote:
> 
> 
> On 22-05-2019 06:37 PM, Nicolas George wrote:
> >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);
> 
> Revised patch.
> 
> Gyan
> 
> 

>  cache.c |   21 ++++++++++++++++++++-
>  1 file changed, 20 insertions(+), 1 deletion(-)
> 3cb3458fccd0c253c7ab0d281935145beb03ab25  v2-0001-avformat-cache-delete-cache-file-after-closing-ha.patch
> From 54b24428fcf6b513574f5c1b626ebb505b123d77 Mon Sep 17 00:00:00 2001
> From: Gyan Doshi <ffmpeg@gyani.pro>
> Date: Wed, 22 May 2019 18:05:04 +0530
> Subject: [PATCH v2] avformat/cache - delete cache file after closing handle
> 
> Ensures cache file deletion on Windows, when compiled under MinGW.
> ---
>  libavformat/cache.c | 21 ++++++++++++++++++++-
>  1 file changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/libavformat/cache.c b/libavformat/cache.c
> index 66bbbf54c9..ee1b868157 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;
> @@ -72,6 +73,7 @@ static int cmp(const void *key, const void *node)
>  
>  static int cache_open(URLContext *h, const char *arg, int flags, AVDictionary **options)
>  {
> +    int ret;
>      char *buffername;
>      Context *c= h->priv_data;
>  
> @@ -83,7 +85,18 @@ static int cache_open(URLContext *h, const char *arg, int flags, AVDictionary **
>          return c->fd;
>      }
>  
> -    unlink(buffername);
> +    ret = avpriv_io_delete(buffername);
> +
> +    if (ret < 0) {
> +        c->filename = av_strdup(buffername);
> +        if (!c->filename) {
> +            close(c->fd);
> +            avpriv_io_delete(buffername);
> +            av_freep(&buffername);
> +            return AVERROR(ENOMEM);
> +        }
> +    }

you can use buffername directly and skip freeing it instead of strdup()
this avoids the need for the error handling

thx

[...]

Patch hide | download patch | download mbox

diff --git a/libavformat/cache.c b/libavformat/cache.c
index 66bbbf54c9..ee1b868157 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;
@@ -72,6 +73,7 @@  static int cmp(const void *key, const void *node)
 
 static int cache_open(URLContext *h, const char *arg, int flags, AVDictionary **options)
 {
+    int ret;
     char *buffername;
     Context *c= h->priv_data;
 
@@ -83,7 +85,18 @@  static int cache_open(URLContext *h, const char *arg, int flags, AVDictionary **
         return c->fd;
     }
 
-    unlink(buffername);
+    ret = avpriv_io_delete(buffername);
+
+    if (ret < 0) {
+        c->filename = av_strdup(buffername);
+        if (!c->filename) {
+            close(c->fd);
+            avpriv_io_delete(buffername);
+            av_freep(&buffername);
+            return AVERROR(ENOMEM);
+        }
+    }
+
     av_freep(&buffername);
 
     return ffurl_open_whitelist(&c->inner, arg, flags, &h->interrupt_callback,
@@ -292,11 +305,17 @@  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);
+    if (c->filename) {
+        ret = avpriv_io_delete(c->filename);
+        if (ret < 0)
+            av_log(h, AV_LOG_ERROR, "Could not delete %s.\n", c->filename);
+    }
     ffurl_close(c->inner);
     av_tree_enumerate(c->root, NULL, NULL, enu_free);
     av_tree_destroy(c->root);