On Thu, 17 Dec 2015, Inca R wrote:

> I have been working on curl tfo support and managed to get a working
> version. This is my first pass at TFO with some basic understanding of curl
> code. Would love if you can have a look at it and guide me with whats needed
> to be done to make it in upstream. I have attached the patch for it. If you
> prefer a pull request on github I can do that as well.

Whatever you prefer. Either a patch here or a pull-request, both are fine with
me.

First out what's really missing in your patch:

* Documentation for the new option.

* Make sure all test cases still run fine. Would be good to enable the option
   too and run to make sure that everything works like that too.

* configure checks to see if MSG_FASTOPEN exists! Have you tested this on
   anything else than Linux? (I don't know what others that might support it.)

And then some other things I thought about when reading it:

* You've left printf() calls in there. For debugging/tracing, you can check
   out for example http2.c how we log stuff.

* If a client supports TFO, is there a downside if we'd enable it by default?

* The swriteto() macro seems pointless since it is only used at one place

* Did you try this with a host that has both IPv4+IPv6 addresses to see that
   happy eyeballs play fine with it?

* Does it handle failed connects properly and try the next address? Run a
   server on localhost and connect to bad10.haxx.se for example. (It is a DNS
   entry that returns 5 addresses, 4 of them are 10.0.0.X and one is 127.0.0.1)

(I haven't tried it yet!)

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