On Sat, 1 May 2010, Howard Chu wrote:

> I started looking into adding librtmp to libcurl

Whoa, that sounds awesome! Cool.

Which librtmp is this? I googled for it but it wasn't clear to me if there's
just one or if they are many.

> I have a few concerns about the library structure. E.g.

> +#define CURLPROTO_RTMP (1<<19)
> +#define CURLPROTO_RTMPT (1<<20)
> +#define CURLPROTO_RTMPE (1<<21)
> +#define CURLPROTO_RTMPTE (1<<22)
> +#define CURLPROTO_RTMPS (1<<23)

This looks... special. What's the need for adding RTMP as five protocols?

> It strikes me that using an enum for the base protocol types would have been
> better than using a bitmask. You're going to run out of bits very soon.

Right, as we've reached 19 in 12 years, we will reach 32 in just another 8
years if we keep up the same pace. Can we really think of 13 addititional
relevant protocols to add?

Besides, if you just look at the code for another 15 minutes you should soon
find out that just using an enum is not doable without modifying how things
work. Like CURLOPT_PROTOCOLS and CURLOPT_REDIR_PROTOCOLS and more.

>>>>>
> /* (1<<18) is currently the highest used bit in the public bitmask. We make
> sure we use "private bits" above the public ones to make things easier. */
>
> #define PROT_EXTMASK 0xfffff
>
> #define PROT_SSL (1<<25) /* protocol requires SSL */

PROT_SSL can actually be modified to not steal a bit but instead be something
like:

#define PROT_SSL (PROT_HTTPS|PROT_FTPS|PROT_IMAPS|PROT_POP3S|PROT_SMTPS)

... and perhaps adapt some code slightly to make sure it works with this.

> Also from a coding practices standpoint, bitmasks should be used for options
> that may potentially be used together.

Exactly.

> Protocol types are strictly exclusive;

No they're not.

> you cannot have a connection be both HTTP and FTP simultaneously.

That's very true, and yes we don't need a bitmask to know what protocol a
particular connection uses.

> On a related note, it seems redundant to need a separate Curl_handler struct
> for each variant of the base protocol.

I completely agree, but I don't see anything in libcurl's design that forces
that. I don't know much about RTMP so by all means, please elaborate why you
made it so in the first place or what makes RTMP this special.

-- 
  / daniel.haxx.se
-------------------------------------------------------------------
List admin: http://cool.haxx.se/list/listinfo/curl-library
Etiquette:  http://curl.haxx.se/mail/etiquette.html