[FFmpeg-devel] [PATCH] url_split() ipv6 support

[Michael Niedermayer]

Hi

On Thu, Sep 27, 2007 at 10:04:51AM -0400, Ronald S. Bultje wrote:
> Hi Michael,
> 
> On 9/27/07, Michael Niedermayer <michaelni at gmx.at> wrote:
> >
> > url_split() looks messy, maybe we should clean it up first, maybe
> > something
> > like:
> >
> > char tmp[256];
> > int count;
> > proto[0] = auth[0]= 0;
> > if(sscanf(p, "%255[^:]:%*[:/]%n", &tmp, &count) == 2){
> >     av_strlcpy(proto, tmp, proto_size);
> >     p+= count;
> > }
> > if(sscanf(p, "%255[^@/]@%n", &tmp, &count) == 2){
> >     av_strlcpy(auth, tmp, auth_size);
> >     p+= count;
> > }
> >
> >
> > could be used?
> 
> 
> It is messy, I agree, however, sscanf() isn't good for a few reasons (may or
> may not be important):
> - relatively unreadable compared to strchr(), but same LOC
> - (probably?) slower than strchr()
> - sscanf() should probably directly write into the buffer instead of into a
> tmp
> 
> I took a slightly different approach of using strchr() in different order
> (first separate proto, then path, then parse the hostname and auth/port info
> in it), it looks pretty good (=simple/readable), see attached. It doesn't
> parse ipv6 urls yet, to separate the rewrite from the new functionality. I
> hope this is more to your taste, then I'll add ipv6 to this one.
> 
> The patch exposes a (API) problem in av_strlcpy(), where I can't (API-wise)
> straightforwardly copy only n characters (like strncpy()), because
> av_strlcpy(proto, "http://bla", 4); only copies 3 chars (4th being the
> '\0'). I have to add +1 to each av_strlcpy() to actually copy all characters
> up to the non-\0-endpoint into the target buffer. Maybe we need a
> av_strlncpy() to make this API more fluffy?
> 
> Ronald

> Index: utils.c
> ===================================================================
> --- utils.c	(revision 9789)
> +++ utils.c	(working copy)
> @@ -2793,68 +2793,51 @@
>                 char *path, int path_size,
>                 const char *url)
>  {
> +    const char *p, *ls;
>  
> +    if (authorization_size > 0) authorization[0] = '\0';
> +    if (hostname_size > 0) hostname[0] = '\0';

these can be vertically aligned and 0 is enough no '\0' needed


>  
> +    /* parse protocol */
> +    if ((p = strchr(url, ':'))) {
>          p++;
> +        av_strlcpy(proto, url, FFMIN(proto_size, p - url));
> +        if (*p == '/') p++;
> +        if (*p == '/') p++;
>      } else {
> +        /* no protocol means plain filename */

> +        if (proto_size > 0) proto[0] = '\0';

this can be placed below the other 2 = 0 above


> +        av_strlcpy(path, url, path_size);
> +        return;
> +    }
>  
> +    /* separate path from hostname */
> +    if ((ls = strchr(p, '/')))
> +        av_strlcpy(path, ls, path_size);
> +    else {
> +        ls = &p[strlen(p)]; // XXX

> +        if (path_size > 0) path[0] = '\0';

this can as well be placed at the top


> +    }
>  
> +    /* the rest is hostname, use that to parse auth/port */
> +    if (ls != p) {

> +        const char *at, *col;

these can be merged with the char * at the top


>  
> +        /* authorization (user[:pass]@hostname) */
> +        if ((at = strchr(p, '@')) && at < ls) {

> +            at++;
> +            av_strlcpy(authorization, p, FFMIN(authorization_size, at - p));

at + 1 - p


> +            p = at;
>          }
> +
> +        /* port */
> +        if ((col = strchr(p, ':')) && col < ls) {
> +            ls = col;

> +            if (port_ptr) *port_ptr = atoi(&col[1]);

col+1

except these this looks very nice, and can be commited as soon as the ones
above are fixed and it works

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I hate to see young programmers poisoned by the kind of thinking
Ulrich Drepper puts forward since it is simply too narrow -- Roman Shaposhnik
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070927/b959d525/attachment.pgp>