[FFmpeg-devel] [PATCH v6 1/1] avformat: Add IPFS protocol support.

[Mark Gaiser]

On Wed, Feb 16, 2022 at 10:40 AM Tomas Härdin <tjoppen at acc.umu.se> wrote:

>
> > +    // Test $IPFS_GATEWAY.
> > +    if (getenv("IPFS_GATEWAY") != NULL) {
> > +        snprintf(c->gateway_buffer, sizeof(c->gateway_buffer), "%s",
> > +                 getenv("IPFS_GATEWAY"));
>
> might want to error check this one
>
> > +        ret = 1;
> > +        goto err;
> > +    } else
> > +        av_log(h, AV_LOG_DEBUG, "$IPFS_GATEWAY is empty.\n");
> > +
> > +    // We need to know the IPFS folder to - eventually - read the
> > contents of
> > +    // the "gateway" file which would tell us the gateway to use.
> > +    if (getenv("IPFS_PATH") == NULL) {
> > +        av_log(h, AV_LOG_DEBUG, "$IPFS_PATH is empty.\n");
> > +
> > +        // Try via the home folder.
> > +        if (getenv("HOME") == NULL) {
> > +            av_log(h, AV_LOG_ERROR, "$HOME appears to be empty.\n");
> > +            ret = AVERROR(EINVAL);
> > +            goto err;
> > +        }
> > +
> > +        // Verify the composed path fits.
> > +        if (snprintf(ipfs_full_data_folder,
> > sizeof(ipfs_full_data_folder),
> > +                     "%s/.ipfs/", getenv("HOME")) >
> > sizeof(ipfs_full_data_folder)) {
>
> >= not > since snprintf() returns the number of character written sans
> the terminating NUL
>
> > +            av_log(h, AV_LOG_ERROR, "The IPFS data path exceeds the
> > max path length (%li)\n", sizeof(ipfs_full_data_folder));
> > +            ret = AVERROR(EINVAL);
> > +            goto err;
> > +        }
> > +
> > +        // 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
> > +        if (stat_ret < 0) {
> > +            av_log(h, AV_LOG_INFO, "Unable to find IPFS folder. We
> > tried:\n");
> > +            av_log(h, AV_LOG_INFO, "- $IPFS_PATH, which was
> > empty.\n");
> > +            av_log(h, AV_LOG_INFO, "- $HOME/.ipfs (full uri: %s)
> > which doesn't exist.\n", ipfs_full_data_folder);
> > +            ret = AVERROR(ENOENT);
> > +            goto err;
> > +        }
> > +    } else
> > +        snprintf(ipfs_full_data_folder,
> > sizeof(ipfs_full_data_folder), "%s",
> > +                 getenv("IPFS_PATH"));
>
> not checked
>
> > +
> > +    // Copy the fully composed gateway path into ipfs_gateway_file.
> > +    if (snprintf(ipfs_gateway_file, sizeof(gateway_file_data),
> > "%sgateway",
> > +                 ipfs_full_data_folder) > sizeof(ipfs_gateway_file))
> > {
>
> >=
>
> > +    // At this point gateway_file_data contains at least something.
> > +    // Copy it into c->gateway_buffer.
> > +    if (snprintf(c->gateway_buffer, sizeof(c->gateway_buffer), "%s",
> > +                 gateway_file_data) > 0) {
> > +        ret = 1;
> > +        goto err;
> > +    } else
> > +        av_log(h, AV_LOG_DEBUG, "Unknown error in the IPFS gateway
> > file.\n");
>
> why not read directly into c->gateway_buffer?
>

Yes! That would be cleaner, thank you for this suggestion!
I didn't do that before because there was no buffer like this before. But
now that it's there, I might as well use it.

>
> > +    // Pppulate c->gateway_buffer with whatever is in c->gateway
> > +    if (c->gateway != NULL)
> > +        snprintf(c->gateway_buffer, sizeof(c->gateway_buffer), "%s",
> > c->gateway);
> > +    else
> > +        c->gateway_buffer[0] = '\0';
> > +
> > +    // Only do the auto detection logic if the gateway_buffer is
> > empty
> > +    if (c->gateway_buffer[0] == '\0') {
>
> these two ifs can be rolled together
>

Smart!

>
> > +    // Concatenate the url.
> > +    // This ends up with something like:
> > http://localhost:8080/ipfs/Qm.....
> > +    fulluri = av_asprintf("%s%s%s", c->gateway_buffer,
> > +                          (is_ipns) ? "ipns/" : "ipfs/",
> > +                          ipfs_cid);
>
> it is here that I mean you can stick in the / if necessary. that would
> make the code much simpler
>

Would it?
I specifically tried to keep gateway url adjustments in
sanitize_ipfs_gateway.
At this moment all that logic is in sanitize_ipfs_gateway. Are you sure
you want me to pull out one part and have it here instead?

Adding it here would essentially wrap the first argument in a ternary like:
(c->gateway_buffer[strlen(c->gateway_buffer) - 1] == '/') ? "%s%s%s" :
"%s/%s%s"

I prefer to keep this in sanitize_ipfs_gateway.
I'll wait for your response on this one to see if I need to move it or if
it can stay as is.



>
> /Tomas
>
>