[GitHub] trafficserver pull request #1294: TS-5059: Port TCP Fast Open BIO to OpenSSL...

[bryancall]

Github user bryancall closed the pull request at:

https://github.com/apache/trafficserver/pull/1294


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #1294: TS-5059: Port TCP Fast Open BIO to OpenSSL...

[jablko]

Github user jablko commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/1294#discussion_r94839085
  
--- Diff: iocore/net/BIO_fastopen.cc ---
@@ -116,39 +115,21 @@ static long
 fastopen_ctrl(BIO *bio, int cmd, long larg, void *ptr)
 {
   switch (cmd) {
-  case BIO_C_SET_FD:
-ink_assert(larg == BIO_CLOSE || larg == BIO_NOCLOSE);
-ink_assert(bio->num == NO_FD);
-
-bio->init = 1;
-bio->shutdown = larg;
-bio->num  = *reinterpret_cast(ptr);
-return 0;
-
   case BIO_C_SET_CONNECT:
 // We only support BIO_set_conn_address(), which sets a sockaddr.
 ink_assert(larg == 2);
-bio->ptr = ptr;
+BIO_set_data(bio, ptr);
 return 0;
 
-  // We are unbuffered so unconditionally succeed on BIO_flush().
-  case BIO_CTRL_FLUSH:
-return 1;
-
-  case BIO_CTRL_PUSH:
-  case BIO_CTRL_POP:
-return 0;
-
-  default:
-#if DEBUG
-ink_abort("unsupported BIO control cmd=%d larg=%ld ptr=%p", cmd, larg, 
ptr);
-#endif
-
-return 0;
+  case BIO_C_SET_FD:
+ink_assert(larg == BIO_CLOSE || larg == BIO_NOCLOSE);
+ink_assert(BIO_get_fd(bio, nullptr) == NO_FD);
--- End diff --

Done.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #1294: TS-5059: Port TCP Fast Open BIO to OpenSSL...

[jablko]

Github user jablko commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/1294#discussion_r94839028
  
--- Diff: iocore/net/BIO_fastopen.cc ---
@@ -58,26 +57,26 @@ fastopen_bwrite(BIO *bio, const char *in, int insz)
 
   errno = 0;
   BIO_clear_retry_flags(bio);
-  ink_assert(bio->num != NO_FD);
+  ink_assert(BIO_get_fd(bio, nullptr) != NO_FD);
--- End diff --

Done.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #1294: TS-5059: Port TCP Fast Open BIO to OpenSSL...

[jablko]

Github user jablko commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/1294#discussion_r94838912
  
--- Diff: iocore/net/BIO_fastopen.cc ---
@@ -160,9 +174,24 @@ static const BIO_METHOD fastopen_methods = {
   .destroy   = fastopen_destroy,
   .callback_ctrl = nullptr,
 };
+#else
+static BIO_METHOD *fastopen_methods = nullptr;
+#endif
 
 const BIO_METHOD *
 BIO_s_fastopen()
 {
+#if OPENSSL_VERSION_NUMBER < 0x1010L
   return _methods;
+#else
+  if (!fastopen_methods) {
+fastopen_methods = BIO_meth_new(BIO_TYPE_SOCKET, "fastopen");
+BIO_meth_set_write(fastopen_methods, fastopen_bwrite);
+BIO_meth_set_read(fastopen_methods, fastopen_bread);
+BIO_meth_set_ctrl(fastopen_methods, fastopen_ctrl);
+BIO_meth_set_create(fastopen_methods, fastopen_create);
+BIO_meth_set_destroy(fastopen_methods, fastopen_destroy);
--- End diff --

Is the following equivalent?

```C
#if OPENSSL_VERSION_NUMBER < 0x1010L
static const BIO_METHOD fastopen_methods[] = {{
...
}};
#else
static const BIO_METHOD *fastopen_methods = [] {
  BIO_METHOD *fastopen_methods = BIO_meth_new(BIO_TYPE_SOCKET, "fastopen");
  ...
  return fastopen_methods;
}();
#endif

const BIO_METHOD *
BIO_s_fastopen()
{
  return fastopen_methods;
}


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #1294: TS-5059: Port TCP Fast Open BIO to OpenSSL...

[jablko]

Github user jablko commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/1294#discussion_r94836394
  
--- Diff: iocore/net/BIO_fastopen.cc ---
@@ -44,8 +57,7 @@ fastopen_destroy(BIO *bio)
   if (bio) {
 // We expect this BIO to not own the socket, so we must always
 // be in NOCLOSE mode.
-ink_assert(bio->shutdown == BIO_NOCLOSE);
-fastopen_create(bio);
+ink_assert(BIO_get_shutdown(bio) == BIO_NOCLOSE);
--- End diff --

The struct is gone now.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #1294: TS-5059: Port TCP Fast Open BIO to OpenSSL...

[jablko]

Github user jablko commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/1294#discussion_r94835352
  
--- Diff: iocore/net/BIO_fastopen.cc ---
@@ -27,13 +27,26 @@
 
 #include "BIO_fastopen.h"
 
+#if OPENSSL_VERSION_NUMBER < 0x1010L
+#define BIO_set_data(a, ptr) ((a)->ptr = (ptr))
+#define BIO_get_data(a) ((a)->ptr)
+#define BIO_set_init(a, init) ((a)->init = (init))
+#define BIO_set_shutdown(a, shut) ((a)->shutdown = (shut))
+#define BIO_get_shutdown(a) ((a)->shutdown)
+#endif
+
+struct Data {
+  Data() : fd(NO_FD), dst(nullptr) {}
+  int fd;
+  const sockaddr *dst;
+};
--- End diff --

Nice! Done.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #1294: TS-5059: Port TCP Fast Open BIO to OpenSSL...

[jablko]

Github user jablko commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/1294#discussion_r94835039
  
--- Diff: iocore/net/BIO_fastopen.cc ---
@@ -27,13 +27,26 @@
 
 #include "BIO_fastopen.h"
 
+#if OPENSSL_VERSION_NUMBER < 0x1010L
+#define BIO_set_data(a, ptr) ((a)->ptr = (ptr))
--- End diff --

Done.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #1294: TS-5059: Port TCP Fast Open BIO to OpenSSL...

[jablko]

Github user jablko commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/1294#discussion_r94818857
  
--- Diff: iocore/net/BIO_fastopen.cc ---
@@ -27,28 +27,27 @@
 
 #include "BIO_fastopen.h"
 
-static int
-fastopen_create(BIO *bio)
-{
-  bio->init  = 0;
-  bio->num   = NO_FD;
-  bio->flags = 0;
-  bio->ptr   = nullptr;
+#if OPENSSL_VERSION_NUMBER < 0x1010L
+#define BIO_set_data(a, _ptr) ((a)->ptr = (_ptr))
+#define BIO_get_data(a) ((a)->ptr)
+#define BIO_get_shutdown(a) ((a)->shutdown)
+#define BIO_meth_get_ctrl(biom) ((biom)->ctrl)
+#define BIO_meth_get_create(biom) ((biom)->create)
+#define BIO_meth_get_destroy(biom) ((biom)->destroy)
+#endif
 
-  return 1;
-}
+static int (*fastopen_create)(BIO *) = 
BIO_meth_get_create(const_cast(BIO_s_socket()));
 
 static int
 fastopen_destroy(BIO *bio)
 {
   if (bio) {
 // We expect this BIO to not own the socket, so we must always
 // be in NOCLOSE mode.
-ink_assert(bio->shutdown == BIO_NOCLOSE);
-fastopen_create(bio);
+ink_assert(BIO_get_shutdown(bio) == BIO_NOCLOSE);
   }
 
-  return 1;
+  return BIO_meth_get_destroy(const_cast(BIO_s_socket()))(bio);
--- End diff --

Agreed. https://github.com/openssl/openssl/pull/2181


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #1294: TS-5059: Port TCP Fast Open BIO to OpenSSL...

[jpeach]

Github user jpeach commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/1294#discussion_r94717030
  
--- Diff: iocore/net/BIO_fastopen.cc ---
@@ -58,26 +57,26 @@ fastopen_bwrite(BIO *bio, const char *in, int insz)
 
   errno = 0;
   BIO_clear_retry_flags(bio);
-  ink_assert(bio->num != NO_FD);
+  ink_assert(BIO_get_fd(bio, nullptr) != NO_FD);
--- End diff --

Rather than calling `BIO_get_fd` multiple times, call it once and stash the 
fd in a local variable. Same for the write path.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #1294: TS-5059: Port TCP Fast Open BIO to OpenSSL...

[jpeach]

Github user jpeach commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/1294#discussion_r94717201
  
--- Diff: iocore/net/BIO_fastopen.cc ---
@@ -27,28 +27,27 @@
 
 #include "BIO_fastopen.h"
 
-static int
-fastopen_create(BIO *bio)
-{
-  bio->init  = 0;
-  bio->num   = NO_FD;
-  bio->flags = 0;
-  bio->ptr   = nullptr;
+#if OPENSSL_VERSION_NUMBER < 0x1010L
+#define BIO_set_data(a, _ptr) ((a)->ptr = (_ptr))
+#define BIO_get_data(a) ((a)->ptr)
+#define BIO_get_shutdown(a) ((a)->shutdown)
+#define BIO_meth_get_ctrl(biom) ((biom)->ctrl)
+#define BIO_meth_get_create(biom) ((biom)->create)
+#define BIO_meth_get_destroy(biom) ((biom)->destroy)
+#endif
 
-  return 1;
-}
+static int (*fastopen_create)(BIO *) = 
BIO_meth_get_create(const_cast(BIO_s_socket()));
 
 static int
 fastopen_destroy(BIO *bio)
 {
   if (bio) {
 // We expect this BIO to not own the socket, so we must always
 // be in NOCLOSE mode.
-ink_assert(bio->shutdown == BIO_NOCLOSE);
-fastopen_create(bio);
+ink_assert(BIO_get_shutdown(bio) == BIO_NOCLOSE);
   }
 
-  return 1;
+  return BIO_meth_get_destroy(const_cast(BIO_s_socket()))(bio);
--- End diff --

Seems pretty unfortunate that they make you cast away the const here :(


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #1294: TS-5059: Port TCP Fast Open BIO to OpenSSL...

[jpeach]

Github user jpeach commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/1294#discussion_r94717094
  
--- Diff: iocore/net/BIO_fastopen.cc ---
@@ -116,39 +115,21 @@ static long
 fastopen_ctrl(BIO *bio, int cmd, long larg, void *ptr)
 {
   switch (cmd) {
-  case BIO_C_SET_FD:
-ink_assert(larg == BIO_CLOSE || larg == BIO_NOCLOSE);
-ink_assert(bio->num == NO_FD);
-
-bio->init = 1;
-bio->shutdown = larg;
-bio->num  = *reinterpret_cast(ptr);
-return 0;
-
   case BIO_C_SET_CONNECT:
 // We only support BIO_set_conn_address(), which sets a sockaddr.
 ink_assert(larg == 2);
-bio->ptr = ptr;
+BIO_set_data(bio, ptr);
 return 0;
 
-  // We are unbuffered so unconditionally succeed on BIO_flush().
-  case BIO_CTRL_FLUSH:
-return 1;
-
-  case BIO_CTRL_PUSH:
-  case BIO_CTRL_POP:
-return 0;
-
-  default:
-#if DEBUG
-ink_abort("unsupported BIO control cmd=%d larg=%ld ptr=%p", cmd, larg, 
ptr);
-#endif
-
-return 0;
+  case BIO_C_SET_FD:
+ink_assert(larg == BIO_CLOSE || larg == BIO_NOCLOSE);
+ink_assert(BIO_get_fd(bio, nullptr) == NO_FD);
--- End diff --

I don't think you need to worry about keeping this any more.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #1294: TS-5059: Port TCP Fast Open BIO to OpenSSL...

[jpeach]

Github user jpeach commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/1294#discussion_r94530155
  
--- Diff: iocore/net/BIO_fastopen.cc ---
@@ -44,8 +57,7 @@ fastopen_destroy(BIO *bio)
   if (bio) {
 // We expect this BIO to not own the socket, so we must always
 // be in NOCLOSE mode.
-ink_assert(bio->shutdown == BIO_NOCLOSE);
-fastopen_create(bio);
+ink_assert(BIO_get_shutdown(bio) == BIO_NOCLOSE);
--- End diff --

The `Data` struct is getting leaked here, but (see above) I think we can 
get away without it.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #1294: TS-5059: Port TCP Fast Open BIO to OpenSSL...

[jpeach]

Github user jpeach commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/1294#discussion_r94529780
  
--- Diff: iocore/net/BIO_fastopen.cc ---
@@ -27,13 +27,26 @@
 
 #include "BIO_fastopen.h"
 
+#if OPENSSL_VERSION_NUMBER < 0x1010L
+#define BIO_set_data(a, ptr) ((a)->ptr = (ptr))
--- End diff --

Check the preprocessed source, but I think this should be:
```C
#define BIO_set_data(a, _ptr) ((a)->ptr = (_ptr))
```

Same for the others below.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #1294: TS-5059: Port TCP Fast Open BIO to OpenSSL...

[jpeach]

Github user jpeach commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/1294#discussion_r94529615
  
--- Diff: iocore/net/BIO_fastopen.cc ---
@@ -160,9 +174,24 @@ static const BIO_METHOD fastopen_methods = {
   .destroy   = fastopen_destroy,
   .callback_ctrl = nullptr,
 };
+#else
+static BIO_METHOD *fastopen_methods = nullptr;
+#endif
 
 const BIO_METHOD *
 BIO_s_fastopen()
 {
+#if OPENSSL_VERSION_NUMBER < 0x1010L
   return _methods;
+#else
+  if (!fastopen_methods) {
+fastopen_methods = BIO_meth_new(BIO_TYPE_SOCKET, "fastopen");
+BIO_meth_set_write(fastopen_methods, fastopen_bwrite);
+BIO_meth_set_read(fastopen_methods, fastopen_bread);
+BIO_meth_set_ctrl(fastopen_methods, fastopen_ctrl);
+BIO_meth_set_create(fastopen_methods, fastopen_create);
+BIO_meth_set_destroy(fastopen_methods, fastopen_destroy);
--- End diff --

I think this would be a little cleaner (and avoid the race condition) if 
you used a static initializer:

```C
struct fastopen_bio
{
fastopen_bio() {
#if OPENSSL_VERSION_NUMBER < 0x1010L
  static const BIO_METHOD m = {
  ...
  };

  method = 
#else
  method = BIO_meth_new(BIO_TYPE_SOCKET, "fastopen");
  BIO_meth_set_write(method, fastopen_bwrite);
  ...
#endif
}

const BIO_METHOD *method; 
};

static fastopen_bio fastopen;

const BIO_METHOD *
BIO_s_fastopen()
{
return fastopen.methods;
}
```


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #1294: TS-5059: Port TCP Fast Open BIO to OpenSSL...

[jpeach]

Github user jpeach commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/1294#discussion_r94530947
  
--- Diff: iocore/net/BIO_fastopen.cc ---
@@ -27,13 +27,26 @@
 
 #include "BIO_fastopen.h"
 
+#if OPENSSL_VERSION_NUMBER < 0x1010L
+#define BIO_set_data(a, ptr) ((a)->ptr = (ptr))
+#define BIO_get_data(a) ((a)->ptr)
+#define BIO_set_init(a, init) ((a)->init = (init))
+#define BIO_set_shutdown(a, shut) ((a)->shutdown = (shut))
+#define BIO_get_shutdown(a) ((a)->shutdown)
+#endif
+
+struct Data {
+  Data() : fd(NO_FD), dst(nullptr) {}
+  int fd;
+  const sockaddr *dst;
+};
--- End diff --

Actually, to support `BIO_set_conn_address`, you'll need your own `ctrl` 
method that handles `BIO_C_SET_CONNECT` and forwards the remaining commands to 
the socket method.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #1294: TS-5059: Port TCP Fast Open BIO to OpenSSL...

[jpeach]

Github user jpeach commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/1294#discussion_r94530637
  
--- Diff: iocore/net/BIO_fastopen.cc ---
@@ -27,13 +27,26 @@
 
 #include "BIO_fastopen.h"
 
+#if OPENSSL_VERSION_NUMBER < 0x1010L
+#define BIO_set_data(a, ptr) ((a)->ptr = (ptr))
+#define BIO_get_data(a) ((a)->ptr)
+#define BIO_set_init(a, init) ((a)->init = (init))
+#define BIO_set_shutdown(a, shut) ((a)->shutdown = (shut))
+#define BIO_get_shutdown(a) ((a)->shutdown)
+#endif
+
+struct Data {
+  Data() : fd(NO_FD), dst(nullptr) {}
+  int fd;
+  const sockaddr *dst;
+};
--- End diff --

I think that we can work around needing a separate `Data` struct. Since we 
actually have a socket we can now directly hook the `ctrl` method from the 
socket `BIO`:

```C
const BIO_METHOD * s = BIO_s_socket();
BIO_meth_set_ctrl(fastopen.method, BIO_meth_get_ctrl(s));
```
We can set the `ptr` field with `BIO_set_data`.  We can use `BIO_get_fd` to 
get the file descriptor when we need it.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #1294: TS-5059: Port TCP Fast Open BIO to OpenSSL...

[jpeach]

Github user jpeach commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/1294#discussion_r94530049
  
--- Diff: iocore/net/BIO_fastopen.cc ---
@@ -27,13 +27,26 @@
 
 #include "BIO_fastopen.h"
 
+#if OPENSSL_VERSION_NUMBER < 0x1010L
+#define BIO_set_data(a, ptr) ((a)->ptr = (ptr))
+#define BIO_get_data(a) ((a)->ptr)
+#define BIO_set_init(a, init) ((a)->init = (init))
+#define BIO_set_shutdown(a, shut) ((a)->shutdown = (shut))
+#define BIO_get_shutdown(a) ((a)->shutdown)
+#endif
+
+struct Data {
+  Data() : fd(NO_FD), dst(nullptr) {}
+  int fd;
+  const sockaddr *dst;
+};
+
 static int
 fastopen_create(BIO *bio)
 {
-  bio->init  = 0;
-  bio->num   = NO_FD;
-  bio->flags = 0;
-  bio->ptr   = nullptr;
+  Data *data = new Data;
--- End diff --

Right, the reinitialization in `fastopen_destroy` is just being defensive.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #1294: TS-5059: Port TCP Fast Open BIO to OpenSSL...

[jablko]

Github user jablko commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/1294#discussion_r94481428
  
--- Diff: iocore/net/BIO_fastopen.cc ---
@@ -27,13 +27,26 @@
 
 #include "BIO_fastopen.h"
 
+#if OPENSSL_VERSION_NUMBER < 0x1010L
+#define BIO_set_data(a, ptr) ((a)->ptr = (ptr))
+#define BIO_get_data(a) ((a)->ptr)
+#define BIO_set_init(a, init) ((a)->init = (init))
+#define BIO_set_shutdown(a, shut) ((a)->shutdown = (shut))
+#define BIO_get_shutdown(a) ((a)->shutdown)
+#endif
+
+struct Data {
+  Data() : fd(NO_FD), dst(nullptr) {}
+  int fd;
+  const sockaddr *dst;
+};
+
 static int
 fastopen_create(BIO *bio)
 {
-  bio->init  = 0;
-  bio->num   = NO_FD;
-  bio->flags = 0;
-  bio->ptr   = nullptr;
+  Data *data = new Data;
--- End diff --

Right, previously we stashed the file descriptor in bio->num and the 
destination address in bio->ptr, but now BIO is opaque and I found no way to 
access bio->num anymore -- so I think I now need to stash both in bio->ptr?

Good thinking re: an existing data structure, however I think we get the 
file descriptor and destination address via separate calls to fastopen_ctrl(), 
so I don't know if that's feasible, even if a candidate structure exists?

I understand why bio->num and bio->ptr needed to be initialized in 
fastopen_create(). I assumed initializing them again in fastopen_destroy() (by 
calling fastopen_create()) was just defensive coding? Now that deallocation is 
involved, I'm not sure what the right thing to is. "delete data", I guess? or 
something more/else?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #1294: TS-5059: Port TCP Fast Open BIO to OpenSSL...

[PSUdaemon]

Github user PSUdaemon commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/1294#discussion_r94471863
  
--- Diff: iocore/net/BIO_fastopen.cc ---
@@ -27,13 +27,26 @@
 
 #include "BIO_fastopen.h"
 
+#if OPENSSL_VERSION_NUMBER < 0x1010L
+#define BIO_set_data(a, ptr) ((a)->ptr = (ptr))
+#define BIO_get_data(a) ((a)->ptr)
+#define BIO_set_init(a, init) ((a)->init = (init))
+#define BIO_set_shutdown(a, shut) ((a)->shutdown = (shut))
+#define BIO_get_shutdown(a) ((a)->shutdown)
+#endif
+
+struct Data {
--- End diff --

I'm not picky. Something like `BIOData` would be fine with me, just to 
remind people what it is when looking at it in the code. I'm sure someone else 
might have a better idea. Perhaps @shinrich, @SolidWallOfCode, or @jpeach.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #1294: TS-5059: Port TCP Fast Open BIO to OpenSSL...

[PSUdaemon]

Github user PSUdaemon commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/1294#discussion_r94472934
  
--- Diff: iocore/net/BIO_fastopen.cc ---
@@ -27,13 +27,26 @@
 
 #include "BIO_fastopen.h"
 
+#if OPENSSL_VERSION_NUMBER < 0x1010L
+#define BIO_set_data(a, ptr) ((a)->ptr = (ptr))
+#define BIO_get_data(a) ((a)->ptr)
+#define BIO_set_init(a, init) ((a)->init = (init))
+#define BIO_set_shutdown(a, shut) ((a)->shutdown = (shut))
+#define BIO_get_shutdown(a) ((a)->shutdown)
+#endif
+
+struct Data {
+  Data() : fd(NO_FD), dst(nullptr) {}
+  int fd;
+  const sockaddr *dst;
+};
+
 static int
 fastopen_create(BIO *bio)
 {
-  bio->init  = 0;
-  bio->num   = NO_FD;
-  bio->flags = 0;
-  bio->ptr   = nullptr;
+  Data *data = new Data;
--- End diff --

TBH, I am a bit confused by the original code. For example, why was 
`fastopen_create` called in `fastopen_destroy`? Then why did you remove that?

It also seems like we were previously overloading these values in the `BIO` 
struct out of convenience, and maybe we can just use a pointer to an existing 
data structure? I'm not really familiar with how these BIO's were meant to be 
used before or not.

It looks like @jpeach wrote this code originally so maybe it'd be good for 
him to review.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #1294: TS-5059: Port TCP Fast Open BIO to OpenSSL...

[jablko]

Github user jablko commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/1294#discussion_r94465164
  
--- Diff: iocore/net/BIO_fastopen.cc ---
@@ -27,13 +27,26 @@
 
 #include "BIO_fastopen.h"
 
+#if OPENSSL_VERSION_NUMBER < 0x1010L
+#define BIO_set_data(a, ptr) ((a)->ptr = (ptr))
+#define BIO_get_data(a) ((a)->ptr)
+#define BIO_set_init(a, init) ((a)->init = (init))
+#define BIO_set_shutdown(a, shut) ((a)->shutdown = (shut))
+#define BIO_get_shutdown(a) ((a)->shutdown)
+#endif
+
+struct Data {
+  Data() : fd(NO_FD), dst(nullptr) {}
+  int fd;
+  const sockaddr *dst;
+};
+
 static int
 fastopen_create(BIO *bio)
 {
-  bio->init  = 0;
-  bio->num   = NO_FD;
-  bio->flags = 0;
-  bio->ptr   = nullptr;
+  Data *data = new Data;
--- End diff --

Is this correct? Should I "delete data" in fastopen_destroy()?
Sorry, I'm sketchy on Traffic Server and C++ memory management patterns ...


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #1294: TS-5059: Port TCP Fast Open BIO to OpenSSL...

[jablko]

Github user jablko commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/1294#discussion_r94464331
  
--- Diff: iocore/net/BIO_fastopen.cc ---
@@ -27,13 +27,26 @@
 
 #include "BIO_fastopen.h"
 
+#if OPENSSL_VERSION_NUMBER < 0x1010L
+#define BIO_set_data(a, ptr) ((a)->ptr = (ptr))
+#define BIO_get_data(a) ((a)->ptr)
+#define BIO_set_init(a, init) ((a)->init = (init))
+#define BIO_set_shutdown(a, shut) ((a)->shutdown = (shut))
+#define BIO_get_shutdown(a) ((a)->shutdown)
+#endif
+
+struct Data {
--- End diff --

No, I agree. Can you help me come up with the right name?
How are the other symbols, data->fd and data->dst?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #1294: TS-5059: Port TCP Fast Open BIO to OpenSSL...

[PSUdaemon]

Github user PSUdaemon commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/1294#discussion_r94357370
  
--- Diff: iocore/net/BIO_fastopen.cc ---
@@ -27,13 +27,26 @@
 
 #include "BIO_fastopen.h"
 
+#if OPENSSL_VERSION_NUMBER < 0x1010L
+#define BIO_set_data(a, ptr) ((a)->ptr = (ptr))
+#define BIO_get_data(a) ((a)->ptr)
+#define BIO_set_init(a, init) ((a)->init = (init))
+#define BIO_set_shutdown(a, shut) ((a)->shutdown = (shut))
+#define BIO_get_shutdown(a) ((a)->shutdown)
+#endif
+
+struct Data {
--- End diff --

Don't mean to bike shed, but this seems like we could have a better name 
for this.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #1294: TS-5059: Port TCP Fast Open BIO to OpenSSL...

[jablko]

GitHub user jablko opened a pull request:

https://github.com/apache/trafficserver/pull/1294

TS-5059: Port TCP Fast Open BIO to OpenSSL 1.1.0

BIO and BIO_METHOD were made opaque in OpenSSL 1.1.0 [1].

[1] https://www.openssl.org/news/changelog#x4

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/jablko/trafficserver fastopen

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/trafficserver/pull/1294.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #1294


commit f28f2b5164a6c018e8025363c9b8734843c94d42
Author: Jack Bates 
Date:   2017-01-02T22:00:13Z

TS-5059: Port TCP Fast Open BIO to OpenSSL 1.1.0

BIO and BIO_METHOD were made opaque in OpenSSL 1.1.0 [1].

[1] https://www.openssl.org/news/changelog#x4




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---