| Message ID | ab746cdf-242f-eda7-52f6-e84663de8714@gyani.pro |
|---|---|
| State | Superseded |
| Headers | show |
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,
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
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,
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
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
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);