| Message ID | 2b67cafd-f865-a901-a3ab-e54836c52dfb@gyani.pro |
|---|---|
| State | Superseded |
| Headers | show |
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 [...]
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);