From 8201c93238b743363a97188b6092f5cb163c2cf6 Mon Sep 17 00:00:00 2001 From: Adi Zaimi Date: Fri, 10 Jan 2020 11:16:05 -0500 Subject: [PATCH 1/9] fix bad msglen and cleanup osql_comm_signal_sqlthr_rc --- db/osqlcomm.c | 174 ++++++++++++++++++++------------------------------ 1 file changed, 71 insertions(+), 103 deletions(-) diff --git a/db/osqlcomm.c b/db/osqlcomm.c index 734da3894b..5c759ba1e0 100644 --- a/db/osqlcomm.c +++ b/db/osqlcomm.c @@ -5092,26 +5092,10 @@ int osql_comm_send_socksqlreq(char *tohost, const char *sql, int sqlen, int osql_comm_signal_sqlthr_rc(osql_sess_t *sorese, struct errstat *xerr, int rc) { - - int irc = 0; int msglen = 0; char uuid[37]; int type; - - /* slightly kludgy - we're constructing one of 4 message types - get a - * buffer - * big enough for the biggest of them */ - uint8_t *buf; - int max = 0; - if (OSQLCOMM_DONE_XERR_UUID_RPL_LEN > max) - max = OSQLCOMM_DONE_XERR_UUID_RPL_LEN; - if (OSQLCOMM_DONE_UUID_RPL_LEN > max) - max = OSQLCOMM_DONE_UUID_RPL_LEN; - if (OSQLCOMM_DONE_XERR_RPL_LEN > max) - max = OSQLCOMM_DONE_XERR_RPL_LEN; - if (OSQLCOMM_DONE_RPL_LEN > max) - max = OSQLCOMM_DONE_RPL_LEN; - buf = alloca(max); + uint8_t *buf = NULL; /* test if the sql thread was the one closing the request, and if so, don't send anything back @@ -5121,106 +5105,90 @@ int osql_comm_signal_sqlthr_rc(osql_sess_t *sorese, struct errstat *xerr, return 0; /* if error, lets send the error string */ - if (sorese->host) { - /* remote */ - if (sorese->rqid == OSQL_RQID_USE_UUID) { - osql_done_xerr_uuid_t rpl_xerr = {{0}}; - osql_done_uuid_rpl_t rpl_ok = {{0}}; - - if (rc) { - uint8_t *p_buf = buf; - uint8_t *p_buf_end = buf + OSQLCOMM_DONE_XERR_UUID_RPL_LEN; - rpl_xerr.hd.type = OSQL_XERR; - comdb2uuidcpy(rpl_xerr.hd.uuid, sorese->uuid); - rpl_xerr.dt = *xerr; - - osqlcomm_done_xerr_uuid_type_put(&(rpl_xerr), p_buf, p_buf_end); - logmsg(LOGMSG_DEBUG, - "%s line %d master signaling %s uuid %s with rc=%d " - "xerr=%d\n", - __func__, __LINE__, sorese->host, - comdb2uuidstr(sorese->uuid, uuid), rc, xerr->errval); - - msglen = OSQLCOMM_DONE_XERR_UUID_RPL_LEN; - - } else { - uint8_t *p_buf = buf; - uint8_t *p_buf_end = buf + OSQLCOMM_DONE_UUID_RPL_LEN; + if (!sorese->host) { + /* local */ + return osql_chkboard_sqlsession_rc(sorese->rqid, sorese->uuid, + sorese->nops, NULL, xerr); + } - rpl_ok.hd.type = OSQL_DONE; - comdb2uuidcpy(rpl_ok.hd.uuid, sorese->uuid); - rpl_ok.dt.rc = 0; - rpl_ok.dt.nops = sorese->nops; + /* remote */ + if (sorese->rqid == OSQL_RQID_USE_UUID) { + osql_done_xerr_uuid_t rpl_xerr = {{0}}; + osql_done_uuid_rpl_t rpl_ok = {{0}}; - osqlcomm_done_uuid_rpl_put(&(rpl_ok), p_buf, p_buf_end); + if (rc) { + msglen = OSQLCOMM_DONE_XERR_UUID_RPL_LEN; + buf = alloca(msglen); + uint8_t *p_buf = buf; + uint8_t *p_buf_end = buf + msglen; + rpl_xerr.hd.type = OSQL_XERR; + comdb2uuidcpy(rpl_xerr.hd.uuid, sorese->uuid); + rpl_xerr.dt = *xerr; - logmsg(LOGMSG_DEBUG, - "%s line %d master signaling %s uuid %s with rc=%d " - "xerr=%d\n", - __func__, __LINE__, sorese->host, - comdb2uuidstr(sorese->uuid, uuid), rc, xerr->errval); + osqlcomm_done_xerr_uuid_type_put(&(rpl_xerr), p_buf, p_buf_end); - msglen = OSQLCOMM_DONE_RPL_LEN; - } - type = osql_net_type_to_net_uuid_type(NET_OSQL_SIGNAL); } else { - osql_done_xerr_t rpl_xerr = {{0}}; - osql_done_rpl_t rpl_ok = {{0}}; - - if (rc) { - uint8_t *p_buf = buf; - uint8_t *p_buf_end = buf + OSQLCOMM_DONE_XERR_RPL_LEN; - rpl_xerr.hd.type = OSQL_XERR; - rpl_xerr.hd.sid = sorese->rqid; - rpl_xerr.dt = *xerr; + msglen = OSQLCOMM_DONE_UUID_RPL_LEN; + uint8_t *p_buf = buf; + buf = alloca(msglen); + uint8_t *p_buf_end = buf + msglen; - logmsg(LOGMSG_DEBUG, - "%s line %d master signaling %s rqid %llu with rc=%d " - "xerr=%d\n", - __func__, __LINE__, sorese->host, sorese->rqid, rc, - xerr->errval); + rpl_ok.hd.type = OSQL_DONE; + comdb2uuidcpy(rpl_ok.hd.uuid, sorese->uuid); + rpl_ok.dt.rc = 0; + rpl_ok.dt.nops = sorese->nops; - osqlcomm_done_xerr_type_put(&(rpl_xerr), p_buf, p_buf_end); - - msglen = OSQLCOMM_DONE_XERR_RPL_LEN; - - } else { - uint8_t *p_buf = buf; - uint8_t *p_buf_end = buf + OSQLCOMM_DONE_RPL_LEN; + osqlcomm_done_uuid_rpl_put(&(rpl_ok), p_buf, p_buf_end); + } - rpl_ok.hd.type = OSQL_DONE; - rpl_ok.hd.sid = sorese->rqid; - rpl_ok.dt.rc = 0; - rpl_ok.dt.nops = sorese->nops; + type = osql_net_type_to_net_uuid_type(NET_OSQL_SIGNAL); + logmsg(LOGMSG_DEBUG, + "%s:%d master signaling %s uuid %s with rc=%d xerr=%d\n", + __func__, __LINE__, sorese->host, + comdb2uuidstr(sorese->uuid, uuid), rc, xerr->errval); + } else { + osql_done_xerr_t rpl_xerr = {{0}}; + osql_done_rpl_t rpl_ok = {{0}}; - logmsg(LOGMSG_DEBUG, - "%s line %d master signaling %s rqid %llu with rc=%d " - "xerr=%d\n", - __func__, __LINE__, sorese->host, sorese->rqid, rc, - xerr->errval); + if (rc) { + msglen = OSQLCOMM_DONE_XERR_RPL_LEN; + buf = alloca(msglen); + uint8_t *p_buf = buf; + uint8_t *p_buf_end = buf + msglen; + rpl_xerr.hd.type = OSQL_XERR; + rpl_xerr.hd.sid = sorese->rqid; + rpl_xerr.dt = *xerr; + + osqlcomm_done_xerr_type_put(&(rpl_xerr), p_buf, p_buf_end); + } else { + msglen = OSQLCOMM_DONE_RPL_LEN; + buf = alloca(msglen); + uint8_t *p_buf = buf; + uint8_t *p_buf_end = buf + msglen; - osqlcomm_done_rpl_put(&(rpl_ok), p_buf, p_buf_end); + rpl_ok.hd.type = OSQL_DONE; + rpl_ok.hd.sid = sorese->rqid; + rpl_ok.dt.rc = 0; + rpl_ok.dt.nops = sorese->nops; - msglen = OSQLCOMM_DONE_RPL_LEN; - } - type = NET_OSQL_SIGNAL; + osqlcomm_done_rpl_put(&(rpl_ok), p_buf, p_buf_end); } + + type = NET_OSQL_SIGNAL; + logmsg(LOGMSG_DEBUG, + "%s:%d master signaling %s rqid %llu with rc=%d xerr=%d\n", + __func__, __LINE__, sorese->host, sorese->rqid, rc, + xerr->errval); + } #if 0 - printf("Send %d rqid=%llu tmp=%llu\n", NET_OSQL_SIGNAL, sorese->rqid, osql_log_time()); + printf("Send %d rqid=%llu tmp=%llu\n", NET_OSQL_SIGNAL, sorese->rqid, osql_log_time()); #endif - /* lazy again, works just because node!=0 */ - irc = offload_net_send(sorese->host, type, buf, msglen, 1); - if (irc) { - irc = -1; - logmsg(LOGMSG_ERROR, "%s: error sending done to %s!\n", __func__, - sorese->host); - } - - } else { - /* local */ - - irc = osql_chkboard_sqlsession_rc(sorese->rqid, sorese->uuid, - sorese->nops, NULL, xerr); + /* lazy again, works just because node!=0 */ + int irc = offload_net_send(sorese->host, type, buf, msglen, 1); + if (irc) { + irc = -1; + logmsg(LOGMSG_ERROR, "%s: error sending done to %s!\n", __func__, + sorese->host); } return irc; From afbcb45cf5434f1de96287031388a6541a247a8f Mon Sep 17 00:00:00 2001 From: Adi Zaimi Date: Fri, 10 Jan 2020 13:05:18 -0500 Subject: [PATCH 2/9] refactor --- db/osqlrepository.c | 32 ++++++++++++-------------------- 1 file changed, 12 insertions(+), 20 deletions(-) diff --git a/db/osqlrepository.c b/db/osqlrepository.c index 7db93a7d7a..6daab9f2e0 100644 --- a/db/osqlrepository.c +++ b/db/osqlrepository.c @@ -172,12 +172,7 @@ int osql_repository_add(osql_sess_t *sess, int *replaced) Pthread_rwlock_unlock(&theosql->hshlck); return -1; } - if (rc == 0) { - /* old request was terminated successfully, let's add the new one */ - logmsg(LOGMSG_INFO, - "%s: cancelled old request for rqid=%llx, uuid=%s\n", - __func__, sess->rqid, p); - } else { + if (rc) { /* old request was already processed, ignore new ones */ Pthread_rwlock_unlock(&theosql->hshlck); *replaced = 1; @@ -186,6 +181,10 @@ int osql_repository_add(osql_sess_t *sess, int *replaced) __func__, sess->rqid, p); return 0; } + /* old request was terminated successfully, let's add the new one */ + logmsg(LOGMSG_INFO, + "%s: cancelled old request for rqid=%llx, uuid=%s\n", + __func__, sess->rqid, p); } if (sess->rqid == OSQL_RQID_USE_UUID) @@ -218,7 +217,7 @@ int gbl_abort_on_missing_osql_session = 0; * Remove an osql session from the repository * return 0 on success */ -int osql_repository_rem(osql_sess_t *sess, int lock, const char *func, const char *callfunc, int line) +int osql_repository_rem(osql_sess_t *sess, const int lock, const char *func, const char *callfunc, int line) { osql_repository_t *theosql = get_theosql(); if (theosql == NULL) { @@ -226,9 +225,8 @@ int osql_repository_rem(osql_sess_t *sess, int lock, const char *func, const cha } int rc = 0; - if (lock) { + if (lock) Pthread_rwlock_wrlock(&theosql->hshlck); - } if (sess->rqid == OSQL_RQID_USE_UUID) { rc = hash_del(theosql->rqsuuid, sess); @@ -236,6 +234,10 @@ int osql_repository_rem(osql_sess_t *sess, int lock, const char *func, const cha rc = hash_del(theosql->rqs, sess); } + if (lock) + Pthread_rwlock_unlock(&theosql->hshlck); + + #ifdef TRACK_OSQL_SESSIONS static uuid_t uuid_list[MAX_UUID_LIST]; static const char *uuid_func[MAX_UUID_LIST]; @@ -272,16 +274,10 @@ int osql_repository_rem(osql_sess_t *sess, int lock, const char *func, const cha logmsg(LOGMSG_ERROR, "%s unable to find previous uuid %s\n", __func__, p); } else { - logmsg(LOGMSG_ERROR, "%s found %s %d times in tracking array\n", __func__, - p, found_uuid); + logmsg(LOGMSG_ERROR, "%s found %s %d times in tracking array\n", __func__, p, found_uuid); } #endif - if (lock) { - Pthread_rwlock_unlock(&theosql->hshlck); - lock = 0; - } - /* This can happen legitimately on master swing */ if (gbl_abort_on_missing_osql_session) abort(); @@ -297,10 +293,6 @@ int osql_repository_rem(osql_sess_t *sess, int lock, const char *func, const cha } #endif - if (lock) { - Pthread_rwlock_unlock(&theosql->hshlck); - } - return 0; } From 59e246bce2b6a7cd2ad8643a8bb0aff2c8bbc02a Mon Sep 17 00:00:00 2001 From: Adi Zaimi Date: Fri, 10 Jan 2020 13:09:29 -0500 Subject: [PATCH 3/9] more cleanup --- db/osqlcomm.c | 116 ++++++++++++++++++++++---------------------------- 1 file changed, 51 insertions(+), 65 deletions(-) diff --git a/db/osqlcomm.c b/db/osqlcomm.c index 5c759ba1e0..680c866bdc 100644 --- a/db/osqlcomm.c +++ b/db/osqlcomm.c @@ -7265,8 +7265,11 @@ int osql_process_packet(struct ireq *iq, unsigned long long rqid, uuid_t uuid, static int sorese_rcvreq(char *fromhost, void *dtap, int dtalen, int type, int nettype) { - osql_sess_t *sess = NULL; uint8_t *malcd = malloc(OSQL_BP_MAXLEN); + if (!malcd) + goto done; + + osql_sess_t *sess = NULL; struct ireq *iq = NULL; osql_req_t req; bool is_reorder_on = false; @@ -7282,15 +7285,11 @@ static int sorese_rcvreq(char *fromhost, void *dtap, int dtalen, int type, uuid_t uuid; int replaced = 0; - if (!malcd) - goto done; - /* grab the request */ if (osql_nettype_is_uuid(nettype)) { osql_uuid_req_t uuid_req; sql = (char *)osqlcomm_req_uuid_type_get(&uuid_req, p_req_buf, p_req_buf_end); - req.type = uuid_req.type; req.rqlen = uuid_req.rqlen; req.sqlqlen = uuid_req.sqlqlen; @@ -7387,15 +7386,11 @@ static int sorese_rcvreq(char *fromhost, void *dtap, int dtalen, int type, debug = 1; } - /* - Blockproc does not create a copy of the request, + /* Blockproc does not create a copy of the request, but creates a different thread to work on it - Let THAT thread free it... - free(p_buf); - */ + Let THAT thread free p_buf (malcd)... */ - /* for socksql, is this a retry that need to be checked for self-deadlock? - */ + /* for socksql, is it a retry that needs to be checked for self-deadlock? */ if ((type == OSQL_SOCK_REQ || type == OSQL_SOCK_REQ_COST) && (req.flags & OSQL_FLAGS_CHECK_SELFLOCK)) { /* just make sure we are above the threshold */ @@ -7403,73 +7398,64 @@ static int sorese_rcvreq(char *fromhost, void *dtap, int dtalen, int type, } done: - - if (rc) { - int rc2; - - if (malcd) - free(malcd); - - if (iq) - destroy_ireq(thedb, iq); - - /* notify the sql thread there will be no response! */ - struct errstat generr = {0}; - - generr.errval = ERR_TRAN_FAILED; - if (rc == -4) { - strncpy0(generr.errstr, "fail to create block processor log", - sizeof(generr.errstr)); - } else { - strncpy0(generr.errstr, "failed to create transaction", - sizeof(generr.errstr)); - } - - int onstack = 0; - if (!sess) { - onstack = 1; /* used to avoid debugging confusion */ - sess = alloca(sizeof(osql_sess_t)); - sess->host = fromhost; - sess->rqid = req.rqid; - comdb2uuidcpy(sess->uuid, uuid); - sess->nops = 0; - } - - rc2 = osql_comm_signal_sqlthr_rc(sess, &generr, RC_INTERNAL_RETRY); - if (rc2) { - uuidstr_t us; - comdb2uuidstr(uuid, us); - logmsg(LOGMSG_ERROR, "%s: failed to signaled rqid=[%llx %s] host=%s of " - "error to create bplog\n", - __func__, req.rqid, us, fromhost); - } - if (onstack) - sess = NULL; - else - osql_close_session(&sess, 0, __func__, NULL, __LINE__); - - } else { - int rc2; - + if (rc == 0) { /* - successful, let the session lose + successful, let the session loose It is possible that we are clearing sessions due to master being rtcpu-ed, and it will wait for the session clients to disappear before it will wipe out the session */ - rc2 = osql_sess_remclient(sess); + int rc2 = osql_sess_remclient(sess); if (rc2) { uuidstr_t us; comdb2uuidstr(uuid, us); logmsg(LOGMSG_ERROR, "%s: failed to release session rqid=[%llx %s] node=%s\n", __func__, req.rqid, us, fromhost); } + return 0; } -#if 0 - printf( "Done in here rc=%d %llu\n", rc, osql_log_time()); -#endif + if (malcd) + free(malcd); + + if (iq) + destroy_ireq(thedb, iq); + + /* notify the sql thread there will be no response! */ + struct errstat generr = {0}; + + generr.errval = ERR_TRAN_FAILED; + if (rc == -4) { + strncpy0(generr.errstr, "fail to create block processor log", + sizeof(generr.errstr)); + } else { + strncpy0(generr.errstr, "failed to create transaction", + sizeof(generr.errstr)); + } + + int onstack = 0; + if (!sess) { + onstack = 1; /* used to avoid debugging confusion */ + sess = alloca(sizeof(osql_sess_t)); + sess->host = fromhost; + sess->rqid = req.rqid; + comdb2uuidcpy(sess->uuid, uuid); + sess->nops = 0; + } + + int rc2 = osql_comm_signal_sqlthr_rc(sess, &generr, RC_INTERNAL_RETRY); + if (rc2) { + uuidstr_t us; + comdb2uuidstr(uuid, us); + logmsg(LOGMSG_ERROR, "%s: failed to signaled rqid=[%llx %s] host=%s of " + "error to create bplog\n", + __func__, req.rqid, us, fromhost); + } + if (onstack) + sess = NULL; + else + osql_close_session(&sess, 0, __func__, NULL, __LINE__); return rc; } From c11d9226d0ad3b0ce94d349154d075b37f996c69 Mon Sep 17 00:00:00 2001 From: Adi Zaimi Date: Fri, 10 Jan 2020 13:47:38 -0500 Subject: [PATCH 4/9] unlock locks before calling osql_repository_rem to avoid crash --- db/osqlblockproc.c | 2 +- db/osqlsession.c | 25 ++++++++++++++----------- 2 files changed, 15 insertions(+), 12 deletions(-) diff --git a/db/osqlblockproc.c b/db/osqlblockproc.c index 39c45f8552..90eb33f7d4 100644 --- a/db/osqlblockproc.c +++ b/db/osqlblockproc.c @@ -503,7 +503,7 @@ void osql_bplog_free(struct ireq *iq, int are_sessions_linked, const char *func, of sess before removing it from the lookup hash */ - /* remove the sessions from repository and free them */ + /* remove the sessions from repository (if linked) and free them */ osql_close_session(&tran->sess, are_sessions_linked, func, callfunc, line); iq->sorese = NULL; diff --git a/db/osqlsession.c b/db/osqlsession.c index 583f4bcdbe..0d767b6256 100644 --- a/db/osqlsession.c +++ b/db/osqlsession.c @@ -97,10 +97,8 @@ int osql_close_session(osql_sess_t **psess, int is_linked, const char *func, } #endif - /* - wait for all receivers to go away (in current implem, this is only 1, the - reader_thread - since we removed the hash entry, no new messages are added + /* wait for all receivers to go away, in current implem this is only 1--the + reader_thread, since we removed the hash entry no new messages are added */ if (!rc) { while (ATOMIC_LOAD32(sess->clients) > 0) { @@ -172,11 +170,10 @@ int osql_sess_addclient(osql_sess_t *sess) } /** - * Register client - * Prevent temporary the session destruction - * + * UnRegister client -- atomically decrement client count + * After this session may be destroyed */ -int osql_sess_remclient(osql_sess_t *sess) +inline int osql_sess_remclient(osql_sess_t *sess) { #if 0 uuidstr_t us; @@ -188,8 +185,9 @@ int osql_sess_remclient(osql_sess_t *sess) int loc_clients = ATOMIC_ADD32(sess->clients, -1); if (loc_clients < 0) { + abort(); // remove this in future uuidstr_t us; - fprintf(stderr, + logmsg(LOGMSG_ERROR, "%s: BUG ALERT, session %llu %s freed one too many times\n", __func__, sess->rqid, comdb2uuidstr(sess->uuid, us)); } @@ -693,8 +691,11 @@ static int osql_sess_set_terminate(osql_sess_t *sess) { int rc = 0; sess->terminate = OSQL_TERMINATE; - rc = osql_repository_rem(sess, 0, __func__, NULL, - __LINE__); /* already have exclusive lock */ + Pthread_mutex_unlock(&sess->completed_lock); + Pthread_mutex_unlock(&sess->mtx); + + const int need_lock = 0; /* already have exclusive lock for osql hash */ + rc = osql_repository_rem(sess, need_lock, __func__, NULL, __LINE__); if (rc) { logmsg(LOGMSG_ERROR, "%s: failed to remove session from repository rc=%d\n", __func__, @@ -710,6 +711,8 @@ static int osql_sess_set_terminate(osql_sess_t *sess) /** * Terminate a session if the session is not yet completed/dispatched + * we come here from osql_repository_add() if already in osql hash map + * which can happen in case there is an early replay * Return 0 if session is successfully terminated, * -1 for errors, * 1 otherwise (if session was already processed) From eafb6ab4094b536a607613d558c256229e57284e Mon Sep 17 00:00:00 2001 From: Adi Zaimi Date: Fri, 10 Jan 2020 13:47:54 -0500 Subject: [PATCH 5/9] more refactor --- db/osqlsession.c | 28 +++++++++++++++++----------- 1 file changed, 17 insertions(+), 11 deletions(-) diff --git a/db/osqlsession.c b/db/osqlsession.c index 0d767b6256..9117f3bfee 100644 --- a/db/osqlsession.c +++ b/db/osqlsession.c @@ -74,8 +74,15 @@ static void save_sql(struct ireq *iq, osql_sess_t *sess, const char *sql, * receive message from sql thread). * Returns 0 if success * - * NOTE: it is possible to inline clean a request on master bounce, - * which starts by unlinking the session first, and freeing bplog afterwards + * This function will remove from osql_repository_rem() if is_linked is set + * then wait till there are no more clients using this sess then destroy obj + * + * NOTE: + * - it is possible to inline clean a request on master bounce, + * which starts by unlinking the session first, and freeing bplog afterwards + * + * - if caller has already removed sess from osql repository, they should + * call this function with is_linked = 0 */ int osql_close_session(osql_sess_t **psess, int is_linked, const char *func, const char *callfunc, int line) @@ -97,18 +104,17 @@ int osql_close_session(osql_sess_t **psess, int is_linked, const char *func, } #endif - /* wait for all receivers to go away, in current implem this is only 1--the - reader_thread, since we removed the hash entry no new messages are added - */ - if (!rc) { - while (ATOMIC_LOAD32(sess->clients) > 0) { - poll(NULL, 0, 10); - } + if (rc) + return rc; - _destroy_session(psess, 0); + /* wait for all receivers to go away, in current implem this is only 1--the + reader_thread, since we removed the hash entry no new messages are added */ + while (ATOMIC_LOAD32(sess->clients) > 0) { + poll(NULL, 0, 10); } + _destroy_session(psess, 0); - return rc; + return 0; } static int free_selectv_genids(void *obj, void *arg) From 1dd77820fe3af8cd130eedc687a7fc9c49bfc94c Mon Sep 17 00:00:00 2001 From: Adi Zaimi Date: Fri, 10 Jan 2020 14:43:38 -0500 Subject: [PATCH 6/9] buf needs to have max size regardless --- db/osqlcomm.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/db/osqlcomm.c b/db/osqlcomm.c index 680c866bdc..d9cca53b0d 100644 --- a/db/osqlcomm.c +++ b/db/osqlcomm.c @@ -5095,7 +5095,17 @@ int osql_comm_signal_sqlthr_rc(osql_sess_t *sorese, struct errstat *xerr, int msglen = 0; char uuid[37]; int type; - uint8_t *buf = NULL; + /* Constructing one of 4 message types so get a buffer big enough */ + int max = 0; + if (OSQLCOMM_DONE_XERR_UUID_RPL_LEN > max) + max = OSQLCOMM_DONE_XERR_UUID_RPL_LEN; + if (OSQLCOMM_DONE_UUID_RPL_LEN > max) + max = OSQLCOMM_DONE_UUID_RPL_LEN; + if (OSQLCOMM_DONE_XERR_RPL_LEN > max) + max = OSQLCOMM_DONE_XERR_RPL_LEN; + if (OSQLCOMM_DONE_RPL_LEN > max) + max = OSQLCOMM_DONE_RPL_LEN; + uint8_t *buf = alloca(max); /* test if the sql thread was the one closing the request, and if so, don't send anything back @@ -5118,7 +5128,6 @@ int osql_comm_signal_sqlthr_rc(osql_sess_t *sorese, struct errstat *xerr, if (rc) { msglen = OSQLCOMM_DONE_XERR_UUID_RPL_LEN; - buf = alloca(msglen); uint8_t *p_buf = buf; uint8_t *p_buf_end = buf + msglen; rpl_xerr.hd.type = OSQL_XERR; @@ -5130,7 +5139,6 @@ int osql_comm_signal_sqlthr_rc(osql_sess_t *sorese, struct errstat *xerr, } else { msglen = OSQLCOMM_DONE_UUID_RPL_LEN; uint8_t *p_buf = buf; - buf = alloca(msglen); uint8_t *p_buf_end = buf + msglen; rpl_ok.hd.type = OSQL_DONE; @@ -5152,7 +5160,6 @@ int osql_comm_signal_sqlthr_rc(osql_sess_t *sorese, struct errstat *xerr, if (rc) { msglen = OSQLCOMM_DONE_XERR_RPL_LEN; - buf = alloca(msglen); uint8_t *p_buf = buf; uint8_t *p_buf_end = buf + msglen; rpl_xerr.hd.type = OSQL_XERR; @@ -5162,7 +5169,6 @@ int osql_comm_signal_sqlthr_rc(osql_sess_t *sorese, struct errstat *xerr, osqlcomm_done_xerr_type_put(&(rpl_xerr), p_buf, p_buf_end); } else { msglen = OSQLCOMM_DONE_RPL_LEN; - buf = alloca(msglen); uint8_t *p_buf = buf; uint8_t *p_buf_end = buf + msglen; From 918534c4f53051385b9233dd29fbaa6ce1414237 Mon Sep 17 00:00:00 2001 From: Adi Zaimi Date: Fri, 10 Jan 2020 16:19:01 -0500 Subject: [PATCH 7/9] fix compile errors --- db/osqlcomm.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/db/osqlcomm.c b/db/osqlcomm.c index d9cca53b0d..74c899f10e 100644 --- a/db/osqlcomm.c +++ b/db/osqlcomm.c @@ -7271,17 +7271,17 @@ int osql_process_packet(struct ireq *iq, unsigned long long rqid, uuid_t uuid, static int sorese_rcvreq(char *fromhost, void *dtap, int dtalen, int type, int nettype) { + int rc = 0; + struct ireq *iq = NULL; uint8_t *malcd = malloc(OSQL_BP_MAXLEN); if (!malcd) goto done; osql_sess_t *sess = NULL; - struct ireq *iq = NULL; osql_req_t req; bool is_reorder_on = false; uint8_t *p_req_buf = dtap; const uint8_t *p_req_buf_end = p_req_buf + dtalen; - int rc = 0; uint8_t *p_buf = malcd; const uint8_t *p_buf_end = p_buf + OSQL_BP_MAXLEN; char *sql; From 1cab0d7592e582079070c37a800c823df62269cd Mon Sep 17 00:00:00 2001 From: Adi Zaimi Date: Wed, 15 Jan 2020 14:22:02 -0500 Subject: [PATCH 8/9] have buffer be a union to fit largest message type --- db/osqlcomm.c | 25 ++++++++++--------------- 1 file changed, 10 insertions(+), 15 deletions(-) diff --git a/db/osqlcomm.c b/db/osqlcomm.c index 74c899f10e..1e5a298406 100644 --- a/db/osqlcomm.c +++ b/db/osqlcomm.c @@ -5095,21 +5095,16 @@ int osql_comm_signal_sqlthr_rc(osql_sess_t *sorese, struct errstat *xerr, int msglen = 0; char uuid[37]; int type; - /* Constructing one of 4 message types so get a buffer big enough */ - int max = 0; - if (OSQLCOMM_DONE_XERR_UUID_RPL_LEN > max) - max = OSQLCOMM_DONE_XERR_UUID_RPL_LEN; - if (OSQLCOMM_DONE_UUID_RPL_LEN > max) - max = OSQLCOMM_DONE_UUID_RPL_LEN; - if (OSQLCOMM_DONE_XERR_RPL_LEN > max) - max = OSQLCOMM_DONE_XERR_RPL_LEN; - if (OSQLCOMM_DONE_RPL_LEN > max) - max = OSQLCOMM_DONE_RPL_LEN; - uint8_t *buf = alloca(max); - - /* test if the sql thread was the one closing the - request, and if so, don't send anything back - (request might be gone already anyway) + union { + char a [OSQLCOMM_DONE_XERR_UUID_RPL_LEN]; + char b [OSQLCOMM_DONE_UUID_RPL_LEN]; + char c [OSQLCOMM_DONE_XERR_RPL_LEN]; + char d [OSQLCOMM_DONE_RPL_LEN]; + } largest_message; + uint8_t *buf = (uint8_t *) &largest_message; + + /* test if the sql thread was the one closing the request, + * and if so, don't send anything back, request might be gone already anyway */ if (xerr->errval == SQLITE_ABORT) return 0; From 6e094bf6f116a22c8aed20e1b5cd6844e966ded7 Mon Sep 17 00:00:00 2001 From: Adi Zaimi Date: Wed, 15 Jan 2020 15:29:23 -0500 Subject: [PATCH 9/9] format --- db/osqlcomm.c | 19 ++++++++++--------- db/osqlrepository.c | 11 ++++++----- db/osqlsession.c | 11 ++++++----- 3 files changed, 22 insertions(+), 19 deletions(-) diff --git a/db/osqlcomm.c b/db/osqlcomm.c index 1e5a298406..04c3dae377 100644 --- a/db/osqlcomm.c +++ b/db/osqlcomm.c @@ -5096,14 +5096,14 @@ int osql_comm_signal_sqlthr_rc(osql_sess_t *sorese, struct errstat *xerr, char uuid[37]; int type; union { - char a [OSQLCOMM_DONE_XERR_UUID_RPL_LEN]; - char b [OSQLCOMM_DONE_UUID_RPL_LEN]; - char c [OSQLCOMM_DONE_XERR_RPL_LEN]; - char d [OSQLCOMM_DONE_RPL_LEN]; + char a[OSQLCOMM_DONE_XERR_UUID_RPL_LEN]; + char b[OSQLCOMM_DONE_UUID_RPL_LEN]; + char c[OSQLCOMM_DONE_XERR_RPL_LEN]; + char d[OSQLCOMM_DONE_RPL_LEN]; } largest_message; - uint8_t *buf = (uint8_t *) &largest_message; + uint8_t *buf = (uint8_t *)&largest_message; - /* test if the sql thread was the one closing the request, + /* test if the sql thread was the one closing the request, * and if so, don't send anything back, request might be gone already anyway */ if (xerr->errval == SQLITE_ABORT) @@ -7449,9 +7449,10 @@ static int sorese_rcvreq(char *fromhost, void *dtap, int dtalen, int type, if (rc2) { uuidstr_t us; comdb2uuidstr(uuid, us); - logmsg(LOGMSG_ERROR, "%s: failed to signaled rqid=[%llx %s] host=%s of " - "error to create bplog\n", - __func__, req.rqid, us, fromhost); + logmsg(LOGMSG_ERROR, + "%s: failed to signaled rqid=[%llx %s] host=%s of " + "error to create bplog\n", + __func__, req.rqid, us, fromhost); } if (onstack) sess = NULL; diff --git a/db/osqlrepository.c b/db/osqlrepository.c index 6daab9f2e0..74571a75e3 100644 --- a/db/osqlrepository.c +++ b/db/osqlrepository.c @@ -183,8 +183,8 @@ int osql_repository_add(osql_sess_t *sess, int *replaced) } /* old request was terminated successfully, let's add the new one */ logmsg(LOGMSG_INFO, - "%s: cancelled old request for rqid=%llx, uuid=%s\n", - __func__, sess->rqid, p); + "%s: cancelled old request for rqid=%llx, uuid=%s\n", __func__, + sess->rqid, p); } if (sess->rqid == OSQL_RQID_USE_UUID) @@ -217,7 +217,8 @@ int gbl_abort_on_missing_osql_session = 0; * Remove an osql session from the repository * return 0 on success */ -int osql_repository_rem(osql_sess_t *sess, const int lock, const char *func, const char *callfunc, int line) +int osql_repository_rem(osql_sess_t *sess, const int lock, const char *func, + const char *callfunc, int line) { osql_repository_t *theosql = get_theosql(); if (theosql == NULL) { @@ -237,7 +238,6 @@ int osql_repository_rem(osql_sess_t *sess, const int lock, const char *func, con if (lock) Pthread_rwlock_unlock(&theosql->hshlck); - #ifdef TRACK_OSQL_SESSIONS static uuid_t uuid_list[MAX_UUID_LIST]; static const char *uuid_func[MAX_UUID_LIST]; @@ -274,7 +274,8 @@ int osql_repository_rem(osql_sess_t *sess, const int lock, const char *func, con logmsg(LOGMSG_ERROR, "%s unable to find previous uuid %s\n", __func__, p); } else { - logmsg(LOGMSG_ERROR, "%s found %s %d times in tracking array\n", __func__, p, found_uuid); + logmsg(LOGMSG_ERROR, "%s found %s %d times in tracking array\n", + __func__, p, found_uuid); } #endif diff --git a/db/osqlsession.c b/db/osqlsession.c index 9117f3bfee..b4ddf9bf82 100644 --- a/db/osqlsession.c +++ b/db/osqlsession.c @@ -77,11 +77,11 @@ static void save_sql(struct ireq *iq, osql_sess_t *sess, const char *sql, * This function will remove from osql_repository_rem() if is_linked is set * then wait till there are no more clients using this sess then destroy obj * - * NOTE: + * NOTE: * - it is possible to inline clean a request on master bounce, * which starts by unlinking the session first, and freeing bplog afterwards * - * - if caller has already removed sess from osql repository, they should + * - if caller has already removed sess from osql repository, they should * call this function with is_linked = 0 */ int osql_close_session(osql_sess_t **psess, int is_linked, const char *func, @@ -108,7 +108,8 @@ int osql_close_session(osql_sess_t **psess, int is_linked, const char *func, return rc; /* wait for all receivers to go away, in current implem this is only 1--the - reader_thread, since we removed the hash entry no new messages are added */ + reader_thread, since we removed the hash entry no new messages are added + */ while (ATOMIC_LOAD32(sess->clients) > 0) { poll(NULL, 0, 10); } @@ -194,8 +195,8 @@ inline int osql_sess_remclient(osql_sess_t *sess) abort(); // remove this in future uuidstr_t us; logmsg(LOGMSG_ERROR, - "%s: BUG ALERT, session %llu %s freed one too many times\n", - __func__, sess->rqid, comdb2uuidstr(sess->uuid, us)); + "%s: BUG ALERT, session %llu %s freed one too many times\n", + __func__, sess->rqid, comdb2uuidstr(sess->uuid, us)); } return 0;