From 1baf693b02b2c8377a7e150cc1d7fc7894336b75 Mon Sep 17 00:00:00 2001 From: Martin Brampton Date: Sun, 23 Aug 2015 16:39:08 +0100 Subject: [PATCH 01/85] First changes for lazy session creation. --- server/core/poll.c | 21 ++++----------------- server/core/service.c | 43 +++++++++++++------------------------------ 2 files changed, 17 insertions(+), 47 deletions(-) diff --git a/server/core/poll.c b/server/core/poll.c index 5e913f5f9..d369ff365 100644 --- a/server/core/poll.c +++ b/server/core/poll.c @@ -299,23 +299,6 @@ poll_add_dcb(DCB *dcb) STRDCBSTATE(dcb->state)))); raise(SIGABRT); } - /* - * This test could be wrong. On the face of it, we don't want to add a - * DCB to the poll list if it is not linked to a session because the code - * that handles events will expect to find a session. Test added by - * Martin as an experiment on 23 August 2015 - */ - if (false && NULL == dcb->session) - { - LOGIF(LE, (skygw_log_write_flush( - LOGFILE_ERROR, - "%lu [%s] Error : Attempt to add dcb %p " - "to poll list but it is not linked to a session, crashing.", - __func__, - pthread_self(), - dcb))); - raise(SIGABRT); - } if (DCB_STATE_POLLING == dcb->state || DCB_STATE_LISTENING == dcb->state) { @@ -925,6 +908,10 @@ unsigned long qtime; { if (dcb->state == DCB_STATE_LISTENING) { + if (NULL == dcb->session) + { + dcb->session = session_alloc(dcb->service, dcb); + } LOGIF(LD, (skygw_log_write( LOGFILE_DEBUG, "%lu [poll_waitevents] " diff --git a/server/core/service.c b/server/core/service.c index 224a602b3..12971f016 100644 --- a/server/core/service.c +++ b/server/core/service.c @@ -363,38 +363,21 @@ GWPROTOCOL *funcs; if (port->listener->func.listen(port->listener, config_bind)) { - port->listener->session = session_alloc(service, port->listener); - - if (port->listener->session != NULL) - { - port->listener->session->state = SESSION_STATE_LISTENER; - listeners += 1; - } - else - { - LOGIF(LE, (skygw_log_write_flush( - LOGFILE_ERROR, - "Error : Failed to create session to service %s.", - service->name))); - - users_free(service->users); - dcb_close(port->listener); - port->listener = NULL; - goto retblock; - } - } - else - { - LOGIF(LE, (skygw_log_write_flush( - LOGFILE_ERROR, + listeners += 1; + /* If lazy session creation fails, how does listeners get decremented? */ + } + else + { + LOGIF(LE, (skygw_log_write_flush( + LOGFILE_ERROR, "Error : Unable to start to listen port %d for %s %s.", port->port, - port->protocol, - service->name))); - users_free(service->users); - dcb_close(port->listener); - port->listener = NULL; - } + port->protocol, + service->name))); + users_free(service->users); + dcb_close(port->listener); + port->listener = NULL; + } retblock: return listeners; From 5e2e2585ad44a3b2c1e14d47217b10ff288f10ea Mon Sep 17 00:00:00 2001 From: Martin Brampton Date: Sun, 23 Aug 2015 16:43:07 +0100 Subject: [PATCH 02/85] Fix mistakes. --- server/core/poll.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/server/core/poll.c b/server/core/poll.c index d369ff365..4e34f3657 100644 --- a/server/core/poll.c +++ b/server/core/poll.c @@ -26,6 +26,7 @@ #include #include #include +#include #include #include #include @@ -910,7 +911,11 @@ unsigned long qtime; { if (NULL == dcb->session) { - dcb->session = session_alloc(dcb->service, dcb); + dcb->session = (SESSION *)session_alloc(dcb->service, dcb); + if (dcb->session) + { + dcb->session->state = SESSION_STATE_LISTENER; + } } LOGIF(LD, (skygw_log_write( LOGFILE_DEBUG, From 37ac15879150631efd09e6264b69320dc048e76c Mon Sep 17 00:00:00 2001 From: counterpoint Date: Mon, 24 Aug 2015 12:12:43 +0100 Subject: [PATCH 03/85] Changes to try to eliminate setting dcb->session to NULL with risk of crashing system. --- server/core/poll.c | 9 - server/core/service.c | 44 ++- server/core/session.c | 308 ++++++++---------- server/modules/protocol/httpd.c | 6 +- server/modules/protocol/maxscaled.c | 2 +- server/modules/protocol/telnetd.c | 5 + .../routing/readwritesplit/readwritesplit.c | 2 +- 7 files changed, 182 insertions(+), 194 deletions(-) diff --git a/server/core/poll.c b/server/core/poll.c index 4e34f3657..98f9e83fb 100644 --- a/server/core/poll.c +++ b/server/core/poll.c @@ -26,7 +26,6 @@ #include #include #include -#include #include #include #include @@ -909,14 +908,6 @@ unsigned long qtime; { if (dcb->state == DCB_STATE_LISTENING) { - if (NULL == dcb->session) - { - dcb->session = (SESSION *)session_alloc(dcb->service, dcb); - if (dcb->session) - { - dcb->session->state = SESSION_STATE_LISTENER; - } - } LOGIF(LD, (skygw_log_write( LOGFILE_DEBUG, "%lu [poll_waitevents] " diff --git a/server/core/service.c b/server/core/service.c index 12971f016..804c821c8 100644 --- a/server/core/service.c +++ b/server/core/service.c @@ -354,7 +354,6 @@ GWPROTOCOL *funcs; goto retblock; } memcpy(&(port->listener->func), funcs, sizeof(GWPROTOCOL)); - port->listener->session = NULL; if (port->address) sprintf(config_bind, "%s:%d", port->address, port->port); @@ -363,21 +362,38 @@ GWPROTOCOL *funcs; if (port->listener->func.listen(port->listener, config_bind)) { - listeners += 1; - /* If lazy session creation fails, how does listeners get decremented? */ - } - else - { - LOGIF(LE, (skygw_log_write_flush( - LOGFILE_ERROR, + port->listener->session = session_alloc(service, port->listener); + + if (port->listener->session != NULL) + { + port->listener->session->state = SESSION_STATE_LISTENER; + listeners += 1; + } + else + { + LOGIF(LE, (skygw_log_write_flush( + LOGFILE_ERROR, + "Error : Failed to create session to service %s.", + service->name))); + + users_free(service->users); + dcb_close(port->listener); + port->listener = NULL; + goto retblock; + } + } + else + { + LOGIF(LE, (skygw_log_write_flush( + LOGFILE_ERROR, "Error : Unable to start to listen port %d for %s %s.", port->port, - port->protocol, - service->name))); - users_free(service->users); - dcb_close(port->listener); - port->listener = NULL; - } + port->protocol, + service->name))); + users_free(service->users); + dcb_close(port->listener); + port->listener = NULL; + } retblock: return listeners; diff --git a/server/core/session.c b/server/core/session.c index 06737b2fb..9c0710d9a 100644 --- a/server/core/session.c +++ b/server/core/session.c @@ -58,6 +58,7 @@ static SESSION *allSessions = NULL; static int session_setup_filters(SESSION *session); +static void session_simple_free(SESSION *session, DCB *dcb); /** * Allocate a new session for a new client of the specified service. @@ -76,8 +77,7 @@ session_alloc(SERVICE *service, DCB *client_dcb) SESSION *session; session = (SESSION *)calloc(1, sizeof(SESSION)); - ss_info_dassert(session != NULL, - "Allocating memory for session failed."); + ss_info_dassert(session != NULL, "Allocating memory for session failed."); if (session == NULL) { @@ -87,43 +87,29 @@ session_alloc(SERVICE *service, DCB *client_dcb) "session object due error %d, %s.", errno, strerror(errno)))); - if (client_dcb->data && !DCB_IS_CLONE(client_dcb)) - { - void *clientdata = client_dcb->data; - client_dcb->data = NULL; - free(clientdata); - } - goto return_session; + session_simple_free(session, client_dcb); + return NULL; } #if defined(SS_DEBUG) session->ses_chk_top = CHK_NUM_SESSION; session->ses_chk_tail = CHK_NUM_SESSION; #endif - if (DCB_IS_CLONE(client_dcb)) - { - session->ses_is_child = true; - } + session->ses_is_child = (bool) DCB_IS_CLONE(client_dcb); spinlock_init(&session->ses_lock); - /*< - * Prevent backend threads from accessing before session is completely - * initialized. - */ - spinlock_acquire(&session->ses_lock); session->service = service; - session->client = client_dcb; - session->n_filters = 0; - memset(&session->stats, 0, sizeof(SESSION_STATS)); - session->stats.connect = time(0); - session->state = SESSION_STATE_ALLOC; + session->client = client_dcb; + session->n_filters = 0; + memset(&session->stats, 0, sizeof(SESSION_STATS)); + session->stats.connect = time(0); + session->state = SESSION_STATE_ALLOC; /*< - * Associate the session to the client DCB and set the reference count on - * the session to indicate that there is a single reference to the + * Associate the session to the client DCB and set the reference count on + * the session to indicate that there is a single reference to the * session. There is no need to protect this or use atomic add as the * session has not been made available to the other threads at this * point. */ session->data = client_dcb->data; - client_dcb->session = session; session->refcount = 1; /*< * This indicates that session is ready to be shared with backend @@ -131,139 +117,99 @@ session_alloc(SERVICE *service, DCB *client_dcb) */ session->state = SESSION_STATE_READY; - /*< Release session lock */ - spinlock_release(&session->ses_lock); - - /* - * Only create a router session if we are not the listening - * DCB or an internal DCB. Creating a router session may create a connection to a - * backend server, depending upon the router module implementation - * and should be avoided for the listener session - * - * Router session creation may create other DCBs that link to the - * session, therefore it is important that the session lock is + /* + * Only create a router session if we are not the listening + * DCB or an internal DCB. Creating a router session may create a connection to a + * backend server, depending upon the router module implementation + * and should be avoided for the listener session + * + * Router session creation may create other DCBs that link to the + * session, therefore it is important that the session lock is * relinquished before the router call. - */ - if (client_dcb->state != DCB_STATE_LISTENING && + */ + if (client_dcb->state != DCB_STATE_LISTENING && client_dcb->dcb_role != DCB_ROLE_INTERNAL) - { - session->router_session = - service->router->newSession(service->router_instance, session); + { + session->router_session = service->router->newSession(service->router_instance, session); if (session->router_session == NULL) - { - /** - * Inform other threads that session is closing. - */ - session->state = SESSION_STATE_STOPPING; - /*< - * Decrease refcount, set dcb's session pointer NULL - * and set session pointer to NULL. - */ - session->client = NULL; - session_free(session); - client_dcb->session = NULL; - session = NULL; + { + session_simple_free(session, client_dcb); + LOGIF(LE, (skygw_log_write_flush( LOGFILE_ERROR, - "Error : Failed to create %s session.", + "%lu [%s] Error : Failed to create %s session because router" + "could not establish a new router session, see earlier error.", + pthread_self(), + __func__, service->name))); - goto return_session; + return NULL; } - /* - * Pending filter chain being setup set the head of the chain to - * be the router. As filters are inserted the current head will - * be pushed to the filter and the head updated. - * - * NB This dictates that filters are created starting at the end - * of the chain nearest the router working back to the client - * protocol end of the chain. - */ - session->head.instance = service->router_instance; - session->head.session = session->router_session; + /* + * Pending filter chain being setup set the head of the chain to + * be the router. As filters are inserted the current head will + * be pushed to the filter and the head updated. + * + * NB This dictates that filters are created starting at the end + * of the chain nearest the router working back to the client + * protocol end of the chain. + */ + session->head.instance = service->router_instance; + session->head.session = session->router_session; - session->head.routeQuery = (void *)(service->router->routeQuery); + session->head.routeQuery = (void *)(service->router->routeQuery); - session->tail.instance = session; - session->tail.session = session; - session->tail.clientReply = session_reply; + session->tail.instance = session; + session->tail.session = session; + session->tail.clientReply = session_reply; - if (service->n_filters > 0) - { - if (!session_setup_filters(session)) - { - /** - * Inform other threads that session is closing. - */ - session->state = SESSION_STATE_STOPPING; - /*< - * Decrease refcount, set dcb's session pointer NULL - * and set session pointer to NULL. - */ - session->client = NULL; - session_free(session); - client_dcb->session = NULL; - session = NULL; - LOGIF(LE, (skygw_log_write( - LOGFILE_ERROR, - "Error : Setting up filters failed. " - "Terminating session %s.", - service->name))); - goto return_session; - } - } - } - - spinlock_acquire(&session->ses_lock); - - if (session->state != SESSION_STATE_READY) + if (service->n_filters > 0) { - spinlock_release(&session->ses_lock); - session->client = NULL; - session_free(session); - client_dcb->session = NULL; - session = NULL; - LOGIF(LE, (skygw_log_write_flush( - LOGFILE_ERROR, - "Error : Failed to create %s session.", - service->name))); - spinlock_release(&session_spin); + if (!session_setup_filters(session)) + { + session_simple_free(session, client_dcb); + LOGIF(LE, (skygw_log_write( + LOGFILE_ERROR, + "Error : Setting up filters failed. " + "Terminating session %s.", + service->name))); + return NULL; + } } - else - { - session->state = SESSION_STATE_ROUTER_READY; - spinlock_release(&session->ses_lock); - spinlock_acquire(&session_spin); - /** Assign a session id and increase */ - session->ses_id = ++session_id; - session->next = allSessions; - allSessions = session; - spinlock_release(&session_spin); + } + + session->state = SESSION_STATE_ROUTER_READY; + spinlock_acquire(&session_spin); + /** Assign a session id and increase, insert session into list */ + session->ses_id = ++session_id; + session->next = allSessions; + allSessions = session; + spinlock_release(&session_spin); - if (session->client->user == NULL) - { - LOGIF(LT, (skygw_log_write( - LOGFILE_TRACE, - "Started session [%lu] for %s service ", - session->ses_id, - service->name))); - } - else - { - LOGIF(LT, (skygw_log_write( - LOGFILE_TRACE, - "Started %s client session [%lu] for '%s' from %s", - service->name, - session->ses_id, - session->client->user, - session->client->remote))); - } - atomic_add(&service->stats.n_sessions, 1); - atomic_add(&service->stats.n_current, 1); - CHK_SESSION(session); - } -return_session: - return session; + if (session->client->user == NULL) + { + LOGIF(LT, (skygw_log_write( + LOGFILE_TRACE, + "Started session [%lu] for %s service ", + session->ses_id, + service->name))); + } + else + { + LOGIF(LT, (skygw_log_write( + LOGFILE_TRACE, + "Started %s client session [%lu] for '%s' from %s", + service->name, + session->ses_id, + session->client->user, + session->client->remote))); + } + atomic_add(&service->stats.n_sessions, 1); + atomic_add(&service->stats.n_current, 1); + CHK_SESSION(session); + + client_dcb->session = session; + return session; } /** @@ -360,30 +306,57 @@ int session_unlink_dcb( return nlink; } +/** + * Deallocate the specified session, minimal actions during session_alloc + * + * @param session The session to deallocate + */ +static void +session_simple_free(SESSION *session, DCB *dcb) +{ + /* Does this possibly need a lock? */ + if (dcb->data && !DCB_IS_CLONE(dcb)) + { + void * clientdata = dcb->data; + dcb->data = NULL; + free(clientdata); + } + if (session) + { + if (session && session->router_session) + { + session->service->router->freeSession( + session->service->router_instance, + session->router_session); + } + session->state = SESSION_STATE_STOPPING; + } + + free(session); +} + + /** * Deallocate the specified session * * @param session The session to deallocate */ -bool session_free( - SESSION *session) +bool +session_free(SESSION *session) { - bool succp = false; - SESSION *ptr; - int nlink; - int i; - CHK_SESSION(session); - /*< + + /* * Remove one reference. If there are no references left, * free session. */ - nlink = session_unlink_dcb(session, NULL); - - if (nlink != 0) { - ss_dassert(nlink > 0); - goto return_succp; + if (atomic_add(&session->refcount, -1) - 1) + { + /* Must be one or more references left */ + ss_dassert(nlink > 0); + return false; } + session->state = SESSION_STATE_TO_BE_FREED; /* First of all remove from the linked list */ spinlock_acquire(&session_spin); @@ -393,13 +366,16 @@ bool session_free( } else { - ptr = allSessions; - while (ptr && ptr->next != session) + SESSION *chksession; + chksession = allSessions; + while (chksession && chksession->next != session) { - ptr = ptr->next; + chksession = chksession->next; } - if (ptr) - ptr->next = session->next; + if (chksession) + { + chksession->next = session->next; + } } spinlock_release(&session_spin); atomic_add(&session->service->stats.n_current, -1); @@ -416,6 +392,7 @@ bool session_free( } if (session->n_filters) { + int i; for (i = 0; i < session->n_filters; i++) { if (session->filters[i].filter) @@ -453,10 +430,7 @@ bool session_free( } free(session); } - succp = true; - -return_succp : - return succp; + return true; } /** diff --git a/server/modules/protocol/httpd.c b/server/modules/protocol/httpd.c index fa7418157..e3c955bdc 100644 --- a/server/modules/protocol/httpd.c +++ b/server/modules/protocol/httpd.c @@ -351,7 +351,8 @@ int n_connect = 0; memcpy(&client->func, &MyObject, sizeof(GWPROTOCOL)); /* we don't need the session */ - client->session = NULL; + /* But not clear that we have one! */ + /* client->session = NULL; */ /* create the session data for HTTPD */ client_data = (HTTPD_session *)calloc(1, sizeof(HTTPD_session)); @@ -360,9 +361,10 @@ int n_connect = 0; client->session = session_alloc(dcb->session->service, client); - if (poll_add_dcb(client) == -1) + if (NULL == client->session || poll_add_dcb(client) == -1) { close(so); + dcb_close(client); return n_connect; } n_connect++; diff --git a/server/modules/protocol/maxscaled.c b/server/modules/protocol/maxscaled.c index 6b1b5e570..d57304390 100644 --- a/server/modules/protocol/maxscaled.c +++ b/server/modules/protocol/maxscaled.c @@ -291,7 +291,7 @@ int n_connect = 0; client_dcb->session = session_alloc(dcb->session->service, client_dcb); - if (poll_add_dcb(client_dcb)) + if (NULL == client_dcb->session || poll_add_dcb(client_dcb)) { dcb_close(dcb); return n_connect; diff --git a/server/modules/protocol/telnetd.c b/server/modules/protocol/telnetd.c index 8274c91cf..d184d3093 100644 --- a/server/modules/protocol/telnetd.c +++ b/server/modules/protocol/telnetd.c @@ -311,6 +311,11 @@ int n_connect = 0; memcpy(&client_dcb->func, &MyObject, sizeof(GWPROTOCOL)); client_dcb->session = session_alloc(dcb->session->service, client_dcb); + if (NULL == client_dcb->session) + { + dcb_close(client_dcb); + return n_connect; + } telnetd_pr = (TELNETD *)malloc(sizeof(TELNETD)); client_dcb->protocol = (void *)telnetd_pr; diff --git a/server/modules/routing/readwritesplit/readwritesplit.c b/server/modules/routing/readwritesplit/readwritesplit.c index 06932e206..417abdfe7 100644 --- a/server/modules/routing/readwritesplit/readwritesplit.c +++ b/server/modules/routing/readwritesplit/readwritesplit.c @@ -1684,7 +1684,7 @@ static skygw_query_type_t is_read_tmp_table( { skygw_log_write(LE,"[%s] Error: Master server reference is NULL.", __FUNCTION__); - return; + return type; } rses_prop_tmp = router_cli_ses->rses_properties[RSES_PROP_TYPE_TMPTABLES]; From 65c42e2d80f7c228e1a5c556ac500054e8832341 Mon Sep 17 00:00:00 2001 From: counterpoint Date: Mon, 24 Aug 2015 16:19:25 +0100 Subject: [PATCH 04/85] Move removal of closing DCB from poll list to the kill zombies processing, rather than immediately on close; modify persistent connections to obtain candidates for the pool from the kill zombies processing to be sure that they really are finished all previous processing. --- server/core/dcb.c | 79 ++++++++++++++++++++++--------------------- server/core/session.c | 1 - server/include/dcb.h | 1 + 3 files changed, 41 insertions(+), 40 deletions(-) diff --git a/server/core/dcb.c b/server/core/dcb.c index 14fbdd3ff..37879ed4c 100644 --- a/server/core/dcb.c +++ b/server/core/dcb.c @@ -536,6 +536,33 @@ dcb_process_victim_queue(DCB *listofdcb) while (dcb != NULL) { DCB *nextdcb = NULL; + /*< + * Stop dcb's listening and modify state accordingly. + */ + if (dcb->state == DCB_STATE_POLLING || dcb->state == DCB_STATE_LISTENING) + { + if (dcb->state == DCB_STATE_LISTENING) + { + LOGIF(LE, (skygw_log_write( + LOGFILE_ERROR, + "%lu [%s] Error : Removing DCB %p but was in state %s " + "which is not expected for a call to dcb_close, although it" + "should be processed correctly. ", + pthread_self(), + __func__, + dcb, + STRDCBSTATE(dcb->state)))); + } + if ((dcb->state == DCB_STATE_POLLING && !dcb_maybe_add_persistent(dcb)) + || (dcb->state == DCB_STATE_LISTENING)) + { + dcb_close_finish(dcb); + } + } + + /* If DCB was put into persistent queue, will no longer be flagged zombie */ + if (!dcb->dcb_is_zombie) continue; + if (dcb->fd > 0) { /*< @@ -1794,11 +1821,6 @@ dcb_close(DCB *dcb) dcb, dcb ? STRDCBSTATE(dcb->state) : "Invalid DCB"))); - if (DCB_STATE_ZOMBIE == dcb->state) - { - return; - } - if (DCB_STATE_UNDEFINED == dcb->state || DCB_STATE_DISCONNECTED == dcb->state) { @@ -1822,43 +1844,21 @@ dcb_close(DCB *dcb) return; } - /*< - * Stop dcb's listening and modify state accordingly. - */ - if (dcb->state == DCB_STATE_POLLING || dcb->state == DCB_STATE_LISTENING) - { - if (dcb->state == DCB_STATE_LISTENING) - { - LOGIF(LE, (skygw_log_write( - LOGFILE_ERROR, - "%lu [dcb_close] Error : Removing DCB %p but was in state %s " - "which is not expected for a call to dcb_close, although it" - "should be processed correctly. ", - pthread_self(), - dcb, - STRDCBSTATE(dcb->state)))); - } - if ((dcb->state == DCB_STATE_POLLING && !dcb_maybe_add_persistent(dcb)) - || (dcb->state == DCB_STATE_LISTENING)) - { - dcb_close_finish(dcb); - } - } - spinlock_acquire(&zombiespin); - if (dcb->state == DCB_STATE_NOPOLLING || dcb->state == DCB_STATE_ALLOC) + if (dcb->dcb_is_zombie) { - /*< - * Add closing dcb to the top of the list. - */ - dcb->memdata.next = zombies; - zombies = dcb; - /*< - * Set state which indicates that it has been added to zombies - * list. - */ - dcb->state = DCB_STATE_ZOMBIE; + return; } + /*< + * Add closing dcb to the top of the list. + */ + dcb->dcb_is_zombie = true; + dcb->memdata.next = zombies; + zombies = dcb; + /*< + * Set state which indicates that it has been added to zombies + * list. + */ spinlock_release(&zombiespin); } @@ -1889,6 +1889,7 @@ dcb_maybe_add_persistent(DCB *dcb) pthread_self(), user))); dcb->user = strdup(user); + dcb->dcb_is_zombie = false; dcb->persistentstart = time(NULL); session_unlink_dcb(dcb->session, dcb); spinlock_acquire(&dcb->server->persistlock); diff --git a/server/core/session.c b/server/core/session.c index 9c0710d9a..31bbbd637 100644 --- a/server/core/session.c +++ b/server/core/session.c @@ -353,7 +353,6 @@ session_free(SESSION *session) if (atomic_add(&session->refcount, -1) - 1) { /* Must be one or more references left */ - ss_dassert(nlink > 0); return false; } session->state = SESSION_STATE_TO_BE_FREED; diff --git a/server/include/dcb.h b/server/include/dcb.h index 7339286a1..feb0a3387 100644 --- a/server/include/dcb.h +++ b/server/include/dcb.h @@ -227,6 +227,7 @@ typedef struct dcb_callback { typedef struct dcb { skygw_chk_t dcb_chk_top; bool dcb_errhandle_called; /*< this can be called only once */ + bool dcb_is_zombie; /**< Whether the DCB is in the zombie list */ dcb_role_t dcb_role; SPINLOCK dcb_initlock; DCBEVENTQ evq; /**< The event queue for this DCB */ From ae669c6f888ba0aec07e4cb12a260bbf72c14020 Mon Sep 17 00:00:00 2001 From: counterpoint Date: Mon, 24 Aug 2015 16:29:41 +0100 Subject: [PATCH 05/85] Fix mistake --- server/core/dcb.c | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/server/core/dcb.c b/server/core/dcb.c index 37879ed4c..5ded385c9 100644 --- a/server/core/dcb.c +++ b/server/core/dcb.c @@ -1845,20 +1845,15 @@ dcb_close(DCB *dcb) } spinlock_acquire(&zombiespin); - if (dcb->dcb_is_zombie) + if (!dcb->dcb_is_zombie) { - return; + /*< + * Add closing dcb to the top of the list, setting zombie marker + */ + dcb->dcb_is_zombie = true; + dcb->memdata.next = zombies; + zombies = dcb; } - /*< - * Add closing dcb to the top of the list. - */ - dcb->dcb_is_zombie = true; - dcb->memdata.next = zombies; - zombies = dcb; - /*< - * Set state which indicates that it has been added to zombies - * list. - */ spinlock_release(&zombiespin); } From 12922225b85baca8b0a1f744894bbb1e56e674d3 Mon Sep 17 00:00:00 2001 From: counterpoint Date: Tue, 25 Aug 2015 09:11:44 +0100 Subject: [PATCH 06/85] Remove redundant DCB state DCB_STATE_FREED, remove obsolete assertion from poll.c, tidy up. --- server/core/dcb.c | 32 ++++++++++++++------------------ server/core/poll.c | 1 - server/include/dcb.h | 1 - utils/skygw_debug.h | 5 ++--- 4 files changed, 16 insertions(+), 23 deletions(-) diff --git a/server/core/dcb.c b/server/core/dcb.c index 5ded385c9..1f6eea6a8 100644 --- a/server/core/dcb.c +++ b/server/core/dcb.c @@ -409,7 +409,6 @@ dcb_process_zombies(int threadid) DCB *zombiedcb, *previousdcb; DCB *listofdcb = NULL; DCB *dcb = NULL; -bool succp = false; /** * Perform a dirty read to see if there is anything in the queue. @@ -470,15 +469,14 @@ bool succp = false; LOGIF(LD, (skygw_log_write_flush( LOGFILE_DEBUG, - "%lu [dcb_process_zombies] Remove dcb " + "%lu [%s] Remove dcb " "%p fd %d in state %s from the " "list of zombies.", pthread_self(), + __func__, zombiedcb, zombiedcb->fd, STRDCBSTATE(zombiedcb->state)))); - ss_info_dassert(zombiedcb->state == DCB_STATE_ZOMBIE, - "dcb not in DCB_STATE_ZOMBIE state."); /*< * Move zombie dcb to linked list of victim dcbs. * The variable dcb is used to hold the last DCB @@ -553,15 +551,16 @@ dcb_process_victim_queue(DCB *listofdcb) dcb, STRDCBSTATE(dcb->state)))); } - if ((dcb->state == DCB_STATE_POLLING && !dcb_maybe_add_persistent(dcb)) - || (dcb->state == DCB_STATE_LISTENING)) - { - dcb_close_finish(dcb); + else { + /* Must be DCB_STATE_POLLING */ + if (dcb_maybe_add_persistent(dcb)) + { + /* Have taken DCB into persistent pool, no further killing */ + continue; + } } + dcb_close_finish(dcb); } - - /* If DCB was put into persistent queue, will no longer be flagged zombie */ - if (!dcb->dcb_is_zombie) continue; if (dcb->fd > 0) { @@ -603,10 +602,10 @@ dcb_process_victim_queue(DCB *listofdcb) &tls_log_info.li_sesid, &tls_log_info.li_enabled_logs))); - dcb->state = DCB_STATE_DISCONNECTED; - nextdcb = dcb->memdata.next; - dcb_final_free(dcb); - dcb = nextdcb; + dcb->state = DCB_STATE_DISCONNECTED; + nextdcb = dcb->memdata.next; + dcb_final_free(dcb); + dcb = nextdcb; } /** Reset threads session data */ LOGIF(LT, tls_log_info.li_sesid = 0); @@ -732,7 +731,6 @@ dcb_connect(SERVER *server, SESSION *session, const char *protocol) session->client, session->client->fd))); } - ss_dassert(dcb->fd == DCBFD_CLOSED); /*< must be uninitialized at this point */ /** * Successfully connected to backend. Assign file descriptor to dcb */ @@ -2281,8 +2279,6 @@ gw_dcb_state2string (int state) return "DCB for listening socket"; case DCB_STATE_DISCONNECTED: return "DCB socket closed"; - case DCB_STATE_FREED: - return "DCB memory could be freed"; case DCB_STATE_ZOMBIE: return "DCB Zombie"; case DCB_STATE_UNDEFINED: diff --git a/server/core/poll.c b/server/core/poll.c index 98f9e83fb..da996f3b6 100644 --- a/server/core/poll.c +++ b/server/core/poll.c @@ -866,7 +866,6 @@ unsigned long qtime; ss_debug(spinlock_acquire(&dcb->dcb_initlock);) ss_dassert(dcb->state != DCB_STATE_ALLOC); ss_dassert(dcb->state != DCB_STATE_DISCONNECTED); - ss_dassert(dcb->state != DCB_STATE_FREED); ss_debug(spinlock_release(&dcb->dcb_initlock);) LOGIF(LD, (skygw_log_write( diff --git a/server/include/dcb.h b/server/include/dcb.h index feb0a3387..c9312a128 100644 --- a/server/include/dcb.h +++ b/server/include/dcb.h @@ -179,7 +179,6 @@ typedef enum { DCB_STATE_DISCONNECTED, /*< The socket is now closed */ DCB_STATE_NOPOLLING, /*< Removed from poll mask */ DCB_STATE_ZOMBIE, /*< DCB is no longer active, waiting to free it */ - DCB_STATE_FREED /*< Memory freed */ } dcb_state_t; typedef enum { diff --git a/utils/skygw_debug.h b/utils/skygw_debug.h index f9b586a0c..1aaa3f2ae 100644 --- a/utils/skygw_debug.h +++ b/utils/skygw_debug.h @@ -206,9 +206,8 @@ typedef enum skygw_chk_t { ((s) == DCB_STATE_LISTENING ? "DCB_STATE_LISTENING" : \ ((s) == DCB_STATE_DISCONNECTED ? "DCB_STATE_DISCONNECTED" : \ ((s) == DCB_STATE_NOPOLLING ? "DCB_STATE_NOPOLLING" : \ - ((s) == DCB_STATE_FREED ? "DCB_STATE_FREED" : \ - ((s) == DCB_STATE_ZOMBIE ? "DCB_STATE_ZOMBIE" : \ - ((s) == DCB_STATE_UNDEFINED ? "DCB_STATE_UNDEFINED" : "DCB_STATE_UNKNOWN")))))))) + ((s) == DCB_STATE_ZOMBIE ? "DCB_STATE_ZOMBIE" : \ + ((s) == DCB_STATE_UNDEFINED ? "DCB_STATE_UNDEFINED" : "DCB_STATE_UNKNOWN")))))))) #define STRSESSIONSTATE(s) ((s) == SESSION_STATE_ALLOC ? "SESSION_STATE_ALLOC" : \ ((s) == SESSION_STATE_READY ? "SESSION_STATE_READY" : \ From f18f233de25f6af2453d381899a092694c712e8d Mon Sep 17 00:00:00 2001 From: counterpoint Date: Tue, 25 Aug 2015 09:23:24 +0100 Subject: [PATCH 07/85] Try to resolve unexpected compiler errors --- server/core/dcb.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/server/core/dcb.c b/server/core/dcb.c index 1f6eea6a8..e38f2e93b 100644 --- a/server/core/dcb.c +++ b/server/core/dcb.c @@ -462,12 +462,18 @@ DCB *dcb = NULL; * queue or NULL if the DCB is at the head of the * queue. Remove zombiedcb from the zombies list. */ - if (previousdcb == NULL) + if (NULL == previousdcb) + { zombies = zombiedcb->memdata.next; - else + } + else + { previousdcb->memdata.next = zombiedcb->memdata.next; + } - LOGIF(LD, (skygw_log_write_flush( + if (LOG_IS_ENABLED(LD)) + { + (skygw_log_write_flush( LOGFILE_DEBUG, "%lu [%s] Remove dcb " "%p fd %d in state %s from the " @@ -476,7 +482,8 @@ DCB *dcb = NULL; __func__, zombiedcb, zombiedcb->fd, - STRDCBSTATE(zombiedcb->state)))); + STRDCBSTATE(zombiedcb->state))); + } /*< * Move zombie dcb to linked list of victim dcbs. * The variable dcb is used to hold the last DCB From d27ffcf06a32b6f172290dfb6ddcbd754dffe176 Mon Sep 17 00:00:00 2001 From: counterpoint Date: Tue, 25 Aug 2015 09:31:54 +0100 Subject: [PATCH 08/85] Fix mistake in debug STRDCBSTATE() --- server/core/dcb.c | 6 ++---- utils/skygw_debug.h | 2 +- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/server/core/dcb.c b/server/core/dcb.c index e38f2e93b..0051de676 100644 --- a/server/core/dcb.c +++ b/server/core/dcb.c @@ -471,9 +471,7 @@ DCB *dcb = NULL; previousdcb->memdata.next = zombiedcb->memdata.next; } - if (LOG_IS_ENABLED(LD)) - { - (skygw_log_write_flush( + LOGIF(LD, (skygw_log_write_flush( LOGFILE_DEBUG, "%lu [%s] Remove dcb " "%p fd %d in state %s from the " @@ -482,7 +480,7 @@ DCB *dcb = NULL; __func__, zombiedcb, zombiedcb->fd, - STRDCBSTATE(zombiedcb->state))); + STRDCBSTATE(zombiedcb->state)))); } /*< * Move zombie dcb to linked list of victim dcbs. diff --git a/utils/skygw_debug.h b/utils/skygw_debug.h index 1aaa3f2ae..ff5d5c9c0 100644 --- a/utils/skygw_debug.h +++ b/utils/skygw_debug.h @@ -207,7 +207,7 @@ typedef enum skygw_chk_t { ((s) == DCB_STATE_DISCONNECTED ? "DCB_STATE_DISCONNECTED" : \ ((s) == DCB_STATE_NOPOLLING ? "DCB_STATE_NOPOLLING" : \ ((s) == DCB_STATE_ZOMBIE ? "DCB_STATE_ZOMBIE" : \ - ((s) == DCB_STATE_UNDEFINED ? "DCB_STATE_UNDEFINED" : "DCB_STATE_UNKNOWN")))))))) + ((s) == DCB_STATE_UNDEFINED ? "DCB_STATE_UNDEFINED" : "DCB_STATE_UNKNOWN"))))))) #define STRSESSIONSTATE(s) ((s) == SESSION_STATE_ALLOC ? "SESSION_STATE_ALLOC" : \ ((s) == SESSION_STATE_READY ? "SESSION_STATE_READY" : \ From 980b56e7fa967c20c55826fe7ad16f06a362b6b2 Mon Sep 17 00:00:00 2001 From: counterpoint Date: Tue, 25 Aug 2015 09:33:40 +0100 Subject: [PATCH 09/85] Fix stupid extra } --- server/core/dcb.c | 1 - 1 file changed, 1 deletion(-) diff --git a/server/core/dcb.c b/server/core/dcb.c index 0051de676..8ca414ccc 100644 --- a/server/core/dcb.c +++ b/server/core/dcb.c @@ -481,7 +481,6 @@ DCB *dcb = NULL; zombiedcb, zombiedcb->fd, STRDCBSTATE(zombiedcb->state)))); - } /*< * Move zombie dcb to linked list of victim dcbs. * The variable dcb is used to hold the last DCB From 3dd20cb9ecf7747fc39b12d790b148ac6471f6a2 Mon Sep 17 00:00:00 2001 From: counterpoint Date: Tue, 25 Aug 2015 09:53:01 +0100 Subject: [PATCH 10/85] Acquire user for DCB from DCB session sooner, needed for persistent connection handling. --- server/core/dcb.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/server/core/dcb.c b/server/core/dcb.c index 8ca414ccc..89d3e524b 100644 --- a/server/core/dcb.c +++ b/server/core/dcb.c @@ -1849,6 +1849,9 @@ dcb_close(DCB *dcb) spinlock_acquire(&zombiespin); if (!dcb->dcb_is_zombie) { + char *user; + user = session_getUser(dcb->session); + dcb->user = strdup(user); /*< * Add closing dcb to the top of the list, setting zombie marker */ @@ -1869,11 +1872,9 @@ dcb_close(DCB *dcb) static bool dcb_maybe_add_persistent(DCB *dcb) { - char *user; int poolcount = -1; - user = session_getUser(dcb->session); - if (user - && strlen(user) + if (dcb->user + && strlen(dcb->user) && dcb->server && dcb->server->persistpoolmax && !dcb->dcb_errhandle_called From ea09918312d5c4431dee3c0dc1f701f4ae99ee72 Mon Sep 17 00:00:00 2001 From: counterpoint Date: Tue, 25 Aug 2015 09:54:56 +0100 Subject: [PATCH 11/85] Fix mistakes. --- server/core/dcb.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/server/core/dcb.c b/server/core/dcb.c index 89d3e524b..76fffc5db 100644 --- a/server/core/dcb.c +++ b/server/core/dcb.c @@ -1885,8 +1885,7 @@ dcb_maybe_add_persistent(DCB *dcb) LOGFILE_DEBUG, "%lu [dcb_maybe_add_persistent] Adding DCB to persistent pool, user %s.\n", pthread_self(), - user))); - dcb->user = strdup(user); + dcb->user))); dcb->dcb_is_zombie = false; dcb->persistentstart = time(NULL); session_unlink_dcb(dcb->session, dcb); @@ -1906,7 +1905,7 @@ dcb_maybe_add_persistent(DCB *dcb) "max for pool %d, error handle called %s, hung flag %s, pool count %d.\n", pthread_self(), dcb, - user ? user : "", + dcb->user ? dcb->user : "", (dcb->server && dcb->server->persistpoolmax) ? dcb->server->persistpoolmax : 0, dcb->dcb_errhandle_called ? "true" : "false", (dcb->flags & DCBF_HUNG) ? "true" : "false", From 8425deab18cc64950b4b17dab89e5cacbc1984a0 Mon Sep 17 00:00:00 2001 From: counterpoint Date: Tue, 25 Aug 2015 11:46:25 +0100 Subject: [PATCH 12/85] Fixed bugs by moving setting of thread bit mask from polling to DCB closing, fixed other mistakes. --- server/core/dcb.c | 8 ++++++-- server/core/poll.c | 4 ---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/server/core/dcb.c b/server/core/dcb.c index 76fffc5db..61b945114 100644 --- a/server/core/dcb.c +++ b/server/core/dcb.c @@ -560,6 +560,7 @@ dcb_process_victim_queue(DCB *listofdcb) if (dcb_maybe_add_persistent(dcb)) { /* Have taken DCB into persistent pool, no further killing */ + dcb = dcb->memdata.next; continue; } } @@ -1858,6 +1859,10 @@ dcb_close(DCB *dcb) dcb->dcb_is_zombie = true; dcb->memdata.next = zombies; zombies = dcb; + /*< Set bit for each maxscale thread. This should be done before + * the state is changed, so as to protect the DCB from premature + * destruction. */ + bitmask_copy(&dcb->memdata.bitmask, poll_bitmask()); } spinlock_release(&zombiespin); } @@ -1873,7 +1878,7 @@ static bool dcb_maybe_add_persistent(DCB *dcb) { int poolcount = -1; - if (dcb->user + if (dcb->user != NULL && strlen(dcb->user) && dcb->server && dcb->server->persistpoolmax @@ -1888,7 +1893,6 @@ dcb_maybe_add_persistent(DCB *dcb) dcb->user))); dcb->dcb_is_zombie = false; dcb->persistentstart = time(NULL); - session_unlink_dcb(dcb->session, dcb); spinlock_acquire(&dcb->server->persistlock); dcb->nextpersistent = dcb->server->persistent; dcb->server->persistent = dcb; diff --git a/server/core/poll.c b/server/core/poll.c index da996f3b6..eecd457f4 100644 --- a/server/core/poll.c +++ b/server/core/poll.c @@ -368,10 +368,6 @@ poll_remove_dcb(DCB *dcb) dcb, STRDCBSTATE(dcb->state)))); } - /*< Set bit for each maxscale thread. This should be done before - * the state is changed, so as to protect the DCB from premature - * destruction. */ - bitmask_copy(&dcb->memdata.bitmask, poll_bitmask()); /*< * Set state to NOPOLLING and remove dcb from poll set. */ From e7c74c39cfaa16e96eca2cabc8092d4c592e0cdf Mon Sep 17 00:00:00 2001 From: counterpoint Date: Tue, 25 Aug 2015 12:19:02 +0100 Subject: [PATCH 13/85] Fix bug in persistent connections; add code to check for DCB session pointer in poll loop before invoking processing. --- server/core/dcb.c | 2 +- server/core/poll.c | 55 ++++++++++++++++++++++++++++++++++++++++------ 2 files changed, 49 insertions(+), 8 deletions(-) diff --git a/server/core/dcb.c b/server/core/dcb.c index 61b945114..7ae8622fe 100644 --- a/server/core/dcb.c +++ b/server/core/dcb.c @@ -1852,7 +1852,7 @@ dcb_close(DCB *dcb) { char *user; user = session_getUser(dcb->session); - dcb->user = strdup(user); + if (NULL != user) dcb->user = strdup(user); /*< * Add closing dcb to the top of the list, setting zombie marker */ diff --git a/server/core/poll.c b/server/core/poll.c index eecd457f4..182a1385f 100644 --- a/server/core/poll.c +++ b/server/core/poll.c @@ -96,7 +96,7 @@ static simple_mutex_t epoll_wait_mutex; /*< serializes calls to epoll_wait */ static int n_waiting = 0; /*< No. of threads in epoll_wait */ static int process_pollq(int thread_id); static void poll_add_event_to_dcb(DCB* dcb, GWBUF* buf, __uint32_t ev); - +static bool poll_dcb_session_check(DCB *dcb); DCB *eventq = NULL; SPINLOCK pollqlock = SPINLOCK_INIT; @@ -885,7 +885,10 @@ unsigned long qtime; dcb, &tls_log_info.li_sesid, &tls_log_info.li_enabled_logs))); - dcb->func.write_ready(dcb); + if (poll_dcb_session_check(dcb)) + { + dcb->func.write_ready(dcb); + } } else { LOGIF(LD, (skygw_log_write( LOGFILE_DEBUG, @@ -915,7 +918,10 @@ unsigned long qtime; dcb, &tls_log_info.li_sesid, &tls_log_info.li_enabled_logs))); - dcb->func.accept(dcb); + if (poll_dcb_session_check(dcb)) + { + dcb->func.accept(dcb); + } } else { @@ -932,7 +938,10 @@ unsigned long qtime; dcb, &tls_log_info.li_sesid, &tls_log_info.li_enabled_logs))); - dcb->func.read(dcb); + if (poll_dcb_session_check(dcb)) + { + dcb->func.read(dcb); + } } } if (ev & EPOLLERR) @@ -967,7 +976,10 @@ unsigned long qtime; dcb, &tls_log_info.li_sesid, &tls_log_info.li_enabled_logs))); - dcb->func.error(dcb); + if (poll_dcb_session_check(dcb)) + { + dcb->func.error(dcb); + } } if (ev & EPOLLHUP) @@ -996,7 +1008,10 @@ unsigned long qtime; dcb, &tls_log_info.li_sesid, &tls_log_info.li_enabled_logs))); - dcb->func.hangup(dcb); + if (poll_dcb_session_check(dcb)) + { + dcb->func.hangup(dcb); + } } else spinlock_release(&dcb->dcb_initlock); @@ -1029,7 +1044,10 @@ unsigned long qtime; dcb, &tls_log_info.li_sesid, &tls_log_info.li_enabled_logs))); - dcb->func.hangup(dcb); + if (poll_dcb_session_check(dcb)) + { + dcb->func.hangup(dcb); + } } else spinlock_release(&dcb->dcb_initlock); @@ -1098,6 +1116,29 @@ unsigned long qtime; } /** + * + * Check that the DCB has a session link before processing. + * If not, log an error. Processing will be bypassed + * + * @param dcb The DCB to check + * @return bool Does the DCB have a non-null session link + */ +static bool +poll_dcb_session_check(DCB *dcb) +{ + if (dcb->session) + { + return true; + } + else + { + LOGIF; + return false; + } +} + +/** + * * Shutdown the polling loop */ void From 72b301785bcb223faa72e43b949a27547ef5e8cc Mon Sep 17 00:00:00 2001 From: counterpoint Date: Tue, 25 Aug 2015 12:25:36 +0100 Subject: [PATCH 14/85] Complete implementation of error logging when no session pointer in DCB. --- server/core/poll.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/server/core/poll.c b/server/core/poll.c index 182a1385f..5a9c72829 100644 --- a/server/core/poll.c +++ b/server/core/poll.c @@ -1132,7 +1132,13 @@ poll_dcb_session_check(DCB *dcb) } else { - LOGIF; + LOGIF(LE, (skygw_log_write_flush( + LOGFILE_ERROR, + "%lu [%s] The dcb %p that was about to be processed does not " + "have a non-null session pointer " + pthread_self(), + __func__, + dcb))); return false; } } From 1f6b544f339469006a72f9ff457b8c2a57ccf394 Mon Sep 17 00:00:00 2001 From: counterpoint Date: Wed, 26 Aug 2015 15:43:21 +0100 Subject: [PATCH 15/85] Tidy dcb_free (prefer use of dcb_close) and remove from test code; add good random number generator. --- server/core/dcb.c | 14 +++----------- server/core/secrets.c | 6 ++---- server/core/test/testdcb.c | 6 +++--- server/core/test/testpoll.c | 2 +- server/core/test/testsession.c | 2 +- server/core/utils.c | 4 ++-- utils/skygw_utils.cc | 3 ++- 7 files changed, 14 insertions(+), 23 deletions(-) diff --git a/server/core/dcb.c b/server/core/dcb.c index 7ae8622fe..448c9a091 100644 --- a/server/core/dcb.c +++ b/server/core/dcb.c @@ -230,23 +230,15 @@ DCB *newdcb; /** - * Free a DCB that has not been associated with a descriptor. + * Provided only for consistency, simply calls dcb_close to guarantee + * safe disposal of a DCB * * @param dcb The DCB to free */ void dcb_free(DCB *dcb) { - if (dcb->fd != DCBFD_CLOSED) - { - LOGIF(LE, (skygw_log_write_flush( - LOGFILE_ERROR, - "Error : Attempt to free a DCB via dcb_free " - "that has been associated with a descriptor."))); - } - raise(SIGABRT); - /* Another statement to avoid a compiler warning */ - dcb_final_free(dcb); + dcb_close(dcb); } /* diff --git a/server/core/secrets.c b/server/core/secrets.c index d26833e5c..b614a5555 100644 --- a/server/core/secrets.c +++ b/server/core/secrets.c @@ -23,6 +23,7 @@ #include #include #include +#include /** Defined in log_manager.cc */ extern int lm_enabled_logfiles_bitmask; @@ -36,15 +37,13 @@ extern __thread log_info_t tls_log_info; static unsigned char secrets_randomchar() { - return (char)((rand() % ('~' - ' ')) + ' '); + return (char)((random_jkiss() % ('~' - ' ')) + ' '); } static int secrets_random_str(unsigned char *output, int len) { int i; - srand((unsigned long )time(0L) ^ (unsigned long )output); - for ( i = 0; i < len; ++i ) { output[i] = secrets_randomchar(); @@ -273,7 +272,6 @@ if(strlen(path) > PATH_MAX) } close(randfd); - srand(randval); secrets_random_str(key.enckey, MAXSCALE_KEYLEN); secrets_random_str(key.initvector, MAXSCALE_IV_LEN); diff --git a/server/core/test/testdcb.c b/server/core/test/testdcb.c index 13462fc87..1eb8a1a88 100644 --- a/server/core/test/testdcb.c +++ b/server/core/test/testdcb.c @@ -59,9 +59,9 @@ int buflen; printAllDCBs(); ss_info_dassert(true, "Something is true"); ss_dfprintf(stderr, "\t..done\n"); - dcb_free(dcb); - ss_dfprintf(stderr, "Freed original dcb"); - ss_info_dassert(!dcb_isvalid(dcb), "Freed DCB must not be valid"); + dcb_close(dcb); + ss_dfprintf(stderr, "Closed original dcb"); + ss_info_dassert(!dcb_isvalid(dcb), "Closed DCB must not be valid"); ss_dfprintf(stderr, "\t..done\nMake clone DCB a zombie"); clone->state = DCB_STATE_NOPOLLING; dcb_close(clone); diff --git a/server/core/test/testpoll.c b/server/core/test/testpoll.c index 7b9175b2c..a4e6e71db 100644 --- a/server/core/test/testpoll.c +++ b/server/core/test/testpoll.c @@ -85,7 +85,7 @@ int result; sleep(10); poll_shutdown(); ss_dfprintf(stderr, "\t..done\nTidy up."); - dcb_free(dcb); + dcb_close(dcb); ss_dfprintf(stderr, "\t..done\n"); return 0; diff --git a/server/core/test/testsession.c b/server/core/test/testsession.c index 4d8d4cc04..d471fdad4 100644 --- a/server/core/test/testsession.c +++ b/server/core/test/testsession.c @@ -59,7 +59,7 @@ int result; sleep(10); poll_shutdown(); ss_dfprintf(stderr, "\t..done\nTidy up."); - dcb_free(dcb); + dcb_close(dcb); ss_dfprintf(stderr, "\t..done\n"); return 0; diff --git a/server/core/utils.c b/server/core/utils.c index a26c2b4e5..45b47e632 100644 --- a/server/core/utils.c +++ b/server/core/utils.c @@ -43,6 +43,7 @@ #include #include #include +#include /** Defined in log_manager.cc */ extern int lm_enabled_logfiles_bitmask; @@ -100,7 +101,7 @@ char *gw_strend(register const char *s) { * generate a random char *****************************************/ static char gw_randomchar() { - return (char)((rand() % 78) + 30); + return (char)((random_jkiss() % 78) + 30); } /***************************************** @@ -110,7 +111,6 @@ static char gw_randomchar() { int gw_generate_random_str(char *output, int len) { int i; - srand(time(0L)); for ( i = 0; i < len; ++i ) { output[i] = gw_randomchar(); diff --git a/utils/skygw_utils.cc b/utils/skygw_utils.cc index 26a8e8b88..7ca3baec3 100644 --- a/utils/skygw_utils.cc +++ b/utils/skygw_utils.cc @@ -30,6 +30,7 @@ #include #include "skygw_utils.h" #include +#include #if defined(MLIST) @@ -1265,7 +1266,7 @@ void acquire_lock( misscount += 1; if (misscount > 10) { - ts1.tv_nsec = (rand()%misscount)*1000000; + ts1.tv_nsec = (random_jkiss()%misscount)*1000000; nanosleep(&ts1, NULL); } } From 162db1352389dcc1c8d4154ddc65cbe76ff87a1c Mon Sep 17 00:00:00 2001 From: counterpoint Date: Wed, 26 Aug 2015 15:43:43 +0100 Subject: [PATCH 16/85] Add actual random number generation code. --- server/core/random.c | 97 +++++++++++++++++++++++++++++++++++++++++ server/include/random.h | 22 ++++++++++ 2 files changed, 119 insertions(+) create mode 100644 server/core/random.c create mode 100644 server/include/random.h diff --git a/server/core/random.c b/server/core/random.c new file mode 100644 index 000000000..bba20ade5 --- /dev/null +++ b/server/core/random.c @@ -0,0 +1,97 @@ +/* + * This file is distributed as part of the MariaDB Corporation MaxScale. It is free + * software: you can redistribute it and/or modify it under the terms of the + * GNU General Public License as published by the Free Software Foundation, + * version 2. + * + * This program is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS + * FOR A PARTICULAR PURPOSE. See the GNU General Public License for more + * details. + * + * You should have received a copy of the GNU General Public License along with + * this program; if not, write to the Free Software Foundation, Inc., 51 + * Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + * + * Copyright MariaDB Corporation Ab 2013-2014 + */ + +/** + * @file random.c - Random number generator for the MariaDB Corporation MaxScale + * + * @verbatim + * Revision History + * + * Date Who Description + * 26/08/15 Martin Brampton Initial implementation + * + * @endverbatim + */ + +#include +#include +#include +#include +#include + +/* Public domain code for JKISS RNG - Comments added */ +static unsigned int x = 123456789,y = 987654321,z = 43219876,c = 6543217; /* Seed variables */ +static bool init = false; + +/*** + * + * Return a random number + * + * @return uint Random number + * + */ +unsigned int +random_jkiss(void) +{ + unsigned long long t; + if (!init) random_init_jkiss(); + x = 314527869 * x + 1234567; + y ^= y << 5; y ^= y >> 7; y ^= y << 22; + t = 4294584393ULL * z + c; c = t >> 32; z = t; + return x + y + z; +} + +/* Own code adapted from http://www0.cs.ucl.ac.uk/staff/d.jones/GoodPracticeRNG.pdf */ + +/*** + * + * Obtain a seed random number from /dev/urandom if available. + * Otherwise use constant values + * + * @return uint Random number + * + */ +static unsigned int +random_devrand() +{ + int fn; + unsigned int r; + fn = open("/dev/urandom", O_RDONLY); + if (fn == -1) return 0; + if (read(fn, &r, 4) != 4) return 0; + close(fn); + return r; +} + +/*** + * + * Initialise the generator using /dev/urandom if available, and warm up + * with 1000 iterations + * + */ +static void +random_init_jkiss() +{ + int newrand, i; + if ((newrand = random_devrand()) != 0) x = newrand; + if ((newrand = random_devrand()) != 0) y = newrand; + if ((newrand = random_devrand()) != 0) z = newrand; + if ((newrand = random_devrand()) != 0) + c = newrand % 698769068 + 1; /* Should be less than 698769069 */ + for (i = 0; i < 1000; i++) random_jkiss(); +} \ No newline at end of file diff --git a/server/include/random.h b/server/include/random.h new file mode 100644 index 000000000..b302594d2 --- /dev/null +++ b/server/include/random.h @@ -0,0 +1,22 @@ +/* + * File: random.h + * Author: mbrampton + * + * Created on 26 August 2015, 15:34 + */ + +#ifndef RANDOM_H +#define RANDOM_H + +#ifdef __cplusplus +extern "C" { +#endif + +unsigned int random_jkiss(void); + +#ifdef __cplusplus +} +#endif + +#endif /* RANDOM_H */ + From 820bb4ea0037482c3d9da7f92d88346551b9c759 Mon Sep 17 00:00:00 2001 From: counterpoint Date: Wed, 26 Aug 2015 16:18:08 +0100 Subject: [PATCH 17/85] Avoid name clash - change random to random_jkiss --- server/core/CMakeLists.txt | 6 +++--- server/core/{random.c => random_jkiss.c} | 4 ++-- server/core/secrets.c | 2 +- server/core/utils.c | 2 +- server/include/{random.h => random_jkiss.h} | 2 +- utils/skygw_utils.cc | 2 +- 6 files changed, 9 insertions(+), 9 deletions(-) rename server/core/{random.c => random_jkiss.c} (95%) rename server/include/{random.h => random_jkiss.h} (90%) diff --git a/server/core/CMakeLists.txt b/server/core/CMakeLists.txt index a2e9208e8..43318d018 100644 --- a/server/core/CMakeLists.txt +++ b/server/core/CMakeLists.txt @@ -1,5 +1,5 @@ if(BUILD_TESTS OR BUILD_TOOLS) - add_library(fullcore STATIC adminusers.c atomic.c config.c buffer.c dbusers.c dcb.c filter.c gwbitmask.c gw_utils.c hashtable.c hint.c housekeeper.c load_utils.c memlog.c modutil.c monitor.c poll.c resultset.c secrets.c server.c service.c session.c spinlock.c thread.c users.c utils.c gwdirs.c externcmd.c) + add_library(fullcore STATIC adminusers.c atomic.c config.c buffer.c dbusers.c dcb.c filter.c gwbitmask.c gw_utils.c hashtable.c hint.c housekeeper.c load_utils.c memlog.c modutil.c monitor.c poll.c resultset.c secrets.c server.c service.c session.c spinlock.c thread.c users.c utils.c gwdirs.c externcmd.c random_jkiss.c) if(WITH_JEMALLOC) target_link_libraries(fullcore ${JEMALLOC_LIBRARIES}) elseif(WITH_TCMALLOC) @@ -11,8 +11,8 @@ endif() add_executable(maxscale atomic.c buffer.c spinlock.c gateway.c gw_utils.c utils.c dcb.c load_utils.c session.c service.c server.c poll.c config.c users.c hashtable.c dbusers.c thread.c gwbitmask.c - monitor.c adminusers.c secrets.c filter.c modutil.c hint.c - housekeeper.c memlog.c resultset.c gwdirs.c externcmd.c) + monitor.c adminusers.c secrets.c filter.c modutil.c hint.c + housekeeper.c memlog.c resultset.c gwdirs.c externcmd.c random_jkiss.c) if(WITH_JEMALLOC) target_link_libraries(maxscale ${JEMALLOC_LIBRARIES}) diff --git a/server/core/random.c b/server/core/random_jkiss.c similarity index 95% rename from server/core/random.c rename to server/core/random_jkiss.c index bba20ade5..37e008612 100644 --- a/server/core/random.c +++ b/server/core/random_jkiss.c @@ -17,7 +17,7 @@ */ /** - * @file random.c - Random number generator for the MariaDB Corporation MaxScale + * @file random_jkiss.c - Random number generator for the MariaDB Corporation MaxScale * * @verbatim * Revision History @@ -32,7 +32,7 @@ #include #include #include -#include +#include /* Public domain code for JKISS RNG - Comments added */ static unsigned int x = 123456789,y = 987654321,z = 43219876,c = 6543217; /* Seed variables */ diff --git a/server/core/secrets.c b/server/core/secrets.c index b614a5555..7fbd0d096 100644 --- a/server/core/secrets.c +++ b/server/core/secrets.c @@ -23,7 +23,7 @@ #include #include #include -#include +#include /** Defined in log_manager.cc */ extern int lm_enabled_logfiles_bitmask; diff --git a/server/core/utils.c b/server/core/utils.c index 45b47e632..df16180c2 100644 --- a/server/core/utils.c +++ b/server/core/utils.c @@ -43,7 +43,7 @@ #include #include #include -#include +#include /** Defined in log_manager.cc */ extern int lm_enabled_logfiles_bitmask; diff --git a/server/include/random.h b/server/include/random_jkiss.h similarity index 90% rename from server/include/random.h rename to server/include/random_jkiss.h index b302594d2..ee321afed 100644 --- a/server/include/random.h +++ b/server/include/random_jkiss.h @@ -1,5 +1,5 @@ /* - * File: random.h + * File: random_jkiss.h * Author: mbrampton * * Created on 26 August 2015, 15:34 diff --git a/utils/skygw_utils.cc b/utils/skygw_utils.cc index 7ca3baec3..46976e254 100644 --- a/utils/skygw_utils.cc +++ b/utils/skygw_utils.cc @@ -30,7 +30,7 @@ #include #include "skygw_utils.h" #include -#include +#include #if defined(MLIST) From 4ec5e3b69dad11be732f56a326a500aa4acbfe1c Mon Sep 17 00:00:00 2001 From: counterpoint Date: Wed, 26 Aug 2015 16:18:37 +0100 Subject: [PATCH 18/85] Change header random.h to random_jkiss.h --- server/include/random_jkiss.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/server/include/random_jkiss.h b/server/include/random_jkiss.h index ee321afed..661294437 100644 --- a/server/include/random_jkiss.h +++ b/server/include/random_jkiss.h @@ -5,8 +5,8 @@ * Created on 26 August 2015, 15:34 */ -#ifndef RANDOM_H -#define RANDOM_H +#ifndef RANDOM_JKISS_H +#define RANDOM_JKISS_H #ifdef __cplusplus extern "C" { From b66bcbd36c6da6126b6e237120c14ac97296db8c Mon Sep 17 00:00:00 2001 From: counterpoint Date: Wed, 26 Aug 2015 16:30:08 +0100 Subject: [PATCH 19/85] Correct small mistake --- server/include/random_jkiss.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/include/random_jkiss.h b/server/include/random_jkiss.h index 661294437..21d638c82 100644 --- a/server/include/random_jkiss.h +++ b/server/include/random_jkiss.h @@ -12,7 +12,7 @@ extern "C" { #endif -unsigned int random_jkiss(void); +extern unsigned int random_jkiss(void); #ifdef __cplusplus } From 70d82fd45ee0a01d5788192eb052dcaba8e6457e Mon Sep 17 00:00:00 2001 From: Markus Makela Date: Wed, 26 Aug 2015 18:33:46 +0300 Subject: [PATCH 20/85] Fixed compilation problems. --- server/core/CMakeLists.txt | 4 ++-- server/core/poll.c | 2 +- server/core/random_jkiss.c | 4 +++- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/server/core/CMakeLists.txt b/server/core/CMakeLists.txt index 43318d018..2dabf4d33 100644 --- a/server/core/CMakeLists.txt +++ b/server/core/CMakeLists.txt @@ -23,11 +23,11 @@ endif() target_link_libraries(maxscale ${EMBEDDED_LIB} ${PCRE_LINK_FLAGS} ${CURL_LIBRARIES} log_manager utils ssl aio pthread crypt dl crypto inih z rt m stdc++) install(TARGETS maxscale DESTINATION ${MAXSCALE_BINDIR}) -add_executable(maxkeys maxkeys.c secrets.c utils.c gwdirs.c) +add_executable(maxkeys maxkeys.c secrets.c utils.c gwdirs.c random_jkiss.c) target_link_libraries(maxkeys log_manager utils pthread crypt crypto) install(TARGETS maxkeys DESTINATION ${MAXSCALE_BINDIR}) -add_executable(maxpasswd maxpasswd.c secrets.c utils.c gwdirs.c) +add_executable(maxpasswd maxpasswd.c secrets.c utils.c gwdirs.c random_jkiss.c) target_link_libraries(maxpasswd log_manager utils pthread crypt crypto) install(TARGETS maxpasswd DESTINATION ${MAXSCALE_BINDIR}) diff --git a/server/core/poll.c b/server/core/poll.c index 5a9c72829..e0bcb0d0c 100644 --- a/server/core/poll.c +++ b/server/core/poll.c @@ -1135,7 +1135,7 @@ poll_dcb_session_check(DCB *dcb) LOGIF(LE, (skygw_log_write_flush( LOGFILE_ERROR, "%lu [%s] The dcb %p that was about to be processed does not " - "have a non-null session pointer " + "have a non-null session pointer ", pthread_self(), __func__, dcb))); diff --git a/server/core/random_jkiss.c b/server/core/random_jkiss.c index 37e008612..a4443890b 100644 --- a/server/core/random_jkiss.c +++ b/server/core/random_jkiss.c @@ -38,6 +38,8 @@ static unsigned int x = 123456789,y = 987654321,z = 43219876,c = 6543217; /* Seed variables */ static bool init = false; +static void random_init_jkiss(); + /*** * * Return a random number @@ -94,4 +96,4 @@ random_init_jkiss() if ((newrand = random_devrand()) != 0) c = newrand % 698769068 + 1; /* Should be less than 698769069 */ for (i = 0; i < 1000; i++) random_jkiss(); -} \ No newline at end of file +} From 57b82dcedb16b05703a588db7259b406f30abd55 Mon Sep 17 00:00:00 2001 From: counterpoint Date: Wed, 26 Aug 2015 16:36:08 +0100 Subject: [PATCH 21/85] Correct initialisation logic. --- server/core/random_jkiss.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/server/core/random_jkiss.c b/server/core/random_jkiss.c index a4443890b..350154dd7 100644 --- a/server/core/random_jkiss.c +++ b/server/core/random_jkiss.c @@ -51,7 +51,11 @@ unsigned int random_jkiss(void) { unsigned long long t; - if (!init) random_init_jkiss(); + if (!init) + { + random_init_jkiss(); + init = true; + } x = 314527869 * x + 1234567; y ^= y << 5; y ^= y >> 7; y ^= y << 22; t = 4294584393ULL * z + c; c = t >> 32; z = t; From c01aa6952e9459cf9a362f256394e87f2f9d8f8a Mon Sep 17 00:00:00 2001 From: counterpoint Date: Wed, 26 Aug 2015 17:16:10 +0100 Subject: [PATCH 22/85] Fix initialisation problem; put all statements on separate lines. --- server/core/random_jkiss.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/server/core/random_jkiss.c b/server/core/random_jkiss.c index 350154dd7..bfafd8054 100644 --- a/server/core/random_jkiss.c +++ b/server/core/random_jkiss.c @@ -32,6 +32,7 @@ #include #include #include +#include #include /* Public domain code for JKISS RNG - Comments added */ @@ -53,12 +54,16 @@ random_jkiss(void) unsigned long long t; if (!init) { - random_init_jkiss(); init = true; + random_init_jkiss(); } x = 314527869 * x + 1234567; - y ^= y << 5; y ^= y >> 7; y ^= y << 22; - t = 4294584393ULL * z + c; c = t >> 32; z = t; + y ^= y << 5; + y ^= y >> 7; + y ^= y << 22; + t = 4294584393ULL * z + c; + c = t >> 32; + z = t; return x + y + z; } @@ -99,5 +104,5 @@ random_init_jkiss() if ((newrand = random_devrand()) != 0) z = newrand; if ((newrand = random_devrand()) != 0) c = newrand % 698769068 + 1; /* Should be less than 698769069 */ - for (i = 0; i < 1000; i++) random_jkiss(); + for (i = 0; i < 100; i++) random_jkiss(); } From 0d62f5281284d49185e730ed57d64a8e5083902c Mon Sep 17 00:00:00 2001 From: counterpoint Date: Fri, 28 Aug 2015 09:12:41 +0100 Subject: [PATCH 23/85] Ensure thread safe through use of spinlock; add further comments. --- server/core/random_jkiss.c | 48 +++++++++++++++++++++++++++----------- 1 file changed, 34 insertions(+), 14 deletions(-) diff --git a/server/core/random_jkiss.c b/server/core/random_jkiss.c index bfafd8054..feb0aa19a 100644 --- a/server/core/random_jkiss.c +++ b/server/core/random_jkiss.c @@ -18,6 +18,9 @@ /** * @file random_jkiss.c - Random number generator for the MariaDB Corporation MaxScale + * + * See http://www0.cs.ucl.ac.uk/staff/d.jones/GoodPracticeRNG.pdf for discussion of random + * number generators (RNGs). * * @verbatim * Revision History @@ -35,15 +38,22 @@ #include #include -/* Public domain code for JKISS RNG - Comments added */ +/* Public domain code for JKISS RNG - Comment header added */ + +/* If possible, the seed variables will be set from /dev/urandom but + * should that fail, these arbitrary numbers will be used as a last resort. + */ static unsigned int x = 123456789,y = 987654321,z = 43219876,c = 6543217; /* Seed variables */ static bool init = false; -static void random_init_jkiss(); +static SPINLOCK random_jkiss_spinlock = SPINLOCK_INIT; + +static unsigned int random_jkiss_devrand(void); +static void random_init_jkiss(void); /*** * - * Return a random number + * Return a pseudo-random number that satisfies major tests for random sequences * * @return uint Random number * @@ -52,9 +62,13 @@ unsigned int random_jkiss(void) { unsigned long long t; + unsigned int result; + spinlock_acquire(&random_jkiss_spinlock); if (!init) { + /* Must set init first because initialisation calls this function */ init = true; + spinlock_release(&random_jkiss_spinlock); random_init_jkiss(); } x = 314527869 * x + 1234567; @@ -64,7 +78,9 @@ random_jkiss(void) t = 4294584393ULL * z + c; c = t >> 32; z = t; - return x + y + z; + result = x + y + z; + spinlock_release(&random_jkiss_spinlock); + return result; } /* Own code adapted from http://www0.cs.ucl.ac.uk/staff/d.jones/GoodPracticeRNG.pdf */ @@ -72,19 +88,21 @@ random_jkiss(void) /*** * * Obtain a seed random number from /dev/urandom if available. - * Otherwise use constant values * * @return uint Random number * */ static unsigned int -random_devrand() +random_jkiss_devrand(void) { int fn; unsigned int r; - fn = open("/dev/urandom", O_RDONLY); - if (fn == -1) return 0; - if (read(fn, &r, 4) != 4) return 0; + if ((fn = open("/dev/urandom", O_RDONLY)) == -1) return 0; + if (read(fn, &r, 4) != 4) + { + close(fn); + return 0; + } close(fn); return r; } @@ -96,13 +114,15 @@ random_devrand() * */ static void -random_init_jkiss() +random_init_jkiss(void) { int newrand, i; - if ((newrand = random_devrand()) != 0) x = newrand; - if ((newrand = random_devrand()) != 0) y = newrand; - if ((newrand = random_devrand()) != 0) z = newrand; - if ((newrand = random_devrand()) != 0) + if ((newrand = random_jkiss_devrand()) != 0) x = newrand; + if ((newrand = random_jkiss_devrand()) != 0) y = newrand; + if ((newrand = random_jkiss_devrand()) != 0) z = newrand; + if ((newrand = random_jkiss_devrand()) != 0) c = newrand % 698769068 + 1; /* Should be less than 698769069 */ + + /* "Warm up" our random number generator */ for (i = 0; i < 100; i++) random_jkiss(); } From 9c5f6224818a99f0fb93ea87494ff25e0058f6f9 Mon Sep 17 00:00:00 2001 From: counterpoint Date: Fri, 28 Aug 2015 15:30:06 +0100 Subject: [PATCH 24/85] Additional spinlock in random_jkiss. Initial attempt at implementing dummy sessions to provide total consistency - used in mysql_client in relation to authentication - a single static dummy session is used and linked from the client dcb when authentication is not yet complete. --- server/core/dcb.c | 5 ++- server/core/random_jkiss.c | 2 + server/core/session.c | 53 +++++++++++++++++++++++++ server/include/session.h | 4 +- server/modules/protocol/mysql_backend.c | 2 +- server/modules/protocol/mysql_client.c | 5 ++- utils/skygw_debug.h | 9 +++-- 7 files changed, 71 insertions(+), 9 deletions(-) diff --git a/server/core/dcb.c b/server/core/dcb.c index 448c9a091..013029de6 100644 --- a/server/core/dcb.c +++ b/server/core/dcb.c @@ -346,7 +346,10 @@ dcb_final_free(DCB *dcb) local_session->client = NULL; spinlock_release(&local_session->ses_lock); } - session_free(local_session); + if (SESSION_STATE_DUMMY != local_session->state) + { + session_free(local_session); + } } } diff --git a/server/core/random_jkiss.c b/server/core/random_jkiss.c index feb0aa19a..f60b4beee 100644 --- a/server/core/random_jkiss.c +++ b/server/core/random_jkiss.c @@ -117,11 +117,13 @@ static void random_init_jkiss(void) { int newrand, i; + spinlock_acquire(&random_jkiss_spinlock); if ((newrand = random_jkiss_devrand()) != 0) x = newrand; if ((newrand = random_jkiss_devrand()) != 0) y = newrand; if ((newrand = random_jkiss_devrand()) != 0) z = newrand; if ((newrand = random_jkiss_devrand()) != 0) c = newrand % 698769068 + 1; /* Should be less than 698769069 */ + spinlock_release(&random_jkiss_spinlock); /* "Warm up" our random number generator */ for (i = 0; i < 100; i++) random_jkiss(); diff --git a/server/core/session.c b/server/core/session.c index 31bbbd637..b50d5e0c8 100644 --- a/server/core/session.c +++ b/server/core/session.c @@ -56,6 +56,7 @@ static size_t session_id; static SPINLOCK session_spin = SPINLOCK_INIT; static SESSION *allSessions = NULL; +static struct session session_dummy_struct; static int session_setup_filters(SESSION *session); static void session_simple_free(SESSION *session, DCB *dcb); @@ -212,6 +213,48 @@ session_alloc(SERVICE *service, DCB *client_dcb) return session; } +/** + * Allocate a dummy session so that DCBs can always have sessions. + * + * Only one dummy session exists, it is statically declared + * + * @param client_dcb The client side DCB + * @return The dummy created session + */ +SESSION * +session_alloc_dummy(DCB *client_dcb) +{ + SESSION *session; + + session = &session_dummy_struct; +#if defined(SS_DEBUG) + session->ses_chk_top = CHK_NUM_SESSION; + session->ses_chk_tail = CHK_NUM_SESSION; +#endif + session->ses_is_child = false; + spinlock_init(&session->ses_lock); + session->service = NULL; + session->client = NULL; + session->n_filters = 0; + memset(&session->stats, 0, sizeof(SESSION_STATS)); + session->stats.connect = 0; + session->state = SESSION_STATE_DUMMY; + /*< + * Associate the session to the client DCB and set the reference count on + * the session to indicate that there is a single reference to the + * session. There is no need to protect this or use atomic add as the + * session has not been made available to the other threads at this + * point. + */ + session->data = NULL; + session->refcount = 1; + session->ses_id = 0; + session->next = NULL; + + client_dcb->session = session; + return session; +} + /** * Enable specified logging for the current session and increase logger * counter. @@ -323,6 +366,10 @@ session_simple_free(SESSION *session, DCB *dcb) } if (session) { + if (SESSION_STATE_DUMMY == session->state) + { + return; + } if (session && session->router_session) { session->service->router->freeSession( @@ -344,6 +391,10 @@ session_simple_free(SESSION *session, DCB *dcb) bool session_free(SESSION *session) { + if (session && SESSION_STATE_DUMMY == session->state) + { + return true; + } CHK_SESSION(session); /* @@ -711,6 +762,8 @@ session_state(int state) { case SESSION_STATE_ALLOC: return "Session Allocated"; + case SESSION_STATE_DUMMY: + return "Dummy Session"; case SESSION_STATE_READY: return "Session Ready"; case SESSION_STATE_ROUTER_READY: diff --git a/server/include/session.h b/server/include/session.h index 7a9381507..e916c902e 100644 --- a/server/include/session.h +++ b/server/include/session.h @@ -64,7 +64,8 @@ typedef enum { SESSION_STATE_LISTENER, /*< for listener session */ SESSION_STATE_LISTENER_STOPPED, /*< for listener session */ SESSION_STATE_TO_BE_FREED, /*< ready to be freed as soon as there are no references */ - SESSION_STATE_FREE /*< for all sessions */ + SESSION_STATE_FREE, /*< for all sessions */ + SESSION_STATE_DUMMY /*< dummy session for consistency */ } session_state_t; /** @@ -162,6 +163,7 @@ typedef struct session { SESSION *get_all_sessions(); SESSION *session_alloc(struct service *, struct dcb *); +SESSION *session_alloc_dummy(struct dcb *); bool session_free(SESSION *); int session_isvalid(SESSION *); int session_reply(void *inst, void *session, GWBUF *data); diff --git a/server/modules/protocol/mysql_backend.c b/server/modules/protocol/mysql_backend.c index de178fe60..6a024c348 100644 --- a/server/modules/protocol/mysql_backend.c +++ b/server/modules/protocol/mysql_backend.c @@ -140,7 +140,7 @@ static MYSQL_session* gw_get_shared_session_auth_info( spinlock_acquire(&dcb->session->ses_lock); - if (dcb->session->state != SESSION_STATE_ALLOC) { + if (dcb->session->state != SESSION_STATE_ALLOC && dcb->session->state != SESSION_STATE_DUMMY) { auth_info = dcb->session->data; } else { LOGIF(LE, (skygw_log_write_flush( diff --git a/server/modules/protocol/mysql_client.c b/server/modules/protocol/mysql_client.c index 70ddcac84..33515c89b 100644 --- a/server/modules/protocol/mysql_client.c +++ b/server/modules/protocol/mysql_client.c @@ -912,7 +912,7 @@ int gw_read_client_event( if (session != NULL) { CHK_SESSION(session); - ss_dassert(session->state != SESSION_STATE_ALLOC); + ss_dassert(session->state != SESSION_STATE_ALLOC && session->state != SESSION_STATE_DUMMY); protocol->protocol_auth_state = MYSQL_IDLE; /** @@ -1012,7 +1012,7 @@ int gw_read_client_event( if (session != NULL) { CHK_SESSION(session); - ss_dassert(session->state != SESSION_STATE_ALLOC); + ss_dassert(session->state != SESSION_STATE_ALLOC && session->state != SESSION_STATE_DUMMY); protocol->protocol_auth_state = MYSQL_IDLE; /** @@ -1643,6 +1643,7 @@ int gw_MySQLAccept(DCB *listener) } client_dcb->service = listener->session->service; + client_dcb->session = session_alloc_dummy(client_dcb); client_dcb->fd = c_sock; // get client address diff --git a/utils/skygw_debug.h b/utils/skygw_debug.h index ff5d5c9c0..8f164b828 100644 --- a/utils/skygw_debug.h +++ b/utils/skygw_debug.h @@ -210,10 +210,11 @@ typedef enum skygw_chk_t { ((s) == DCB_STATE_UNDEFINED ? "DCB_STATE_UNDEFINED" : "DCB_STATE_UNKNOWN"))))))) #define STRSESSIONSTATE(s) ((s) == SESSION_STATE_ALLOC ? "SESSION_STATE_ALLOC" : \ - ((s) == SESSION_STATE_READY ? "SESSION_STATE_READY" : \ - ((s) == SESSION_STATE_LISTENER ? "SESSION_STATE_LISTENER" : \ - ((s) == SESSION_STATE_LISTENER_STOPPED ? "SESSION_STATE_LISTENER_STOPPED" : \ - (s) == SESSION_STATE_ROUTER_READY ? "SESSION_STATE_ROUTER_READY":\ + ((s) == SESSION_STATE_DUMMY ? "SESSION_STATE_DUMMY" : \ + ((s) == SESSION_STATE_READY ? "SESSION_STATE_READY" : \ + ((s) == SESSION_STATE_LISTENER ? "SESSION_STATE_LISTENER" : \ + ((s) == SESSION_STATE_LISTENER_STOPPED ? "SESSION_STATE_LISTENER_STOPPED" : \ + (s) == SESSION_STATE_ROUTER_READY ? "SESSION_STATE_ROUTER_READY":\ "SESSION_STATE_UNKNOWN")))) #define STRPROTOCOLSTATE(s) ((s) == MYSQL_ALLOC ? "MYSQL_ALLOC" : \ From 753746f5c50e8c206df14dbdc1cdc320e4ba417a Mon Sep 17 00:00:00 2001 From: counterpoint Date: Fri, 28 Aug 2015 16:12:36 +0100 Subject: [PATCH 25/85] Fix mistakes --- server/core/CMakeLists.txt | 4 ++-- server/core/random_jkiss.c | 2 ++ utils/skygw_debug.h | 2 +- 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/server/core/CMakeLists.txt b/server/core/CMakeLists.txt index 2dabf4d33..5ad84ce59 100644 --- a/server/core/CMakeLists.txt +++ b/server/core/CMakeLists.txt @@ -23,11 +23,11 @@ endif() target_link_libraries(maxscale ${EMBEDDED_LIB} ${PCRE_LINK_FLAGS} ${CURL_LIBRARIES} log_manager utils ssl aio pthread crypt dl crypto inih z rt m stdc++) install(TARGETS maxscale DESTINATION ${MAXSCALE_BINDIR}) -add_executable(maxkeys maxkeys.c secrets.c utils.c gwdirs.c random_jkiss.c) +add_executable(maxkeys maxkeys.c secrets.c utils.c gwdirs.c spinlock.c random_jkiss.c) target_link_libraries(maxkeys log_manager utils pthread crypt crypto) install(TARGETS maxkeys DESTINATION ${MAXSCALE_BINDIR}) -add_executable(maxpasswd maxpasswd.c secrets.c utils.c gwdirs.c random_jkiss.c) +add_executable(maxpasswd maxpasswd.c secrets.c utils.c gwdirs.c spinlock.c random_jkiss.c) target_link_libraries(maxpasswd log_manager utils pthread crypt crypto) install(TARGETS maxpasswd DESTINATION ${MAXSCALE_BINDIR}) diff --git a/server/core/random_jkiss.c b/server/core/random_jkiss.c index f60b4beee..6eea33739 100644 --- a/server/core/random_jkiss.c +++ b/server/core/random_jkiss.c @@ -36,6 +36,7 @@ #include #include #include +#include #include /* Public domain code for JKISS RNG - Comment header added */ @@ -63,6 +64,7 @@ random_jkiss(void) { unsigned long long t; unsigned int result; + spinlock_acquire(&random_jkiss_spinlock); if (!init) { diff --git a/utils/skygw_debug.h b/utils/skygw_debug.h index 8f164b828..c5c1c0ce9 100644 --- a/utils/skygw_debug.h +++ b/utils/skygw_debug.h @@ -215,7 +215,7 @@ typedef enum skygw_chk_t { ((s) == SESSION_STATE_LISTENER ? "SESSION_STATE_LISTENER" : \ ((s) == SESSION_STATE_LISTENER_STOPPED ? "SESSION_STATE_LISTENER_STOPPED" : \ (s) == SESSION_STATE_ROUTER_READY ? "SESSION_STATE_ROUTER_READY":\ - "SESSION_STATE_UNKNOWN")))) + "SESSION_STATE_UNKNOWN"))))) #define STRPROTOCOLSTATE(s) ((s) == MYSQL_ALLOC ? "MYSQL_ALLOC" : \ ((s) == MYSQL_PENDING_CONNECT ? "MYSQL_PENDING_CONNECT" : \ From 068ec77d053d16aab8715237f6ca78e229e74711 Mon Sep 17 00:00:00 2001 From: counterpoint Date: Fri, 28 Aug 2015 16:44:40 +0100 Subject: [PATCH 26/85] Fix bugs. --- server/modules/protocol/mysql_client.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/server/modules/protocol/mysql_client.c b/server/modules/protocol/mysql_client.c index 33515c89b..a90feab39 100644 --- a/server/modules/protocol/mysql_client.c +++ b/server/modules/protocol/mysql_client.c @@ -750,7 +750,7 @@ int gw_read_client_event( session = dcb->session; - if (protocol->protocol_auth_state == MYSQL_IDLE && session != NULL) + if (protocol->protocol_auth_state == MYSQL_IDLE && session != NULL && SESSION_STATE_DUMMY != session->state) { CHK_SESSION(session); router = session->service->router; @@ -1096,7 +1096,7 @@ int gw_read_client_event( session_state_t ses_state; session = dcb->session; - ss_dassert(session!= NULL); + ss_dassert(session!= NULL && SESSION_STATE_DUMMY != session->state); if (session != NULL) { @@ -1803,7 +1803,7 @@ gw_client_close(DCB *dcb) * session may be NULL if session_alloc failed. * In that case, router session wasn't created. */ - if (session != NULL) + if (session != NULL && SESSION_STATE_DUMMY != session->state) { CHK_SESSION(session); spinlock_acquire(&session->ses_lock); From 53383189003c4270a69c00573e2684a71023dd52 Mon Sep 17 00:00:00 2001 From: counterpoint Date: Fri, 28 Aug 2015 17:25:41 +0100 Subject: [PATCH 27/85] Improve error message when DCB has no session pointer in poll loop. --- server/core/poll.c | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/server/core/poll.c b/server/core/poll.c index e0bcb0d0c..15e31d313 100644 --- a/server/core/poll.c +++ b/server/core/poll.c @@ -96,7 +96,7 @@ static simple_mutex_t epoll_wait_mutex; /*< serializes calls to epoll_wait */ static int n_waiting = 0; /*< No. of threads in epoll_wait */ static int process_pollq(int thread_id); static void poll_add_event_to_dcb(DCB* dcb, GWBUF* buf, __uint32_t ev); -static bool poll_dcb_session_check(DCB *dcb); +static bool poll_dcb_session_check(DCB *dcb, const char *); DCB *eventq = NULL; SPINLOCK pollqlock = SPINLOCK_INIT; @@ -885,7 +885,7 @@ unsigned long qtime; dcb, &tls_log_info.li_sesid, &tls_log_info.li_enabled_logs))); - if (poll_dcb_session_check(dcb)) + if (poll_dcb_session_check(dcb, "write_ready")) { dcb->func.write_ready(dcb); } @@ -918,7 +918,7 @@ unsigned long qtime; dcb, &tls_log_info.li_sesid, &tls_log_info.li_enabled_logs))); - if (poll_dcb_session_check(dcb)) + if (poll_dcb_session_check(dcb, "accept")) { dcb->func.accept(dcb); } @@ -938,7 +938,7 @@ unsigned long qtime; dcb, &tls_log_info.li_sesid, &tls_log_info.li_enabled_logs))); - if (poll_dcb_session_check(dcb)) + if (poll_dcb_session_check(dcb, "read")) { dcb->func.read(dcb); } @@ -976,7 +976,7 @@ unsigned long qtime; dcb, &tls_log_info.li_sesid, &tls_log_info.li_enabled_logs))); - if (poll_dcb_session_check(dcb)) + if (poll_dcb_session_check(dcb, "error")) { dcb->func.error(dcb); } @@ -1008,7 +1008,7 @@ unsigned long qtime; dcb, &tls_log_info.li_sesid, &tls_log_info.li_enabled_logs))); - if (poll_dcb_session_check(dcb)) + if (poll_dcb_session_check(dcb, "hangup EPOLLHUP")) { dcb->func.hangup(dcb); } @@ -1044,7 +1044,7 @@ unsigned long qtime; dcb, &tls_log_info.li_sesid, &tls_log_info.li_enabled_logs))); - if (poll_dcb_session_check(dcb)) + if (poll_dcb_session_check(dcb, "hangup EPOLLRDHUP")) { dcb->func.hangup(dcb); } @@ -1120,11 +1120,12 @@ unsigned long qtime; * Check that the DCB has a session link before processing. * If not, log an error. Processing will be bypassed * - * @param dcb The DCB to check - * @return bool Does the DCB have a non-null session link + * @param dcb The DCB to check + * @param function The name of the function about to be called + * @return bool Does the DCB have a non-null session link */ static bool -poll_dcb_session_check(DCB *dcb) +poll_dcb_session_check(DCB *dcb, const char *function) { if (dcb->session) { @@ -1134,11 +1135,12 @@ poll_dcb_session_check(DCB *dcb) { LOGIF(LE, (skygw_log_write_flush( LOGFILE_ERROR, - "%lu [%s] The dcb %p that was about to be processed does not " + "%lu [%s] The dcb %p that was about to be processed by %s does not " "have a non-null session pointer ", pthread_self(), __func__, - dcb))); + dcb, + function))); return false; } } From d29c5909a63f736455acd43530abf6615820f17b Mon Sep 17 00:00:00 2001 From: Markus Makela Date: Fri, 28 Aug 2015 19:49:03 +0300 Subject: [PATCH 28/85] Properly close the branch session of the tee filter. --- server/modules/filter/tee.c | 61 +++++++++++++++++-------------------- 1 file changed, 28 insertions(+), 33 deletions(-) diff --git a/server/modules/filter/tee.c b/server/modules/filter/tee.c index 1557166f9..996f247db 100644 --- a/server/modules/filter/tee.c +++ b/server/modules/filter/tee.c @@ -212,6 +212,7 @@ int route_single_query(TEE_INSTANCE* my_instance, GWBUF* buffer, GWBUF* clone); int reset_session_state(TEE_SESSION* my_session, GWBUF* buffer); +void create_orphan(SESSION* ses); static void orphan_free(void* data) @@ -557,6 +558,7 @@ char *remote, *userName; if ((ses = session_alloc(my_instance->service, dcb)) == NULL) { + filter_free(dummy); dcb_close(dcb); freeSession(instance, (void *)my_session); my_session = NULL; @@ -572,39 +574,27 @@ char *remote, *userName; dummy->obj = GetModuleObject(); dummy->filter = NULL; - + my_session->branch_session = ses; + my_session->branch_dcb = dcb; + my_session->dummy_filterdef = dummy; - if((dummy_upstream = filterUpstream( + if(true || (dummy_upstream = filterUpstream( dummy, my_session, &ses->tail)) == NULL) { - spinlock_acquire(&ses->ses_lock); - ses->state = SESSION_STATE_STOPPING; - spinlock_release(&ses->ses_lock); - - ses->service->router->closeSession( - ses->service->router_instance, - ses->router_session); - - ses->client = NULL; - dcb->session = NULL; - session_free(ses); + filter_free(dummy); + closeSession(instance,(void*)my_session); dcb_close(dcb); - freeSession(instance, (void *) my_session); - my_session = NULL; + free(my_session); LOGIF(LE, (skygw_log_write_flush( LOGFILE_ERROR, "Error : tee: Allocating memory for" "dummy upstream failed." " Terminating session."))); - goto retblock; + return NULL; } ses->tail = *dummy_upstream; - my_session->branch_session = ses; - my_session->branch_dcb = dcb; - my_session->dummy_filterdef = dummy; - MySQLProtocol* protocol = (MySQLProtocol*)session->client->protocol; my_session->use_ok = protocol->client_capabilities & (1 << 6); free(dummy_upstream); @@ -712,19 +702,7 @@ skygw_log_write(LOGFILE_TRACE,"Tee free: %d", atomic_add(&debug_seq,1)); } else if(state == SESSION_STATE_STOPPING) { - orphan_session_t* orphan; - if((orphan = malloc(sizeof(orphan_session_t))) == NULL) - { - skygw_log_write(LOGFILE_ERROR,"Error : Failed to " - "allocate memory for orphan session struct, " - "child session might leak memory."); - }else{ - orphan->session = ses; - spinlock_acquire(&orphanLock); - orphan->next = allOrphans; - allOrphans = orphan; - spinlock_release(&orphanLock); - } + create_orphan(ses); } } if (my_session->dummy_filterdef) @@ -1400,4 +1378,21 @@ int reset_session_state(TEE_SESSION* my_session, GWBUF* buffer) my_session->command = command; return 1; +} + +void create_orphan(SESSION* ses) +{ + orphan_session_t* orphan; + if((orphan = malloc(sizeof(orphan_session_t))) == NULL) + { + skygw_log_write(LOGFILE_ERROR,"Error : Failed to " + "allocate memory for orphan session struct, " + "child session might leak memory."); + }else{ + orphan->session = ses; + spinlock_acquire(&orphanLock); + orphan->next = allOrphans; + allOrphans = orphan; + spinlock_release(&orphanLock); + } } \ No newline at end of file From f1c3b65b1539fdff6f7a4c019652be05ba45a344 Mon Sep 17 00:00:00 2001 From: Markus Makela Date: Fri, 28 Aug 2015 19:52:02 +0300 Subject: [PATCH 29/85] Fixed mistake. --- server/modules/filter/tee.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/modules/filter/tee.c b/server/modules/filter/tee.c index 996f247db..6486a9a19 100644 --- a/server/modules/filter/tee.c +++ b/server/modules/filter/tee.c @@ -578,7 +578,7 @@ char *remote, *userName; my_session->branch_dcb = dcb; my_session->dummy_filterdef = dummy; - if(true || (dummy_upstream = filterUpstream( + if((dummy_upstream = filterUpstream( dummy, my_session, &ses->tail)) == NULL) { filter_free(dummy); From a711b25fec07074e31962fe25b2cded635a8ccfc Mon Sep 17 00:00:00 2001 From: counterpoint Date: Fri, 28 Aug 2015 18:20:18 +0100 Subject: [PATCH 30/85] Improve user name setting in DCB for persistent connections and to fix bug; change name of session_alloc_dummy to session_set_dummy to be more informative. --- server/core/dcb.c | 11 ++++++----- server/core/session.c | 2 +- server/include/session.h | 2 +- server/modules/protocol/mysql_client.c | 2 +- 4 files changed, 9 insertions(+), 8 deletions(-) diff --git a/server/core/dcb.c b/server/core/dcb.c index 013029de6..ab1e3638e 100644 --- a/server/core/dcb.c +++ b/server/core/dcb.c @@ -1845,9 +1845,6 @@ dcb_close(DCB *dcb) spinlock_acquire(&zombiespin); if (!dcb->dcb_is_zombie) { - char *user; - user = session_getUser(dcb->session); - if (NULL != user) dcb->user = strdup(user); /*< * Add closing dcb to the top of the list, setting zombie marker */ @@ -1873,8 +1870,10 @@ static bool dcb_maybe_add_persistent(DCB *dcb) { int poolcount = -1; - if (dcb->user != NULL - && strlen(dcb->user) + char *user; + user = session_getUser(dcb->session); + if (user != NULL + && strlen(user) && dcb->server && dcb->server->persistpoolmax && !dcb->dcb_errhandle_called @@ -1888,6 +1887,8 @@ dcb_maybe_add_persistent(DCB *dcb) dcb->user))); dcb->dcb_is_zombie = false; dcb->persistentstart = time(NULL); + if (dcb->user) free(dcb->user); + dcb->user = strdup(user); spinlock_acquire(&dcb->server->persistlock); dcb->nextpersistent = dcb->server->persistent; dcb->server->persistent = dcb; diff --git a/server/core/session.c b/server/core/session.c index b50d5e0c8..1411d34bd 100644 --- a/server/core/session.c +++ b/server/core/session.c @@ -222,7 +222,7 @@ session_alloc(SERVICE *service, DCB *client_dcb) * @return The dummy created session */ SESSION * -session_alloc_dummy(DCB *client_dcb) +session_set_dummy(DCB *client_dcb) { SESSION *session; diff --git a/server/include/session.h b/server/include/session.h index e916c902e..edb4af1d4 100644 --- a/server/include/session.h +++ b/server/include/session.h @@ -163,7 +163,7 @@ typedef struct session { SESSION *get_all_sessions(); SESSION *session_alloc(struct service *, struct dcb *); -SESSION *session_alloc_dummy(struct dcb *); +SESSION *session_set_dummy(struct dcb *); bool session_free(SESSION *); int session_isvalid(SESSION *); int session_reply(void *inst, void *session, GWBUF *data); diff --git a/server/modules/protocol/mysql_client.c b/server/modules/protocol/mysql_client.c index a90feab39..077ec6918 100644 --- a/server/modules/protocol/mysql_client.c +++ b/server/modules/protocol/mysql_client.c @@ -1643,7 +1643,7 @@ int gw_MySQLAccept(DCB *listener) } client_dcb->service = listener->session->service; - client_dcb->session = session_alloc_dummy(client_dcb); + client_dcb->session = session_set_dummy(client_dcb); client_dcb->fd = c_sock; // get client address From d74990833bb67ff3ed9193f7ad4d18fd202d4e52 Mon Sep 17 00:00:00 2001 From: counterpoint Date: Tue, 1 Sep 2015 09:59:34 +0100 Subject: [PATCH 31/85] Move capture of user name for persistent connections; expand error message in mysql client to give more information. --- server/core/dcb.c | 18 ++++++++++++------ server/modules/protocol/mysql_client.c | 6 ++++-- 2 files changed, 16 insertions(+), 8 deletions(-) diff --git a/server/core/dcb.c b/server/core/dcb.c index ab1e3638e..7112ef7a3 100644 --- a/server/core/dcb.c +++ b/server/core/dcb.c @@ -1845,6 +1845,16 @@ dcb_close(DCB *dcb) spinlock_acquire(&zombiespin); if (!dcb->dcb_is_zombie) { + if (dcb->server && DCB_STATE_POLLING == dcb->state) + { + /* May be a candidate for persistence, so save user name */ + char *user; + user = session_getUser(dcb->session); + if (user && strlen(user) && !dcb->user) + { + dcb->user = strdup(user); + } + } /*< * Add closing dcb to the top of the list, setting zombie marker */ @@ -1870,10 +1880,8 @@ static bool dcb_maybe_add_persistent(DCB *dcb) { int poolcount = -1; - char *user; - user = session_getUser(dcb->session); - if (user != NULL - && strlen(user) + if (dcb->user != NULL + && strlen(dcb->user) && dcb->server && dcb->server->persistpoolmax && !dcb->dcb_errhandle_called @@ -1887,8 +1895,6 @@ dcb_maybe_add_persistent(DCB *dcb) dcb->user))); dcb->dcb_is_zombie = false; dcb->persistentstart = time(NULL); - if (dcb->user) free(dcb->user); - dcb->user = strdup(user); spinlock_acquire(&dcb->server->persistlock); dcb->nextpersistent = dcb->server->persistent; dcb->server->persistent = dcb; diff --git a/server/modules/protocol/mysql_client.c b/server/modules/protocol/mysql_client.c index 077ec6918..ac5381df3 100644 --- a/server/modules/protocol/mysql_client.c +++ b/server/modules/protocol/mysql_client.c @@ -1859,8 +1859,10 @@ gw_client_hangup_event(DCB *dcb) } #if defined(SS_DEBUG) LOGIF(LE, (skygw_log_write_flush( - LOGFILE_ERROR, - "Client hangup error handling."))); + LOGFILE_ERROR, + "Client hangup error handling, session state %s, dcb state %s.", + session_state(session->state), + STRDCBSTATE(dcb->state)))); #endif dcb_close(dcb); From d3cdaa43467ae37d55618a45dd4ce1b8c03a8c2e Mon Sep 17 00:00:00 2001 From: counterpoint Date: Fri, 4 Sep 2015 18:09:43 +0100 Subject: [PATCH 32/85] No need to process zombie victims if queue is empty. --- server/core/dcb.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/server/core/dcb.c b/server/core/dcb.c index 7112ef7a3..63a345284 100644 --- a/server/core/dcb.c +++ b/server/core/dcb.c @@ -510,7 +510,10 @@ DCB *dcb = NULL; } spinlock_release(&zombiespin); - dcb_process_victim_queue(listofdcb); + if (listofdcb) + { + dcb_process_victim_queue(listofdcb); + } return zombies; } @@ -527,12 +530,11 @@ DCB *dcb = NULL; static inline void dcb_process_victim_queue(DCB *listofdcb) { - DCB *dcb; + DCB *dcb = listofdcb; - dcb = listofdcb; while (dcb != NULL) { - DCB *nextdcb = NULL; + DCB *nextdcb; /*< * Stop dcb's listening and modify state accordingly. */ @@ -1813,12 +1815,6 @@ dcb_close(DCB *dcb) { CHK_DCB(dcb); - LOGIF(LD, (skygw_log_write(LOGFILE_DEBUG, - "%lu [dcb_close] DCB %p in state %s", - pthread_self(), - dcb, - dcb ? STRDCBSTATE(dcb->state) : "Invalid DCB"))); - if (DCB_STATE_UNDEFINED == dcb->state || DCB_STATE_DISCONNECTED == dcb->state) { From 42c9532a5642134ad10cdbaf36edafe741ffe638 Mon Sep 17 00:00:00 2001 From: Martin Brampton Date: Sat, 5 Sep 2015 00:32:29 +0100 Subject: [PATCH 33/85] Simplify logic and reverse list to kill, so as to cancel out the reversal in the original zombie list. Probably not significant, but might be helpful. --- server/core/dcb.c | 32 ++++++++++---------------------- 1 file changed, 10 insertions(+), 22 deletions(-) diff --git a/server/core/dcb.c b/server/core/dcb.c index 63a345284..7cc647a81 100644 --- a/server/core/dcb.c +++ b/server/core/dcb.c @@ -401,9 +401,9 @@ dcb_final_free(DCB *dcb) DCB * dcb_process_zombies(int threadid) { -DCB *zombiedcb, *previousdcb; +DCB *zombiedcb; +DCB *previousdcb = NULL, *nextdcb; DCB *listofdcb = NULL; -DCB *dcb = NULL; /** * Perform a dirty read to see if there is anything in the queue. @@ -413,7 +413,9 @@ DCB *dcb = NULL; * dcb_final_free. */ if (!zombies) + { return NULL; + } /* * Process the zombie queue and create a list of DCB's that can be @@ -426,11 +428,10 @@ DCB *dcb = NULL; */ spinlock_acquire(&zombiespin); zombiedcb = zombies; - previousdcb = NULL; while (zombiedcb) { CHK_DCB(zombiedcb); - + nextdcb = zombiedcb->memdata.next; /* * Skip processing of DCB's that are * in the event queue waiting to be processed. @@ -438,7 +439,6 @@ DCB *dcb = NULL; if (zombiedcb->evq.next || zombiedcb->evq.prev) { previousdcb = zombiedcb; - zombiedcb = zombiedcb->memdata.next; } else { @@ -484,29 +484,17 @@ DCB *dcb = NULL; * (listofdcb) is not NULL, then it follows that * dcb will also not be null. */ - if (listofdcb == NULL) - { - listofdcb = zombiedcb; - } - else - { - dcb->memdata.next = zombiedcb; - } - /* Set dcb for next iteration of loop */ - dcb = zombiedcb; - zombiedcb = zombiedcb->memdata.next; - /* After we've moved zombiedcb forward, set - link to null as dcb is last of the new list */ - dcb->memdata.next = NULL; + zombiedcb->memdata.next = listofdcb; + listofdcb = zombiedcb; } else { - /* Since we didn't remove this dcb from the zombies - list, we need to advance the previous pointer */ + /* Since we didn't remove this dcb from the zombies + list, we need to advance the previous pointer */ previousdcb = zombiedcb; - zombiedcb = zombiedcb->memdata.next; } } + zombiedcb = nextdcb; } spinlock_release(&zombiespin); From 4a1ad3df6930733435817d642ee5708293d24a18 Mon Sep 17 00:00:00 2001 From: Martin Brampton Date: Sat, 5 Sep 2015 08:53:19 +0100 Subject: [PATCH 34/85] Attempt solution to crash caused by leaving link to backend DCB in router session. --- server/modules/protocol/mysql_backend.c | 1 - server/modules/routing/readconnroute.c | 10 ++++++++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/server/modules/protocol/mysql_backend.c b/server/modules/protocol/mysql_backend.c index 6a024c348..b737d0f58 100644 --- a/server/modules/protocol/mysql_backend.c +++ b/server/modules/protocol/mysql_backend.c @@ -1145,7 +1145,6 @@ gw_backend_hangup(DCB *dcb) spinlock_release(&session->ses_lock); } ss_dassert(dcb->dcb_errhandle_called); - dcb_close(dcb); retblock: return 1; diff --git a/server/modules/routing/readconnroute.c b/server/modules/routing/readconnroute.c index ccb071e1a..a8dfb1f72 100644 --- a/server/modules/routing/readconnroute.c +++ b/server/modules/routing/readconnroute.c @@ -74,6 +74,7 @@ #include #include #include +#include #include #include #include @@ -855,6 +856,7 @@ static void handleError( DCB *client_dcb; SESSION *session = backend_dcb->session; session_state_t sesstate; + ROUTER_CLIENT_SES *router_cli_ses = (ROUTER_CLIENT_SES *)router_session; /** Reset error handle flag from a given DCB */ if (action == ERRACT_RESET) @@ -888,6 +890,14 @@ static void handleError( { spinlock_release(&session->ses_lock); } + + if (backend_dcb != router_cli_ses->backend_dcb) + { + /* Linkages have gone badly wrong - this may not be best solution */ + raise(SIGABRT); + } + router_cli_ses->backend_dcb = NULL; + dcb_close(backend_dcb); /** false because connection is not available anymore */ *succp = false; From 986c918d52cdeba5b92685371482705b3708a982 Mon Sep 17 00:00:00 2001 From: counterpoint Date: Wed, 9 Sep 2015 08:31:59 +0100 Subject: [PATCH 35/85] Remove ERRACT_RESET action from router error handler; remove sole call from mysql_client. Correct comments on parameters for router error handlers. --- server/include/router.h | 3 +-- server/modules/protocol/mysql_client.c | 2 +- server/modules/routing/binlog/blr.c | 10 ++-------- server/modules/routing/maxinfo/maxinfo.c | 10 ++-------- server/modules/routing/readconnroute.c | 10 ++-------- server/modules/routing/readwritesplit/readwritesplit.c | 5 ++--- server/modules/routing/schemarouter/schemarouter.c | 9 +++------ server/modules/routing/schemarouter/shardrouter.c | 8 +++----- 8 files changed, 16 insertions(+), 41 deletions(-) diff --git a/server/include/router.h b/server/include/router.h index 3815253bf..4faa2ca81 100644 --- a/server/include/router.h +++ b/server/include/router.h @@ -45,8 +45,7 @@ typedef void *ROUTER; typedef enum error_action { ERRACT_NEW_CONNECTION = 0x001, - ERRACT_REPLY_CLIENT = 0x002, - ERRACT_RESET = 0x004 + ERRACT_REPLY_CLIENT = 0x002 } error_action_t; /** diff --git a/server/modules/protocol/mysql_client.c b/server/modules/protocol/mysql_client.c index ac5381df3..6d0561abd 100644 --- a/server/modules/protocol/mysql_client.c +++ b/server/modules/protocol/mysql_client.c @@ -1130,7 +1130,7 @@ int gw_read_client_event( else { /** Reset error handler when routing of the new query begins */ - router->handleError(NULL, NULL, NULL, dcb, ERRACT_RESET, NULL); + dcb->dcb_errhandle_called = false; if (stmt_input) { diff --git a/server/modules/routing/binlog/blr.c b/server/modules/routing/binlog/blr.c index 630665628..4d3e356f5 100644 --- a/server/modules/routing/binlog/blr.c +++ b/server/modules/routing/binlog/blr.c @@ -1072,8 +1072,8 @@ int len; * @param router_session The router session * @param message The error message to reply * @param backend_dcb The backend DCB - * @param action The action: REPLY, REPLY_AND_CLOSE, NEW_CONNECTION - * @param succp Result of action + * @param action The action: ERRACT_NEW_CONNECTION or ERRACT_REPLY_CLIENT + * @param succp Result of action: true iff router can continue * */ static void @@ -1084,12 +1084,6 @@ int error; socklen_t len; char msg[85], *errmsg; - if (action == ERRACT_RESET) - { - backend_dcb->dcb_errhandle_called = false; - return; - } - /** Don't handle same error twice on same DCB */ if (backend_dcb->dcb_errhandle_called) { diff --git a/server/modules/routing/maxinfo/maxinfo.c b/server/modules/routing/maxinfo/maxinfo.c index 386a4c850..8ac6ce8cc 100644 --- a/server/modules/routing/maxinfo/maxinfo.c +++ b/server/modules/routing/maxinfo/maxinfo.c @@ -291,7 +291,8 @@ static void freeSession( * @param router_session The router session * @param message The error message to reply * @param backend_dcb The backend DCB - * @param action The action: REPLY, REPLY_AND_CLOSE, NEW_CONNECTION + * @param action The action: ERRACT_NEW_CONNECTION or ERRACT_REPLY_CLIENT + * @param succp Result of action: true iff router can continue * */ static void handleError( @@ -307,13 +308,6 @@ static void handleError( SESSION *session = backend_dcb->session; session_state_t sesstate; - /** Reset error handle flag from a given DCB */ - if (action == ERRACT_RESET) - { - backend_dcb->dcb_errhandle_called = false; - return; - } - /** Don't handle same error twice on same DCB */ if (backend_dcb->dcb_errhandle_called) { diff --git a/server/modules/routing/readconnroute.c b/server/modules/routing/readconnroute.c index a8dfb1f72..3be509444 100644 --- a/server/modules/routing/readconnroute.c +++ b/server/modules/routing/readconnroute.c @@ -841,7 +841,8 @@ clientReply( * @param router_session The router session * @param message The error message to reply * @param backend_dcb The backend DCB - * @param action The action: REPLY, REPLY_AND_CLOSE, NEW_CONNECTION + * @param action The action: ERRACT_NEW_CONNECTION or ERRACT_REPLY_CLIENT + * @param succp Result of action: true if router can continue * */ static void handleError( @@ -858,13 +859,6 @@ static void handleError( session_state_t sesstate; ROUTER_CLIENT_SES *router_cli_ses = (ROUTER_CLIENT_SES *)router_session; - /** Reset error handle flag from a given DCB */ - if (action == ERRACT_RESET) - { - backend_dcb->dcb_errhandle_called = false; - return; - } - /** Don't handle same error twice on same DCB */ if (backend_dcb->dcb_errhandle_called) { diff --git a/server/modules/routing/readwritesplit/readwritesplit.c b/server/modules/routing/readwritesplit/readwritesplit.c index 417abdfe7..7f014bf55 100644 --- a/server/modules/routing/readwritesplit/readwritesplit.c +++ b/server/modules/routing/readwritesplit/readwritesplit.c @@ -4805,9 +4805,8 @@ static void rwsplit_process_router_options( * @param router_session The router session * @param errmsgbuf The error message to reply * @param backend_dcb The backend DCB - * @param action The action: REPLY, REPLY_AND_CLOSE, NEW_CONNECTION - * @param succp Result of action. True if there is at least master - * and enough slaves to continue session. Otherwise false. + * @param action The action: ERRACT_NEW_CONNECTION or ERRACT_REPLY_CLIENT + * @param succp Result of action: true iff router can continue * * Even if succp == true connecting to new slave may have failed. succp is to * tell whether router has enough master/slave connections to continue work. diff --git a/server/modules/routing/schemarouter/schemarouter.c b/server/modules/routing/schemarouter/schemarouter.c index d0c6ad59c..a058db7e7 100644 --- a/server/modules/routing/schemarouter/schemarouter.c +++ b/server/modules/routing/schemarouter/schemarouter.c @@ -4096,8 +4096,8 @@ return_succp: * @param router_session The router session * @param errmsgbuf The error message to reply * @param backend_dcb The backend DCB - * @param action The action: REPLY, REPLY_AND_CLOSE, NEW_CONNECTION - * @param succp Result of action. + * @param action The action: ERRACT_NEW_CONNECTION or ERRACT_REPLY_CLIENT + * @param succp Result of action: true iff router can continue * * Even if succp == true connecting to new slave may have failed. succp is to * tell whether router has enough master/slave connections to continue work. @@ -4115,10 +4115,7 @@ static void handleError ( ROUTER_CLIENT_SES* rses = (ROUTER_CLIENT_SES *)router_session; CHK_DCB(backend_dcb); - if(succp == NULL || action == ERRACT_RESET) - { - return; - } + /** Don't handle same error twice on same DCB */ if (backend_dcb->dcb_errhandle_called) { diff --git a/server/modules/routing/schemarouter/shardrouter.c b/server/modules/routing/schemarouter/shardrouter.c index 1122b0b5c..ab4a6a0e0 100644 --- a/server/modules/routing/schemarouter/shardrouter.c +++ b/server/modules/routing/schemarouter/shardrouter.c @@ -2791,8 +2791,8 @@ return_succp: * @param router_session The router session * @param errmsgbuf The error message to reply * @param backend_dcb The backend DCB - * @param action The action: REPLY, REPLY_AND_CLOSE, NEW_CONNECTION - * @param succp Result of action. + * @param action The action: ERRACT_NEW_CONNECTION or ERRACT_REPLY_CLIENT + * @param succp Result of action: true if router can continue * * Even if succp == true connecting to new slave may have failed. succp is to * tell whether router has enough master/slave connections to continue work. @@ -2809,10 +2809,8 @@ handleError( SESSION* session; ROUTER_CLIENT_SES* rses = (ROUTER_CLIENT_SES *) router_session; - if(action == ERRACT_RESET) - return; - CHK_DCB(backend_dcb); + /** Don't handle same error twice on same DCB */ if(backend_dcb->dcb_errhandle_called) { From 2e50dfd4840755381d283448959362361a9e7589 Mon Sep 17 00:00:00 2001 From: counterpoint Date: Wed, 9 Sep 2015 08:37:40 +0100 Subject: [PATCH 36/85] Readjust indentation in handleError function of read connection router. --- server/modules/routing/readconnroute.c | 70 +++++++++++++------------- 1 file changed, 35 insertions(+), 35 deletions(-) diff --git a/server/modules/routing/readconnroute.c b/server/modules/routing/readconnroute.c index 3be509444..b94d164e3 100644 --- a/server/modules/routing/readconnroute.c +++ b/server/modules/routing/readconnroute.c @@ -846,44 +846,44 @@ clientReply( * */ static void handleError( - ROUTER *instance, - void *router_session, - GWBUF *errbuf, - DCB *backend_dcb, - error_action_t action, - bool *succp) + ROUTER *instance, + void *router_session, + GWBUF *errbuf, + DCB *backend_dcb, + error_action_t action, + bool *succp) { - DCB *client_dcb; - SESSION *session = backend_dcb->session; - session_state_t sesstate; + DCB *client_dcb; + SESSION *session = backend_dcb->session; + session_state_t sesstate; ROUTER_CLIENT_SES *router_cli_ses = (ROUTER_CLIENT_SES *)router_session; - /** Don't handle same error twice on same DCB */ - if (backend_dcb->dcb_errhandle_called) - { - /** we optimistically assume that previous call succeed */ - *succp = true; - return; - } - else - { - backend_dcb->dcb_errhandle_called = true; - } - spinlock_acquire(&session->ses_lock); - sesstate = session->state; - client_dcb = session->client; + /** Don't handle same error twice on same DCB */ + if (backend_dcb->dcb_errhandle_called) + { + /** we optimistically assume that previous call succeed */ + *succp = true; + return; + } + else + { + backend_dcb->dcb_errhandle_called = true; + } + spinlock_acquire(&session->ses_lock); + sesstate = session->state; + client_dcb = session->client; - if (sesstate == SESSION_STATE_ROUTER_READY) - { - CHK_DCB(client_dcb); - spinlock_release(&session->ses_lock); - client_dcb->func.write(client_dcb, gwbuf_clone(errbuf)); - } - else - { - spinlock_release(&session->ses_lock); - } + if (sesstate == SESSION_STATE_ROUTER_READY) + { + CHK_DCB(client_dcb); + spinlock_release(&session->ses_lock); + client_dcb->func.write(client_dcb, gwbuf_clone(errbuf)); + } + else + { + spinlock_release(&session->ses_lock); + } if (backend_dcb != router_cli_ses->backend_dcb) { @@ -893,8 +893,8 @@ static void handleError( router_cli_ses->backend_dcb = NULL; dcb_close(backend_dcb); - /** false because connection is not available anymore */ - *succp = false; + /** false because connection is not available anymore */ + *succp = false; } /** to be inline'd */ From f6916a23bd2a847fca6d4536d7b489ce7fd3a266 Mon Sep 17 00:00:00 2001 From: counterpoint Date: Wed, 9 Sep 2015 09:33:00 +0100 Subject: [PATCH 37/85] Move responsibility for closing DCB on error to router error handling. Check that routers remove or disable links to closed DCB. --- server/core/dcb.c | 1 + server/core/poll.c | 3 +- server/modules/protocol/mysql_backend.c | 8 --- server/modules/protocol/mysql_client.c | 4 +- server/modules/routing/binlog/blr.c | 3 +- server/modules/routing/maxinfo/maxinfo.c | 2 + server/modules/routing/readconnroute.c | 3 +- .../routing/readwritesplit/readwritesplit.c | 28 +++++---- .../routing/schemarouter/schemarouter.c | 60 +++++++++--------- .../routing/schemarouter/shardrouter.c | 62 ++++++++++--------- 10 files changed, 89 insertions(+), 85 deletions(-) diff --git a/server/core/dcb.c b/server/core/dcb.c index 7cc647a81..1c3e08338 100644 --- a/server/core/dcb.c +++ b/server/core/dcb.c @@ -55,6 +55,7 @@ * fixes for various error situations, * remove dcb_set_state etc, simplifications. * 10/07/2015 Martin Brampton Simplify, merge dcb_read and dcb_read_n + * 04/09/2015 Martin Brampton Changes to ensure DCB always has session pointer * * @endverbatim */ diff --git a/server/core/poll.c b/server/core/poll.c index 15e31d313..f3ee8528b 100644 --- a/server/core/poll.c +++ b/server/core/poll.c @@ -74,8 +74,7 @@ int max_poll_sleep; * thread utilisation and fairer scheduling of the event * processing. * 07/07/15 Martin Brampton Simplified add and remove DCB, improve error handling. - * 23/08/15 Martin Brampton Provisionally added test so only DCB with a - * session link can be added to the poll list + * 23/08/15 Martin Brampton Added test so only DCB with a session link can be added to the poll list * * @endverbatim */ diff --git a/server/modules/protocol/mysql_backend.c b/server/modules/protocol/mysql_backend.c index b737d0f58..a75330f16 100644 --- a/server/modules/protocol/mysql_backend.c +++ b/server/modules/protocol/mysql_backend.c @@ -415,7 +415,6 @@ static int gw_read_backend_event(DCB *dcb) { session->state = SESSION_STATE_STOPPING; spinlock_release(&session->ses_lock); ss_dassert(dcb->dcb_errhandle_called); - dcb_close(dcb); rc = 1; goto return_rc; } @@ -488,8 +487,6 @@ static int gw_read_backend_event(DCB *dcb) { session->state = SESSION_STATE_STOPPING; spinlock_release(&session->ses_lock); } - ss_dassert(dcb->dcb_errhandle_called); - dcb_close(dcb); rc = 0; goto return_rc; } @@ -912,8 +909,6 @@ static int gw_error_backend_event(DCB *dcb) session->state = SESSION_STATE_STOPPING; spinlock_release(&session->ses_lock); } - ss_dassert(dcb->dcb_errhandle_called); - dcb_close(dcb); retblock: return 1; @@ -1144,7 +1139,6 @@ gw_backend_hangup(DCB *dcb) session->state = SESSION_STATE_STOPPING; spinlock_release(&session->ses_lock); } - ss_dassert(dcb->dcb_errhandle_called); retblock: return 1; @@ -1324,8 +1318,6 @@ static int backend_write_delayqueue(DCB *dcb) spinlock_acquire(&session->ses_lock); session->state = SESSION_STATE_STOPPING; spinlock_release(&session->ses_lock); - ss_dassert(dcb->dcb_errhandle_called); - dcb_close(dcb); } } } diff --git a/server/modules/protocol/mysql_client.c b/server/modules/protocol/mysql_client.c index 6d0561abd..845e6eb63 100644 --- a/server/modules/protocol/mysql_client.c +++ b/server/modules/protocol/mysql_client.c @@ -39,6 +39,8 @@ * 10/11/2014 Massimiliano Pinto Added: client charset added to protocol struct * 29/05/2015 Markus Makela Added SSL support * 11/06/2015 Martin Brampton COM_QUIT suppressed for persistent connections + * 04/09/2015 Martin Brampton Introduce DUMMY session to fulfill guarantee DCB always has session + * 09/09/2015 Martin Brampton Modify error handler calls */ #include #include @@ -813,7 +815,6 @@ int gw_read_client_event( LOGFILE_ERROR, "Error : Routing the query failed. " "Session will be closed."))); - dcb_close(dcb); } rc = 1; goto return_rc; @@ -1192,7 +1193,6 @@ int gw_read_client_event( "Error : Routing the query failed. " "Session will be closed."))); - dcb_close(dcb); } } } diff --git a/server/modules/routing/binlog/blr.c b/server/modules/routing/binlog/blr.c index 4d3e356f5..bbc947e48 100644 --- a/server/modules/routing/binlog/blr.c +++ b/server/modules/routing/binlog/blr.c @@ -37,7 +37,7 @@ * 18/02/2015 Massimiliano Pinto Addition of dcb_close in closeSession * 07/05/2015 Massimiliano Pinto Addition of MariaDB 10 compatibility support * 12/06/2015 Massimiliano Pinto Addition of MariaDB 10 events in diagnostics() - + * 09/09/2015 Martin Brampton Modify error handler * * @endverbatim */ @@ -1114,6 +1114,7 @@ char msg[85], *errmsg; if (errmsg) free(errmsg); *succp = true; + dcb_close(backend_dcb); LOGIF(LM, (skygw_log_write_flush( LOGFILE_MESSAGE, "%s: Master %s disconnected after %ld seconds. " diff --git a/server/modules/routing/maxinfo/maxinfo.c b/server/modules/routing/maxinfo/maxinfo.c index 8ac6ce8cc..43a43e746 100644 --- a/server/modules/routing/maxinfo/maxinfo.c +++ b/server/modules/routing/maxinfo/maxinfo.c @@ -26,6 +26,7 @@ * Date Who Description * 16/02/15 Mark Riddoch Initial implementation * 27/02/15 Massimiliano Pinto Added maxinfo_add_mysql_user + * 09/09/2015 Martin Brampton Modify error handler * * @endverbatim */ @@ -335,6 +336,7 @@ static void handleError( } /** false because connection is not available anymore */ + dcb_close(backend_dcb); *succp = false; } diff --git a/server/modules/routing/readconnroute.c b/server/modules/routing/readconnroute.c index b94d164e3..3db858932 100644 --- a/server/modules/routing/readconnroute.c +++ b/server/modules/routing/readconnroute.c @@ -67,7 +67,8 @@ * 06/03/2014 Massimiliano Pinto Server connection counter is now updated in closeSession * 24/06/2014 Massimiliano Pinto New rules for selecting the Master server * 27/06/2014 Mark Riddoch Addition of server weighting - * 11/06/2015 Martin Brampton Remove decrement n_current (moved to dcb.c) + * 11/06/2015 Martin Brampton Remove decrement n_current (moved to dcb.c) + * 09/09/2015 Martin Brampton Modify error handler * * @endverbatim */ diff --git a/server/modules/routing/readwritesplit/readwritesplit.c b/server/modules/routing/readwritesplit/readwritesplit.c index 7f014bf55..cfecd53dd 100644 --- a/server/modules/routing/readwritesplit/readwritesplit.c +++ b/server/modules/routing/readwritesplit/readwritesplit.c @@ -67,6 +67,8 @@ extern __thread log_info_t tls_log_info; * 18/07/2013 Massimiliano Pinto routeQuery now handles COM_QUIT * as QUERY_TYPE_SESSION_WRITE * 17/07/2014 Massimiliano Pinto Server connection counter is updated in closeSession + * + * 09/09/2015 Martin Brampton Modify error handler * * @endverbatim */ @@ -4825,17 +4827,14 @@ static void handleError ( CHK_DCB(backend_dcb); - /** Reset error handle flag from a given DCB */ - if (action == ERRACT_RESET) - { - backend_dcb->dcb_errhandle_called = false; - return; - } - /** Don't handle same error twice on same DCB */ if (backend_dcb->dcb_errhandle_called) { - /** we optimistically assume that previous call succeed */ + /** we optimistically assume that previous call succeed */ + /* + * The return of true is potentially misleading, but appears to + * be safe with the code as it stands on 9 Sept 2015 - MNB + */ *succp = true; return; } @@ -4848,12 +4847,13 @@ static void handleError ( if (session == NULL || rses == NULL) { *succp = false; - return; } - CHK_SESSION(session); - CHK_CLIENT_RSES(rses); + else + { + CHK_SESSION(session); + CHK_CLIENT_RSES(rses); - switch (action) { + switch (action) { case ERRACT_NEW_CONNECTION: { SERVER* srv; @@ -4861,7 +4861,7 @@ static void handleError ( if (!rses_begin_locked_router_action(rses)) { *succp = false; - return; + break; } srv = rses->rses_master_ref->bref_backend->backend_server; /** @@ -4914,7 +4914,9 @@ static void handleError ( default: *succp = false; break; + } } + dcb_close(backend_dcb); } diff --git a/server/modules/routing/schemarouter/schemarouter.c b/server/modules/routing/schemarouter/schemarouter.c index a058db7e7..67875056a 100644 --- a/server/modules/routing/schemarouter/schemarouter.c +++ b/server/modules/routing/schemarouter/schemarouter.c @@ -58,6 +58,7 @@ extern __thread log_info_t tls_log_info; * * Date Who Description * 01/12/2014 Vilho Raatikka/Markus Mäkelä Initial implementation + * 09/09/2015 Martin Brampton Modify error handler * * @endverbatim */ @@ -4103,37 +4104,38 @@ return_succp: * tell whether router has enough master/slave connections to continue work. */ static void handleError ( - ROUTER* instance, - void* router_session, - GWBUF* errmsgbuf, - DCB* backend_dcb, - error_action_t action, - bool* succp) + ROUTER* instance, + void* router_session, + GWBUF* errmsgbuf, + DCB* backend_dcb, + error_action_t action, + bool* succp) { - SESSION* session; - ROUTER_INSTANCE* inst = (ROUTER_INSTANCE *)instance; - ROUTER_CLIENT_SES* rses = (ROUTER_CLIENT_SES *)router_session; + SESSION* session; + ROUTER_INSTANCE* inst = (ROUTER_INSTANCE *)instance; + ROUTER_CLIENT_SES* rses = (ROUTER_CLIENT_SES *)router_session; - CHK_DCB(backend_dcb); + CHK_DCB(backend_dcb); - /** Don't handle same error twice on same DCB */ - if (backend_dcb->dcb_errhandle_called) - { - /** we optimistically assume that previous call succeed */ - *succp = true; - return; - } - else - { - backend_dcb->dcb_errhandle_called = true; - } - session = backend_dcb->session; + /** Don't handle same error twice on same DCB */ + if (backend_dcb->dcb_errhandle_called) + { + /** we optimistically assume that previous call succeed */ + *succp = true; + return; + } + else + { + backend_dcb->dcb_errhandle_called = true; + } + session = backend_dcb->session; - if (session == NULL || rses == NULL) - { - *succp = false; - return; - } + if (session == NULL || rses == NULL) + { + *succp = false; + } + else + { CHK_SESSION(session); CHK_CLIENT_RSES(rses); @@ -4143,7 +4145,7 @@ static void handleError ( if (!rses_begin_locked_router_action(rses)) { *succp = false; - return; + break; } /** * This is called in hope of getting replacement for @@ -4171,6 +4173,8 @@ static void handleError ( *succp = false; break; } + } + dcb_close(backend_dcb); } diff --git a/server/modules/routing/schemarouter/shardrouter.c b/server/modules/routing/schemarouter/shardrouter.c index ab4a6a0e0..e01d37eae 100644 --- a/server/modules/routing/schemarouter/shardrouter.c +++ b/server/modules/routing/schemarouter/shardrouter.c @@ -63,6 +63,7 @@ extern __thread log_info_t tls_log_info; * * Date Who Description * 20/01/2015 Markus Mäkelä/Vilho Raatikka Initial implementation + * 09/09/2015 Martin Brampton Modify error handler * * @endverbatim */ @@ -2826,38 +2827,39 @@ handleError( if(session == NULL || rses == NULL) { - if(succp) - *succp = false; - return; - } - CHK_SESSION(session); - CHK_CLIENT_RSES(rses); - - switch(action) - { - case ERRACT_NEW_CONNECTION: - { - if(!rses_begin_locked_router_action(rses)) - { - *succp = false; - return; - } - - rses_end_locked_router_action(rses); - break; - } - - case ERRACT_REPLY_CLIENT: - { - - *succp = false; /*< no new backend servers were made available */ - break; - } - - default: *succp = false; - break; } + else + { + CHK_SESSION(session); + CHK_CLIENT_RSES(rses); + + switch(action) + { + case ERRACT_NEW_CONNECTION: + { + if(!rses_begin_locked_router_action(rses)) + { + *succp = false; + break; + } + + rses_end_locked_router_action(rses); + break; + } + + case ERRACT_REPLY_CLIENT: + { + *succp = false; /*< no new backend servers were made available */ + break; + } + + default: + *succp = false; + break; + } + } + dcb_close(backend_dcb); } From 296e306daa2b0fe8ee8b8c7b883c22e611594c64 Mon Sep 17 00:00:00 2001 From: counterpoint Date: Wed, 9 Sep 2015 16:58:03 +0100 Subject: [PATCH 38/85] Set session pointer to client dcb to null when dcb is closed. --- server/modules/protocol/mysql_backend.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/server/modules/protocol/mysql_backend.c b/server/modules/protocol/mysql_backend.c index a75330f16..4d36351e1 100644 --- a/server/modules/protocol/mysql_backend.c +++ b/server/modules/protocol/mysql_backend.c @@ -1190,10 +1190,13 @@ gw_backend_close(DCB *dcb) { if (session->client->state == DCB_STATE_POLLING) { + DCB *temp; spinlock_release(&session->ses_lock); /** Close client DCB */ - dcb_close(session->client); + temp = session->client; + session->client = NULL; + dcb_close(temp); } else { From c1194a5ee8871346fac5f64c010066596d034403 Mon Sep 17 00:00:00 2001 From: Markus Makela Date: Thu, 10 Sep 2015 18:04:55 +0300 Subject: [PATCH 39/85] Fixed test build failures. --- log_manager/test/CMakeLists.txt | 6 +++--- query_classifier/test/CMakeLists.txt | 2 +- query_classifier/test/canonical_tests/CMakeLists.txt | 2 +- server/core/CMakeLists.txt | 2 +- server/core/test/CMakeLists.txt | 10 +++++----- 5 files changed, 11 insertions(+), 11 deletions(-) diff --git a/log_manager/test/CMakeLists.txt b/log_manager/test/CMakeLists.txt index bbcfd5791..6018e746d 100644 --- a/log_manager/test/CMakeLists.txt +++ b/log_manager/test/CMakeLists.txt @@ -1,5 +1,5 @@ add_executable(testlog testlog.c) -add_executable(testorder testorder.c) -target_link_libraries(testlog pthread log_manager utils) -target_link_libraries(testorder pthread log_manager utils) +add_executable(testorder testorder.c ../../server/core/random_jkiss.c) +target_link_libraries(testlog pthread log_manager utils fullcore) +target_link_libraries(testorder pthread log_manager utils fullcore) add_test(NAME Internal-TestLogOrder COMMAND ${CMAKE_CURRENT_SOURCE_DIR}/logorder.sh 200 0 1000 ${CMAKE_CURRENT_BINARY_DIR}/logorder.log) diff --git a/query_classifier/test/CMakeLists.txt b/query_classifier/test/CMakeLists.txt index 44b66e461..66c56eca7 100644 --- a/query_classifier/test/CMakeLists.txt +++ b/query_classifier/test/CMakeLists.txt @@ -10,5 +10,5 @@ endif() add_subdirectory(canonical_tests) add_executable(classify classify.c) -target_link_libraries(classify query_classifier fullcore) +target_link_libraries(classify query_classifier ${CURL_LIBRARIES} utils log_manager pthread ${EMBEDDED_LIB} ${PCRE_LINK_FLAGS} ssl aio rt crypt dl crypto inih z m stdc++ fullcore) add_test(Internal-TestQueryClassifier classify ${CMAKE_CURRENT_SOURCE_DIR}/input.sql ${CMAKE_CURRENT_SOURCE_DIR}/expected.sql) diff --git a/query_classifier/test/canonical_tests/CMakeLists.txt b/query_classifier/test/canonical_tests/CMakeLists.txt index 1dafc428e..925a3dd4f 100644 --- a/query_classifier/test/canonical_tests/CMakeLists.txt +++ b/query_classifier/test/canonical_tests/CMakeLists.txt @@ -7,7 +7,7 @@ else() file(COPY ${ERRMSG} DESTINATION ${CMAKE_CURRENT_BINARY_DIR}) endif() endif() -add_executable(canonizer canonizer.c) +add_executable(canonizer canonizer.c ${CMAKE_SOURCE_DIR}/server/core/random_jkiss.c) target_link_libraries(canonizer pthread query_classifier z dl ssl aio crypt crypto rt m ${EMBEDDED_LIB} fullcore stdc++) add_test(NAME Internal-TestCanonicalQuery COMMAND ${CMAKE_CURRENT_SOURCE_DIR}/canontest.sh ${CMAKE_CURRENT_BINARY_DIR}/test.log diff --git a/server/core/CMakeLists.txt b/server/core/CMakeLists.txt index 5ad84ce59..0f0b00884 100644 --- a/server/core/CMakeLists.txt +++ b/server/core/CMakeLists.txt @@ -1,5 +1,5 @@ if(BUILD_TESTS OR BUILD_TOOLS) - add_library(fullcore STATIC adminusers.c atomic.c config.c buffer.c dbusers.c dcb.c filter.c gwbitmask.c gw_utils.c hashtable.c hint.c housekeeper.c load_utils.c memlog.c modutil.c monitor.c poll.c resultset.c secrets.c server.c service.c session.c spinlock.c thread.c users.c utils.c gwdirs.c externcmd.c random_jkiss.c) + add_library(fullcore STATIC random_jkiss.c adminusers.c atomic.c config.c buffer.c dbusers.c dcb.c filter.c gwbitmask.c gw_utils.c hashtable.c hint.c housekeeper.c load_utils.c memlog.c modutil.c monitor.c poll.c resultset.c secrets.c server.c service.c session.c spinlock.c thread.c users.c utils.c gwdirs.c externcmd.c) if(WITH_JEMALLOC) target_link_libraries(fullcore ${JEMALLOC_LIBRARIES}) elseif(WITH_TCMALLOC) diff --git a/server/core/test/CMakeLists.txt b/server/core/test/CMakeLists.txt index 2d32d55b1..0c9f4201f 100644 --- a/server/core/test/CMakeLists.txt +++ b/server/core/test/CMakeLists.txt @@ -1,10 +1,10 @@ execute_process(COMMAND ${CMAKE_COMMAND} -E copy ${ERRMSG} ${CMAKE_CURRENT_BINARY_DIR}) add_executable(test_mysql_users test_mysql_users.c) -add_executable(test_hash testhash.c) -add_executable(test_hint testhint.c) -add_executable(test_spinlock testspinlock.c) +add_executable(test_hash testhash.c ../random_jkiss.c) +add_executable(test_hint testhint.c ../random_jkiss.c) +add_executable(test_spinlock testspinlock.c ../random_jkiss.c) add_executable(test_filter testfilter.c) -add_executable(test_buffer testbuffer.c) +add_executable(test_buffer testbuffer.c ../random_jkiss.c) add_executable(test_dcb testdcb.c) add_executable(test_modutil testmodutil.c) add_executable(test_poll testpoll.c) @@ -12,7 +12,7 @@ add_executable(test_service testservice.c) add_executable(test_server testserver.c) add_executable(test_users testusers.c) add_executable(test_adminusers testadminusers.c) -add_executable(testmemlog testmemlog.c) +add_executable(testmemlog testmemlog.c ../random_jkiss.c) add_executable(testfeedback testfeedback.c) target_link_libraries(test_mysql_users MySQLClient fullcore) target_link_libraries(test_hash fullcore log_manager) From 0cf4b2cf68766637a85d96139cc0f7a144e7bb2f Mon Sep 17 00:00:00 2001 From: Martin Brampton Date: Tue, 15 Sep 2015 08:37:41 +0100 Subject: [PATCH 40/85] Fix to overcome failure on certain packets. --- server/modules/routing/binlog/blr_master.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/server/modules/routing/binlog/blr_master.c b/server/modules/routing/binlog/blr_master.c index 0a80a4d54..3001b2cef 100644 --- a/server/modules/routing/binlog/blr_master.c +++ b/server/modules/routing/binlog/blr_master.c @@ -1480,6 +1480,14 @@ char *rval; // Finally we have reached the row len = EXTRACT24(ptr); ptr += 4; + + /** The first EOF packet signals the start of the resultset rows and the second + EOF packet signals the end of the result set. If the resultset + contains a second EOF packet right after the first one, the result set is empty and + contains no rows. */ + if(len == 5 && *ptr == 0xfe) + return NULL; + while (--col > 0) { collen = *ptr++; From fdbe070e802077815e0f8c0d5b5583522176314a Mon Sep 17 00:00:00 2001 From: counterpoint Date: Tue, 15 Sep 2015 15:22:44 +0100 Subject: [PATCH 41/85] Change abort to error message when read connection router finds mismatch between router client session DCB and given backend DCB; improve order of actions when closing DCB in read-write router. --- server/modules/routing/readconnroute.c | 21 ++++++++++++------- .../routing/readwritesplit/readwritesplit.c | 2 +- 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/server/modules/routing/readconnroute.c b/server/modules/routing/readconnroute.c index 3db858932..e32bdd677 100644 --- a/server/modules/routing/readconnroute.c +++ b/server/modules/routing/readconnroute.c @@ -885,14 +885,21 @@ static void handleError( { spinlock_release(&session->ses_lock); } - - if (backend_dcb != router_cli_ses->backend_dcb) - { - /* Linkages have gone badly wrong - this may not be best solution */ - raise(SIGABRT); + + if (router_cli_ses->backend_dcb) { + if (backend_dcb != router_cli_ses->backend_dcb) + { + /* Linkages have gone badly wrong */ + LOGIF(LE, (skygw_log_write(LOGFILE_ERROR, + "Read Connection Router error in handleError: router client " + "session DCB %p is not null, but does not match backend DCB %p " + "either. \n", + router_cli_ses->backend_dcb, + backend_dcb))); + } + router_cli_ses->backend_dcb = NULL; + dcb_close(backend_dcb); } - router_cli_ses->backend_dcb = NULL; - dcb_close(backend_dcb); /** false because connection is not available anymore */ *succp = false; diff --git a/server/modules/routing/readwritesplit/readwritesplit.c b/server/modules/routing/readwritesplit/readwritesplit.c index 0ad00886c..6b9dce74c 100644 --- a/server/modules/routing/readwritesplit/readwritesplit.c +++ b/server/modules/routing/readwritesplit/readwritesplit.c @@ -3655,10 +3655,10 @@ static bool select_connect_backend_servers( ss_dassert(backend_ref[i].bref_backend->backend_conn_count > 0); /** disconnect opened connections */ - dcb_close(backend_ref[i].bref_dcb); bref_clear_state(&backend_ref[i], BREF_IN_USE); /** Decrease backend's connection counter. */ atomic_add(&backend_ref[i].bref_backend->backend_conn_count, -1); + dcb_close(backend_ref[i].bref_dcb); } } } From 30239f395a5650efe9fc65812ed8673e16af2206 Mon Sep 17 00:00:00 2001 From: Martin Brampton Date: Tue, 15 Sep 2015 20:07:56 +0100 Subject: [PATCH 42/85] Fix bref when backend server fails, error message if fails. --- .../routing/readwritesplit/readwritesplit.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/server/modules/routing/readwritesplit/readwritesplit.c b/server/modules/routing/readwritesplit/readwritesplit.c index 6b9dce74c..ee1d6fb50 100644 --- a/server/modules/routing/readwritesplit/readwritesplit.c +++ b/server/modules/routing/readwritesplit/readwritesplit.c @@ -4871,6 +4871,24 @@ static void handleError ( if (rses->rses_master_ref->bref_dcb == backend_dcb && !SERVER_IS_MASTER(srv)) { + backend_ref_t* bref; + bref = get_bref_from_dcb(rses, backend_dcb); + if (bref != NULL) + { + CHK_BACKEND_REF(bref); + bref_clear_state(bref, BREF_IN_USE); + bref_set_state(bref, BREF_CLOSED); + } + else + { + LOGIF(LE, (skygw_log_write_flush( + LOGFILE_ERROR, + "Error : server %s:%d lost the " + "master status but could not locate the " + "corresponding backend ref.", + srv->name, + srv->port))); + } if (!srv->master_err_is_logged) { LOGIF(LE, (skygw_log_write_flush( From 0cba9b797f0a7532b0c4622e4c1804c2c68d1ac2 Mon Sep 17 00:00:00 2001 From: Martin Brampton Date: Thu, 17 Sep 2015 08:15:32 +0100 Subject: [PATCH 43/85] Changes to deal with failed session creation by keeping the new session in existence until all related DCBs have closed; minor changes in response to reviews. --- server/core/random_jkiss.c | 5 +- server/core/session.c | 102 ++++++++++++++++++++------------ server/modules/protocol/httpd.c | 4 -- 3 files changed, 65 insertions(+), 46 deletions(-) diff --git a/server/core/random_jkiss.c b/server/core/random_jkiss.c index 6eea33739..97a09fa34 100644 --- a/server/core/random_jkiss.c +++ b/server/core/random_jkiss.c @@ -100,10 +100,9 @@ random_jkiss_devrand(void) int fn; unsigned int r; if ((fn = open("/dev/urandom", O_RDONLY)) == -1) return 0; - if (read(fn, &r, 4) != 4) + if (read(fn, &r, sizeof(r)) != sizeof(r)) { - close(fn); - return 0; + r = 0; } close(fn); return r; diff --git a/server/core/session.c b/server/core/session.c index c46ffcf9b..dcda5af51 100644 --- a/server/core/session.c +++ b/server/core/session.c @@ -27,6 +27,7 @@ * 02/09/13 Massimiliano Pinto Added session refcounter * 29/05/14 Mark Riddoch Addition of filter mechanism * 23/08/15 Martin Brampton Tidying; slight improvement in safety + * 17/09/15 Martin Brampton Keep failed session in existence - leave DCBs to close * * @endverbatim */ @@ -82,14 +83,30 @@ session_alloc(SERVICE *service, DCB *client_dcb) if (session == NULL) { - char errbuf[STRERROR_BUFLEN]; + char errbuf[STRERROR_BUFLEN]; LOGIF(LE, (skygw_log_write_flush( LOGFILE_ERROR, "Error : Failed to allocate memory for " "session object due error %d, %s.", errno, strerror_r(errno, errbuf, sizeof(errbuf))))); - session_simple_free(session, client_dcb); + /* Does this possibly need a lock? */ + /* + * This is really not the right way to do this. The data in a DCB is + * router specific and should be freed by a function in the relevant + * router. This would be better achieved by placing a function reference + * in the DCB and having dcb_final_free call it to dispose of the data + * at the final destruction of the DCB. However, this piece of code is + * only run following a calloc failure, so the system is probably on + * the point of crashing anyway. + * + */ + if (dcb->data && !DCB_IS_CLONE(dcb)) + { + void * clientdata = dcb->data; + dcb->data = NULL; + free(clientdata); + } return NULL; } #if defined(SS_DEBUG) @@ -135,7 +152,7 @@ session_alloc(SERVICE *service, DCB *client_dcb) session->router_session = service->router->newSession(service->router_instance, session); if (session->router_session == NULL) { - session_simple_free(session, client_dcb); + session->state = SESSION_STATE_TO_BE_FREED; LOGIF(LE, (skygw_log_write_flush( LOGFILE_ERROR, @@ -145,7 +162,6 @@ session_alloc(SERVICE *service, DCB *client_dcb) __func__, service->name))); - return NULL; } /* * Pending filter chain being setup set the head of the chain to @@ -165,53 +181,65 @@ session_alloc(SERVICE *service, DCB *client_dcb) session->tail.session = session; session->tail.clientReply = session_reply; - if (service->n_filters > 0) + if (SESSION_STATE_TO_BE_FREED != session->state + && service->n_filters > 0 + && !session_setup_filters(session)) { - if (!session_setup_filters(session)) - { - session_simple_free(session, client_dcb); + session->state = SESSION_STATE_TO_BE_FREED; LOGIF(LE, (skygw_log_write( LOGFILE_ERROR, "Error : Setting up filters failed. " "Terminating session %s.", service->name))); - return NULL; - } } } - session->state = SESSION_STATE_ROUTER_READY; + if (SESSION_STATE_TO_BE_FREED != session->state) + { + session->state = SESSION_STATE_ROUTER_READY; + + if (session->client->user == NULL) + { + LOGIF(LT, (skygw_log_write( + LOGFILE_TRACE, + "Started session [%lu] for %s service ", + session->ses_id, + service->name))); + } + else + { + LOGIF(LT, (skygw_log_write( + LOGFILE_TRACE, + "Started %s client session [%lu] for '%s' from %s", + service->name, + session->ses_id, + session->client->user, + session->client->remote))); + } + } + else + { + LOGIF(LT, (skygw_log_write( + LOGFILE_TRACE, + "Start %s client session [%lu] for '%s' from %s failed, will be " + "closed as soon as all related DCBs have been closed.", + service->name, + session->ses_id, + session->client->user, + session->client->remote))); + } spinlock_acquire(&session_spin); /** Assign a session id and increase, insert session into list */ session->ses_id = ++session_id; session->next = allSessions; allSessions = session; spinlock_release(&session_spin); - - if (session->client->user == NULL) - { - LOGIF(LT, (skygw_log_write( - LOGFILE_TRACE, - "Started session [%lu] for %s service ", - session->ses_id, - service->name))); - } - else - { - LOGIF(LT, (skygw_log_write( - LOGFILE_TRACE, - "Started %s client session [%lu] for '%s' from %s", - service->name, - session->ses_id, - session->client->user, - session->client->remote))); - } atomic_add(&service->stats.n_sessions, 1); atomic_add(&service->stats.n_current, 1); CHK_SESSION(session); client_dcb->session = session; - return session; + return SESSION_STATE_TO_BE_FREED == session->state ? NULL : session; } /** @@ -240,13 +268,6 @@ session_set_dummy(DCB *client_dcb) memset(&session->stats, 0, sizeof(SESSION_STATS)); session->stats.connect = 0; session->state = SESSION_STATE_DUMMY; - /*< - * Associate the session to the client DCB and set the reference count on - * the session to indicate that there is a single reference to the - * session. There is no need to protect this or use atomic add as the - * session has not been made available to the other threads at this - * point. - */ session->data = NULL; session->refcount = 1; session->ses_id = 0; @@ -352,6 +373,9 @@ int session_unlink_dcb( /** * Deallocate the specified session, minimal actions during session_alloc + * Since changes to keep new session in existence until all related DCBs + * have been destroyed, this function is redundant. Just left until we are + * sure of the direction taken. * * @param session The session to deallocate */ @@ -402,7 +426,7 @@ session_free(SESSION *session) * Remove one reference. If there are no references left, * free session. */ - if (atomic_add(&session->refcount, -1) - 1) + if (atomic_add(&session->refcount, -1) > 1) { /* Must be one or more references left */ return false; diff --git a/server/modules/protocol/httpd.c b/server/modules/protocol/httpd.c index 8a9cf0212..b0c40bc21 100644 --- a/server/modules/protocol/httpd.c +++ b/server/modules/protocol/httpd.c @@ -350,10 +350,6 @@ int n_connect = 0; client->remote = strdup(inet_ntoa(addr.sin_addr)); memcpy(&client->func, &MyObject, sizeof(GWPROTOCOL)); - /* we don't need the session */ - /* But not clear that we have one! */ - /* client->session = NULL; */ - /* create the session data for HTTPD */ client_data = (HTTPD_session *)calloc(1, sizeof(HTTPD_session)); client->data = client_data; From 91dd3bb9bd0eb994ad50345b3ae44534b323ebb7 Mon Sep 17 00:00:00 2001 From: Martin Brampton Date: Thu, 17 Sep 2015 08:18:47 +0100 Subject: [PATCH 44/85] Fix mistake. --- server/core/session.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/server/core/session.c b/server/core/session.c index dcda5af51..e9d45b8e7 100644 --- a/server/core/session.c +++ b/server/core/session.c @@ -101,10 +101,10 @@ session_alloc(SERVICE *service, DCB *client_dcb) * the point of crashing anyway. * */ - if (dcb->data && !DCB_IS_CLONE(dcb)) + if (client_dcb->data && !DCB_IS_CLONE(client_dcb)) { - void * clientdata = dcb->data; - dcb->data = NULL; + void * clientdata = client_dcb->data; + client_dcb->data = NULL; free(clientdata); } return NULL; From 583c9b62feca379224f21adf36521ef155de6487 Mon Sep 17 00:00:00 2001 From: Martin Brampton Date: Thu, 17 Sep 2015 11:58:19 +0100 Subject: [PATCH 45/85] Close DCB in handleError only if it can be found in a backend reference. --- server/modules/routing/readwritesplit/readwritesplit.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/server/modules/routing/readwritesplit/readwritesplit.c b/server/modules/routing/readwritesplit/readwritesplit.c index ee1d6fb50..beb16cb59 100644 --- a/server/modules/routing/readwritesplit/readwritesplit.c +++ b/server/modules/routing/readwritesplit/readwritesplit.c @@ -4878,6 +4878,7 @@ static void handleError ( CHK_BACKEND_REF(bref); bref_clear_state(bref, BREF_IN_USE); bref_set_state(bref, BREF_CLOSED); + dcb_close(backend_dcb); } else { @@ -4934,7 +4935,6 @@ static void handleError ( break; } } - dcb_close(backend_dcb); } @@ -4961,6 +4961,7 @@ static void handle_error_reply_client( CHK_BACKEND_REF(bref); bref_clear_state(bref, BREF_IN_USE); bref_set_state(bref, BREF_CLOSED); + dcb_close(backend_dcb); } if (sesstate == SESSION_STATE_ROUTER_READY) @@ -5048,6 +5049,7 @@ static bool handle_error_new_connection( DCB_REASON_NOT_RESPONDING, &router_handle_state_switch, (void *)bref); + dcb_close(backend_dcb); router_nservers = router_get_servercount(inst); max_nslaves = rses_get_max_slavecount(myrses, router_nservers); From 357c4bcae548875f422abe37f4a22b6748b811f0 Mon Sep 17 00:00:00 2001 From: Martin Brampton Date: Thu, 17 Sep 2015 12:53:59 +0100 Subject: [PATCH 46/85] Add to or take from persistent pool only if server is running; add conditions to DCB close in read-write handleError to check backend reference was in use. --- server/core/dcb.c | 3 +++ .../routing/readwritesplit/readwritesplit.c | 20 ++++++++++++++----- 2 files changed, 18 insertions(+), 5 deletions(-) diff --git a/server/core/dcb.c b/server/core/dcb.c index 508e869f2..1654aec3c 100644 --- a/server/core/dcb.c +++ b/server/core/dcb.c @@ -1881,6 +1881,7 @@ dcb_maybe_add_persistent(DCB *dcb) && strlen(dcb->user) && dcb->server && dcb->server->persistpoolmax + && (dcb->server->status & SERVER_RUNNING) && !dcb->dcb_errhandle_called && !(dcb->flags & DCBF_HUNG) && (poolcount = dcb_persistent_clean_count(dcb, false)) < dcb->server->persistpoolmax) @@ -2815,6 +2816,8 @@ dcb_persistent_clean_count(DCB *dcb, bool cleanall) if (cleanall || persistentdcb-> dcb_errhandle_called || count >= server->persistpoolmax + || persistentdcb->server == NULL + || !(persistentdcb->server->status & SERVER_RUNNING) || (time(NULL) - persistentdcb->persistentstart) > server->persistmaxtime) { /* Remove from persistent pool */ diff --git a/server/modules/routing/readwritesplit/readwritesplit.c b/server/modules/routing/readwritesplit/readwritesplit.c index beb16cb59..c9103fae9 100644 --- a/server/modules/routing/readwritesplit/readwritesplit.c +++ b/server/modules/routing/readwritesplit/readwritesplit.c @@ -4875,10 +4875,12 @@ static void handleError ( bref = get_bref_from_dcb(rses, backend_dcb); if (bref != NULL) { + bool bref_was_in_use = BREF_IS_IN_USE(bref); CHK_BACKEND_REF(bref); bref_clear_state(bref, BREF_IN_USE); bref_set_state(bref, BREF_CLOSED); - dcb_close(backend_dcb); + if (bref_was_in_use) + dcb_close(backend_dcb); } else { @@ -4958,10 +4960,14 @@ static void handle_error_reply_client( */ if ((bref = get_bref_from_dcb(rses, backend_dcb)) != NULL) { + bool bref_was_in_use = BREF_IS_IN_USE(bref); CHK_BACKEND_REF(bref); bref_clear_state(bref, BREF_IN_USE); bref_set_state(bref, BREF_CLOSED); - dcb_close(backend_dcb); + if (bref_was_in_use) + { + dcb_close(backend_dcb); + } } if (sesstate == SESSION_STATE_ROUTER_READY) @@ -4991,13 +4997,14 @@ static bool handle_error_new_connection( DCB* backend_dcb, GWBUF* errmsg) { - ROUTER_CLIENT_SES* myrses; + ROUTER_CLIENT_SES* myrses; SESSION* ses; int router_nservers; int max_nslaves; int max_slave_rlag; backend_ref_t* bref; bool succp; + bool bref_was_in_use; myrses = *rses; ss_dassert(SPINLOCK_IS_LOCKED(&myrses->rses_lock)); @@ -5014,6 +5021,7 @@ static bool handle_error_new_connection( goto return_succp; } CHK_BACKEND_REF(bref); + bref_was_in_use = BREF_IS_IN_USE(bref); /** * If query was sent through the bref and it is waiting for reply from @@ -5049,8 +5057,10 @@ static bool handle_error_new_connection( DCB_REASON_NOT_RESPONDING, &router_handle_state_switch, (void *)bref); - dcb_close(backend_dcb); - + if (bref_was_in_use) + { + dcb_close(backend_dcb); + } router_nservers = router_get_servercount(inst); max_nslaves = rses_get_max_slavecount(myrses, router_nservers); max_slave_rlag = rses_get_max_replication_lag(myrses); From 1ad8e27c919517e6421fb90e0f607fa89e698525 Mon Sep 17 00:00:00 2001 From: Martin Brampton Date: Thu, 17 Sep 2015 13:27:25 +0100 Subject: [PATCH 47/85] Try a different arrangement of DCB closures in handleError of read-write split. --- .../modules/routing/readwritesplit/readwritesplit.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/server/modules/routing/readwritesplit/readwritesplit.c b/server/modules/routing/readwritesplit/readwritesplit.c index c9103fae9..40b9f1ed1 100644 --- a/server/modules/routing/readwritesplit/readwritesplit.c +++ b/server/modules/routing/readwritesplit/readwritesplit.c @@ -4890,7 +4890,8 @@ static void handleError ( "master status but could not locate the " "corresponding backend ref.", srv->name, - srv->port))); + srv->port))); + dcb_close(backend_dcb); } if (!srv->master_err_is_logged) { @@ -4969,6 +4970,10 @@ static void handle_error_reply_client( dcb_close(backend_dcb); } } + else + { + dcb_close(backend_dcb); + } if (sesstate == SESSION_STATE_ROUTER_READY) { @@ -5004,9 +5009,9 @@ static bool handle_error_new_connection( int max_slave_rlag; backend_ref_t* bref; bool succp; - bool bref_was_in_use; + bool bref_was_in_use; - myrses = *rses; + myrses = *rses; ss_dassert(SPINLOCK_IS_LOCKED(&myrses->rses_lock)); ses = backend_dcb->session; @@ -5018,6 +5023,7 @@ static bool handle_error_new_connection( if ((bref = get_bref_from_dcb(myrses, backend_dcb)) == NULL) { succp = true; + dcb_close(backend_dcb); goto return_succp; } CHK_BACKEND_REF(bref); From dc3b0b067b9d97655df0f52cf2ef2b6055e60c2d Mon Sep 17 00:00:00 2001 From: Martin Brampton Date: Thu, 17 Sep 2015 13:35:18 +0100 Subject: [PATCH 48/85] Revert the dcb_close changes in handleError. --- .../routing/readwritesplit/readwritesplit.c | 16 +--------------- 1 file changed, 1 insertion(+), 15 deletions(-) diff --git a/server/modules/routing/readwritesplit/readwritesplit.c b/server/modules/routing/readwritesplit/readwritesplit.c index 40b9f1ed1..46fce6cee 100644 --- a/server/modules/routing/readwritesplit/readwritesplit.c +++ b/server/modules/routing/readwritesplit/readwritesplit.c @@ -4879,8 +4879,6 @@ static void handleError ( CHK_BACKEND_REF(bref); bref_clear_state(bref, BREF_IN_USE); bref_set_state(bref, BREF_CLOSED); - if (bref_was_in_use) - dcb_close(backend_dcb); } else { @@ -4938,6 +4936,7 @@ static void handleError ( break; } } + dcb_close(backend_dcb); } @@ -4965,15 +4964,7 @@ static void handle_error_reply_client( CHK_BACKEND_REF(bref); bref_clear_state(bref, BREF_IN_USE); bref_set_state(bref, BREF_CLOSED); - if (bref_was_in_use) - { - dcb_close(backend_dcb); - } } - else - { - dcb_close(backend_dcb); - } if (sesstate == SESSION_STATE_ROUTER_READY) { @@ -5023,7 +5014,6 @@ static bool handle_error_new_connection( if ((bref = get_bref_from_dcb(myrses, backend_dcb)) == NULL) { succp = true; - dcb_close(backend_dcb); goto return_succp; } CHK_BACKEND_REF(bref); @@ -5063,10 +5053,6 @@ static bool handle_error_new_connection( DCB_REASON_NOT_RESPONDING, &router_handle_state_switch, (void *)bref); - if (bref_was_in_use) - { - dcb_close(backend_dcb); - } router_nservers = router_get_servercount(inst); max_nslaves = rses_get_max_slavecount(myrses, router_nservers); max_slave_rlag = rses_get_max_replication_lag(myrses); From 31c6666278287c69192ccbcfa66c45e52f70663f Mon Sep 17 00:00:00 2001 From: Martin Brampton Date: Thu, 17 Sep 2015 14:38:56 +0100 Subject: [PATCH 49/85] Ensure DCB for closing session does not become persistent; remove bref_was_not_in_use. --- server/core/dcb.c | 14 ++++++++++++++ .../routing/readwritesplit/readwritesplit.c | 4 ---- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/server/core/dcb.c b/server/core/dcb.c index 1654aec3c..ddbdf19ca 100644 --- a/server/core/dcb.c +++ b/server/core/dcb.c @@ -1838,6 +1838,15 @@ dcb_close(DCB *dcb) dcb_final_free(dcb); return; } + + /* + * If DCB is in persistent pool, mark it as an error and exit + */ + if (dcb->persistentstart) + { + dcb->dcb_errhandle_called = true; + return; + } spinlock_acquire(&zombiespin); if (!dcb->dcb_is_zombie) @@ -1879,6 +1888,8 @@ dcb_maybe_add_persistent(DCB *dcb) int poolcount = -1; if (dcb->user != NULL && strlen(dcb->user) + && dcb->session->state != SESSION_STATE_STOPPING + && dcb->session->state != SESSION_STATE_TO_BE_FREED && dcb->server && dcb->server->persistpoolmax && (dcb->server->status & SERVER_RUNNING) @@ -2817,6 +2828,8 @@ dcb_persistent_clean_count(DCB *dcb, bool cleanall) || persistentdcb-> dcb_errhandle_called || count >= server->persistpoolmax || persistentdcb->server == NULL + || persistentdcb->session->state == SESSION_STATE_STOPPING + || persistentdcb->session->state == SESSION_STATE_TO_BE_FREED || !(persistentdcb->server->status & SERVER_RUNNING) || (time(NULL) - persistentdcb->persistentstart) > server->persistmaxtime) { @@ -2846,6 +2859,7 @@ dcb_persistent_clean_count(DCB *dcb, bool cleanall) while (disposals) { nextdcb = disposals->nextpersistent; + disposals->persistentstart = 0; dcb_close_finish(disposals); dcb_close(disposals); disposals = nextdcb; diff --git a/server/modules/routing/readwritesplit/readwritesplit.c b/server/modules/routing/readwritesplit/readwritesplit.c index 46fce6cee..d8d8033f0 100644 --- a/server/modules/routing/readwritesplit/readwritesplit.c +++ b/server/modules/routing/readwritesplit/readwritesplit.c @@ -4875,7 +4875,6 @@ static void handleError ( bref = get_bref_from_dcb(rses, backend_dcb); if (bref != NULL) { - bool bref_was_in_use = BREF_IS_IN_USE(bref); CHK_BACKEND_REF(bref); bref_clear_state(bref, BREF_IN_USE); bref_set_state(bref, BREF_CLOSED); @@ -4960,7 +4959,6 @@ static void handle_error_reply_client( */ if ((bref = get_bref_from_dcb(rses, backend_dcb)) != NULL) { - bool bref_was_in_use = BREF_IS_IN_USE(bref); CHK_BACKEND_REF(bref); bref_clear_state(bref, BREF_IN_USE); bref_set_state(bref, BREF_CLOSED); @@ -5000,7 +4998,6 @@ static bool handle_error_new_connection( int max_slave_rlag; backend_ref_t* bref; bool succp; - bool bref_was_in_use; myrses = *rses; ss_dassert(SPINLOCK_IS_LOCKED(&myrses->rses_lock)); @@ -5017,7 +5014,6 @@ static bool handle_error_new_connection( goto return_succp; } CHK_BACKEND_REF(bref); - bref_was_in_use = BREF_IS_IN_USE(bref); /** * If query was sent through the bref and it is waiting for reply from From c69658889cc7cd67f87c7d842bf210b7930fd7c8 Mon Sep 17 00:00:00 2001 From: counterpoint Date: Fri, 18 Sep 2015 08:59:06 +0100 Subject: [PATCH 50/85] Handle client input case where no router session exists by sending error message to client. --- server/modules/protocol/mysql_client.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/server/modules/protocol/mysql_client.c b/server/modules/protocol/mysql_client.c index 1f983650c..edda2b888 100644 --- a/server/modules/protocol/mysql_client.c +++ b/server/modules/protocol/mysql_client.c @@ -758,9 +758,9 @@ int gw_read_client_event( router = session->service->router; router_instance = session->service->router_instance; rsession = session->router_session; - ss_dassert(rsession != NULL); - if (router_instance != NULL && rsession != NULL) { + if (router_instance != NULL && rsession != NULL) + { /** Ask what type of input the router expects */ cap = router->getCapabilities(router_instance, rsession); @@ -820,7 +820,17 @@ int gw_read_client_event( goto return_rc; } } - } + else + { + /** Send ERR 1045 to client */ + mysql_send_auth_error( + dcb, + 2, + 0, + "failed to create new session"); + return 0; + } + } if (stmt_input) { From f3560512ffec1b796f329a1d2450c66d23506283 Mon Sep 17 00:00:00 2001 From: counterpoint Date: Fri, 18 Sep 2015 09:04:32 +0100 Subject: [PATCH 51/85] Suppress call to router error handling where there is no router session. --- server/modules/protocol/mysql_backend.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/server/modules/protocol/mysql_backend.c b/server/modules/protocol/mysql_backend.c index 85c90319d..29d7dc2b9 100644 --- a/server/modules/protocol/mysql_backend.c +++ b/server/modules/protocol/mysql_backend.c @@ -395,7 +395,8 @@ static int gw_read_backend_event(DCB *dcb) { "Authentication with backend failed. " "Session will be closed."); - router->handleError(router_instance, + if (rsession) + router->handleError(router_instance, rsession, errbuf, dcb, From e507933c4841b1ae3b3a4bcd42d0294ce4656ecf Mon Sep 17 00:00:00 2001 From: counterpoint Date: Fri, 18 Sep 2015 09:19:32 +0100 Subject: [PATCH 52/85] Need to mark the DCB dcb_errhandle_called indicator if the router error handler is not called. --- server/modules/protocol/mysql_backend.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/server/modules/protocol/mysql_backend.c b/server/modules/protocol/mysql_backend.c index 29d7dc2b9..94efb669c 100644 --- a/server/modules/protocol/mysql_backend.c +++ b/server/modules/protocol/mysql_backend.c @@ -396,12 +396,18 @@ static int gw_read_backend_event(DCB *dcb) { "Session will be closed."); if (rsession) + { router->handleError(router_instance, rsession, errbuf, dcb, ERRACT_REPLY_CLIENT, &succp); + } + else + { + dcb->dcb_errhandle_called = true; + } gwbuf_free(errbuf); LOGIF(LD, (skygw_log_write( LOGFILE_DEBUG, From b8af047a2528a294bb619ac5387124cfabe7becb Mon Sep 17 00:00:00 2001 From: counterpoint Date: Fri, 18 Sep 2015 11:03:23 +0100 Subject: [PATCH 53/85] Remove excessively tight conditions for selecting persistent connections, add more information to debug output when connection is rejected. --- server/core/dcb.c | 10 ++++------ server/core/server.c | 5 ++++- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/server/core/dcb.c b/server/core/dcb.c index ddbdf19ca..4bbcc80c9 100644 --- a/server/core/dcb.c +++ b/server/core/dcb.c @@ -1888,8 +1888,6 @@ dcb_maybe_add_persistent(DCB *dcb) int poolcount = -1; if (dcb->user != NULL && strlen(dcb->user) - && dcb->session->state != SESSION_STATE_STOPPING - && dcb->session->state != SESSION_STATE_TO_BE_FREED && dcb->server && dcb->server->persistpoolmax && (dcb->server->status & SERVER_RUNNING) @@ -1916,14 +1914,16 @@ dcb_maybe_add_persistent(DCB *dcb) { LOGIF(LD, (skygw_log_write( LOGFILE_DEBUG, - "%lu [dcb_maybe_add_persistent] Not adding DCB %p to persistent pool, user %s, " - "max for pool %d, error handle called %s, hung flag %s, pool count %d.\n", + "%lu [dcb_maybe_add_persistent] Not adding DCB %p to persistent pool, " + "user %s, max for pool %d, error handle called %s, hung flag %s, " + "server status %d, pool count %d.\n", pthread_self(), dcb, dcb->user ? dcb->user : "", (dcb->server && dcb->server->persistpoolmax) ? dcb->server->persistpoolmax : 0, dcb->dcb_errhandle_called ? "true" : "false", (dcb->flags & DCBF_HUNG) ? "true" : "false", + dcb->server ? dcb->server->status : 0, poolcount))); } return false; @@ -2828,8 +2828,6 @@ dcb_persistent_clean_count(DCB *dcb, bool cleanall) || persistentdcb-> dcb_errhandle_called || count >= server->persistpoolmax || persistentdcb->server == NULL - || persistentdcb->session->state == SESSION_STATE_STOPPING - || persistentdcb->session->state == SESSION_STATE_TO_BE_FREED || !(persistentdcb->server->status & SERVER_RUNNING) || (time(NULL) - persistentdcb->persistentstart) > server->persistmaxtime) { diff --git a/server/core/server.c b/server/core/server.c index 2625cf1ce..b494ef257 100644 --- a/server/core/server.c +++ b/server/core/server.c @@ -153,7 +153,10 @@ server_get_persistent(SERVER *server, char *user, const char *protocol) { DCB *dcb, *previous = NULL; - if (server->persistent && dcb_persistent_clean_count(server->persistent, false) && server->persistent) + if (server->persistent + && dcb_persistent_clean_count(server->persistent, false) + && server->persistent + && (server->status & SERVER_RUNNING)) { spinlock_acquire(&server->persistlock); dcb = server->persistent; From 88716c35fba54ca95120ee2f579996cdcf967c67 Mon Sep 17 00:00:00 2001 From: counterpoint Date: Mon, 21 Sep 2015 09:23:22 +0100 Subject: [PATCH 54/85] Various changes to block loopholes in different cases and tidy up. --- server/core/dcb.c | 147 ++++++++++++------ server/core/poll.c | 7 +- server/modules/protocol/mysql_backend.c | 16 +- server/modules/routing/readconnroute.c | 6 +- .../routing/readwritesplit/readwritesplit.c | 9 +- 5 files changed, 122 insertions(+), 63 deletions(-) diff --git a/server/core/dcb.c b/server/core/dcb.c index 4bbcc80c9..bd7d5677e 100644 --- a/server/core/dcb.c +++ b/server/core/dcb.c @@ -289,10 +289,10 @@ dcb_final_free(DCB *dcb) { DCB_CALLBACK *cb; - CHK_DCB(dcb); - ss_info_dassert(dcb->state == DCB_STATE_DISCONNECTED || - dcb->state == DCB_STATE_ALLOC, - "dcb not in DCB_STATE_DISCONNECTED not in DCB_STATE_ALLOC state."); + CHK_DCB(dcb); + ss_info_dassert(dcb->state == DCB_STATE_DISCONNECTED || + dcb->state == DCB_STATE_ALLOC, + "dcb not in DCB_STATE_DISCONNECTED not in DCB_STATE_ALLOC state."); if (DCB_POLL_BUSY(dcb)) { @@ -326,53 +326,58 @@ dcb_final_free(DCB *dcb) if (ptr) ptr->next = dcb->next; } - spinlock_release(&dcbspin); + spinlock_release(&dcbspin); - if (dcb->session) { - /*< - * Terminate client session. - */ - { - SESSION *local_session = dcb->session; - dcb->session = NULL; - CHK_SESSION(local_session); - /** - * Set session's client pointer NULL so that other threads - * won't try to call dcb_close for client DCB - * after this call. - */ - if (local_session->client == dcb) - { - spinlock_acquire(&local_session->ses_lock); - local_session->client = NULL; - spinlock_release(&local_session->ses_lock); - } - if (SESSION_STATE_DUMMY != local_session->state) - { - session_free(local_session); - } - } + if (dcb->session) { + /*< + * Terminate client session. + */ + SESSION *local_session = dcb->session; + dcb->session = NULL; + CHK_SESSION(local_session); + /** + * Set session's client pointer NULL so that other threads + * won't try to call dcb_close for client DCB + * after this call. + */ + if (local_session->client == dcb) + { + spinlock_acquire(&local_session->ses_lock); + local_session->client = NULL; + spinlock_release(&local_session->ses_lock); + } + if (SESSION_STATE_DUMMY != local_session->state) + { + session_free(local_session); + } } if (dcb->protocol && (!DCB_IS_CLONE(dcb))) free(dcb->protocol); - if (dcb->protoname) - free(dcb->protoname); + if (dcb->protoname) + free(dcb->protoname); if (dcb->remote) free(dcb->remote); if (dcb->user) free(dcb->user); - /* Clear write and read buffers */ - if (dcb->delayq) { - GWBUF *queue = dcb->delayq; - while ((queue = gwbuf_consume(queue, GWBUF_LENGTH(queue))) != NULL); - } - if (dcb->dcb_readqueue) - { - GWBUF* queue = dcb->dcb_readqueue; - while ((queue = gwbuf_consume(queue, GWBUF_LENGTH(queue))) != NULL); - } + /* Clear write and read buffers */ + if (dcb->delayq) { + GWBUF *queue = dcb->delayq; + while ((queue = gwbuf_consume(queue, GWBUF_LENGTH(queue))) != NULL); + dcb->delayq = NULL; + } + if (dcb->writeq) { + GWBUF *queue = dcb->writeq; + while ((queue = gwbuf_consume(queue, GWBUF_LENGTH(queue))) != NULL); + dcb->writeq = NULL; + } + if (dcb->dcb_readqueue) + { + GWBUF* queue = dcb->dcb_readqueue; + while ((queue = gwbuf_consume(queue, GWBUF_LENGTH(queue))) != NULL); + dcb->dcb_readqueue = NULL; + } spinlock_acquire(&dcb->cb_lock); while ((cb = dcb->callbacks) != NULL) @@ -549,10 +554,36 @@ dcb_process_victim_queue(DCB *listofdcb) dcb = dcb->memdata.next; continue; } + else + { + DCB *nextdcb; + poll_remove_dcb(dcb); + spinlock_acquire(&zombiespin); + bitmask_copy(&dcb->memdata.bitmask, poll_bitmask()); + nextdcb = dcb->memdata.next; + dcb->memdata.next = zombies; + zombies = dcb; + spinlock_release(&zombiespin); + if (dcb->server) + { + atomic_add(&dcb->server->stats.n_current, -1); + } + dcb = nextdcb; + continue; + } } - dcb_close_finish(dcb); } - + /** + * close protocol and router session + */ + if (dcb->func.close != NULL) + { + dcb->func.close(dcb); + } + /** Call possible callback for this DCB in case of close */ + dcb_call_callback(dcb, DCB_REASON_CLOSE); + + if (dcb->fd > 0) { /*< @@ -1826,7 +1857,7 @@ dcb_close(DCB *dcb) pthread_self(), dcb, STRDCBSTATE(dcb->state)))); - raise(SIGABRT); + raise(SIGABRT); } /** @@ -1902,6 +1933,20 @@ dcb_maybe_add_persistent(DCB *dcb) dcb->user))); dcb->dcb_is_zombie = false; dcb->persistentstart = time(NULL); + if (dcb->session) + /*< + * Terminate client session. + */ + { + SESSION *local_session = dcb->session; + session_set_dummy(dcb); + CHK_SESSION(local_session); + if (SESSION_STATE_DUMMY != local_session->state) + { + session_free(local_session); + } + } + dcb->callbacks = NULL; spinlock_acquire(&dcb->server->persistlock); dcb->nextpersistent = dcb->server->persistent; dcb->server->persistent = dcb; @@ -2063,6 +2108,15 @@ dprintOneDCB(DCB *pdcb, DCB *dcb) if (dcb->remote) dcb_printf(pdcb, "\tConnected to: %s\n", dcb->remote); + if (dcb->server) + { + if (dcb->server->name) + dcb_printf(pdcb, "\tServer name/IP: %s\n", + dcb->server->name); + if (dcb->server->port) + dcb_printf(pdcb, "\tPort number: %d\n", + dcb->server->port); + } if (dcb->user) dcb_printf(pdcb, "\tUsername: %s\n", dcb->user); @@ -2858,7 +2912,10 @@ dcb_persistent_clean_count(DCB *dcb, bool cleanall) { nextdcb = disposals->nextpersistent; disposals->persistentstart = 0; - dcb_close_finish(disposals); + if (DCB_STATE_POLLING == dcb->state) + { + poll_remove_dcb(dcb); + } dcb_close(disposals); disposals = nextdcb; } diff --git a/server/core/poll.c b/server/core/poll.c index b3fb82920..2e6745987 100644 --- a/server/core/poll.c +++ b/server/core/poll.c @@ -860,7 +860,12 @@ unsigned long qtime; #endif /* FAKE_CODE */ ss_debug(spinlock_acquire(&dcb->dcb_initlock);) ss_dassert(dcb->state != DCB_STATE_ALLOC); - ss_dassert(dcb->state != DCB_STATE_DISCONNECTED); + /* It isn't obvious that this is impossible */ + /* ss_dassert(dcb->state != DCB_STATE_DISCONNECTED); */ + if (DCB_STATE_DISCONNECTED == dcb->state) + { + return 0; + } ss_debug(spinlock_release(&dcb->dcb_initlock);) LOGIF(LD, (skygw_log_write( diff --git a/server/modules/protocol/mysql_backend.c b/server/modules/protocol/mysql_backend.c index 94efb669c..d72d6a639 100644 --- a/server/modules/protocol/mysql_backend.c +++ b/server/modules/protocol/mysql_backend.c @@ -167,7 +167,7 @@ static int gw_read_backend_event(DCB *dcb) { int rc = 0; CHK_DCB(dcb); - if (!dcb->session && dcb->persistentstart) + if (dcb->persistentstart) { dcb->dcb_errhandle_called = true; goto return_rc; @@ -407,16 +407,11 @@ static int gw_read_backend_event(DCB *dcb) { else { dcb->dcb_errhandle_called = true; + dcb_close(dcb); + rc = 1; + goto return_rc; } gwbuf_free(errbuf); - LOGIF(LD, (skygw_log_write( - LOGFILE_DEBUG, - "%lu [gw_read_backend_event] " - "after calling handleError. Backend " - "DCB %p, session %p", - pthread_self(), - dcb, - dcb->session))); spinlock_acquire(&session->ses_lock); session->state = SESSION_STATE_STOPPING; @@ -1066,7 +1061,7 @@ gw_backend_hangup(DCB *dcb) session_state_t ses_state; CHK_DCB(dcb); - if (!dcb->session && dcb->persistentstart) + if (dcb->persistentstart) { dcb->dcb_errhandle_called = true; goto retblock; @@ -1124,6 +1119,7 @@ gw_backend_hangup(DCB *dcb) } } gwbuf_free(errbuf); + dcb_close(dcb); goto retblock; } #if defined(SS_DEBUG) diff --git a/server/modules/routing/readconnroute.c b/server/modules/routing/readconnroute.c index e32bdd677..ceeb0a627 100644 --- a/server/modules/routing/readconnroute.c +++ b/server/modules/routing/readconnroute.c @@ -886,7 +886,7 @@ static void handleError( spinlock_release(&session->ses_lock); } - if (router_cli_ses->backend_dcb) { + if (router_cli_ses && router_cli_ses->backend_dcb) { if (backend_dcb != router_cli_ses->backend_dcb) { /* Linkages have gone badly wrong */ @@ -898,9 +898,9 @@ static void handleError( backend_dcb))); } router_cli_ses->backend_dcb = NULL; - dcb_close(backend_dcb); } - + dcb_close(backend_dcb); + /** false because connection is not available anymore */ *succp = false; } diff --git a/server/modules/routing/readwritesplit/readwritesplit.c b/server/modules/routing/readwritesplit/readwritesplit.c index d8d8033f0..1a42f1b12 100644 --- a/server/modules/routing/readwritesplit/readwritesplit.c +++ b/server/modules/routing/readwritesplit/readwritesplit.c @@ -990,7 +990,7 @@ static void closeSession( int i; /** * This sets router closed. Nobody is allowed to use router - * whithout checking this first. + * without checking this first. */ router_cli_ses->rses_closed = true; @@ -5306,8 +5306,6 @@ static int router_handle_state_switch( backend_ref_t* bref; int rc = 1; SERVER* srv; - ROUTER_CLIENT_SES* rses; - SESSION* ses; CHK_DCB(dcb); bref = (backend_ref_t *)data; CHK_BACKEND_REF(bref); @@ -5327,7 +5325,10 @@ static int router_handle_state_switch( srv->port, STRSRVSTATUS(srv)))); CHK_SESSION(((SESSION*)dcb->session)); - CHK_CLIENT_RSES(((ROUTER_CLIENT_SES *)dcb->session->router_session)); + if (dcb->session->router_session) + { + CHK_CLIENT_RSES(((ROUTER_CLIENT_SES *)dcb->session->router_session)); + } switch (reason) { case DCB_REASON_NOT_RESPONDING: From 7aa36b77ea4baef7c845a3fb83b6dc6e6c294faa Mon Sep 17 00:00:00 2001 From: counterpoint Date: Mon, 21 Sep 2015 14:25:12 +0100 Subject: [PATCH 55/85] Guarantee router session is present for call to clientReply; properly free callbacks; attempt to set all necessary values for dbusers; do more to ensure buffers freed. --- server/core/dbusers.c | 1 + server/core/dcb.c | 9 ++++++++- server/core/hashtable.c | 6 +++++- server/modules/protocol/maxscaled.c | 6 +++--- server/modules/protocol/mysql_backend.c | 3 ++- 5 files changed, 19 insertions(+), 6 deletions(-) diff --git a/server/core/dbusers.c b/server/core/dbusers.c index 701d59764..f54f11048 100644 --- a/server/core/dbusers.c +++ b/server/core/dbusers.c @@ -1782,6 +1782,7 @@ static void *uh_keydup(void* key) { if (current_key->resource) rval->resource = strdup(current_key->resource); + else rval->resource = NULL; return (void *) rval; } diff --git a/server/core/dcb.c b/server/core/dcb.c index bd7d5677e..5dc3ee186 100644 --- a/server/core/dcb.c +++ b/server/core/dcb.c @@ -1926,6 +1926,7 @@ dcb_maybe_add_persistent(DCB *dcb) && !(dcb->flags & DCBF_HUNG) && (poolcount = dcb_persistent_clean_count(dcb, false)) < dcb->server->persistpoolmax) { + DCB_CALLBACK *loopcallback; LOGIF(LD, (skygw_log_write( LOGFILE_DEBUG, "%lu [dcb_maybe_add_persistent] Adding DCB to persistent pool, user %s.\n", @@ -1946,7 +1947,13 @@ dcb_maybe_add_persistent(DCB *dcb) session_free(local_session); } } - dcb->callbacks = NULL; + spinlock_acquire(&dcb->cb_lock); + while ((loopcallback = dcb->callbacks) != NULL) + { + dcb->callbacks = loopcallback->next; + free(loopcallback); + } + spinlock_release(&dcb->cb_lock); spinlock_acquire(&dcb->server->persistlock); dcb->nextpersistent = dcb->server->persistent; dcb->server->persistent = dcb; diff --git a/server/core/hashtable.c b/server/core/hashtable.c index c43b76916..4f7620349 100644 --- a/server/core/hashtable.c +++ b/server/core/hashtable.c @@ -762,7 +762,11 @@ char buf[40]; key = keyread(fd); value = valueread(fd); if (key == NULL || value == NULL) - break; + { + free(key); + free(value); + break; + } hashtable_add(table, key, value); rval++; } diff --git a/server/modules/protocol/maxscaled.c b/server/modules/protocol/maxscaled.c index 1f05498f1..a00a47d3c 100644 --- a/server/modules/protocol/maxscaled.c +++ b/server/modules/protocol/maxscaled.c @@ -156,7 +156,7 @@ char *password; maxscaled->username = strndup(GWBUF_DATA(head), GWBUF_LENGTH(head)); maxscaled->state = MAXSCALED_STATE_PASSWD; dcb_printf(dcb, "PASSWORD"); - gwbuf_consume(head, GWBUF_LENGTH(head)); + while ((head = gwbuf_consume(head, GWBUF_LENGTH(head))) != NULL); break; case MAXSCALED_STATE_PASSWD: password = strndup(GWBUF_DATA(head), GWBUF_LENGTH(head)); @@ -170,7 +170,7 @@ char *password; dcb_printf(dcb, "FAILED"); maxscaled->state = MAXSCALED_STATE_LOGIN; } - gwbuf_consume(head, GWBUF_LENGTH(head)); + while ((head = gwbuf_consume(head, GWBUF_LENGTH(head))) != NULL); free(password); break; case MAXSCALED_STATE_DATA: @@ -182,7 +182,7 @@ char *password; else { // Force the free of the buffer header - gwbuf_consume(head, 0); + while ((head = gwbuf_consume(head, GWBUF_LENGTH(head))) != NULL); } } } diff --git a/server/modules/protocol/mysql_backend.c b/server/modules/protocol/mysql_backend.c index d72d6a639..28c2d2a43 100644 --- a/server/modules/protocol/mysql_backend.c +++ b/server/modules/protocol/mysql_backend.c @@ -569,7 +569,8 @@ static int gw_read_backend_event(DCB *dcb) { */ if (dcb->session->state == SESSION_STATE_ROUTER_READY && dcb->session->client != NULL && - dcb->session->client->state == DCB_STATE_POLLING) + dcb->session->client->state == DCB_STATE_POLLING && + session->router_session) { client_protocol = SESSION_PROTOCOL(dcb->session, MySQLProtocol); From 15f042f083a718602977dc19eaf0258eb6f2d455 Mon Sep 17 00:00:00 2001 From: counterpoint Date: Mon, 21 Sep 2015 15:49:57 +0100 Subject: [PATCH 56/85] On reflection, freeing keys and values in hashtable processing is not a good idea because we don't know what they are, and can put up with some small memory losses. --- server/core/hashtable.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/server/core/hashtable.c b/server/core/hashtable.c index 4f7620349..7fd45e898 100644 --- a/server/core/hashtable.c +++ b/server/core/hashtable.c @@ -763,8 +763,6 @@ char buf[40]; value = valueread(fd); if (key == NULL || value == NULL) { - free(key); - free(value); break; } hashtable_add(table, key, value); From 95a4daecc974ad1c60e2103d47093d5bb55e877c Mon Sep 17 00:00:00 2001 From: counterpoint Date: Tue, 22 Sep 2015 11:54:47 +0100 Subject: [PATCH 57/85] Add GWBUF_POINTER_IN_BUFFER macro; add extra free calls to remove memory leaks. --- server/include/buffer.h | 3 +++ server/modules/protocol/mysql_backend.c | 1 + server/modules/protocol/mysql_client.c | 12 ++++++++++++ 3 files changed, 16 insertions(+) diff --git a/server/include/buffer.h b/server/include/buffer.h index c0555bae4..6be9fc7aa 100644 --- a/server/include/buffer.h +++ b/server/include/buffer.h @@ -168,6 +168,9 @@ typedef struct gwbuf { /*< Consume a number of bytes in the buffer */ #define GWBUF_CONSUME(b, bytes) ((b)->start = bytes > ((char *)(b)->end - (char *)(b)->start) ? (b)->end : (void *)((char *)(b)->start + (bytes))); +/*< Check if a given pointer is within the buffer */ +#define GWBUF_POINTER_IN_BUFFER (ptr, b) ((char *)(ptr) >= (char *)(b)->start && (char *)(ptr) < (char *)(b)->end) + /*< Consume a complete buffer */ #define GWBUF_CONSUME_ALL(b) gwbuf_consume((b), GWBUF_LENGTH((b))) diff --git a/server/modules/protocol/mysql_backend.c b/server/modules/protocol/mysql_backend.c index 28c2d2a43..fe23c9778 100644 --- a/server/modules/protocol/mysql_backend.c +++ b/server/modules/protocol/mysql_backend.c @@ -406,6 +406,7 @@ static int gw_read_backend_event(DCB *dcb) { } else { + gwbuf_free(errbuf); dcb->dcb_errhandle_called = true; dcb_close(dcb); rc = 1; diff --git a/server/modules/protocol/mysql_client.c b/server/modules/protocol/mysql_client.c index edda2b888..861b20da0 100644 --- a/server/modules/protocol/mysql_client.c +++ b/server/modules/protocol/mysql_client.c @@ -817,6 +817,10 @@ int gw_read_client_event( "Session will be closed."))); } rc = 1; + while (read_buffer) + { + read_buffer = gwbuf_consume(read_buffer, GWBUF_LENGTH(read_buffer)); + } goto return_rc; } } @@ -828,6 +832,10 @@ int gw_read_client_event( 2, 0, "failed to create new session"); + while (read_buffer) + { + read_buffer = gwbuf_consume(read_buffer, GWBUF_LENGTH(read_buffer)); + } return 0; } } @@ -1204,6 +1212,10 @@ int gw_read_client_event( "Session will be closed."))); } + while (read_buffer) + { + read_buffer = gwbuf_consume(read_buffer, GWBUF_LENGTH(read_buffer)); + } } } } From 89667294b36b2fb7cdad43bd2b17b50193003258 Mon Sep 17 00:00:00 2001 From: Martin Brampton Date: Thu, 24 Sep 2015 07:39:47 +0100 Subject: [PATCH 58/85] Fix exceptional cases in DCB dcb_call_callback and in MySQL backend gw_error_backend_event - close DCB and return. --- server/core/dcb.c | 5 +++++ server/modules/protocol/mysql_backend.c | 5 +++++ 2 files changed, 10 insertions(+) diff --git a/server/core/dcb.c b/server/core/dcb.c index 5dc3ee186..e17b4126a 100644 --- a/server/core/dcb.c +++ b/server/core/dcb.c @@ -2672,6 +2672,11 @@ dcb_call_callback(DCB *dcb, DCB_REASON reason) { DCB_CALLBACK *cb, *nextcb; + if (NULL == dcb->session->router_session) + { + dcb_close(dcb); + return; + } spinlock_acquire(&dcb->cb_lock); cb = dcb->callbacks; while (cb) diff --git a/server/modules/protocol/mysql_backend.c b/server/modules/protocol/mysql_backend.c index fe23c9778..6907db4d9 100644 --- a/server/modules/protocol/mysql_backend.c +++ b/server/modules/protocol/mysql_backend.c @@ -828,6 +828,11 @@ static int gw_error_backend_event(DCB *dcb) CHK_DCB(dcb); session = dcb->session; CHK_SESSION(session); + if (SESSION_STATE_DUMMY == session->state) + { + dcb_close(dcb); + return 1; + } rsession = session->router_session; router = session->service->router; router_instance = session->service->router_instance; From 2231d0870c5a992eec37cee4e1e8eb8e1a57782c Mon Sep 17 00:00:00 2001 From: counterpoint Date: Fri, 25 Sep 2015 12:17:11 +0100 Subject: [PATCH 59/85] Place checks in callback routines because DCB will not always contain a reference to a router session, and the associated data will be invalid in this case. --- server/core/dcb.c | 5 ----- server/modules/routing/binlog/blr_slave.c | 10 ++++++++++ server/modules/routing/readconnroute.c | 9 +++++++++ server/modules/routing/readwritesplit/readwritesplit.c | 10 ++++++++++ server/modules/routing/schemarouter/schemarouter.c | 10 ++++++++++ 5 files changed, 39 insertions(+), 5 deletions(-) diff --git a/server/core/dcb.c b/server/core/dcb.c index 4c643de53..01717667c 100644 --- a/server/core/dcb.c +++ b/server/core/dcb.c @@ -2574,11 +2574,6 @@ dcb_call_callback(DCB *dcb, DCB_REASON reason) { DCB_CALLBACK *cb, *nextcb; - if (NULL == dcb->session->router_session) - { - dcb_close(dcb); - return; - } spinlock_acquire(&dcb->cb_lock); cb = dcb->callbacks; while (cb) diff --git a/server/modules/routing/binlog/blr_slave.c b/server/modules/routing/binlog/blr_slave.c index e92beb277..a2a9ebb2a 100644 --- a/server/modules/routing/binlog/blr_slave.c +++ b/server/modules/routing/binlog/blr_slave.c @@ -38,6 +38,7 @@ * 19/03/2015 Massimiliano Pinto Addition of basic MariaDB 10 compatibility support * 07/05/2015 Massimiliano Pinto Added MariaDB 10 Compatibility * 11/05/2015 Massimiliano Pinto Only MariaDB 10 Slaves can register to binlog router with a MariaDB 10 Master + * 25/09/2015 Martin Brampton Block callback processing when no router session in the DCB * * @endverbatim */ @@ -1606,6 +1607,15 @@ blr_slave_callback(DCB *dcb, DCB_REASON reason, void *data) ROUTER_SLAVE *slave = (ROUTER_SLAVE *)data; ROUTER_INSTANCE *router = slave->router; + if (NULL == dcb->session->router_session) + { + /* + * The following processing will fail if there is no router session, + * because the "data" parameter will not contain meaningful data, + * so we have no choice but to stop here. + */ + return; + } if (reason == DCB_REASON_DRAINED) { if (slave->state == BLRS_DUMPING) diff --git a/server/modules/routing/readconnroute.c b/server/modules/routing/readconnroute.c index ceeb0a627..2f7faba4f 100644 --- a/server/modules/routing/readconnroute.c +++ b/server/modules/routing/readconnroute.c @@ -69,6 +69,7 @@ * 27/06/2014 Mark Riddoch Addition of server weighting * 11/06/2015 Martin Brampton Remove decrement n_current (moved to dcb.c) * 09/09/2015 Martin Brampton Modify error handler + * 25/09/2015 Martin Brampton Block callback processing when no router session in the DCB * * @endverbatim */ @@ -1010,6 +1011,14 @@ static int handle_state_switch(DCB* dcb,DCB_REASON reason, void * routersession) SERVICE* service = session->service; ROUTER* router = (ROUTER *)service->router; + if (NULL == dcb->session->router_session && DCB_REASON_ERROR != reason) + { + /* + * We cannot handle a DCB that does not have a router session, + * except in the case where error processing is invoked. + */ + return; + } switch(reason) { case DCB_REASON_CLOSE: diff --git a/server/modules/routing/readwritesplit/readwritesplit.c b/server/modules/routing/readwritesplit/readwritesplit.c index e40031d24..074b40475 100644 --- a/server/modules/routing/readwritesplit/readwritesplit.c +++ b/server/modules/routing/readwritesplit/readwritesplit.c @@ -71,6 +71,7 @@ extern __thread log_info_t tls_log_info; * 17/07/2014 Massimiliano Pinto Server connection counter is updated in closeSession * * 09/09/2015 Martin Brampton Modify error handler + * 25/09/2015 Martin Brampton Block callback processing when no router session in the DCB * * @endverbatim */ @@ -5308,6 +5309,15 @@ static int router_handle_state_switch( int rc = 1; SERVER* srv; CHK_DCB(dcb); + if (NULL == dcb->session->router_session) + { + /* + * The following processing will fail if there is no router session, + * because the "data" parameter will not contain meaningful data, + * so we have no choice but to stop here. + */ + return; + } bref = (backend_ref_t *)data; CHK_BACKEND_REF(bref); diff --git a/server/modules/routing/schemarouter/schemarouter.c b/server/modules/routing/schemarouter/schemarouter.c index b2d1d9eb5..ed29d95e2 100644 --- a/server/modules/routing/schemarouter/schemarouter.c +++ b/server/modules/routing/schemarouter/schemarouter.c @@ -59,6 +59,7 @@ extern __thread log_info_t tls_log_info; * Date Who Description * 01/12/2014 Vilho Raatikka/Markus Mäkelä Initial implementation * 09/09/2015 Martin Brampton Modify error handler + * 25/09/2015 Martin Brampton Block callback processing when no router session in the DCB * * @endverbatim */ @@ -4404,6 +4405,15 @@ router_handle_state_switch( SERVER* srv; CHK_DCB(dcb); + if (NULL == dcb->session->router_session) + { + /* + * The following processing will fail if there is no router session, + * because the "data" parameter will not contain meaningful data, + * so we have no choice but to stop here. + */ + return; + } bref = (backend_ref_t *) data; CHK_BACKEND_REF(bref); From d582c5880c8700397c60ba7baf8981922997f5b1 Mon Sep 17 00:00:00 2001 From: counterpoint Date: Tue, 29 Sep 2015 11:58:31 +0100 Subject: [PATCH 60/85] Impose locking on dcb_call_foreach DCB callback mechanism. Add counters and maxima for DCBs and zombies to aid diagnosis. --- server/core/dcb.c | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-) diff --git a/server/core/dcb.c b/server/core/dcb.c index 01717667c..80dd84cec 100644 --- a/server/core/dcb.c +++ b/server/core/dcb.c @@ -56,6 +56,8 @@ * remove dcb_set_state etc, simplifications. * 10/07/2015 Martin Brampton Simplify, merge dcb_read and dcb_read_n * 04/09/2015 Martin Brampton Changes to ensure DCB always has session pointer + * 28/09/2015 Martin Brampton Add counters, maxima for DCBs and zombies + * 29/05/2015 Martin Brampton Impose locking in dcb_call_foreach callbacks * * @endverbatim */ @@ -87,7 +89,11 @@ extern size_t log_ses_count[]; extern __thread log_info_t tls_log_info; static DCB *allDCBs = NULL; /* Diagnostics need a list of DCBs */ +static int nDCBs = 0; +static int maxDCBs = 0; static DCB *zombies = NULL; +static int nzombies = 0; +static int maxzombies = 0; static SPINLOCK dcbspin = SPINLOCK_INIT; static SPINLOCK zombiespin = SPINLOCK_INIT; @@ -227,6 +233,8 @@ DCB *newdcb; ptr = ptr->next; ptr->next = newdcb; } + nDCBs++; + if (nDCBs > maxDCBs) maxDCBs = nDCBs; spinlock_release(&dcbspin); return newdcb; } @@ -328,6 +336,7 @@ dcb_final_free(DCB *dcb) if (ptr) ptr->next = dcb->next; } + nDCBs--; spinlock_release(&dcbspin); if (dcb->session) { @@ -492,6 +501,7 @@ DCB *listofdcb = NULL; * (listofdcb) is not NULL, then it follows that * dcb will also not be null. */ + nzombies--; zombiedcb->memdata.next = listofdcb; listofdcb = zombiedcb; } @@ -565,6 +575,8 @@ dcb_process_victim_queue(DCB *listofdcb) nextdcb = dcb->memdata.next; dcb->memdata.next = zombies; zombies = dcb; + nzombies++; + if (nzombies > maxzombies) maxzombies = nzombies; spinlock_release(&zombiespin); if (dcb->server) { @@ -1800,6 +1812,8 @@ dcb_close(DCB *dcb) dcb->dcb_is_zombie = true; dcb->memdata.next = zombies; zombies = dcb; + nzombies++; + if (nzombies > maxzombies) maxzombies = nzombies; /*< Set bit for each maxscale thread. This should be done before * the state is changed, so as to protect the DCB from premature * destruction. */ @@ -2695,17 +2709,21 @@ dcb_call_foreach(struct server* server, DCB_REASON reason) case DCB_REASON_NOT_RESPONDING: { DCB *dcb; - dcb = dcb_get_next(NULL); - + spinlock_acquire(&dcbspin); + dcb = allDCBs; + while (dcb != NULL) { + spinlock_acquire(&dcb->dcb_initlock); if (dcb->state == DCB_STATE_POLLING && dcb->server && - strcmp(dcb->server->unique_name,server->unique_name) == 0) + strcmp(dcb->server->unique_name,server->unique_name) == 0) { dcb_call_callback(dcb, DCB_REASON_NOT_RESPONDING); } - dcb = dcb_get_next(dcb); + spinlock_release(&dcb->dcb_initlock); + dcb = dcb->next; } + spinlock_release(&dcbspin); break; } From e38ea9d07d9c3ddaa6b45035b7a8129a6bab77bf Mon Sep 17 00:00:00 2001 From: counterpoint Date: Fri, 2 Oct 2015 16:19:59 +0100 Subject: [PATCH 61/85] Correct missing return value. --- server/modules/routing/binlog/blr_slave.c | 2 +- server/modules/routing/readconnroute.c | 2 +- server/modules/routing/readwritesplit/readwritesplit.c | 2 +- server/modules/routing/schemarouter/schemarouter.c | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/server/modules/routing/binlog/blr_slave.c b/server/modules/routing/binlog/blr_slave.c index a2a9ebb2a..693aff5bc 100644 --- a/server/modules/routing/binlog/blr_slave.c +++ b/server/modules/routing/binlog/blr_slave.c @@ -1614,7 +1614,7 @@ ROUTER_INSTANCE *router = slave->router; * because the "data" parameter will not contain meaningful data, * so we have no choice but to stop here. */ - return; + return 0; } if (reason == DCB_REASON_DRAINED) { diff --git a/server/modules/routing/readconnroute.c b/server/modules/routing/readconnroute.c index 2f7faba4f..65eb8e8d4 100644 --- a/server/modules/routing/readconnroute.c +++ b/server/modules/routing/readconnroute.c @@ -1017,7 +1017,7 @@ static int handle_state_switch(DCB* dcb,DCB_REASON reason, void * routersession) * We cannot handle a DCB that does not have a router session, * except in the case where error processing is invoked. */ - return; + return 0; } switch(reason) { diff --git a/server/modules/routing/readwritesplit/readwritesplit.c b/server/modules/routing/readwritesplit/readwritesplit.c index 92332d8ec..113d57ea6 100644 --- a/server/modules/routing/readwritesplit/readwritesplit.c +++ b/server/modules/routing/readwritesplit/readwritesplit.c @@ -5309,7 +5309,7 @@ static int router_handle_state_switch( * because the "data" parameter will not contain meaningful data, * so we have no choice but to stop here. */ - return; + return 0; } bref = (backend_ref_t *)data; CHK_BACKEND_REF(bref); diff --git a/server/modules/routing/schemarouter/schemarouter.c b/server/modules/routing/schemarouter/schemarouter.c index ed29d95e2..55a15caa2 100644 --- a/server/modules/routing/schemarouter/schemarouter.c +++ b/server/modules/routing/schemarouter/schemarouter.c @@ -4412,7 +4412,7 @@ router_handle_state_switch( * because the "data" parameter will not contain meaningful data, * so we have no choice but to stop here. */ - return; + return 0; } bref = (backend_ref_t *) data; CHK_BACKEND_REF(bref); From 5c985b42ba7f0ed95f96d5dd36e695cdcdff17be Mon Sep 17 00:00:00 2001 From: counterpoint Date: Mon, 5 Oct 2015 16:36:07 +0100 Subject: [PATCH 62/85] Fix problem with persistent DCB disposal --- server/core/dcb.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/server/core/dcb.c b/server/core/dcb.c index 80dd84cec..b9ccbbfd5 100644 --- a/server/core/dcb.c +++ b/server/core/dcb.c @@ -560,7 +560,7 @@ dcb_process_victim_queue(DCB *listofdcb) } else { /* Must be DCB_STATE_POLLING */ - if (dcb_maybe_add_persistent(dcb)) + if (0 == dcb->persistentstart && dcb_maybe_add_persistent(dcb)) { /* Have taken DCB into persistent pool, no further killing */ dcb = dcb->memdata.next; @@ -1787,7 +1787,7 @@ dcb_close(DCB *dcb) /* * If DCB is in persistent pool, mark it as an error and exit */ - if (dcb->persistentstart) + if (dcb->persistentstart > 0) { dcb->dcb_errhandle_called = true; return; @@ -1796,7 +1796,7 @@ dcb_close(DCB *dcb) spinlock_acquire(&zombiespin); if (!dcb->dcb_is_zombie) { - if (dcb->server && DCB_STATE_POLLING == dcb->state) + if (0 == dcb->persistentstart && dcb->server && DCB_STATE_POLLING == dcb->state) { /* May be a candidate for persistence, so save user name */ char *user; @@ -2838,7 +2838,7 @@ dcb_persistent_clean_count(DCB *dcb, bool cleanall) while (disposals) { nextdcb = disposals->nextpersistent; - disposals->persistentstart = 0; + disposals->persistentstart = -1; if (DCB_STATE_POLLING == dcb->state) { poll_remove_dcb(dcb); From bb53eb0f6d865b103bf46589c3769c1d2c409830 Mon Sep 17 00:00:00 2001 From: counterpoint Date: Wed, 7 Oct 2015 17:06:21 +0100 Subject: [PATCH 63/85] Put extra check in hashtable_fetch to return if zero entries (should never happen but will crash if not checked); remove dcb_close from mysql_backend where it closes backend DCBs, as these should be closed by the router. --- server/core/hashtable.c | 2 +- server/modules/protocol/mysql_backend.c | 15 +++++++++++++-- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/server/core/hashtable.c b/server/core/hashtable.c index 7fd45e898..dbcd56fd9 100644 --- a/server/core/hashtable.c +++ b/server/core/hashtable.c @@ -379,7 +379,7 @@ hashtable_fetch(HASHTABLE *table, void *key) unsigned int hashkey; HASHENTRIES *entry; - if(table == NULL || key == NULL) + if(table == NULL || key == NULL || 0 == table->hashsize) return NULL; hashkey = table->hashfn(key) % table->hashsize; diff --git a/server/modules/protocol/mysql_backend.c b/server/modules/protocol/mysql_backend.c index 4874697fa..d6ada9fa8 100644 --- a/server/modules/protocol/mysql_backend.c +++ b/server/modules/protocol/mysql_backend.c @@ -44,6 +44,7 @@ * 24/10/2014 Massimiliano Pinto Added Mysql user@host @db authentication support * 10/11/2014 Massimiliano Pinto Client charset is passed to backend * 19/06/2015 Martin Brampton Persistent connection handling + * 07/10/2015 Martin Brampton Remove calls to dcb_close - should be done by routers * */ #include @@ -408,7 +409,12 @@ static int gw_read_backend_event(DCB *dcb) { { gwbuf_free(errbuf); dcb->dcb_errhandle_called = true; - dcb_close(dcb); + /* + * I'm pretty certain this is best removed and + * causes trouble if present, but have left it + * here just for now as a comment. Martin + */ + /* dcb_close(dcb); */ rc = 1; goto return_rc; } @@ -1129,7 +1135,12 @@ gw_backend_hangup(DCB *dcb) } } gwbuf_free(errbuf); - dcb_close(dcb); + /* + * I'm pretty certain this is best removed and + * causes trouble if present, but have left it + * here just for now as a comment. Martin + */ + /* dcb_close(dcb); */ goto retblock; } #if defined(SS_DEBUG) From cc42707dc0e23788f3bc4ccf739f03e4a92bc69e Mon Sep 17 00:00:00 2001 From: counterpoint Date: Tue, 13 Oct 2015 15:10:55 +0200 Subject: [PATCH 64/85] The read_buffer pointer must be set to null in situations where the buffer has been freed (or consumed). --- server/modules/protocol/mysql_client.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/server/modules/protocol/mysql_client.c b/server/modules/protocol/mysql_client.c index 3587eb8fb..eebe1f3d7 100644 --- a/server/modules/protocol/mysql_client.c +++ b/server/modules/protocol/mysql_client.c @@ -1141,6 +1141,7 @@ int gw_read_client_event( /* Temporarily suppressed: SESSION_ROUTE_QUERY(session, read_buffer); */ /* Replaced with freeing the read buffer. */ gwbuf_free(read_buffer); + read_buffer = NULL; /** * Close router session which causes closing of backends. */ @@ -1163,12 +1164,14 @@ int gw_read_client_event( { /** add incomplete mysql packet to read queue */ dcb->dcb_readqueue = gwbuf_append(dcb->dcb_readqueue, read_buffer); + read_buffer = NULL; } } else { /** Feed whole packet to router */ rc = SESSION_ROUTE_QUERY(session, read_buffer); + read_buffer = NULL; } /** Routing succeed */ From efc0c7420e8784c558711b4a45b5bc1bb419b133 Mon Sep 17 00:00:00 2001 From: counterpoint Date: Tue, 13 Oct 2015 16:19:24 +0200 Subject: [PATCH 65/85] Correct misplacement of decrementing current connections counter. --- server/core/dcb.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/server/core/dcb.c b/server/core/dcb.c index b9ccbbfd5..29ea757a1 100644 --- a/server/core/dcb.c +++ b/server/core/dcb.c @@ -578,15 +578,19 @@ dcb_process_victim_queue(DCB *listofdcb) nzombies++; if (nzombies > maxzombies) maxzombies = nzombies; spinlock_release(&zombiespin); - if (dcb->server) - { - atomic_add(&dcb->server->stats.n_current, -1); - } dcb = nextdcb; continue; } } } + /* + * Into the final close logic, so if DCB is for backend server, we + * must decrement the number of current connections. + */ + if (dcb->server) + { + atomic_add(&dcb->server->stats.n_current, -1); + } /** * close protocol and router session */ From d000af33f65c713ca687c96a99995710649b274d Mon Sep 17 00:00:00 2001 From: counterpoint Date: Tue, 13 Oct 2015 16:21:17 +0200 Subject: [PATCH 66/85] Remove obsolete function. --- server/core/dcb.c | 40 ---------------------------------------- 1 file changed, 40 deletions(-) diff --git a/server/core/dcb.c b/server/core/dcb.c index 29ea757a1..0027abbb1 100644 --- a/server/core/dcb.c +++ b/server/core/dcb.c @@ -106,7 +106,6 @@ static int dcb_null_auth(DCB *dcb, SERVER *server, SESSION *session, GWBUF *buf static inline int dcb_isvalid_nolock(DCB *dcb); static inline DCB * dcb_find_in_list(DCB *dcb); static inline void dcb_process_victim_queue(DCB *listofdcb); -static void dcb_close_finish(DCB *); static bool dcb_maybe_add_persistent(DCB *); static inline bool dcb_write_parameter_check(DCB *dcb, GWBUF *queue); #if defined(FAKE_CODE) @@ -1901,45 +1900,6 @@ dcb_maybe_add_persistent(DCB *dcb) return false; } -/** - * Final calls for DCB close - * - * @param dcb The DCB to print - * - */ -static void -dcb_close_finish(DCB *dcb) -{ - poll_remove_dcb(dcb); - /* - * Return will always be 0 or function will have crashed, so we - * threw away return value. - */ - LOGIF(LD, (skygw_log_write( - LOGFILE_DEBUG, - "%lu [dcb_close] Removed dcb %p in state %s from poll set.", - pthread_self(), - dcb, - STRDCBSTATE(dcb->state)))); - /** - * Do a consistency check, then adjust counter if not from persistent pool - */ - if (dcb->server) - { - if (dcb->server->persistent) CHK_DCB(dcb->server->persistent); - if (0 == dcb->persistentstart) atomic_add(&dcb->server->stats.n_current, -1); - } - /** - * close protocol and router session - */ - if (dcb->func.close != NULL) - { - dcb->func.close(dcb); - } - /** Call possible callback for this DCB in case of close */ - dcb_call_callback(dcb, DCB_REASON_CLOSE); - } - /** * Diagnostic to print a DCB * From 12ceb0db0268056d852ea862127eee58bbf9eaa7 Mon Sep 17 00:00:00 2001 From: counterpoint Date: Thu, 15 Oct 2015 14:17:49 +0200 Subject: [PATCH 67/85] Check for dummy session in mysql_backend protocol and ignore. --- server/modules/protocol/mysql_backend.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/modules/protocol/mysql_backend.c b/server/modules/protocol/mysql_backend.c index d6ada9fa8..ba32b9882 100644 --- a/server/modules/protocol/mysql_backend.c +++ b/server/modules/protocol/mysql_backend.c @@ -286,7 +286,7 @@ static int gw_read_backend_event(DCB *dcb) { SESSION *session = dcb->session; int receive_rc = 0; - if (session == NULL) + if (SESSION_STATE_DUMMY == session->state) { rc = 0; goto return_with_lock; From 57f5dd15bc6168ccd6aeac554557c6dbf3041068 Mon Sep 17 00:00:00 2001 From: counterpoint Date: Fri, 16 Oct 2015 17:55:07 +0100 Subject: [PATCH 68/85] Resolve problem of lingering backend database processes; alter MySQL monitor to insert fake events when backend server unavailable; fix problem with count of current connections. --- server/core/dcb.c | 81 ++++++++++++++++++++++++++++++++++------------ server/core/poll.c | 47 +++++++++++++++++++++++++++ 2 files changed, 108 insertions(+), 20 deletions(-) diff --git a/server/core/dcb.c b/server/core/dcb.c index 0027abbb1..c92e2e2de 100644 --- a/server/core/dcb.c +++ b/server/core/dcb.c @@ -106,6 +106,7 @@ static int dcb_null_auth(DCB *dcb, SERVER *server, SESSION *session, GWBUF *buf static inline int dcb_isvalid_nolock(DCB *dcb); static inline DCB * dcb_find_in_list(DCB *dcb); static inline void dcb_process_victim_queue(DCB *listofdcb); +static void dcb_stop_polling_and_shutdown (DCB *dcb); static bool dcb_maybe_add_persistent(DCB *); static inline bool dcb_write_parameter_check(DCB *dcb, GWBUF *queue); #if defined(FAKE_CODE) @@ -568,7 +569,7 @@ dcb_process_victim_queue(DCB *listofdcb) else { DCB *nextdcb; - poll_remove_dcb(dcb); + dcb_stop_polling_and_shutdown(dcb); spinlock_acquire(&zombiespin); bitmask_copy(&dcb->memdata.bitmask, poll_bitmask()); nextdcb = dcb->memdata.next; @@ -586,21 +587,11 @@ dcb_process_victim_queue(DCB *listofdcb) * Into the final close logic, so if DCB is for backend server, we * must decrement the number of current connections. */ - if (dcb->server) + if (dcb->server && 0 == dcb->persistentstart) { atomic_add(&dcb->server->stats.n_current, -1); } - /** - * close protocol and router session - */ - if (dcb->func.close != NULL) - { - dcb->func.close(dcb); - } - /** Call possible callback for this DCB in case of close */ - dcb_call_callback(dcb, DCB_REASON_CLOSE); - - + if (dcb->fd > 0) { /*< @@ -651,6 +642,26 @@ dcb_process_victim_queue(DCB *listofdcb) LOGIF(LT, tls_log_info.li_sesid = 0); } +/** + * Remove a DCB from the poll list and trigger shutdown mechanisms. + * + * @param dcb The DCB to be processed + */ +static void +dcb_stop_polling_and_shutdown (DCB *dcb) +{ + poll_remove_dcb(dcb); + /** + * close protocol and router session + */ + if (dcb->func.close != NULL) + { + dcb->func.close(dcb); + } + /** Call possible callback for this DCB in case of close */ + dcb_call_callback(dcb, DCB_REASON_CLOSE); +} + /** * Connect to a server * @@ -2697,6 +2708,36 @@ dcb_call_foreach(struct server* server, DCB_REASON reason) return; } +/** + * Call all the callbacks on all DCB's that match the server and the reason given + * + * @param reason The DCB_REASON that triggers the callback + */ +void +dcb_hangup_foreach(struct server* server) +{ + LOGIF(LD, (skygw_log_write(LOGFILE_DEBUG, + "%lu [dcb_hangup_foreach]", + pthread_self()))); + + DCB *dcb; + spinlock_acquire(&dcbspin); + dcb = allDCBs; + + while (dcb != NULL) + { + spinlock_acquire(&dcb->dcb_initlock); + if (dcb->state == DCB_STATE_POLLING && dcb->server && + strcmp(dcb->server->unique_name,server->unique_name) == 0) + { + poll_fake_hangup_event(dcb); + } + spinlock_release(&dcb->dcb_initlock); + dcb = dcb->next; + } + spinlock_release(&dcbspin); +} + /** * Null protocol write routine used for cloned dcb's. It merely consumes @@ -2770,11 +2811,11 @@ dcb_persistent_clean_count(DCB *dcb, bool cleanall) CHK_DCB(persistentdcb); nextdcb = persistentdcb->nextpersistent; if (cleanall - || persistentdcb-> dcb_errhandle_called - || count >= server->persistpoolmax - || persistentdcb->server == NULL - || !(persistentdcb->server->status & SERVER_RUNNING) - || (time(NULL) - persistentdcb->persistentstart) > server->persistmaxtime) + || persistentdcb-> dcb_errhandle_called + || count >= server->persistpoolmax + || persistentdcb->server == NULL + || !(persistentdcb->server->status & SERVER_RUNNING) + || (time(NULL) - persistentdcb->persistentstart) > server->persistmaxtime) { /* Remove from persistent pool */ if (previousdcb) { @@ -2803,9 +2844,9 @@ dcb_persistent_clean_count(DCB *dcb, bool cleanall) { nextdcb = disposals->nextpersistent; disposals->persistentstart = -1; - if (DCB_STATE_POLLING == dcb->state) + if (DCB_STATE_POLLING == disposals->state) { - poll_remove_dcb(dcb); + dcb_stop_polling_and_shutdown(disposals); } dcb_close(disposals); disposals = nextdcb; diff --git a/server/core/poll.c b/server/core/poll.c index 2e6745987..41a66233c 100644 --- a/server/core/poll.c +++ b/server/core/poll.c @@ -1579,6 +1579,53 @@ uint32_t ev = EPOLLOUT; spinlock_release(&pollqlock); } +/* + * Insert a fake hangup event for a DCB into the polling queue. + * + * This is used when a monitor detects that a server is not responding. + * + * @param dcb DCB to emulate an EPOLLOUT event for + */ +void +poll_fake_hangup_event(DCB *dcb) +{ +uint32_t ev = EPOLLRDHUP; + + spinlock_acquire(&pollqlock); + if (DCB_POLL_BUSY(dcb)) + { + if (dcb->evq.pending_events == 0) + pollStats.evq_pending++; + dcb->evq.pending_events |= ev; + } + else + { + dcb->evq.pending_events = ev; + dcb->evq.inserted = hkheartbeat; + if (eventq) + { + dcb->evq.prev = eventq->evq.prev; + eventq->evq.prev->evq.next = dcb; + eventq->evq.prev = dcb; + dcb->evq.next = eventq; + } + else + { + eventq = dcb; + dcb->evq.prev = dcb; + dcb->evq.next = dcb; + } + pollStats.evq_length++; + pollStats.evq_pending++; + dcb->evq.inserted = hkheartbeat; + if (pollStats.evq_length > pollStats.evq_max) + { + pollStats.evq_max = pollStats.evq_length; + } + } + spinlock_release(&pollqlock); +} + /** * Print the event queue contents * From 482db5e84da4abc9da1bc1e09bb784c2fe283501 Mon Sep 17 00:00:00 2001 From: counterpoint Date: Sat, 17 Oct 2015 20:00:05 +0100 Subject: [PATCH 69/85] User friendly bit mask display for DCB print; monitors to work via inserting hangups instead of callbacks. --- server/core/dcb.c | 20 ++++- server/core/gwbitmask.c | 124 +++++++++++++++++++++++++++-- server/include/gwbitmask.h | 3 + server/include/poll.h | 7 +- server/modules/monitor/galeramon.c | 5 +- server/modules/monitor/mmmon.c | 3 +- server/modules/monitor/mysql_mon.c | 4 +- 7 files changed, 152 insertions(+), 14 deletions(-) diff --git a/server/core/dcb.c b/server/core/dcb.c index c92e2e2de..5c55be7dd 100644 --- a/server/core/dcb.c +++ b/server/core/dcb.c @@ -58,6 +58,7 @@ * 04/09/2015 Martin Brampton Changes to ensure DCB always has session pointer * 28/09/2015 Martin Brampton Add counters, maxima for DCBs and zombies * 29/05/2015 Martin Brampton Impose locking in dcb_call_foreach callbacks + * 17/10/2015 Martin Brampton Add hangup for each and bitmask display MaxAdmin * * @endverbatim */ @@ -544,6 +545,7 @@ dcb_process_victim_queue(DCB *listofdcb) /*< * Stop dcb's listening and modify state accordingly. */ + spinlock_acquire(&dcb->dcb_initlock); if (dcb->state == DCB_STATE_POLLING || dcb->state == DCB_STATE_LISTENING) { if (dcb->state == DCB_STATE_LISTENING) @@ -560,6 +562,7 @@ dcb_process_victim_queue(DCB *listofdcb) } else { /* Must be DCB_STATE_POLLING */ + spinlock_release(&dcb->dcb_initlock); if (0 == dcb->persistentstart && dcb_maybe_add_persistent(dcb)) { /* Have taken DCB into persistent pool, no further killing */ @@ -635,6 +638,7 @@ dcb_process_victim_queue(DCB *listofdcb) dcb->state = DCB_STATE_DISCONNECTED; nextdcb = dcb->memdata.next; + spinlock_release(&dcb->dcb_initlock); dcb_final_free(dcb); dcb = nextdcb; } @@ -1831,7 +1835,10 @@ dcb_close(DCB *dcb) /*< Set bit for each maxscale thread. This should be done before * the state is changed, so as to protect the DCB from premature * destruction. */ - bitmask_copy(&dcb->memdata.bitmask, poll_bitmask()); + if (dcb->server) + { + bitmask_copy(&dcb->memdata.bitmask, poll_bitmask()); + } } spinlock_release(&zombiespin); } @@ -2036,6 +2043,15 @@ dprintOneDCB(DCB *pdcb, DCB *dcb) dcb_printf(pdcb, "\tRole: %s\n", rolename); free(rolename); } + if (!bitmask_isallclear(&dcb->memdata.bitmask)) + { + char *bitmasktext = bitmask_render_readable(&dcb->memdata.bitmask); + if (bitmasktext) + { + dcb_printf(pdcb, "\tBitMask: %s\n", bitmasktext); + free(bitmasktext); + } + } dcb_printf(pdcb, "\tStatistics:\n"); dcb_printf(pdcb, "\t\tNo. of Reads: %d\n", dcb->stats.n_reads); dcb_printf(pdcb, "\t\tNo. of Writes: %d\n", dcb->stats.n_writes); @@ -2245,7 +2261,7 @@ gw_dcb_state2string (int state) case DCB_STATE_POLLING: return "DCB in the polling loop"; case DCB_STATE_NOPOLLING: - return "DCB not in the polling loop"; + return "DCB not in polling loop"; case DCB_STATE_LISTENING: return "DCB for listening socket"; case DCB_STATE_DISCONNECTED: diff --git a/server/core/gwbitmask.c b/server/core/gwbitmask.c index 765864d72..294a64c0f 100644 --- a/server/core/gwbitmask.c +++ b/server/core/gwbitmask.c @@ -17,6 +17,7 @@ */ #include #include +#include #include /** @@ -47,10 +48,14 @@ * Date Who Description * 28/06/13 Mark Riddoch Initial implementation * 20/08/15 Martin Brampton Added caveats about limitations (above) + * 17/10/15 Martin Brampton Added display of bitmask * * @endverbatim */ +static int bitmask_isset_without_spinlock(GWBITMASK *bitmask, int bit); +static int bitmask_count_bits_set(GWBITMASK *bitmask); + /** * Initialise a bitmask * @@ -151,19 +156,42 @@ unsigned char mask; * Return a non-zero value if the bit at the specified bit * position in the bitmask is set. * The bitmask will automatically be extended if the bit is - * beyond the current bitmask length. This could be optimised - * by assuming that a bit beyond the length is unset. + * beyond the current bitmask length. The work is done in the function + * bitmask_isset_without_spinlock, which can be called when a spinlock + * has already been acquired. * * @param bitmask Pointer the bitmask - * @param bit Bit to clear + * @param bit Bit to test */ int bitmask_isset(GWBITMASK *bitmask, int bit) { +int result; + + spinlock_acquire(&bitmask->lock); + result = bitmask_isset_without_spinlock(bitmask, bit); + spinlock_release(&bitmask->lock); + return result; +} + +/** + * Return a non-zero value if the bit at the specified bit + * position in the bitmask is set. Should be called while holding a + * lock on the bitmask. + * + * The bitmask will automatically be extended if the bit is + * beyond the current bitmask length. This could be optimised + * by assuming that a bit beyond the length is unset. + * + * @param bitmask Pointer the bitmask + * @param bit Bit to test + */ +static int +bitmask_isset_without_spinlock(GWBITMASK *bitmask, int bit) +{ unsigned char *ptr; unsigned char mask; - spinlock_acquire(&bitmask->lock); if (bit >= bitmask->length) { bitmask->bits = realloc(bitmask->bits, @@ -174,7 +202,6 @@ unsigned char mask; } ptr = bitmask->bits + (bit / 8); mask = 1 << (bit % 8); - spinlock_release(&bitmask->lock); return *ptr & mask; } @@ -239,3 +266,90 @@ bitmask_copy(GWBITMASK *dest, GWBITMASK *src) spinlock_release(&src->lock); } +/** + * Return a comma separated list of the numbers of the bits that are set in + * a bitmask, numbering starting at zero. Constrained to reject requests that + * could require more than three digit numbers. The returned string must be + * freed by the caller (unless it is null on account of memory allocation + * failure). + * + * @param bitmask Bitmap to make readable + * @return pointer to the newly allocated string, or null if no memory + */ +char * +bitmask_render_readable(GWBITMASK *bitmask) +{ + char *toobig = "Bitmask is too large to render readable"; + char *empty = "No bits are set";t + char onebit[5]; + char *result; + int count_set = 0; + + spinlock_acquire(&bitmask->lock); + if (999 < bitmask->length) + { + result = malloc(strlen(toobig)); + if (result) + { + strcpy(result, toobig); + } + } + else + { + count_set = bitmask_count_bits_set(bitmask); + if (count_set) + { + result = malloc(1 + (4 * count_set)); + if (result) + { + result[0] = 0; + for (int i = 0; ilength; i++) + { + if (bitmask_isset_without_spinlock(bitmask, i)) + { + sprintf(onebit, "%d,", i); + strcat(result, onebit); + } + } + result[strlen(result)-1] = 0; + } + } + else + { + result = malloc(strlen(empty)); + if (result) + { + strcpy(result, empty); + } + } + } + spinlock_release(&bitmask->lock); + return result; +} + +/** + * Return a count of the number of bits set in a bitmask. Helpful for setting + * the size of string needed to show the set bits in readable form. + * + * @param bitmask Bitmap whose bits are to be counted + * @return int Number of set bits + */ +static int +bitmask_count_bits_set(GWBITMASK *bitmask) +{ + const unsigned char oneBits[] = {0,1,1,2,1,2,2,3,1,2,2,3,2,3,3,4}; + unsigned char partresults; + int result = 0; + unsigned char *ptr, *eptr; + + ptr = bitmask->bits; + eptr = ptr + (bitmask->length / 8); + while (ptr < eptr) + { + partresults = oneBits[*ptr&0x0f]; + partresults += oneBits[*ptr>>4]; + result += partresults; + ptr++; + } + return result; +} \ No newline at end of file diff --git a/server/include/gwbitmask.h b/server/include/gwbitmask.h index baeb1ef77..26d5e3621 100644 --- a/server/include/gwbitmask.h +++ b/server/include/gwbitmask.h @@ -27,6 +27,7 @@ * * Date Who Description * 28/06/13 Mark Riddoch Initial implementation + * 17/10/15 Martin Brampton Add bitmask_render_readable * * @endverbatim */ @@ -51,4 +52,6 @@ extern void bitmask_clear(GWBITMASK *, int); extern int bitmask_isset(GWBITMASK *, int); extern int bitmask_isallclear(GWBITMASK *); extern void bitmask_copy(GWBITMASK *, GWBITMASK *); +extern char *bitmask_render_readable(GWBITMASK *bitmask); + #endif diff --git a/server/include/poll.h b/server/include/poll.h index e778c580c..f054667d4 100644 --- a/server/include/poll.h +++ b/server/include/poll.h @@ -29,6 +29,7 @@ * * Date Who Description * 19/06/13 Mark Riddoch Initial implementation + * 17/10/15 Martin Brampton Declare fake event functions * * @endverbatim */ @@ -60,9 +61,11 @@ extern void poll_set_maxwait(unsigned int); extern void poll_set_nonblocking_polls(unsigned int); extern void dprintPollStats(DCB *); extern void dShowThreads(DCB *dcb); -void poll_add_epollin_event_to_dcb(DCB* dcb, GWBUF* buf); +void poll_add_epollin_event_to_dcb(DCB* dcb, GWBUF* buf); extern void dShowEventQ(DCB *dcb); extern void dShowEventStats(DCB *dcb); -extern int poll_get_stat(POLL_STAT stat); +extern int poll_get_stat(POLL_STAT stat); extern RESULTSET *eventTimesGetList(); +extern void poll_fake_hangup_event(DCB *dcb); +extern void poll_fake_write_event(DCB *dcb); #endif diff --git a/server/modules/monitor/galeramon.c b/server/modules/monitor/galeramon.c index 2e3dd4ddd..5dde7a401 100644 --- a/server/modules/monitor/galeramon.c +++ b/server/modules/monitor/galeramon.c @@ -35,6 +35,7 @@ * 20/04/15 Guillaume Lefranc Added availableWhenDonor feature * 22/04/15 Martin Brampton Addition of disableMasterRoleSetting * 08/05/15 Markus Makela Addition of launchable scripts + * 17/10/15 Martin Brampton Change DCB callback to hangup * * @endverbatim */ @@ -541,13 +542,13 @@ monitor_event_t evtype; if (!(SERVER_IS_RUNNING(ptr->server)) || !(SERVER_IS_IN_CLUSTER(ptr->server))) { - dcb_call_foreach(ptr->server,DCB_REASON_NOT_RESPONDING); + dcb_hangup_foreach(ptr->server); } if (SERVER_IS_DOWN(ptr->server)) { /** Increase this server'e error count */ - dcb_call_foreach(ptr->server,DCB_REASON_NOT_RESPONDING); + dcb_hangup_foreach(ptr->server); ptr->mon_err_count += 1; } diff --git a/server/modules/monitor/mmmon.c b/server/modules/monitor/mmmon.c index 248fb92fd..1ada6ad8d 100644 --- a/server/modules/monitor/mmmon.c +++ b/server/modules/monitor/mmmon.c @@ -25,6 +25,7 @@ * Date Who Description * 08/09/14 Massimiliano Pinto Initial implementation * 08/05/15 Markus Makela Addition of launchable scripts + * 17/10/15 Martin Brampton Change DCB callback to hangup * * @endverbatim */ @@ -625,7 +626,7 @@ detect_stale_master = handle->detectStaleMaster; if (mon_status_changed(ptr)) { - dcb_call_foreach(ptr->server,DCB_REASON_NOT_RESPONDING); + dcb_hangup_foreach(ptr->server); } if (mon_status_changed(ptr) || diff --git a/server/modules/monitor/mysql_mon.c b/server/modules/monitor/mysql_mon.c index 4bb8a59b5..aca071c21 100644 --- a/server/modules/monitor/mysql_mon.c +++ b/server/modules/monitor/mysql_mon.c @@ -47,6 +47,7 @@ * 18/11/14 Massimiliano Pinto One server only in configuration becomes master. * servers=server1 must be present in mysql_mon and in router sections as well. * 08/05/15 Markus Makela Added launchable scripts + * 17/10/15 Martin Brampton Change DCB callback to hangup * * @endverbatim */ @@ -861,8 +862,7 @@ detect_stale_master = handle->detectStaleMaster; if (!(SERVER_IS_RUNNING(ptr->server)) || !(SERVER_IS_IN_CLUSTER(ptr->server))) { - dcb_call_foreach(ptr->server,DCB_REASON_NOT_RESPONDING); - + dcb_hangup_foreach(ptr->server); } From c74d320851307d8e0a332fdd99186103e228d725 Mon Sep 17 00:00:00 2001 From: counterpoint Date: Sat, 17 Oct 2015 20:02:59 +0100 Subject: [PATCH 70/85] Remove stray character. --- server/core/gwbitmask.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/core/gwbitmask.c b/server/core/gwbitmask.c index 294a64c0f..0068c846f 100644 --- a/server/core/gwbitmask.c +++ b/server/core/gwbitmask.c @@ -280,7 +280,7 @@ char * bitmask_render_readable(GWBITMASK *bitmask) { char *toobig = "Bitmask is too large to render readable"; - char *empty = "No bits are set";t + char *empty = "No bits are set"; char onebit[5]; char *result; int count_set = 0; From 6040f1107098f4bd29dfdae5e6efc24e9e6dacb3 Mon Sep 17 00:00:00 2001 From: counterpoint Date: Sat, 17 Oct 2015 20:06:37 +0100 Subject: [PATCH 71/85] Include DCB headers to remove warnings. --- server/modules/monitor/galeramon.c | 1 + server/modules/monitor/mmmon.c | 2 +- server/modules/monitor/mysql_mon.c | 1 + 3 files changed, 3 insertions(+), 1 deletion(-) diff --git a/server/modules/monitor/galeramon.c b/server/modules/monitor/galeramon.c index 5dde7a401..cea0e27ef 100644 --- a/server/modules/monitor/galeramon.c +++ b/server/modules/monitor/galeramon.c @@ -42,6 +42,7 @@ #include +#include /** Defined in log_manager.cc */ extern int lm_enabled_logfiles_bitmask; diff --git a/server/modules/monitor/mmmon.c b/server/modules/monitor/mmmon.c index 1ada6ad8d..794062e33 100644 --- a/server/modules/monitor/mmmon.c +++ b/server/modules/monitor/mmmon.c @@ -30,8 +30,8 @@ * @endverbatim */ - #include +#include /** Defined in log_manager.cc */ extern int lm_enabled_logfiles_bitmask; diff --git a/server/modules/monitor/mysql_mon.c b/server/modules/monitor/mysql_mon.c index aca071c21..d26ef8a75 100644 --- a/server/modules/monitor/mysql_mon.c +++ b/server/modules/monitor/mysql_mon.c @@ -54,6 +54,7 @@ #include +#include /** Defined in log_manager.cc */ extern int lm_enabled_logfiles_bitmask; From 6e9a2a89eb685d3dfcb3f118c9b2953d67454e2d Mon Sep 17 00:00:00 2001 From: counterpoint Date: Sat, 17 Oct 2015 20:09:05 +0100 Subject: [PATCH 72/85] Add dcb_hangup_foreach to DCB header. --- server/include/dcb.h | 1 + 1 file changed, 1 insertion(+) diff --git a/server/include/dcb.h b/server/include/dcb.h index c9312a128..22e8d21aa 100644 --- a/server/include/dcb.h +++ b/server/include/dcb.h @@ -339,6 +339,7 @@ int dcb_count_by_usage(DCB_USAGE); /* Return counts of DCBs */ int dcb_persistent_clean_count(DCB *, bool); /* Clean persistent and return count */ void dcb_call_foreach (struct server* server, DCB_REASON reason); +void dcb_hangup_foreach (struct server* server); size_t dcb_get_session_id(DCB* dcb); bool dcb_get_ses_log_info(DCB* dcb, size_t* sesid, int* enabled_logs); char *dcb_role_name(DCB *); /* Return the name of a role */ From 5112d4118f154f3b8e96c3156e6ff2c852c26877 Mon Sep 17 00:00:00 2001 From: Markus Makela Date: Fri, 16 Oct 2015 09:24:43 +0300 Subject: [PATCH 73/85] Fix to MXS-409: https://mariadb.atlassian.net/browse/MXS-409 Prepared statements are sent to the master instead of all servers. The planned functionality to store the types of the prepared statements was not implemented and all executions of prepared statements are sent to the master. Because of this the preparations should be all sent to the master server instead of sending them to all servers. --- server/modules/routing/readwritesplit/readwritesplit.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/server/modules/routing/readwritesplit/readwritesplit.c b/server/modules/routing/readwritesplit/readwritesplit.c index 113d57ea6..7a6ccc7d2 100644 --- a/server/modules/routing/readwritesplit/readwritesplit.c +++ b/server/modules/routing/readwritesplit/readwritesplit.c @@ -1381,8 +1381,6 @@ static route_target_t get_route_target ( * These queries are not affected by hints */ if (QUERY_IS_TYPE(qtype, QUERY_TYPE_SESSION_WRITE) || - QUERY_IS_TYPE(qtype, QUERY_TYPE_PREPARE_STMT) || - QUERY_IS_TYPE(qtype, QUERY_TYPE_PREPARE_NAMED_STMT) || /** Configured to allow writing variables to all nodes */ (use_sql_variables_in == TYPE_ALL && QUERY_IS_TYPE(qtype, QUERY_TYPE_GSYSVAR_WRITE)) || @@ -1432,6 +1430,8 @@ static route_target_t get_route_target ( QUERY_IS_TYPE(qtype, QUERY_TYPE_USERVAR_READ)|| /*< read user var */ QUERY_IS_TYPE(qtype, QUERY_TYPE_SYSVAR_READ) || /*< read sys var */ QUERY_IS_TYPE(qtype, QUERY_TYPE_EXEC_STMT) || /*< prepared stmt exec */ + QUERY_IS_TYPE(qtype, QUERY_TYPE_PREPARE_STMT) || + QUERY_IS_TYPE(qtype, QUERY_TYPE_PREPARE_NAMED_STMT) || QUERY_IS_TYPE(qtype, QUERY_TYPE_GSYSVAR_READ))) /*< read global sys var */ { /** First set expected targets before evaluating hints */ @@ -1449,6 +1449,8 @@ static route_target_t get_route_target ( if (QUERY_IS_TYPE(qtype, QUERY_TYPE_MASTER_READ) || QUERY_IS_TYPE(qtype, QUERY_TYPE_EXEC_STMT) || + QUERY_IS_TYPE(qtype, QUERY_TYPE_PREPARE_STMT) || + QUERY_IS_TYPE(qtype, QUERY_TYPE_PREPARE_NAMED_STMT) || /** Configured not to allow reading variables from slaves */ (use_sql_variables_in == TYPE_MASTER && (QUERY_IS_TYPE(qtype, QUERY_TYPE_USERVAR_READ) || From 33ba9e1bae07be51e5dd328e72e5527be4b158bb Mon Sep 17 00:00:00 2001 From: Markus Makela Date: Fri, 16 Oct 2015 22:27:38 +0300 Subject: [PATCH 74/85] Added missing maxadmin tests. --- client/CMakeLists.txt | 4 ++ client/test/CMakeLists.txt | 4 ++ client/test/maxadmin_stress.sh | 0 client/test/maxadmin_test.sh | 82 ++++++++++++++-------------------- 4 files changed, 41 insertions(+), 49 deletions(-) create mode 100644 client/test/CMakeLists.txt mode change 100644 => 100755 client/test/maxadmin_stress.sh diff --git a/client/CMakeLists.txt b/client/CMakeLists.txt index 32ab702ea..eb13ae659 100644 --- a/client/CMakeLists.txt +++ b/client/CMakeLists.txt @@ -8,3 +8,7 @@ else() message(STATUS "Could not find editline library. MaxAdmin will be built without it.") endif() install(TARGETS maxadmin DESTINATION ${MAXSCALE_BINDIR}) + +if(BUILD_TESTS) + add_subdirectory(test) +endif() diff --git a/client/test/CMakeLists.txt b/client/test/CMakeLists.txt new file mode 100644 index 000000000..e59eaf7f0 --- /dev/null +++ b/client/test/CMakeLists.txt @@ -0,0 +1,4 @@ +install(PROGRAMS maxadmin_test.sh DESTINATION ${CMAKE_BINARY_DIR}) +install(PROGRAMS maxadmin_stress.sh DESTINATION ${CMAKE_BINARY_DIR}) +add_test(TestMaxAdmin ${CMAKE_BINARY_DIR}/maxadmin_test.sh) +add_test(TestMaxAdminStress ${CMAKE_BINARY_DIR}/maxadmin_stress.sh) diff --git a/client/test/maxadmin_stress.sh b/client/test/maxadmin_stress.sh old mode 100644 new mode 100755 diff --git a/client/test/maxadmin_test.sh b/client/test/maxadmin_test.sh index c58c1512e..03976bc90 100755 --- a/client/test/maxadmin_test.sh +++ b/client/test/maxadmin_test.sh @@ -96,61 +96,45 @@ fi # # Test enable|disable log debug|trace|message|error # -maxadmin -pmariadb enable log debug >& /dev/null -if [ $? -eq "1" ]; then - echo "Enable debug log: Failed" - failure=`expr $failure + 1` -else - passed=`expr $passed + 1` - echo "Enable debug log: Passed" -fi -maxadmin -pmariadb enable log trace >& /dev/null -if [ $? -eq "1" ]; then - echo "Enable trace log: Failed" - failure=`expr $failure + 1` -else - passed=`expr $passed + 1` - echo "Enable trace log: Passed" -fi +for action in enable disable +do + maxadmin -pmariadb $action log debug >& /dev/null + if [ $? -eq "1" ]; then + echo "$action debug log: Failed" + failure=`expr $failure + 1` + else + passed=`expr $passed + 1` + echo "$action debug log: Passed" + fi -maxadmin -pmariadb enable log message >& /dev/null -if [ $? -eq "1" ]; then - echo "Enable message log: Failed" + maxadmin -pmariadb $action log trace >& /dev/null + if [ $? -eq "1" ]; then + echo "$action trace log: Failed" + failure=`expr $failure + 1` + else + passed=`expr $passed + 1` + echo "$action trace log: Passed" + fi + + maxadmin -pmariadb $action log message >& /dev/null + if [ $? -eq "1" ]; then + echo "$action message log: Failed" failure=`expr $failure + 1` -else + else passed=`expr $passed + 1` - echo "Enable message log: Passed" -fi + echo "$action message log: Passed" + fi -maxadmin -pmariadb enable log error >& /dev/null -if [ $? -eq "1" ]; then - echo "Enable error log: Failed" + maxadmin -pmariadb $action log error >& /dev/null + if [ $? -eq "1" ]; then + echo "$action error log: Failed" failure=`expr $failure + 1` -else + else passed=`expr $passed + 1` - echo "Enable error log: Passed" -fi - - - -maxadmin -pmariadb disable log debug >& /dev/null -if [ $? -eq "1" ]; then - echo "Disable debug log: Failed" - failure=`expr $failure + 1` -else - passed=`expr $passed + 1` - echo "Disable debug log: Passed" -fi - -maxadmin -pmariadb disable log trace >& /dev/null -if [ $? -eq "1" ]; then - echo "Disable trace log: Failed" - failure=`expr $failure + 1` -else - passed=`expr $passed + 1` - echo "Disable trace log: Passed" -fi + echo "$action error log: Passed" + fi +done # # Test restart monitor|service without, with invalid and with long invalid argument @@ -186,7 +170,7 @@ do done # -# Test set server qwerty master withaout, with invalid and with long invalid arg +# Test set server qwerty master without, with invalid and with long invalid arg # maxadmin -pmariadb set server qwerty >& /dev/null if [ $? -eq "1" ]; then From 040587c82830eb4544ebfcd7b2866e6bf9956c2f Mon Sep 17 00:00:00 2001 From: Markus Makela Date: Fri, 16 Oct 2015 22:30:08 +0300 Subject: [PATCH 75/85] Fix to MXS-413: https://mariadb.atlassian.net/browse/MXS-413 Added missing terminating newlines so showSession and improved maxadmin logic. --- client/maxadmin.c | 3 ++- server/core/session.c | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/client/maxadmin.c b/client/maxadmin.c index dec941d2f..6f8899163 100644 --- a/client/maxadmin.c +++ b/client/maxadmin.c @@ -466,7 +466,8 @@ int i, j, newline = 1; { if (newline == 1 && buf[j] == 'O') newline = 2; - else if (newline == 2 && buf[j] == 'K' && j == i - 1) + else if ((newline == 2 && buf[j] == 'K' && j == i - 1) || + (j == i - 2 && buf[j] == 'O' && buf[j + 1] == 'K')) { return 1; } diff --git a/server/core/session.c b/server/core/session.c index e9d45b8e7..7a2429bd0 100644 --- a/server/core/session.c +++ b/server/core/session.c @@ -715,11 +715,11 @@ int i; ptr->client->user?ptr->client->user:"", ptr->client->user?"@":"", ptr->client->remote); - dcb_printf(dcb, "\tConnected: %s", + dcb_printf(dcb, "\tConnected: %s\n", asctime_r(localtime_r(&ptr->stats.connect, &result), buf)); if(ptr->client->state == DCB_STATE_POLLING) { - dcb_printf(dcb, "\tIdle: %.0f seconds",idle); + dcb_printf(dcb, "\tIdle: %.0f seconds\n",idle); } } From 06f6b280486aa32404b3c2a13e7954b3217708b7 Mon Sep 17 00:00:00 2001 From: Markus Makela Date: Sat, 17 Oct 2015 18:01:58 +0300 Subject: [PATCH 76/85] Fix to MXS-412: https://mariadb.atlassian.net/browse/MXS-412 service->user is now set to NULL after the users are freed. --- server/core/service.c | 9 ++++++--- server/modules/routing/debugcmd.c | 24 +++++++++++++++++------- 2 files changed, 23 insertions(+), 10 deletions(-) diff --git a/server/core/service.c b/server/core/service.c index 0515c5218..b209c99d4 100644 --- a/server/core/service.c +++ b/server/core/service.c @@ -258,8 +258,8 @@ GWPROTOCOL *funcs; } if (loaded == -1) { - hashtable_free(service->users->data); - free(service->users); + users_free(service->users); + service->users = NULL; dcb_close(port->listener); port->listener = NULL; goto retblock; @@ -346,6 +346,7 @@ GWPROTOCOL *funcs; == NULL) { users_free(service->users); + service->users = NULL; dcb_close(port->listener); port->listener = NULL; LOGIF(LE, (skygw_log_write_flush( @@ -380,7 +381,8 @@ GWPROTOCOL *funcs; service->name))); users_free(service->users); - dcb_close(port->listener); + service->users = NULL; + dcb_close(port->listener); port->listener = NULL; goto retblock; } @@ -394,6 +396,7 @@ GWPROTOCOL *funcs; port->protocol, service->name))); users_free(service->users); + service->users = NULL; dcb_close(port->listener); port->listener = NULL; } diff --git a/server/modules/routing/debugcmd.c b/server/modules/routing/debugcmd.c index b88a67a19..bd15b54ec 100644 --- a/server/modules/routing/debugcmd.c +++ b/server/modules/routing/debugcmd.c @@ -678,13 +678,14 @@ static struct { * Convert a string argument to a numeric, observing prefixes * for number bases, e.g. 0x for hex, 0 for octal * + * @param dcb The client DCB * @param mode The CLI mode * @param arg The string representation of the argument * @param arg_type The target type for the argument * @return The argument as a long integer */ static unsigned long -convert_arg(int mode, char *arg, int arg_type) +convert_arg(DCB* dcb, int mode, char *arg, int arg_type) { unsigned long rval; SERVICE *service; @@ -710,9 +711,18 @@ SERVICE *service; { service = service_find(arg); if (service) + { + if(service->users == NULL) + { + dcb_printf(dcb, "The dbusers for service %s are not loaded. " + "Reload the dbusers and try again.\n", service->name); + } return (unsigned long)(service->users); + } else + { return 0; + } } return rval; case ARG_TYPE_DCB: @@ -927,7 +937,7 @@ int nskip = 0; cmds[i].options[j].fn(dcb); break; case 1: - arg1 = convert_arg(cli->mode, args[2],cmds[i].options[j].arg_types[0]); + arg1 = convert_arg(dcb, cli->mode, args[2],cmds[i].options[j].arg_types[0]); if (arg1) cmds[i].options[j].fn(dcb, arg1); @@ -936,8 +946,8 @@ int nskip = 0; args[2]); break; case 2: - arg1 = convert_arg(cli->mode, args[2],cmds[i].options[j].arg_types[0]); - arg2 = convert_arg(cli->mode, args[3],cmds[i].options[j].arg_types[1]); + arg1 = convert_arg(dcb, cli->mode, args[2],cmds[i].options[j].arg_types[0]); + arg2 = convert_arg(dcb, cli->mode, args[3],cmds[i].options[j].arg_types[1]); if (arg1 && arg2) cmds[i].options[j].fn(dcb, arg1, arg2); else if (arg1 == 0) @@ -948,9 +958,9 @@ int nskip = 0; args[3]); break; case 3: - arg1 = convert_arg(cli->mode, args[2],cmds[i].options[j].arg_types[0]); - arg2 = convert_arg(cli->mode, args[3],cmds[i].options[j].arg_types[1]); - arg3 = convert_arg(cli->mode, args[4],cmds[i].options[j].arg_types[2]); + arg1 = convert_arg(dcb, cli->mode, args[2],cmds[i].options[j].arg_types[0]); + arg2 = convert_arg(dcb, cli->mode, args[3],cmds[i].options[j].arg_types[1]); + arg3 = convert_arg(dcb, cli->mode, args[4],cmds[i].options[j].arg_types[2]); if (arg1 && arg2 && arg3) cmds[i].options[j].fn(dcb, arg1, arg2, arg3); else if (arg1 == 0) From f18d921f4c3644ebd40b62f2e54e746cdda7d7a8 Mon Sep 17 00:00:00 2001 From: Markus Makela Date: Mon, 19 Oct 2015 18:17:12 +0300 Subject: [PATCH 77/85] Fixed internal service test failing due to old assumptions. --- server/core/test/testservice.c | 22 ---------------------- 1 file changed, 22 deletions(-) diff --git a/server/core/test/testservice.c b/server/core/test/testservice.c index 5fa7e0436..5151fce44 100644 --- a/server/core/test/testservice.c +++ b/server/core/test/testservice.c @@ -84,28 +84,6 @@ init_test_env(NULL); result = serviceStartAll(); skygw_log_sync_all(); ss_info_dassert(0 != result, "Start all should succeed"); - - ss_dfprintf(stderr, "\t..done\nTiming out a session."); - - service->conn_timeout = 1; - result = serviceStart(service); - skygw_log_sync_all(); - ss_info_dassert(0 != result, "Start should succeed"); - serviceStop(service); - skygw_log_sync_all(); - ss_info_dassert(service->state == SERVICE_STATE_STOPPED, "Stop should succeed"); - - if((dcb = dcb_alloc(DCB_ROLE_REQUEST_HANDLER)) == NULL) - return 1; - ss_info_dassert(dcb != NULL, "DCB allocation failed"); - - session = session_alloc(service,dcb); - ss_info_dassert(session != NULL, "Session allocation failed"); - dcb->state = DCB_STATE_POLLING; - sleep(15); - - ss_info_dassert(dcb->state != DCB_STATE_POLLING, "Session timeout failed"); - ss_dfprintf(stderr, "\t..done\nStopping Service."); serviceStop(service); ss_info_dassert(service->state == SERVICE_STATE_STOPPED, "Stop should succeed"); From bad61b074021e511b598f42c9978c026a14a8348 Mon Sep 17 00:00:00 2001 From: counterpoint Date: Tue, 27 Oct 2015 14:17:06 +0000 Subject: [PATCH 78/85] Change binlog router to indicate it does not use router sessions via the getCapabilities interface. --- server/include/router.h | 6 ++++-- server/modules/protocol/mysql_backend.c | 3 ++- server/modules/routing/binlog/blr.c | 3 ++- 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/server/include/router.h b/server/include/router.h index 4faa2ca81..5a7152d1b 100644 --- a/server/include/router.h +++ b/server/include/router.h @@ -30,6 +30,7 @@ * 15/07/2013 Massimiliano Pinto Added clientReply entry point * 16/07/2013 Massimiliano Pinto Added router commands values * 22/10/2013 Massimiliano Pinto Added router errorReply entry point + * 27/10/2015 Martin Brampton Add RCAP_TYPE_NO_RSESSION * */ #include @@ -100,8 +101,9 @@ typedef struct router_object { */ typedef enum router_capability_t { RCAP_TYPE_UNDEFINED = 0x00, - RCAP_TYPE_STMT_INPUT = 0x01, /*< statement per buffer */ - RCAP_TYPE_PACKET_INPUT = 0x02 /*< data as it was read from DCB */ + RCAP_TYPE_STMT_INPUT = 0x01, /*< statement per buffer */ + RCAP_TYPE_PACKET_INPUT = 0x02, /*< data as it was read from DCB */ + RCAP_TYPE_NO_RSESSION = 0x04 /*< router does not use router sessions */ } router_capability_t; diff --git a/server/modules/protocol/mysql_backend.c b/server/modules/protocol/mysql_backend.c index ba32b9882..29217e7e3 100644 --- a/server/modules/protocol/mysql_backend.c +++ b/server/modules/protocol/mysql_backend.c @@ -45,6 +45,7 @@ * 10/11/2014 Massimiliano Pinto Client charset is passed to backend * 19/06/2015 Martin Brampton Persistent connection handling * 07/10/2015 Martin Brampton Remove calls to dcb_close - should be done by routers + * 27/10/2015 Martin Brampton Test for RCAP_TYPE_NO_RSESSION before calling clientReply * */ #include @@ -577,7 +578,7 @@ static int gw_read_backend_event(DCB *dcb) { if (dcb->session->state == SESSION_STATE_ROUTER_READY && dcb->session->client != NULL && dcb->session->client->state == DCB_STATE_POLLING && - session->router_session) + (session->router_session || router->getCapabilities(router_instance, NULL) & RCAP_TYPE_NO_RSESSION)) { client_protocol = SESSION_PROTOCOL(dcb->session, MySQLProtocol); diff --git a/server/modules/routing/binlog/blr.c b/server/modules/routing/binlog/blr.c index 51ee2b199..ce95f8895 100644 --- a/server/modules/routing/binlog/blr.c +++ b/server/modules/routing/binlog/blr.c @@ -38,6 +38,7 @@ * 07/05/2015 Massimiliano Pinto Addition of MariaDB 10 compatibility support * 12/06/2015 Massimiliano Pinto Addition of MariaDB 10 events in diagnostics() * 09/09/2015 Martin Brampton Modify error handler + * 27/10/2015 Martin Brampton Amend getCapabilities to return RCAP_TYPE_NO_RSESSION * * @endverbatim */ @@ -1178,7 +1179,7 @@ static void rses_end_locked_router_action(ROUTER_SLAVE * rses) static uint8_t getCapabilities(ROUTER *inst, void *router_session) { - return 0; + return RCAP_TYPE_NO_RSESSION; } /** From 1b04a0cf910fdfdd58f4bb5a49f5416b990d59cf Mon Sep 17 00:00:00 2001 From: counterpoint Date: Mon, 2 Nov 2015 16:27:44 +0000 Subject: [PATCH 79/85] Fix issues with error handling needing to cater for both client and backend DCBs. --- server/modules/routing/readconnroute.c | 35 +++++++++++--------------- 1 file changed, 15 insertions(+), 20 deletions(-) diff --git a/server/modules/routing/readconnroute.c b/server/modules/routing/readconnroute.c index 65eb8e8d4..79d0032dc 100644 --- a/server/modules/routing/readconnroute.c +++ b/server/modules/routing/readconnroute.c @@ -125,7 +125,7 @@ static void handleError( ROUTER *instance, void *router_session, GWBUF *errbuf, - DCB *backend_dcb, + DCB *problem_dcb, error_action_t action, bool *succp); static uint8_t getCapabilities (ROUTER* inst, void* router_session); @@ -837,12 +837,12 @@ clientReply( /** * Error Handler routine * - * The routine will handle errors that occurred in backend writes. + * The routine will handle errors that occurred in writes. * * @param instance The router instance * @param router_session The router session * @param message The error message to reply - * @param backend_dcb The backend DCB + * @param problem_dcb The DCB related to the error * @param action The action: ERRACT_NEW_CONNECTION or ERRACT_REPLY_CLIENT * @param succp Result of action: true if router can continue * @@ -851,18 +851,18 @@ static void handleError( ROUTER *instance, void *router_session, GWBUF *errbuf, - DCB *backend_dcb, + DCB *problem_dcb, error_action_t action, bool *succp) { DCB *client_dcb; - SESSION *session = backend_dcb->session; + SESSION *session = problem_dcb->session; session_state_t sesstate; ROUTER_CLIENT_SES *router_cli_ses = (ROUTER_CLIENT_SES *)router_session; /** Don't handle same error twice on same DCB */ - if (backend_dcb->dcb_errhandle_called) + if (problem_dcb->dcb_errhandle_called) { /** we optimistically assume that previous call succeed */ *succp = true; @@ -870,7 +870,7 @@ static void handleError( } else { - backend_dcb->dcb_errhandle_called = true; + problem_dcb->dcb_errhandle_called = true; } spinlock_acquire(&session->ses_lock); sesstate = session->state; @@ -887,20 +887,15 @@ static void handleError( spinlock_release(&session->ses_lock); } - if (router_cli_ses && router_cli_ses->backend_dcb) { - if (backend_dcb != router_cli_ses->backend_dcb) - { - /* Linkages have gone badly wrong */ - LOGIF(LE, (skygw_log_write(LOGFILE_ERROR, - "Read Connection Router error in handleError: router client " - "session DCB %p is not null, but does not match backend DCB %p " - "either. \n", - router_cli_ses->backend_dcb, - backend_dcb))); - } - router_cli_ses->backend_dcb = NULL; + if (dcb_isclient(problem_dcb)) + { + dcb_close(problem_dcb); + } + else if (router_cli_ses && problem_dcb == router_cli_ses->backend_dcb) + { + router_cli_ses->backend_dcb = NULL; + dcb_close(problem_dcb); } - dcb_close(backend_dcb); /** false because connection is not available anymore */ *succp = false; From 72072778de08c607d32490a9b489d3895b9a0ba2 Mon Sep 17 00:00:00 2001 From: counterpoint Date: Wed, 11 Nov 2015 08:39:47 +0000 Subject: [PATCH 80/85] Consume the given buffer in all cases, as caller has to assume that this will take place. --- server/modules/routing/readconnroute.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/server/modules/routing/readconnroute.c b/server/modules/routing/readconnroute.c index 79d0032dc..cd670285d 100644 --- a/server/modules/routing/readconnroute.c +++ b/server/modules/routing/readconnroute.c @@ -70,6 +70,7 @@ * 11/06/2015 Martin Brampton Remove decrement n_current (moved to dcb.c) * 09/09/2015 Martin Brampton Modify error handler * 25/09/2015 Martin Brampton Block callback processing when no router session in the DCB + * 09/11/2015 Martin Brampton Modified routeQuery - must free "queue" regardless of outcome * * @endverbatim */ @@ -731,6 +732,7 @@ routeQuery(ROUTER *instance, void *router_session, GWBUF *queue) "server.%s", mysql_command,rses_is_closed ? " Session is closed." : ""))); rc = 0; + while((queue = GWBUF_CONSUME_ALL(queue)) != NULL); goto return_rc; } From 1fc6b002118877f966e0b9840cc02779f3fcb1b6 Mon Sep 17 00:00:00 2001 From: counterpoint Date: Wed, 11 Nov 2015 09:03:48 +0000 Subject: [PATCH 81/85] Add conditionally compiled mechanism to "show buffers" to give a list of the currently allocated buffers, with a trace for each one of the calls that led to its creation. --- server/core/buffer.c | 128 +++++++++++++++++++++++++++++- server/include/buffer.h | 5 +- server/modules/routing/debugcmd.c | 10 ++- 3 files changed, 140 insertions(+), 3 deletions(-) diff --git a/server/core/buffer.c b/server/core/buffer.c index 8de0f6851..0c1990c6c 100644 --- a/server/core/buffer.c +++ b/server/core/buffer.c @@ -35,7 +35,9 @@ * 15/07/2014 Mark Riddoch Addition of properties * 28/08/2014 Mark Riddoch Adition of tail pointer to speed * the gwbuf_append process - * + * 09/11/2015 Martin Brampton Add buffer tracing (conditional compilation), + * accessed by "show buffers" maxadmin command + * * @endverbatim */ #include @@ -47,6 +49,13 @@ #include #include +#if defined(BUFFER_TRACE) +#include +#include + +static HASHTABLE *buffer_hashtable = NULL; +#endif + /** Defined in log_manager.cc */ extern int lm_enabled_logfiles_bitmask; extern size_t log_ses_count[]; @@ -56,6 +65,12 @@ static buffer_object_t* gwbuf_remove_buffer_object( GWBUF* buf, buffer_object_t* bufobj); +#if defined(BUFFER_TRACE) +static void gwbuf_add_to_hashtable(GWBUF *buf); +static int bhashfn (void *key); +static int bcmpfn (void *key1, void *key2); +static void gwbuf_remove_from_hashtable(GWBUF *buf); +#endif /** * Allocate a new gateway buffer structure of size bytes. @@ -119,9 +134,111 @@ retblock: "Error : Memory allocation failed due to %s.", strerror_r(errno, errbuf, sizeof(errbuf))))); } +#if defined(BUFFER_TRACE) + else + { + gwbuf_add_to_hashtable(rval); + } +#endif return rval; } +#if defined(BUFFER_TRACE) +/** + * Store a trace of buffer creation + * + * @param buf The buffer to record + */ +static void +gwbuf_add_to_hashtable(GWBUF *buf) +{ + void *array[16]; + size_t size, i, total; + char **strings; + char *tracetext; + + size = backtrace (array, 16); + strings = backtrace_symbols (array, size); + total = (2 * size) + 1; + for (i = 0; i < size; i++) + { + total += strlen(strings[i]); + } + tracetext = (char *)malloc(total); + if (tracetext) + { + char *ptr = tracetext; + for (i = 0; i < size; i++) + { + sprintf(ptr, "\t%s\n", strings[i]); + ptr += (strlen(strings[i]) + 2); + } + free (strings); + + if (NULL == buffer_hashtable) + { + buffer_hashtable = hashtable_alloc(10000, bhashfn, bcmpfn); + hashtable_memory_fns(buffer_hashtable,NULL,NULL,NULL,(HASHMEMORYFN)free); + } + hashtable_add(buffer_hashtable, buf, (void *)tracetext); + } +} + +/** + * Hash a buffer (address) to an integer + * + * @param key The pointer to the buffer + */ +static int +bhashfn(void *key) +{ + return (int)((uintptr_t) key % INT_MAX); +} + +/** + * Compare two buffer keys (pointers) + * + * @param key1 The pointer to the first buffer + * @param key2 The pointer to the second buffer + */ +static int +bcmpfn(void *key1, void *key2) +{ + return key1 == key2 ? 0 : 1; +} + +/** + * Remove a buffer from the store of buffer traces + * + * @param buf The buffer to be removed + */ +static void +gwbuf_remove_from_hashtable(GWBUF *buf) +{ + hashtable_delete(buffer_hashtable, buf); +} + +/** + * Print all buffer traces via a given print DCB + * + * @param pdcb Print DCB for output + */ +void +dprintAllBuffers(void *pdcb) +{ + void *buf; + char *backtrace; + HASHITERATOR *buffers = hashtable_iterator(buffer_hashtable); + while (NULL != (buf = hashtable_next(buffers))) + { + dcb_printf((DCB *)pdcb, "Buffer: %p\n", (void *)buf); + backtrace = hashtable_fetch(buffer_hashtable, buf); + dcb_printf((DCB *)pdcb, "%s", backtrace); + } + hashtable_iterator_free(buffers); +} +#endif + /** * Free a gateway buffer * @@ -162,6 +279,9 @@ BUF_PROPERTY *prop; buf->hint = buf->hint->next; hint_free(h); } +#if defined(BUFFER_TRACE) + gwbuf_remove_from_hashtable(buf); +#endif free(buf); } @@ -201,6 +321,9 @@ GWBUF *rval; rval->tail = rval; rval->next = NULL; CHK_GWBUF(rval); +#if defined(BUFFER_TRACE) + gwbuf_add_to_hashtable(rval); +#endif return rval; } @@ -268,6 +391,9 @@ GWBUF *gwbuf_clone_portion( clonebuf->next = NULL; clonebuf->tail = clonebuf; CHK_GWBUF(clonebuf); +#if defined(BUFFER_TRACE) + gwbuf_add_to_hashtable(clonebuf); +#endif return clonebuf; } diff --git a/server/include/buffer.h b/server/include/buffer.h index 6be9fc7aa..0e1b34246 100644 --- a/server/include/buffer.h +++ b/server/include/buffer.h @@ -43,6 +43,7 @@ * 03/10/2014 Martin Brampton Pointer arithmetic standard conformity * Add more buffer handling macros * Add gwbuf_rtrim (handle chains) + * 09/11/2014 Martin Brampton Add dprintAllBuffers (conditional compilation) * * @endverbatim */ @@ -52,7 +53,6 @@ #include #include - EXTERN_C_BLOCK_BEGIN /** @@ -202,6 +202,9 @@ void gwbuf_add_buffer_object(GWBUF* buf, void* data, void (*donefun_fp)(void *)); void* gwbuf_get_buffer_object_data(GWBUF* buf, bufobj_id_t id); +#if defined(BUFFER_TRACE) +extern void dprintAllBuffers(void *pdcb); +#endif EXTERN_C_BLOCK_END diff --git a/server/modules/routing/debugcmd.c b/server/modules/routing/debugcmd.c index bd15b54ec..5b444d87c 100644 --- a/server/modules/routing/debugcmd.c +++ b/server/modules/routing/debugcmd.c @@ -43,7 +43,8 @@ * 29/05/14 Mark Riddoch Add Filter support * 16/10/14 Mark Riddoch Add show eventq * 05/03/15 Massimiliano Pinto Added enable/disable feedback - * 27/05/15 Martin Brampton Add show persistent [server] + * 27/05/15 Martin Brampton Add show persistent [server] + * 06/11/15 Martin Brampton Add show buffers (conditional compilation) * * @endverbatim */ @@ -60,6 +61,7 @@ #include #include #include +#include #include #include #include @@ -107,6 +109,12 @@ static void telnetdShowUsers(DCB *); * The subcommands of the show command */ struct subcommand showoptions[] = { +#if defined(BUFFER_TRACE) + { "buffers", 0, dprintAllBuffers, + "Show all buffers with backtrace", + "Show all buffers with backtrace", + {0, 0, 0} }, +#endif { "dcbs", 0, dprintAllDCBs, "Show all descriptor control blocks (network connections)", "Show all descriptor control blocks (network connections)", From 2183d5d3c5f582ea82b5279ad1a127b879972edb Mon Sep 17 00:00:00 2001 From: counterpoint Date: Wed, 11 Nov 2015 11:31:07 +0000 Subject: [PATCH 82/85] Fix mistakes in merging. --- server/core/buffer.c | 5 - server/modules/routing/debugcmd.c | 207 +++++++++++++++--------------- 2 files changed, 105 insertions(+), 107 deletions(-) diff --git a/server/core/buffer.c b/server/core/buffer.c index 0c1990c6c..e8a1d9100 100644 --- a/server/core/buffer.c +++ b/server/core/buffer.c @@ -56,11 +56,6 @@ static HASHTABLE *buffer_hashtable = NULL; #endif -/** Defined in log_manager.cc */ -extern int lm_enabled_logfiles_bitmask; -extern size_t log_ses_count[]; -extern __thread log_info_t tls_log_info; - static buffer_object_t* gwbuf_remove_buffer_object( GWBUF* buf, buffer_object_t* bufobj); diff --git a/server/modules/routing/debugcmd.c b/server/modules/routing/debugcmd.c index 6dd780e66..894eec39e 100644 --- a/server/modules/routing/debugcmd.c +++ b/server/modules/routing/debugcmd.c @@ -112,104 +112,108 @@ static void telnetdShowUsers(DCB *); struct subcommand showoptions[] = { #if defined(BUFFER_TRACE) { "buffers", 0, dprintAllBuffers, - "Show all buffers with backtrace", - "Show all buffers with backtrace", - {0, 0, 0} }, + "Show all buffers with backtrace", + "Show all buffers with backtrace", + {0, 0, 0} }, #endif - { "dcbs", 0, dprintAllDCBs, - "Show all descriptor control blocks (network connections)", - "Show all descriptor control blocks (network connections)", - {0, 0, 0} }, - { "dcb", 1, dprintDCB, - "Show a single descriptor control block e.g. show dcb 0x493340", - "Show a single descriptor control block e.g. show dcb 0x493340", - {ARG_TYPE_DCB, 0, 0} }, - { "dbusers", 1, dcb_usersPrint, - "Show statistics and user names for a service's user table.\n\t\tExample : show dbusers ", - "Show statistics and user names for a service's user table.\n\t\tExample : show dbusers |", - {ARG_TYPE_DBUSERS, 0, 0} }, - { "epoll", 0, dprintPollStats, - "Show the poll statistics", - "Show the poll statistics", - {0, 0, 0} }, - { "eventq", 0, dShowEventQ, - "Show the queue of events waiting to be processed", - "Show the queue of events waiting to be processed", - {0, 0, 0} }, - { "eventstats", 0, dShowEventStats, - "Show the event statistics", - "Show the event statistics", - {0, 0, 0} }, - { "feedbackreport", 0, moduleShowFeedbackReport, - "Show the report of MaxScale loaded modules, suitable for Notification Service", - "Show the report of MaxScale loaded modules, suitable for Notification Service", - {0, 0, 0} }, - { "filter", 1, dprintFilter, - "Show details of a filter, called with a filter name", - "Show details of a filter, called with the address of a filter", - {ARG_TYPE_FILTER, 0, 0} }, - { "filters", 0, dprintAllFilters, - "Show all filters", - "Show all filters", - {0, 0, 0} }, - { "modules", 0, dprintAllModules, - "Show all currently loaded modules", - "Show all currently loaded modules", - {0, 0, 0} }, - { "monitor", 1, monitorShow, - "Show the monitor details", - "Show the monitor details", - {ARG_TYPE_MONITOR, 0, 0} }, - { "monitors", 0, monitorShowAll, - "Show the monitors that are configured", - "Show the monitors that are configured", - {0, 0, 0} }, - { "persistent", 1, dprintPersistentDCBs, - "Show persistent pool for a named server, e.g. show persistent dbnode1", - "Show persistent pool for a server, e.g. show persistent 0x485390. The address may also be replaced with the server name from the configuration file", - {ARG_TYPE_SERVER, 0, 0} }, - { "server", 1, dprintServer, - "Show details for a named server, e.g. show server dbnode1", - "Show details for a server, e.g. show server 0x485390. The address may also be repalced with the server name from the configuration file", - {ARG_TYPE_SERVER, 0, 0} }, - { "servers", 0, dprintAllServers, - "Show all configured servers", - "Show all configured servers", - {0, 0, 0} }, - { "serversjson", 0, dprintAllServersJson, - "Show all configured servers in JSON format", - "Show all configured servers in JSON format", - {0, 0, 0} }, - { "services", 0, dprintAllServices, - "Show all configured services in MaxScale", - "Show all configured services in MaxScale", - {0, 0, 0} }, - { "service", 1, dprintService, - "Show a single service in MaxScale, may be passed a service name", - "Show a single service in MaxScale, may be passed a service name or address of a service object", - {ARG_TYPE_SERVICE, 0, 0} }, - { "session", 1, dprintSession, - "Show a single session in MaxScale, e.g. show session 0x284830", - "Show a single session in MaxScale, e.g. show session 0x284830", - {ARG_TYPE_SESSION, 0, 0} }, - { "sessions", 0, dprintAllSessions, - "Show all active sessions in MaxScale", - "Show all active sessions in MaxScale", - {0, 0, 0} }, - { "tasks", 0, hkshow_tasks, - "Show all active housekeeper tasks in MaxScale", - "Show all active housekeeper tasks in MaxScale", - {0, 0, 0} }, - { "threads", 0, dShowThreads, - "Show the status of the polling threads in MaxScale", - "Show the status of the polling threads in MaxScale", - {0, 0, 0} }, - { "users", 0, telnetdShowUsers, - "Show statistics and user names for the debug interface", - "Show statistics and user names for the debug interface", - {0, 0, 0} }, - { NULL, 0, NULL, NULL, NULL, - {0, 0, 0} } + { "dcbs", 0, dprintAllDCBs, + "Show all descriptor control blocks (network connections)", + "Show all descriptor control blocks (network connections)", + {0, 0, 0} }, + { "dcb", 1, dprintDCB, + "Show a single descriptor control block e.g. show dcb 0x493340", + "Show a single descriptor control block e.g. show dcb 0x493340", + {ARG_TYPE_DCB, 0, 0} }, + { "dbusers", 1, dcb_usersPrint, + "Show statistics and user names for a service's user table.\n" + "\t\tExample : show dbusers ", + "Show statistics and user names for a service's user table.\n" + "\t\tExample : show dbusers |", + {ARG_TYPE_DBUSERS, 0, 0} }, + { "epoll", 0, dprintPollStats, + "Show the poll statistics", + "Show the poll statistics", + {0, 0, 0} }, + { "eventq", 0, dShowEventQ, + "Show the queue of events waiting to be processed", + "Show the queue of events waiting to be processed", + {0, 0, 0} }, + { "eventstats", 0, dShowEventStats, + "Show the event statistics", + "Show the event statistics", + {0, 0, 0} }, + { "feedbackreport", 0, moduleShowFeedbackReport, + "Show the report of MaxScale loaded modules, suitable for Notification Service", + "Show the report of MaxScale loaded modules, suitable for Notification Service", + {0, 0, 0} }, + { "filter", 1, dprintFilter, + "Show details of a filter, called with a filter name", + "Show details of a filter, called with the address of a filter", + {ARG_TYPE_FILTER, 0, 0} }, + { "filters", 0, dprintAllFilters, + "Show all filters", + "Show all filters", + {0, 0, 0} }, + { "modules", 0, dprintAllModules, + "Show all currently loaded modules", + "Show all currently loaded modules", + {0, 0, 0} }, + { "monitor", 1, monitorShow, + "Show the monitor details", + "Show the monitor details", + {ARG_TYPE_MONITOR, 0, 0} }, + { "monitors", 0, monitorShowAll, + "Show the monitors that are configured", + "Show the monitors that are configured", + {0, 0, 0} }, + { "persistent", 1, dprintPersistentDCBs, + "Show persistent pool for a named server, e.g. show persistent dbnode1", + "Show persistent pool for a server, e.g. show persistent 0x485390. " + "The address may also be replaced with the server name from the configuration file", + {ARG_TYPE_SERVER, 0, 0} }, + { "server", 1, dprintServer, + "Show details for a named server, e.g. show server dbnode1", + "Show details for a server, e.g. show server 0x485390. The address may also be " + "repalced with the server name from the configuration file", + {ARG_TYPE_SERVER, 0, 0} }, + { "servers", 0, dprintAllServers, + "Show all configured servers", + "Show all configured servers", + {0, 0, 0} }, + { "serversjson", 0, dprintAllServersJson, + "Show all configured servers in JSON format", + "Show all configured servers in JSON format", + {0, 0, 0} }, + { "services", 0, dprintAllServices, + "Show all configured services in MaxScale", + "Show all configured services in MaxScale", + {0, 0, 0} }, + { "service", 1, dprintService, + "Show a single service in MaxScale, may be passed a service name", + "Show a single service in MaxScale, may be passed a service name or address of a service object", + {ARG_TYPE_SERVICE, 0, 0} }, + { "session", 1, dprintSession, + "Show a single session in MaxScale, e.g. show session 0x284830", + "Show a single session in MaxScale, e.g. show session 0x284830", + {ARG_TYPE_SESSION, 0, 0} }, + { "sessions", 0, dprintAllSessions, + "Show all active sessions in MaxScale", + "Show all active sessions in MaxScale", + {0, 0, 0} }, + { "tasks", 0, hkshow_tasks, + "Show all active housekeeper tasks in MaxScale", + "Show all active housekeeper tasks in MaxScale", + {0, 0, 0} }, + { "threads", 0, dShowThreads, + "Show the status of the polling threads in MaxScale", + "Show the status of the polling threads in MaxScale", + {0, 0, 0} }, + { "users", 0, telnetdShowUsers, + "Show statistics and user names for the debug interface", + "Show statistics and user names for the debug interface", + {0, 0, 0} }, + { NULL, 0, NULL, NULL, NULL, + {0, 0, 0} } }; /** @@ -714,14 +718,13 @@ static struct { * Convert a string argument to a numeric, observing prefixes * for number bases, e.g. 0x for hex, 0 for octal * - * @param dcb The client DCB - * @param mode The CLI mode - * @param arg The string representation of the argument - * @param arg_type The target type for the argument + * @param mode The CLI mode + * @param arg The string representation of the argument + * @param arg_type The target type for the argument * @return The argument as a long integer */ static unsigned long -convert_arg(DCB* dcb, int mode, char *arg, int arg_type) +convert_arg(int mode, char *arg, int arg_type) { unsigned long rval; SERVICE *service; From 49d4a2019e40218195a0be6b109262ed63864ec5 Mon Sep 17 00:00:00 2001 From: counterpoint Date: Wed, 11 Nov 2015 15:43:23 +0000 Subject: [PATCH 83/85] Clarify and fix logic around router capabilities, with particular reference to crash relating to binlog router. --- server/modules/protocol/mysql_client.c | 101 ++++++------------------- 1 file changed, 25 insertions(+), 76 deletions(-) diff --git a/server/modules/protocol/mysql_client.c b/server/modules/protocol/mysql_client.c index 3597ceac9..113a10e0f 100644 --- a/server/modules/protocol/mysql_client.c +++ b/server/modules/protocol/mysql_client.c @@ -754,87 +754,32 @@ int gw_read_client_event( router_instance = session->service->router_instance; rsession = session->router_session; - if (router_instance != NULL && rsession != NULL) - { - - /** Ask what type of input the router expects */ - cap = router->getCapabilities(router_instance, rsession); - - if (cap == 0 || (cap == RCAP_TYPE_PACKET_INPUT)) - { - stmt_input = false; - } - else if (cap == RCAP_TYPE_STMT_INPUT) - { - stmt_input = true; - /** Mark buffer to as MySQL type */ - gwbuf_set_type(read_buffer, GWBUF_TYPE_MYSQL); - } - - /** - * If router doesn't implement getCapabilities correctly we end - * up here. - */ - else - { - GWBUF* errbuf; - bool succp; - - LOGIF(LD, (skygw_log_write_flush( - LOGFILE_DEBUG, - "%lu [gw_read_client_event] Reading router " - "capabilities failed.", - pthread_self()))); - - errbuf = mysql_create_custom_error( - 1, - 0, - "Read invalid router capabilities. Routing failed. " - "Session will be closed."); - - router->handleError( - router_instance, - rsession, - errbuf, - dcb, - ERRACT_REPLY_CLIENT, - &succp); - gwbuf_free(errbuf); - /** - * If there are not enough backends close - * session - */ - if (!succp) - { - LOGIF(LE, (skygw_log_write_flush( - LOGFILE_ERROR, - "Error : Routing the query failed. " - "Session will be closed."))); - } - rc = 1; - while (read_buffer) - { - read_buffer = gwbuf_consume(read_buffer, GWBUF_LENGTH(read_buffer)); - } - goto return_rc; - } - } - else - { - /** Send ERR 1045 to client */ - mysql_send_auth_error( + if (NULL == router_instance || NULL == rsession) + { + /** Send ERR 1045 to client */ + mysql_send_auth_error( dcb, 2, 0, "failed to create new session"); - while (read_buffer) - { - read_buffer = gwbuf_consume(read_buffer, GWBUF_LENGTH(read_buffer)); - } - return 0; - } + while (read_buffer) + { + read_buffer = gwbuf_consume(read_buffer, GWBUF_LENGTH(read_buffer)); + } + return 0; } + /** Ask what type of input the router expects */ + cap = router->getCapabilities(router_instance, rsession); + + if (cap & RCAP_TYPE_STMT_INPUT) + { + stmt_input = true; + /** Mark buffer to as MySQL type */ + gwbuf_set_type(read_buffer, GWBUF_TYPE_MYSQL); + } + } + if (stmt_input) { /** @@ -1162,12 +1107,16 @@ int gw_read_client_event( read_buffer = NULL; } } - else + else if (NULL != session->router_session || cap & RCAP_TYPE_NO_RSESSION) { /** Feed whole packet to router */ rc = SESSION_ROUTE_QUERY(session, read_buffer); read_buffer = NULL; } + else + { + rc = 0; + } /** Routing succeed */ if (rc) From 30d1fc66b7dcd7877b2d6b962b22e2aa75d0f1f5 Mon Sep 17 00:00:00 2001 From: Markus Makela Date: Wed, 11 Nov 2015 17:41:48 +0200 Subject: [PATCH 84/85] getCapabilities no longer takes arguments and returns an int. --- server/core/test/CMakeLists.txt | 2 +- server/include/router.h | 2 +- server/modules/include/readwritesplit.h | 1 - server/modules/include/schemarouter.h | 1 - server/modules/include/shardrouter.h | 1 - server/modules/protocol/mysql_backend.c | 2 +- server/modules/routing/binlog/blr.c | 5 ++-- server/modules/routing/cli.c | 6 ++-- server/modules/routing/debugcli.c | 6 ++-- server/modules/routing/maxinfo/maxinfo.c | 8 ++---- server/modules/routing/readconnroute.c | 8 ++---- .../routing/readwritesplit/readwritesplit.c | 25 +++-------------- .../routing/schemarouter/schemarouter.c | 25 +++-------------- .../routing/schemarouter/shardrouter.c | 28 ++++--------------- server/modules/routing/testroute.c | 6 ++-- 15 files changed, 30 insertions(+), 96 deletions(-) diff --git a/server/core/test/CMakeLists.txt b/server/core/test/CMakeLists.txt index 6fc1080d2..149a38f09 100644 --- a/server/core/test/CMakeLists.txt +++ b/server/core/test/CMakeLists.txt @@ -14,7 +14,7 @@ add_executable(test_users testusers.c) add_executable(test_adminusers testadminusers.c) add_executable(testmemlog testmemlog.c ../random_jkiss.c) add_executable(testfeedback testfeedback.c) -add_executable(testmaxscalepcre2 testmaxscalepcre2.c) +add_executable(testmaxscalepcre2 testmaxscalepcre2.c ../random_jkiss.c) target_link_libraries(test_mysql_users MySQLClient fullcore) target_link_libraries(test_hash fullcore log_manager) target_link_libraries(test_hint fullcore log_manager) diff --git a/server/include/router.h b/server/include/router.h index 5a7152d1b..655075be5 100644 --- a/server/include/router.h +++ b/server/include/router.h @@ -86,7 +86,7 @@ typedef struct router_object { DCB* backend_dcb, error_action_t action, bool* succp); - uint8_t (*getCapabilities)(ROUTER *instance, void* router_session); + int (*getCapabilities)(); } ROUTER_OBJECT; /** diff --git a/server/modules/include/readwritesplit.h b/server/modules/include/readwritesplit.h index 58eb48996..0e229f807 100644 --- a/server/modules/include/readwritesplit.h +++ b/server/modules/include/readwritesplit.h @@ -290,7 +290,6 @@ struct router_client_session { rwsplit_config_t rses_config; /*< copied config info from router instance */ int rses_nbackends; int rses_nsescmd; /*< Number of executed session commands */ - int rses_capabilities; /*< input type, for example */ bool rses_autocommit_enabled; bool rses_transaction_active; bool rses_load_active; /*< If LOAD DATA LOCAL INFILE is diff --git a/server/modules/include/schemarouter.h b/server/modules/include/schemarouter.h index e184e96e3..d4f220903 100644 --- a/server/modules/include/schemarouter.h +++ b/server/modules/include/schemarouter.h @@ -316,7 +316,6 @@ struct router_client_session { backend_ref_t* rses_backend_ref; /*< Pointer to backend reference array */ schemarouter_config_t rses_config; /*< Copied config info from router instance */ int rses_nbackends; /*< Number of backends */ - int rses_capabilities; /*< Input type, for example */ bool rses_autocommit_enabled; /*< Is autocommit enabled */ bool rses_transaction_active; /*< Is a transaction active */ struct router_instance *router; /*< The router instance */ diff --git a/server/modules/include/shardrouter.h b/server/modules/include/shardrouter.h index 3fc70fcc1..f288505c1 100644 --- a/server/modules/include/shardrouter.h +++ b/server/modules/include/shardrouter.h @@ -192,7 +192,6 @@ struct router_client_session { rses_property_t* rses_properties[RSES_PROP_TYPE_COUNT]; shard_config_t rses_config; /*< copied config info from router instance */ - int rses_capabilities; /*< input type, for example */ bool rses_autocommit_enabled; bool rses_transaction_active; struct router_instance *router; /*< The router instance */ diff --git a/server/modules/protocol/mysql_backend.c b/server/modules/protocol/mysql_backend.c index 29e5be84a..e48c00f98 100644 --- a/server/modules/protocol/mysql_backend.c +++ b/server/modules/protocol/mysql_backend.c @@ -573,7 +573,7 @@ static int gw_read_backend_event(DCB *dcb) { if (dcb->session->state == SESSION_STATE_ROUTER_READY && dcb->session->client != NULL && dcb->session->client->state == DCB_STATE_POLLING && - (session->router_session || router->getCapabilities(router_instance, NULL) & RCAP_TYPE_NO_RSESSION)) + (session->router_session || router->getCapabilities() & RCAP_TYPE_NO_RSESSION)) { client_protocol = SESSION_PROTOCOL(dcb->session, MySQLProtocol); diff --git a/server/modules/routing/binlog/blr.c b/server/modules/routing/binlog/blr.c index 329e60d0c..854ef1f28 100644 --- a/server/modules/routing/binlog/blr.c +++ b/server/modules/routing/binlog/blr.c @@ -99,7 +99,7 @@ static void errorReply( error_action_t action, bool *succp); -static uint8_t getCapabilities (ROUTER* inst, void* router_session); +static int getCapabilities (); static int blr_handler_config(void *userdata, const char *section, const char *name, const char *value); static int blr_handle_config_item(const char *name, const char *value, ROUTER_INSTANCE *inst); static int blr_set_service_mysql_user(SERVICE *service); @@ -113,7 +113,6 @@ extern int blr_read_events_all_events(ROUTER_INSTANCE *router, int fix, int debu void blr_master_close(ROUTER_INSTANCE *); char * blr_last_event_description(ROUTER_INSTANCE *router); extern int MaxScaleUptime(); -static uint8_t getCapabilities (ROUTER* inst, void* router_session); char *blr_get_event_description(ROUTER_INSTANCE *router, uint8_t event); /** The module object definition */ @@ -1517,7 +1516,7 @@ static void rses_end_locked_router_action(ROUTER_SLAVE * rses) } -static uint8_t getCapabilities(ROUTER *inst, void *router_session) +static int getCapabilities() { return RCAP_TYPE_NO_RSESSION; } diff --git a/server/modules/routing/cli.c b/server/modules/routing/cli.c index 311c15c3f..19bb15cc7 100644 --- a/server/modules/routing/cli.c +++ b/server/modules/routing/cli.c @@ -62,7 +62,7 @@ static void closeSession(ROUTER *instance, void *router_session); static void freeSession(ROUTER *instance, void *router_session); static int execute(ROUTER *instance, void *router_session, GWBUF *queue); static void diagnostics(ROUTER *instance, DCB *dcb); -static uint8_t getCapabilities (ROUTER* inst, void* router_session); +static int getCapabilities (); /** The module object definition */ static ROUTER_OBJECT MyObject = { @@ -287,9 +287,7 @@ diagnostics(ROUTER *instance, DCB *dcb) return; /* Nothing to do currently */ } -static uint8_t getCapabilities( - ROUTER* inst, - void* router_session) +static int getCapabilities() { return 0; } diff --git a/server/modules/routing/debugcli.c b/server/modules/routing/debugcli.c index 3467136e6..9f6213809 100644 --- a/server/modules/routing/debugcli.c +++ b/server/modules/routing/debugcli.c @@ -61,7 +61,7 @@ static void closeSession(ROUTER *instance, void *router_session); static void freeSession(ROUTER *instance, void *router_session); static int execute(ROUTER *instance, void *router_session, GWBUF *queue); static void diagnostics(ROUTER *instance, DCB *dcb); -static uint8_t getCapabilities (ROUTER* inst, void* router_session); +static int getCapabilities (); /** The module object definition */ static ROUTER_OBJECT MyObject = { @@ -310,9 +310,7 @@ diagnostics(ROUTER *instance, DCB *dcb) return; /* Nothing to do currently */ } -static uint8_t getCapabilities( - ROUTER* inst, - void* router_session) +static int getCapabilities() { return 0; } diff --git a/server/modules/routing/maxinfo/maxinfo.c b/server/modules/routing/maxinfo/maxinfo.c index c4de0743d..04d74ca7c 100644 --- a/server/modules/routing/maxinfo/maxinfo.c +++ b/server/modules/routing/maxinfo/maxinfo.c @@ -82,7 +82,7 @@ static void closeSession(ROUTER *instance, void *router_session); static void freeSession(ROUTER *instance, void *router_session); static int execute(ROUTER *instance, void *router_session, GWBUF *queue); static void diagnostics(ROUTER *instance, DCB *dcb); -static uint8_t getCapabilities (ROUTER* inst, void* router_session); +static int getCapabilities (); static void handleError( ROUTER *instance, void *router_session, @@ -415,10 +415,8 @@ diagnostics(ROUTER *instance, DCB *dcb) * * Not used for the maxinfo router */ -static uint8_t -getCapabilities( - ROUTER* inst, - void* router_session) +static int +getCapabilities() { return 0; } diff --git a/server/modules/routing/readconnroute.c b/server/modules/routing/readconnroute.c index 11310a9ec..545cb5423 100644 --- a/server/modules/routing/readconnroute.c +++ b/server/modules/routing/readconnroute.c @@ -124,7 +124,7 @@ static void handleError( DCB *problem_dcb, error_action_t action, bool *succp); -static uint8_t getCapabilities (ROUTER* inst, void* router_session); +static int getCapabilities (); /** The module object definition */ @@ -957,11 +957,9 @@ static void rses_end_locked_router_action( } -static uint8_t getCapabilities( - ROUTER* inst, - void* router_session) +static int getCapabilities() { - return 0; + return RCAP_TYPE_PACKET_INPUT; } /******************************** diff --git a/server/modules/routing/readwritesplit/readwritesplit.c b/server/modules/routing/readwritesplit/readwritesplit.c index 63fa57ad0..086dc4514 100644 --- a/server/modules/routing/readwritesplit/readwritesplit.c +++ b/server/modules/routing/readwritesplit/readwritesplit.c @@ -130,7 +130,7 @@ static bool route_single_stmt( GWBUF* querybuf); -static uint8_t getCapabilities (ROUTER* inst, void* router_session); +static int getCapabilities(); #if defined(NOT_USED) static bool router_option_configured( @@ -912,7 +912,6 @@ static void* newSession( client_rses->rses_master_ref = master_ref; /* assert with master_host */ ss_dassert(master_ref && (master_ref->bref_backend->backend_server && SERVER_MASTER)); - client_rses->rses_capabilities = RCAP_TYPE_STMT_INPUT; client_rses->rses_backend_ref = backend_ref; client_rses->rses_nbackends = router_nservers; /*< # of backend servers */ @@ -4447,27 +4446,11 @@ static void tracelog_routed_query( /** - * Return rc, rc < 0 if router session is closed. rc == 0 if there are no - * capabilities specified, rc > 0 when there are capabilities. + * Return RCAP_TYPE_STMT_INPUT. */ -static uint8_t getCapabilities ( - ROUTER* inst, - void* router_session) +static int getCapabilities () { - ROUTER_CLIENT_SES* rses = (ROUTER_CLIENT_SES *)router_session; - uint8_t rc; - - if (!rses_begin_locked_router_action(rses)) - { - rc = 0xff; - goto return_rc; - } - rc = rses->rses_capabilities; - - rses_end_locked_router_action(rses); - -return_rc: - return rc; + return RCAP_TYPE_STMT_INPUT; } /** diff --git a/server/modules/routing/schemarouter/schemarouter.c b/server/modules/routing/schemarouter/schemarouter.c index 59ca01830..3bf85d16c 100644 --- a/server/modules/routing/schemarouter/schemarouter.c +++ b/server/modules/routing/schemarouter/schemarouter.c @@ -99,7 +99,7 @@ static route_target_t get_shard_route_target ( bool trx_active, HINT* hint); -static uint8_t getCapabilities (ROUTER* inst, void* router_session); +static int getCapabilities (); static bool connect_backend_servers( backend_ref_t* backend_ref, @@ -1241,7 +1241,6 @@ static void* newSession( goto return_rses; } /** Copy backend pointers to router session. */ - client_rses->rses_capabilities = RCAP_TYPE_STMT_INPUT; client_rses->rses_backend_ref = backend_ref; client_rses->rses_nbackends = router_nservers; /*< # of backend servers */ @@ -3934,27 +3933,11 @@ static void tracelog_routed_query( /** - * Return rc, rc < 0 if router session is closed. rc == 0 if there are no - * capabilities specified, rc > 0 when there are capabilities. + * Return RCAP_TYPE_STMT_INPUT. */ -static uint8_t getCapabilities ( - ROUTER* inst, - void* router_session) +static int getCapabilities () { - ROUTER_CLIENT_SES* rses = (ROUTER_CLIENT_SES *)router_session; - uint8_t rc; - - if (!rses_begin_locked_router_action(rses)) - { - rc = 0xff; - goto return_rc; - } - rc = rses->rses_capabilities; - - rses_end_locked_router_action(rses); - -return_rc: - return rc; + return RCAP_TYPE_STMT_INPUT; } /** diff --git a/server/modules/routing/schemarouter/shardrouter.c b/server/modules/routing/schemarouter/shardrouter.c index 45020beef..8e7dd479f 100644 --- a/server/modules/routing/schemarouter/shardrouter.c +++ b/server/modules/routing/schemarouter/shardrouter.c @@ -116,7 +116,7 @@ static route_target_t get_shard_route_target( bool trx_active, HINT* hint); -static uint8_t getCapabilities(ROUTER* inst, void* router_session); +static int getCapabilities(); void subsvc_clear_state(SUBSERVICE* svc,subsvc_state_t state); void subsvc_set_state(SUBSERVICE* svc,subsvc_state_t state); @@ -1158,9 +1158,6 @@ newSession( free(dummy_upstream); } - - /** Copy backend pointers to router session. */ - client_rses->rses_capabilities = RCAP_TYPE_STMT_INPUT; router->stats.n_sessions += 1; /** @@ -2571,27 +2568,12 @@ mysql_sescmd_get_property( } /** - * Return rc, rc < 0 if router session is closed. rc == 0 if there are no - * capabilities specified, rc > 0 when there are capabilities. + * Return RCAP_TYPE_STMT_INPUT */ -static uint8_t -getCapabilities(ROUTER* inst, - void* router_session) +static int +getCapabilities() { - ROUTER_CLIENT_SES* rses = (ROUTER_CLIENT_SES *) router_session; - uint8_t rc; - - if(!rses_begin_locked_router_action(rses)) - { - rc = 0xff; - goto return_rc; - } - rc = rses->rses_capabilities; - - rses_end_locked_router_action(rses); - -return_rc: - return rc; + return RCAP_TYPE_STMT_INPUT; } /** diff --git a/server/modules/routing/testroute.c b/server/modules/routing/testroute.c index 968e3892c..b0d963601 100644 --- a/server/modules/routing/testroute.c +++ b/server/modules/routing/testroute.c @@ -35,7 +35,7 @@ static void freeSession(ROUTER *instance, void *session); static int routeQuery(ROUTER *instance, void *session, GWBUF *queue); static void clientReply(ROUTER *instance, void *session, GWBUF *queue,DCB*); static void diagnostic(ROUTER *instance, DCB *dcb); -static uint8_t getCapabilities (ROUTER* inst, void* router_session); +static int getCapabilities (); static void handleError( ROUTER *instance, void *router_session, @@ -166,9 +166,7 @@ diagnostic(ROUTER *instance, DCB *dcb) { } -static uint8_t getCapabilities( - ROUTER* inst, - void* router_session) +static int getCapabilities() { return 0; } From 76856e6c9080a2116b6a6bded65af4c5eceef35c Mon Sep 17 00:00:00 2001 From: Markus Makela Date: Fri, 13 Nov 2015 11:53:12 +0200 Subject: [PATCH 85/85] Fixed compiler warnings. --- server/core/secrets.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/core/secrets.c b/server/core/secrets.c index cc23d312a..01cf66315 100644 --- a/server/core/secrets.c +++ b/server/core/secrets.c @@ -331,7 +331,7 @@ decryptPassword(const char *crypt) MAXKEYS *keys; AES_KEY aeskey; unsigned char *plain; - char *ptr; + const char *ptr; unsigned char encrypted[80]; int enlen;