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

Submitted by Gyan on May 23, 2019, 5:23 p.m.

Details

Message ID ab746cdf-242f-eda7-52f6-e84663de8714@gyani.pro
State New
Headers show

Commit Message

Gyan May 23, 2019, 5:23 p.m.
On 23-05-2019 09:13 PM, Michael Niedermayer wrote:
> 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
v3 attached.

Thanks.
From 87a1c15dc3ff2e8bde3a2cd19a1c45ff02689600 Mon Sep 17 00:00:00 2001
From: Gyan Doshi <ffmpeg@gyani.pro>
Date: Wed, 22 May 2019 18:05:04 +0530
Subject: [PATCH v3] 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

Nicolas George May 23, 2019, 5:50 p.m.
Gyan (12019-05-23):
> -    unlink(buffername);
> -    av_freep(&buffername);
> +    ret = avpriv_io_delete(buffername);

Is there a reason you replace unlink() with avpriv_io_delete()? unlink()
is more direct, and cache does not support non-file cache.

Regards,
Gyan May 23, 2019, 7:23 p.m.
On 23-05-2019 11:20 PM, Nicolas George wrote:
> Gyan (12019-05-23):
>> -    unlink(buffername);
>> -    av_freep(&buffername);
>> +    ret = avpriv_io_delete(buffername);
> Is there a reason you replace unlink() with avpriv_io_delete()? unlink()
> is more direct, and cache does not support non-file cache.

avpriv_io_delete will call the file protocol's delete which is guarded with a header check, not done here. Since we now have a generic wrapper function, isn't that better for future maintenance? The unlink call was added in 2011, the wrapper in 2015.

Gyan
Nicolas George May 23, 2019, 7:28 p.m.
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()?

> 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.

Regards,
Gyan May 23, 2019, 7:55 p.m.
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.

Gyan
Hendrik Leppkes May 23, 2019, 8:36 p.m.
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.

- Hendrik

Patch hide | download patch | download mbox

diff --git a/libavformat/cache.c b/libavformat/cache.c
index 66bbbf54c9..05dd96c7c0 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 = avpriv_io_delete(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 = avpriv_io_delete(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);