On Thu, 21 Feb 2013, Chrysovaladis Datsios wrote:

> I read no prohibition in putting read function inside main. It's just a
> personal decision. I believe if the patch gets accepted a more "formal" code
> example will be written.

It isn't C89 compliant code so it couldn't be used by us as-is, and we can't
accept the code without some test code being provided along with the patch.

You're not making this very easy for me. I spend time to help you cleanup
things, I make a patch that is a step forward and then hand the work back to
you. You then (seemingly) completely ignore my work and start over with your
stuff and present me an updated patch that isn't based on what I did for you.

So now I need to redo that work again and show it back to you _again_ only to
see you throw it away? While I enjoy working on curl, repeatedly doing the
same things is not my favorite hobby. If you don't want to base your work on
my improvements, then you need to at least look at my patch and incorporate
back some of my changes. I've fixed your "funny" indenting and I updated
copyright years and some little tidbits. And I made a full and proper commit
message that seemed at least something to base further edits on.

Please make a full git commit with your changes when you send a patch,
including a full commit message and proper author.

Now, over to my review of the contents of the 21feb incarnation of this patch
(my comments are in a somewhat random order):

  1 - it doesn't follow our indent style and you use 'char * name' style of
      declarations that we don't use.

  2 - why calloc() a 100K buffer instead of just malloc? And you don't have to
      typecast the return value of calloc() (or malloc) to char *.

  3 - I would appreciate a way that starts off with a much smaller buffer but
      that can grow if needed. 100K seems excessive in just about all
      sensible use-cases.

  4 - Why do you need 'trailer_headers' to be in the connectdata struct? I
      could easily remove it and just have it as a local variable and nothing
      seems to break? That leads me into...

  5 - Please consider a few test cases that proves that your code is working.

  6 - You don't check the return code from calloc() or realloc() which thus
      will crash the lib on out of memory situations. We can't have that and
      this would be easily tested if we had a test case.

  7 - the realloc(NULL) case below the header look is a mystery to me. Why
      realloc? Also, is that not a memory leak? Would've been shown if we had
      test cases...

  8 - Also, the realloc() does two strlen() calls, the first being
      strlen(trailer_headers_data). Isn't that superfluos as that's exactly the
      length 'trlen' holds at that point?

  9 - then you call strlen() again on the line below. Remember that you allow
      100K of data in there, doing strlen() is not a quick operation then...
      In general you do a lot of repeated strlen() calls in that logic.

  10 - the linked list a callback returns to libcurl, where is that freed?

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