diff mbox

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

Message ID 10aefe61-96b9-e395-a34a-b9e2403153c5@gyani.pro
State Accepted
Commit 50789e356d65270698d0d8495323ebe76a46091a
Headers show

Commit Message

Gyan Doshi May 24, 2019, 5:54 a.m. UTC 
On 24-05-2019 02:06 AM, Hendrik Leppkes wrote:
> On Thu, May 23, 2019 at 9:55 PM Gyan <ffmpeg@gyani.pro> wrote:
>>
>>
>> On 24-05-2019 12:58 AM, Nicolas George wrote:
>>> Gyan (12019-05-24):
>>>> avpriv_io_delete will call the file protocol's delete which is guarded
>>>> with a header check, not done here.
>>> Do you have report of a build failure caused by unlink()?
>> No.  I assume that the guard in the file proto callback is germane and
>> the patch was subject to review. Do you know it isn't?
>>
>>>> Since we now have a generic wrapper function, isn't that better for
>>>> future maintenance?
>>> Not that I see. Something that makes it harder to follow the code from
>>> the call site to the actual function is not good for maintenance.
>> The point of modularizing the op here is to have a single interface -
>> why stick with a direct external call? At the time it was added, there
>> was no alternative.
>> Don't mind it either way, but this feels like a useless game of ping-pong.
>>
> Just stick to unlink. If there is no reason to change it, then the
> preference is to just keep it as-is.
> We're not using the file protocol to create the file, so no reason to
> use it to delete it.

Revised.

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

Verified that cache files get deleted on Windows.
---
 libavformat/cache.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

Comments

Gyan Doshi May 24, 2019, 3:23 p.m. UTC | #1 
On 24-05-2019 11:24 AM, Gyan wrote:
>
>
> On 24-05-2019 02:06 AM, Hendrik Leppkes wrote:
>> On Thu, May 23, 2019 at 9:55 PM Gyan <ffmpeg@gyani.pro> wrote:
>>>
>>>
>>> On 24-05-2019 12:58 AM, Nicolas George wrote:
>>>> Gyan (12019-05-24):
>>>>> avpriv_io_delete will call the file protocol's delete which is 
>>>>> guarded
>>>>> with a header check, not done here.
>>>> Do you have report of a build failure caused by unlink()?
>>> No.  I assume that the guard in the file proto callback is germane and
>>> the patch was subject to review. Do you know it isn't?
>>>
>>>>> Since we now have a generic wrapper function, isn't that better for
>>>>> future maintenance?
>>>> Not that I see. Something that makes it harder to follow the code from
>>>> the call site to the actual function is not good for maintenance.
>>> The point of modularizing the op here is to have a single interface -
>>> why stick with a direct external call? At the time it was added, there
>>> was no alternative.
>>> Don't mind it either way, but this feels like a useless game of 
>>> ping-pong.
>>>
>> Just stick to unlink. If there is no reason to change it, then the
>> preference is to just keep it as-is.
>> We're not using the file protocol to create the file, so no reason to
>> use it to delete it.
Will push tonight.

Gyan
Gyan Doshi May 24, 2019, 6:53 p.m. UTC | #2 
On 24-05-2019 08:53 PM, Gyan wrote:
>
>
> On 24-05-2019 11:24 AM, Gyan wrote:
>>
>>
> Will push tonight.

Pushed as 50789e356d65270698d0d8495323ebe76a46091a

Gyan
diff mbox

Patch

diff --git a/libavformat/cache.c b/libavformat/cache.c
index 66bbbf54c9..3425be9350 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,8 +85,12 @@  static int cache_open(URLContext *h, const char *arg, int flags, AVDictionary **
         return c->fd;
     }
 
-    unlink(buffername);
-    av_freep(&buffername);
+    ret = unlink(buffername);
+
+    if (ret >= 0)
+        av_freep(&buffername);
+    else
+        c->filename = buffername;
 
     return ffurl_open_whitelist(&c->inner, arg, flags, &h->interrupt_callback,
                                 options, h->protocol_whitelist, h->protocol_blacklist, h);
@@ -292,11 +298,18 @@  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 = unlink(c->filename);
+        if (ret < 0)
+            av_log(h, AV_LOG_ERROR, "Could not delete %s.\n", c->filename);
+        av_freep(&c->filename);
+    }
     ffurl_close(c->inner);
     av_tree_enumerate(c->root, NULL, NULL, enu_free);
     av_tree_destroy(c->root);