From patchwork Thu May 23 17:23:14 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Gyan Doshi X-Patchwork-Id: 13258 Return-Path: X-Original-To: patchwork@ffaux-bg.ffmpeg.org Delivered-To: patchwork@ffaux-bg.ffmpeg.org Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org [79.124.17.100]) by ffaux.localdomain (Postfix) with ESMTP id 762FA444A34 for ; Thu, 23 May 2019 20:23:48 +0300 (EEST) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 504FF68A518; Thu, 23 May 2019 20:23:48 +0300 (EEST) X-Original-To: ffmpeg-devel@ffmpeg.org Delivered-To: ffmpeg-devel@ffmpeg.org Received: from mx1.mailbox.org (mx1.mailbox.org [80.241.60.212]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id C8DAF6802B4 for ; Thu, 23 May 2019 20:23:42 +0300 (EEST) Received: from smtp2.mailbox.org (smtp2.mailbox.org [IPv6:2001:67c:2050:105:465:1:2:0]) (using TLSv1.2 with cipher ECDHE-RSA-CHACHA20-POLY1305 (256/256 bits)) (No client certificate requested) by mx1.mailbox.org (Postfix) with ESMTPS id 53E965050B for ; Thu, 23 May 2019 19:23:42 +0200 (CEST) X-Virus-Scanned: amavisd-new at heinlein-support.de Received: from smtp2.mailbox.org ([80.241.60.241]) by gerste.heinlein-support.de (gerste.heinlein-support.de [91.198.250.173]) (amavisd-new, port 10030) with ESMTP id nnpYdf69cXrf for ; Thu, 23 May 2019 19:23:16 +0200 (CEST) To: ffmpeg-devel@ffmpeg.org References: <20190522130745.zpljeoow76g5xhtz@phare.normalesup.org> <2b67cafd-f865-a901-a3ab-e54836c52dfb@gyani.pro> <20190523154307.GQ3118@michaelspb> From: Gyan Message-ID: Date: Thu, 23 May 2019 22:53:14 +0530 MIME-Version: 1.0 In-Reply-To: <20190523154307.GQ3118@michaelspb> Content-Language: en-US Subject: Re: [FFmpeg-devel] [PATCH] avformat/cache - delete cache file after closing handle X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: FFmpeg development discussions and patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: FFmpeg development discussions and patches Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" 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 >>>> 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 >> 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 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(-) 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);