On Wed, Feb 2, 2022 at 2:21 PM Tomas Härdin <tjop...@acc.umu.se> wrote:
> tis 2022-02-01 klockan 22:58 +0100 skrev Mark Gaiser: > > > > > +typedef struct Context { > > + AVClass *class; > > + URLContext *inner; > > + char *gateway; > > Is there not a maximum length that an HTTP URL can be? At least without > query parameters. That way you avoid dynamic allocations. You'd have to > separate the AVOption from such a buffer in that case, but I think you > have to anyway. > Could you provide more information on that? Or an example of what you mean exactly? As far as i know there is no hard limit though it's very much advised to not go above 2048 characters. > > > + if (!ipfs_full_data_folder) { > > + av_log(h, AV_LOG_DEBUG, "$IPFS_PATH is empty.\n"); > > + > > + // Try via the home folder. > > + home_folder = getenv("HOME"); > > + ipfs_full_data_folder = av_asprintf("%s/.ipfs/", > > home_folder); > > Memory leak. This applies to most if not all av_asprintf() calls. > Is there an advised way to neatly clean that up? Sure, I can add a bunch of av_free calls to clean it up. But there are places where it's not as straightforward like where the av_asprintf was done in an if statement. How do I maintain the knowledge that av_asprintf was used to call av_free later? In a C++ world I'd use a scoped variable ;) But I kinda miss how to do that properly here. > > + > > + // Stat the folder. It should exist in a default IPFS setup > > when run as local user. > > +#ifndef _WIN32 > > + stat_ret = stat(ipfs_full_data_folder, &st); > > +#else > > + stat_ret = win32_stat(ipfs_full_data_folder, &st); > > +#endif > > Why bother with stat() when you can just check whether fopen() > succeeded? > Ohh! Nice one! It doesn't make the code shorter or easier though. But does get rid of platform dependent stuff so a win imho anyhow. > > > +// For now just makes sure that the gateway ends in url we expect. > > Like http://localhost:8080/. > > +// Explicitly with the traling slash. > > +static void ff_sanitize_ipfs_gateway(URLContext *h) > > +{ > > + Context *c = h->priv_data; > > + const char last_gateway_char = c->gateway[strlen(c->gateway) - > > 1]; > > Can strlen(c->gateway) be zero here? > Hmm, potentially yes. I'll add a check for it. > > > +static int translate_ipfs_to_http(URLContext *h, const char *uri, > > int flags, AVDictionary **options) > > +{ > > + const char *ipfs_cid; > > + const char *protocol_path_suffix = "ipfs/"; > > + char *fulluri; > > + int ret; > > + Context *c = h->priv_data; > > + int is_ipfs = (av_strstart(uri, "ipfs://", &ipfs_cid) || > > av_strstart(uri, "ipfs:", &ipfs_cid)); > > + int is_ipns = (av_strstart(uri, "ipns://", &ipfs_cid) || > > av_strstart(uri, "ipns:", &ipfs_cid)); > > https://docs.ipfs.io/concepts/ipfs-gateway/ claims ipfs:// is the > canonical form. No mentioned is made of any ipfs:{CID} form. Incorrect > URLs should be rejected, not silently patched. > I'd like to make a decision here. This current logic (ipfs:// and ipfs:, same for ipns) is inspired by other protocols that ffmpeg supported. I simply copied how they work to be consistent. Do i: 1. keep it as is and be consistent with the rest? 2. only allow ipfs:// and ipns://? > > Also what happens if c->gateway is "ipfs://[...]"? Infinite recursion? > Nice one, I need to test that! > > /Tomas > > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe". > _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".