Re: [Libguestfs] [PATCH nbdkit 3/3] server: Remove explicit connection parameter, use TLS instead.

On 2/11/20 11:15 AM, Richard W.M. Jones wrote: >Since commit 86fdb48c6a5362d66865493d9d2172166f99722e we have stored >the connection object in thread-local storage. > >In this very large, but mostly mechanical change we stop passing the >connection pointer around everywhere, and instead use the value stored >in thread-local storage. > >This assumes a 1-1 mapping between the connection and the current >thread which is true in *most* places. Occasionally we still have the >explicit connection pointer, especially just before we launch a thread >or call threadlocal_set_conn. >--- > server/internal.h | 191 +++++++++---------- > server/backend.c | 160 +++++++++------- > server/connections.c | 68 ++++--- > server/crypto.c | 14 +- > server/filters.c | 270 +++++++++++++-------------- > server/locks.c | 12 +- > server/plugins.c | 64 +++---- > server/protocol-handshake-newstyle.c | 172 +++++++++-------- > server/protocol-handshake-oldstyle.c | 7 +- > server/protocol-handshake.c | 40 ++-- > server/protocol.c | 127 ++++++------- > server/public.c | 2 +- > server/test-public.c | 2 +- > 13 files changed, 574 insertions(+), 555 deletions(-) Makes sense to me (and not too hard to rebase my NBD_INFO_INIT_STATE stuff on top of it). Are we sure that there is not going to be a speed penalty (from frequent access to the thread-local storage, compared to previous access through a parameter stored in a register)?

A few comments: >+++ b/server/filters.c >@@ -49,13 +49,13 @@ struct backend_filter { > struct nbdkit_filter filter; > }; >-/* Literally a backend, a connection pointer, and the filter's handle. >+/* Literally a backend and the filter's handle. >+ * > * This is the implementation of our handle in .open, and serves as > * a stable ‘void *nxdata’ in the filter API. > */ >-struct b_conn { >+struct b_h { > struct backend *b; >- struct connection *conn; > void *handle; > }; >@@ -186,22 +186,22 @@ filter_config_complete (struct backend *b) > static int > next_preconnect (void *nxdata, int readonly) > { >- struct b_conn *b_conn = nxdata; >- return b_conn->b->preconnect (b_conn->b, b_conn->conn, readonly); >+ struct b_h *b_h = nxdata; >+ return b_h->b->preconnect (b_h->b, readonly); > } None of the next_*() wrappers use b_h->handle, they all stick to b_h->b. I don't think that matters too much, other than... >@@ -267,101 +266,101 @@ filter_close (struct backend *b, struct connection *conn, void *handle) > /* The next_functions structure contains pointers to backend > * functions. However because these functions are all expecting a >- * backend and a connection, we cannot call them directly, but must >+ * backend and a handle, we cannot call them directly, but must > * write some next_* functions that unpack the two parameters from a >- * single ‘void *nxdata’ struct pointer (‘b_conn’). >+ * single ‘void *nxdata’ struct pointer (‘b_h’). ...this comment is slightly off. In fact, if ALL we use is b_h->b, we could probably populate next_ops with backend_* functions rather than next_* wrapper functions: