Daniel hi, it wasn't my intention to make your life hard. I too enjoy working with(and for) libcurl and I want to write something useful for it. You'll hear soon from me!
2013/2/22 Daniel Stenberg <[email protected]>: > 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 ------------------------------------------------------------------- List admin: http://cool.haxx.se/list/listinfo/curl-library Etiquette: http://curl.haxx.se/mail/etiquette.html
