On Fri, 8 Jan 2010, Chris Conroy wrote:

> The patch is against 17.9.7. A patch against CVS will come early next week.

Nice work! I do believe the patch needs some tweaks etc, but it's clearly a
big step in the right direction!

You will see that your patch won't apply cleanly against CVS since some
changes since 7.19.7 will conflict with yours. It will thus make it a better
patch if you make against CVS and show us.

I'll still start with a bunch of first comments that struck me when reading
the patch.

+ * Currently only CURL_RTSP_VERSION_1_0 is supported
+ */
+ CINIT(RTSP_VERSION, LONG, 190),

This seems unnecessary to add if you only support one version anyway. Besides,
in the code you do:

+ result = add_bufferf(req_buffer,
+ "%s %s RTSP/1.0\r\n" /* Request Stream-URI RTSP/1.0 */

... which doesn't care about the option anyway!

+ /* Let the application define a custom write method for RTP data
+ * This is like the other write callbacks with an extra parameter
+ * for the RTP channel.
+ */
+ CINIT(RTP_FUNCTION, FUNCTIONPOINT, 194),

Does RTSP use the regular write callback for something else and the RTP one
for just RTP data? I also would prefer if you removed the underscore in that
name to match other *FUNCTION names and your RTPDATA counterpart.

Regarding several (all?) of the HTTP-private functions you made global within
the library: all such functions with libcurl are to be prefixed with Curl_
(upper case C) to make it obvious to readers and to reduce the risk of the
symbols causing problems on environments that will show those names to the
outside.

I would also like to see your 'struct RTSP' definition get moved to 'rtsp.h'
in the same manner of how we do for all protocols in the current code. It
makes it easier to find protocol-specific details and code.

+ /* convert from the network encoding using a scratch area */
+ char *scratch = calloc(1, strlen(s)+1);
+ if(NULL == scratch) {
+ failf (data, "Failed to calloc memory for conversion!");
+ return FALSE; /* can't return CURLE_OUT_OF_MEMORY so return FALSE */
+ }
+ strcpy(scratch, s);

This would be shorter, faster and nicer to instead just use strdup().

      else if(data->cookies &&
+ !(conn->protocol & PROT_RTSP) &&
              checkprefix("Set-Cookie:", k->p)) {

I'm curious about this change. Does RTSP support cookies somehow but you
disable that support here? Or why do you feel the need to make sure
Set-Cookie: doesn't work with RTSP?

I think we should also work slightly more on moving RTSP (and HTTP) specific
code out from generic areas (such as lib/transfer.c) into their own source
files and use function calls or perhaps even function pointers in the handler
struct for them instead. This to reduce the protocol-specific chatter in
generic code and to make it easier to find the protocol-specific code when
you're browsing the code for it.

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