[FFmpeg-devel] [PATCH 2/2] lavf/url: rewrite ff_make_absolute_url() using ff_url_decompose().

[Nicolas George]

Marton Balint (12020-08-05):
> memmove, because out and in can overlap or they can be the same when
> buf == base in ff_make_absolute_url()

Good catch. Fixed.

> A problem with the function type change is that previously the caller
> assumed that even if there is not enough space, buf will be a 0 terminated
> string. This guarantee is now lost, so all usage of ff_make_absolute_url
> should also be updated to check error code, otherwise crashes might occur.
> Or we could re-introduce that guarantee, at least temporarily, and change
> type and add checks in a later patch.

I do not like the idea of delaying the error check, but the invariant
must be preserved. I think the solution I have found is good.

> These last two checks should be moved up for better readability (to set
> simplify_path in one section)

Done. Unfortunately, I had to re-alter simplify_path after to handle the
last case. I just locally added /* No path at all, leave it */ after
sending the patch to make it more explicit.

> We should add a reference to the function docs to the relevant RFC:
> https://tools.ietf.org/html/rfc3986#section-5

Good idea.

> It would probably make sense to add all tests which are in
> https://tools.ietf.org/html/rfc3986#section-5.4

Done.

> I did a quick check manually, and found one failing test:
> 
> http://a/b/c/d;p?q //g                  => http://g/
> 
> but it supposed to be "http://g". Not that it matters too much...

It probably does not (is there a real protocol where an empty path makes
sense), but better handle it according to the official examples, it was
not hard.

Thanks for the comments. See the new version.

Regards,

-- 
  Nicolas George
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20200805/7172f98c/attachment.sig>