From b594bdc42a1d43be30a13cb7092a2cbd487cfa7e Mon Sep 17 00:00:00 2001 From: Markus Makela Date: Wed, 16 Nov 2016 11:41:14 +0200 Subject: [PATCH 01/29] MXS-975: Remove hard limit on listen backlog The listen() backlog is now set to INT_MAX which should guarantee that the internal limit is always higher than the system limit. This means that the length of the queue always follows /proc/sys/net/ipv4/tcp_max_syn_backlog. --- Documentation/Getting-Started/Configuration-Guide.md | 8 ++++++++ server/core/dcb.c | 9 ++++++++- 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/Documentation/Getting-Started/Configuration-Guide.md b/Documentation/Getting-Started/Configuration-Guide.md index 5aa7a18d2..e89e996c1 100644 --- a/Documentation/Getting-Started/Configuration-Guide.md +++ b/Documentation/Getting-Started/Configuration-Guide.md @@ -723,6 +723,14 @@ This example configuration requires all connections to this server to be encrypt The listener defines a port and protocol pair that is used to listen for connections to a service. A service may have multiple listeners associated with it, either to support multiple protocols or multiple ports. As with other elements of the configuration the section name is the listener name and it can be selected freely. A type parameter is used to identify the section as a listener definition. Address is optional and it allows the user to limit connections to certain interface only. Socket is also optional and used for Unix socket connections. +The network socket where the listener listens will have a backlog of +connections. The size of this backlog is controlled by the +net.ipv4.tcp_max_syn_backlog and net.core.somaxconn kernel parameters. + +Increasing the size of the backlog by modifying the kernel parameters +helps with sudden connection spikes and rejected connections. For more +information see [listen(2)](http://man7.org/linux/man-pages/man2/listen.2.html). + ``` [] type=listener diff --git a/server/core/dcb.c b/server/core/dcb.c index 631d2b839..c922e27c8 100644 --- a/server/core/dcb.c +++ b/server/core/dcb.c @@ -3497,7 +3497,14 @@ dcb_listen(DCB *listener, const char *config, const char *protocol_name) return -1; } - if (listen(listener_socket, 10 * SOMAXCONN) != 0) + /** + * The use of INT_MAX for backlog length in listen() allows the end-user to + * control the backlog length with the net.ipv4.tcp_max_syn_backlog kernel + * option since the parameter is silently truncated to the configured value. + * + * @see man 2 listen + */ + if (listen(listener_socket, INT_MAX) != 0) { char errbuf[STRERROR_BUFLEN]; MXS_ERROR("Failed to start listening on '%s' with protocol '%s': %d, %s", From 59ee5a78c931cf1cdff1093eded50b990ab8c468 Mon Sep 17 00:00:00 2001 From: Johan Wikman Date: Wed, 16 Nov 2016 10:23:09 +0200 Subject: [PATCH 02/29] MXS-969: Report user var modifications properly User variable modifications are now reported as QUERY_TYPE_USERVAR_WRITE and not as QUERY_TYPE_GSYSVAR_WRITE as earlier. --- .../qc_mysqlembedded/qc_mysqlembedded.cc | 134 ++++++++++++++++-- query_classifier/qc_sqlite/qc_sqlite.c | 128 ++++++++++------- query_classifier/test/expected.sql | 1 - query_classifier/test/input.sql | 1 - .../test/qc_sqlite_unsupported.test | 9 ++ query_classifier/test/set.test | 87 ++++++++---- server/core/query_classifier.c | 13 +- server/include/query_classifier.h | 3 +- utils/skygw_debug.h | 3 +- 9 files changed, 282 insertions(+), 97 deletions(-) diff --git a/query_classifier/qc_mysqlembedded/qc_mysqlembedded.cc b/query_classifier/qc_mysqlembedded/qc_mysqlembedded.cc index 3fd8573ba..b7383a2b1 100644 --- a/query_classifier/qc_mysqlembedded/qc_mysqlembedded.cc +++ b/query_classifier/qc_mysqlembedded/qc_mysqlembedded.cc @@ -87,7 +87,7 @@ typedef struct parsing_info_st static THD* get_or_create_thd_for_parsing(MYSQL* mysql, char* query_str); static unsigned long set_client_flags(MYSQL* mysql); static bool create_parse_tree(THD* thd); -static uint32_t resolve_query_type(THD* thd); +static uint32_t resolve_query_type(parsing_info_t*, THD* thd); static bool skygw_stmt_causes_implicit_commit(LEX* lex, int* autocommit_stmt); static int is_autocommit_stmt(LEX* lex); @@ -167,7 +167,7 @@ uint32_t qc_get_type(GWBUF* querybuf) /** Find out the query type */ if (mysql != NULL) { - qtype = resolve_query_type((THD *) mysql->thd); + qtype = resolve_query_type(pi, (THD *) mysql->thd); } } } @@ -435,9 +435,102 @@ return_here: return failp; } +/** + * Sniff whether the statement is + * + * SET ROLE ... + * SET NAMES ... + * SET PASSWORD ... + * SET CHARACTER ... + * + * Depending on what kind of SET statement it is, the parser of the embedded + * library creates instances of set_var_user, set_var, set_var_password, + * set_var_role, etc. that all are derived from set_var_base. However, there + * is no type-information available in set_var_base, which is the type of the + * instances when accessed from the lexer. Consequently, we cannot know what + * kind of statment it is based on that, only whether it is a system variable + * or not. + * + * Consequently, we just look at the string and deduce whether it is a + * set [ROLE|NAMES|PASSWORD|CHARACTER] statement. + */ +bool is_set_specific(const char* s) +{ + bool rv = false; + + // Remove space from the beginning. + while (isspace(*s)) + { + ++s; + } + + const char* token = s; + + // Find next non-space character. + while (!isspace(*s) && (*s != 0)) + { + ++s; + } + + if (s - token == 3) // Might be "set" + { + if (strncasecmp(token, "set", 3) == 0) + { + // YES it was! + while (isspace(*s)) + { + ++s; + } + + token = s; + + while (!isspace(*s) && (*s != 0) && (*s != '=')) + { + ++s; + } + + if (s - token == 4) // Might be "role" + { + if (strncasecmp(token, "role", 4) == 0) + { + // YES it was! + rv = true; + } + } + else if (s - token == 5) // Might be "names" + { + if (strncasecmp(token, "names", 5) == 0) + { + // YES it was! + rv = true; + } + } + else if (s - token == 8) // Might be "password + { + if (strncasecmp(token, "password", 8) == 0) + { + // YES it was! + rv = true; + } + } + else if (s - token == 9) // Might be "character" + { + if (strncasecmp(token, "character", 9) == 0) + { + // YES it was! + rv = true; + } + } + } + } + + return rv; +} + /** * Detect query type by examining parsed representation of it. * + * @param pi The parsing info. * @param thd MariaDB thread context. * * @return Copy of query type value. @@ -449,7 +542,7 @@ return_here: * the resulting type may be different. * */ -static uint32_t resolve_query_type(THD* thd) +static uint32_t resolve_query_type(parsing_info_t *pi, THD* thd) { qc_query_type_t qtype = QUERY_TYPE_UNKNOWN; uint32_t type = QUERY_TYPE_UNKNOWN; @@ -565,7 +658,33 @@ static uint32_t resolve_query_type(THD* thd) else if (lex->sql_command == SQLCOM_SET_OPTION) { /** Either user- or system variable write */ - type |= QUERY_TYPE_GSYSVAR_WRITE; + if (is_set_specific(pi->pi_query_plain_str)) + { + type |= QUERY_TYPE_GSYSVAR_WRITE; + } + else + { + List_iterator ilist(lex->var_list); + size_t n = 0; + + while (set_var_base *var = ilist++) + { + if (var->is_system()) + { + type |= QUERY_TYPE_GSYSVAR_WRITE; + } + else + { + type |= QUERY_TYPE_USERVAR_WRITE; + } + ++n; + } + + if (n == 0) + { + type |= QUERY_TYPE_GSYSVAR_WRITE; + } + } } goto return_qtype; @@ -812,12 +931,7 @@ static uint32_t resolve_query_type(THD* thd) /** User-defined variable modification */ case Item_func::SUSERVAR_FUNC: - /** - * Really it is user variable but we - * don't separate sql variables atm. - * 15.9.14 - */ - func_qtype |= QUERY_TYPE_GSYSVAR_WRITE; + func_qtype |= QUERY_TYPE_USERVAR_WRITE; MXS_DEBUG("%lu [resolve_query_type] " "functype SUSERVAR_FUNC, user " "variable write.", diff --git a/query_classifier/qc_sqlite/qc_sqlite.c b/query_classifier/qc_sqlite/qc_sqlite.c index 33d67de28..f96b4cf57 100644 --- a/query_classifier/qc_sqlite/qc_sqlite.c +++ b/query_classifier/qc_sqlite/qc_sqlite.c @@ -746,8 +746,7 @@ static void update_affected_fields(QC_SQLITE_INFO* info, { if ((prev_token == TK_EQ) && (pos == QC_TOKEN_LEFT)) { - // Yes, QUERY_TYPE_USERVAR_WRITE is currently not available. - info->types |= QUERY_TYPE_GSYSVAR_WRITE; + info->types |= QUERY_TYPE_USERVAR_WRITE; } else { @@ -2036,6 +2035,7 @@ void maxscaleSet(Parse* pParse, int scope, mxs_set_t kind, ExprList* pList) ss_dassert(info); info->status = QC_QUERY_PARSED; + info->types = 0; // Reset what was set in maxscaleKeyword switch (kind) { @@ -2053,10 +2053,6 @@ void maxscaleSet(Parse* pParse, int scope, mxs_set_t kind, ExprList* pList) case MXS_SET_VARIABLES: { - // TODO: qc_mysqlembedded sets this bit on, without checking what - // TODO: kind of variable it is. - info->types = QUERY_TYPE_GSYSVAR_WRITE; - for (int i = 0; i < pList->nExpr; ++i) { const struct ExprList_item* pItem = &pList->a[i]; @@ -2065,19 +2061,48 @@ void maxscaleSet(Parse* pParse, int scope, mxs_set_t kind, ExprList* pList) { case TK_CHARACTER: case TK_NAMES: + info->types |= QUERY_TYPE_GSYSVAR_WRITE; break; case TK_EQ: { const Expr* pEq = pItem->pExpr; - const Expr* pVariable = pEq->pLeft; + const Expr* pVariable; const Expr* pValue = pEq->pRight; - // pVariable is either TK_DOT, TK_VARIABLE or TK_ID. If it's TK_DOT, - // then pVariable->pLeft is either TK_VARIABLE or TK_ID and pVariable->pRight + // pEq->pLeft is either TK_DOT, TK_VARIABLE or TK_ID. If it's TK_DOT, + // then pEq->pLeft->pLeft is either TK_VARIABLE or TK_ID and pEq->pLeft->pRight // is either TK_DOT, TK_VARIABLE or TK_ID. + // Find the left-most part. + pVariable = pEq->pLeft; + while (pVariable->op == TK_DOT) + { + pVariable = pVariable->pLeft; + ss_dassert(pVariable); + } + + // Check what kind of variable it is. + size_t n_at = 0; + const char* zName = pVariable->u.zToken; + + while (*zName == '@') + { + ++n_at; + ++zName; + } + + if (n_at == 1) + { + info->types |= QUERY_TYPE_USERVAR_WRITE; + } + else + { + info->types |= QUERY_TYPE_GSYSVAR_WRITE; + } + // Set pVariable to point to the rightmost part of the name. + pVariable = pEq->pLeft; while (pVariable->op == TK_DOT) { pVariable = pVariable->pRight; @@ -2085,54 +2110,59 @@ void maxscaleSet(Parse* pParse, int scope, mxs_set_t kind, ExprList* pList) ss_dassert((pVariable->op == TK_VARIABLE) || (pVariable->op == TK_ID)); - const char* zName = pVariable->u.zToken; - - while (*zName == '@') + if (n_at != 1) { - ++zName; - } + // If it's not a user-variable we need to check whether it might + // be 'autocommit'. + const char* zName = pVariable->u.zToken; - // As pVariable points to the rightmost part, we'll catch both - // "autocommit" and "@@global.autocommit". - if (strcasecmp(zName, "autocommit") == 0) - { - int enable = -1; - - switch (pValue->op) + while (*zName == '@') { - case TK_INTEGER: - if (pValue->u.iValue == 1) - { - enable = 1; - } - else if (pValue->u.iValue == 0) - { - enable = 0; - } - break; - - case TK_ID: - enable = string_to_truth(pValue->u.zToken); - break; - - default: - break; + ++zName; } - switch (enable) + // As pVariable points to the rightmost part, we'll catch both + // "autocommit" and "@@global.autocommit". + if (strcasecmp(zName, "autocommit") == 0) { - case 0: - info->types |= QUERY_TYPE_BEGIN_TRX; - info->types |= QUERY_TYPE_DISABLE_AUTOCOMMIT; - break; + int enable = -1; - case 1: - info->types |= QUERY_TYPE_ENABLE_AUTOCOMMIT; - info->types |= QUERY_TYPE_COMMIT; - break; + switch (pValue->op) + { + case TK_INTEGER: + if (pValue->u.iValue == 1) + { + enable = 1; + } + else if (pValue->u.iValue == 0) + { + enable = 0; + } + break; - default: - break; + case TK_ID: + enable = string_to_truth(pValue->u.zToken); + break; + + default: + break; + } + + switch (enable) + { + case 0: + info->types |= QUERY_TYPE_BEGIN_TRX; + info->types |= QUERY_TYPE_DISABLE_AUTOCOMMIT; + break; + + case 1: + info->types |= QUERY_TYPE_ENABLE_AUTOCOMMIT; + info->types |= QUERY_TYPE_COMMIT; + break; + + default: + break; + } } } diff --git a/query_classifier/test/expected.sql b/query_classifier/test/expected.sql index e5b7239bc..fd67bcc91 100644 --- a/query_classifier/test/expected.sql +++ b/query_classifier/test/expected.sql @@ -4,7 +4,6 @@ QUERY_TYPE_WRITE QUERY_TYPE_WRITE QUERY_TYPE_WRITE|QUERY_TYPE_COMMIT QUERY_TYPE_WRITE|QUERY_TYPE_CREATE_TMP_TABLE -QUERY_TYPE_GSYSVAR_WRITE QUERY_TYPE_READ|QUERY_TYPE_SYSVAR_READ QUERY_TYPE_READ|QUERY_TYPE_USERVAR_READ QUERY_TYPE_GSYSVAR_WRITE|QUERY_TYPE_ENABLE_AUTOCOMMIT|QUERY_TYPE_COMMIT diff --git a/query_classifier/test/input.sql b/query_classifier/test/input.sql index d9f2473b3..450bf81c8 100644 --- a/query_classifier/test/input.sql +++ b/query_classifier/test/input.sql @@ -4,7 +4,6 @@ insert into tst values ("Jane","Doe"),("Daisy","Duck"),("Marie","Curie"); update tst set fname="Farmer", lname="McDonald" where lname="%Doe" and fname="John"; create table tmp as select * from t1; create temporary table tmp as select * from t1; -/*!40101 SET @OLD_CHARACTER_SET_CLIENT=@@CHARACTER_SET_CLIENT */; select @@server_id; select @OLD_SQL_NOTES; SET autocommit=1; diff --git a/query_classifier/test/qc_sqlite_unsupported.test b/query_classifier/test/qc_sqlite_unsupported.test index 49480924a..be724cf7c 100644 --- a/query_classifier/test/qc_sqlite_unsupported.test +++ b/query_classifier/test/qc_sqlite_unsupported.test @@ -20,3 +20,12 @@ SET @x:= (SELECT h FROM t1 WHERE (a,b,c,d,e,f,g)=(1,2,3,4,5,6,7)); # REMOVE: expr(A) ::= LP(B) expr(X) RP(E). {A.pExpr = X.pExpr; spanSet(&A,&B,&E);} # REMOVE: expr(A) ::= LP expr(X) COMMA(OP) expr(Y) RP. {spanBinaryExpr(&A,pParse,@OP,&X,&Y);} # ADD : expr(A) ::= LP exprlist RP. { ... } + +SET @`a b`='hello'; +set @`test`=1; +set @"tEST"=3; +set @`TeST`=4; +# warning: qc_sqlite: Statement was classified only based on keywords +# (Sqlite3 error: SQL logic error or missing database, unrecognized token: "@"): "set @=4" +# +# sqlite3GetToken needs to be modified to accept a quoted variable name. \ No newline at end of file diff --git a/query_classifier/test/set.test b/query_classifier/test/set.test index 04b8a68c1..d82597526 100644 --- a/query_classifier/test/set.test +++ b/query_classifier/test/set.test @@ -271,7 +271,8 @@ SET timestamp=UNIX_TIMESTAMP('2014-09-30 08:00:00'); SET ROLE role_1; SET ROLE NONE; SET @sum=0; -SET @old_debug= @@session.debug; +# MXS Embedded parser is not aware of the 'debug' variable. +# MXS SET @old_debug= @@session.debug; set debug_dbug='+d,send_kill_after_delete'; set debug_dbug=@old_debug; set local sql_mode=""; @@ -398,7 +399,8 @@ SET collation_connection=gb2312_chinese_ci; set names gb2312; set collation_connection=gb2312_bin; SET NAMES gbk; -SET @`tcontent`:=_binary 0x50434B000900000000000000E9000000 COLLATE `binary`/*!*/; +# MXSTODO qc_sqlite can not parse quoted variables. +# MXSTODO SET @`tcontent`:=_binary 0x50434B000900000000000000E9000000 COLLATE `binary`/*!*/; SET @test_character_set= 'gbk'; SET @test_collation= 'gbk_chinese_ci'; SET NAMES gbk; @@ -1487,7 +1489,8 @@ set global event_scheduler=on; set global event_scheduler=off; set global event_scheduler=original; set global event_scheduler=on; -SET @event_scheduler=@@global.event_scheduler; +# MXS Embedded parser is not aware of the 'global.event_scheduler' variable. +# MXS SET @event_scheduler=@@global.event_scheduler; SET GLOBAL event_scheduler=OFF; SET GLOBAL event_scheduler=OFF; SET GLOBAL event_scheduler=1; @@ -1500,7 +1503,8 @@ SET GLOBAL event_scheduler=2; SET GLOBAL event_scheduler=5; SET GLOBAL event_scheduler=ON; SET GLOBAL event_scheduler=@event_scheduler; -SET @old_event_scheduler=@@event_scheduler; +# MXS Embedded parser is not aware of the 'global.event_scheduler' variable. +# MXS SET @old_event_scheduler=@@event_scheduler; SET GLOBAL event_scheduler=on; SET GLOBAL event_scheduler=off; SET GLOBAL event_scheduler=on; @@ -1574,7 +1578,8 @@ set time_zone= @@global.time_zone; set @a:=0; SET @xml='a1b1c1b2a2'; SET sql_mode=STRICT_TRANS_TABLES; -SET @old_debug= @@session.debug; +# MXS Embedded parser is not aware of the 'debug' variable. +# MXS SET @old_debug= @@session.debug; SET session debug_dbug= '+d,alloc_sort_buffer_fail'; SET session debug_dbug= @old_debug; SET DEBUG_SYNC='filesort_start SIGNAL filesort_started WAIT_FOR filesort_killed'; @@ -1587,7 +1592,8 @@ set global expire_logs_days = 0; SET AUTOCOMMIT=0; SET AUTOCOMMIT=1; set sql_mode=""; -SET @old_innodb_file_per_table= @@GLOBAL.innodb_file_per_table; +# MXS Embedded parser is not aware of innodb. +# MXS SET @old_innodb_file_per_table= @@GLOBAL.innodb_file_per_table; SET GLOBAL innodb_file_per_table= 1; SET @export = 10; SET GLOBAL innodb_file_per_table= @old_innodb_file_per_table; @@ -2267,7 +2273,8 @@ set join_cache_level= @tmp_mdev5037; set @@join_cache_level= @save_join_cache_level; set storage_engine=@save_storage_engine; set optimizer_switch=@innodb_mrr_cpk_tmp; -set @old_innodb_lock_wait_timeout=@@global.innodb_lock_wait_timeout; +# MXS Embedded parser is not aware of innodb. +# MXS SET set @old_innodb_lock_wait_timeout=@@global.innodb_lock_wait_timeout; set global innodb_lock_wait_timeout=300; set session innodb_lock_wait_timeout=300; set @@autocommit=0; @@ -2414,8 +2421,10 @@ set @value= "1aa"; set @value= "aa1"; set @value= "1e+1111111111a"; set @value= "-1e+1111111111a"; -set @value= 1e+1111111111; -set @value= -1e+1111111111; +# MXS ERROR 1367 (22007): Illegal double '1e+1111111111' value found during parsing +# MXS set @value= 1e+1111111111; +# MXS ERROR 1367 (22007): Illegal double '1e+1111111111' value found during parsing +# MXS set @value= -1e+1111111111; set @value= 1e+111; set @value= -1e+111; set @value= 1; @@ -3389,7 +3398,8 @@ SET DEBUG_SYNC= 'RESET'; set @default_storage_engine= @@global.storage_engine; set global storage_engine=myisam; set session storage_engine=myisam; -SET @orig_debug=@@debug; +# MXS Embedded parser is not aware of the 'debug' variable. +# MXS SET @orig_debug=@@debug; SET GLOBAL debug_dbug="+d,myisam_pretend_crashed_table_on_open"; SET GLOBAL debug_dbug=@orig_debug; set global storage_engine=@default_storage_engine; @@ -3416,7 +3426,8 @@ SET NAMES latin1; set autocommit=0; set autocommit=1; set @mrr_icp_extra_tmp=@@optimizer_switch; -SET @aux = @@session.debug; +# MXS Embedded parser is not aware of the 'debug' variable. +# MXS SET @aux = @@session.debug; set @d=4; set sql_safe_updates=1; set sql_safe_updates=0; @@ -3774,15 +3785,19 @@ SET GLOBAL general_log = 0; SET @@global.general_log = @old_general_log_state; SET @old_default_storage_engine = @@default_storage_engine; SET @@default_storage_engine = 'InnoDB'; -SET @save_innodb_stats_on_metadata=@@global.innodb_stats_on_metadata; +# MXS Embedded parser is not aware of innodb. +# MXS SET SET @save_innodb_stats_on_metadata=@@global.innodb_stats_on_metadata; SET @@global.innodb_stats_on_metadata=ON; SET @@global.innodb_stats_on_metadata=@save_innodb_stats_on_metadata; SET @@default_storage_engine = @old_default_storage_engine; set sql_mode=""; set sql_mode=default; -SET @old_innodb_file_format = @@global.innodb_file_format; -SET @old_innodb_file_per_table = @@global.innodb_file_per_table; -SET @old_innodb_strict_mode = @@global.innodb_strict_mode; +# MXS Embedded parser is not aware of innodb. +# MXS SET SET @old_innodb_file_format = @@global.innodb_file_format; +# MXS Embedded parser is not aware of innodb. +# MXS SET SET @old_innodb_file_per_table = @@global.innodb_file_per_table; +# MXS Embedded parser is not aware of innodb. +# MXS SET SET @old_innodb_strict_mode = @@global.innodb_strict_mode; SET @@global.innodb_file_format = Barracuda, @@global.innodb_file_per_table = ON, @@global.innodb_strict_mode = ON; @@ -3817,8 +3832,10 @@ SET TRANSACTION ISOLATION LEVEL REPEATABLE READ; SET TRANSACTION ISOLATION LEVEL READ COMMITTED; set global default_storage_engine='innodb'; set session default_storage_engine='innodb'; -SET @old_innodb_thread_concurrency := @@innodb_thread_concurrency; -SET @old_innodb_thread_sleep_delay := @@innodb_thread_sleep_delay; +# MXS Embedded parser is not aware of innodb. +# MXS SET SET @old_innodb_thread_concurrency := @@innodb_thread_concurrency; +# MXS Embedded parser is not aware of innodb. +# MXS SET SET @old_innodb_thread_sleep_delay := @@innodb_thread_sleep_delay; SET GLOBAL innodb_thread_concurrency = 1; SET GLOBAL innodb_thread_concurrency = @old_innodb_thread_concurrency; SET GLOBAL innodb_thread_sleep_delay = @old_innodb_thread_sleep_delay; @@ -4825,7 +4842,8 @@ SET optimizer_switch=@save_optimizer_switch; SET @pass='my_pw'; SET @wrong='incorrect'; set sql_mode=""; -set @save_debug_dbug= @@debug_dbug; +# MXS ERROR 1193 (HY000): Unknown system variable 'debug_dbug' +# MXS set @save_debug_dbug= @@debug_dbug; set @@debug_dbug= @save_debug_dbug; set @@debug_dbug= @save_debug_dbug; set gtid_domain_id = 10; @@ -4943,7 +4961,8 @@ SET NAMES latin1; SET NAMES latin1; SET NAMES utf8; SET NAMES latin1; -SET @old_debug= @@session.debug; +# MXS Embedded parser is not aware of the 'debug' variable. +# MXS SET @old_debug= @@session.debug; set debug_sync='RESET'; set debug_dbug='+d,show_explain_probe_delete_exec_start'; set @show_explain_probe_select_id=1; @@ -4954,9 +4973,11 @@ set debug_sync='RESET'; set @show_explain_probe_select_id=1; set debug_dbug='d,show_explain_probe_join_exec_start'; set debug_dbug=''; -SET @old_debug= @@session.debug; +# MXS Embedded parser is not aware of the 'debug' variable. +# MXS SET @old_debug= @@session.debug; set debug_sync='RESET'; -SET @old_debug= @@session.debug; +# MXS Embedded parser is not aware of the 'debug' variable. +# MXS SET @old_debug= @@session.debug; set @show_explain_probe_select_id=1; set debug_dbug='+d,show_explain_probe_join_exec_start'; set @show_expl_tmp= @@optimizer_switch; @@ -5169,7 +5190,8 @@ set @@max_sp_recursion_depth= 20; set @@max_sp_recursion_depth= 0; SET sql_mode=ONLY_FULL_GROUP_BY; SET @lock_wait_timeout_saved= @@lock_wait_timeout; -SET @innodb_lock_wait_timeout_saved= @@innodb_lock_wait_timeout; +# MXS Embedded parser is not aware of innodb. +# MXS SET SET @innodb_lock_wait_timeout_saved= @@innodb_lock_wait_timeout; SET @@lock_wait_timeout= 1; SET @@innodb_lock_wait_timeout= 1; SET AUTOCOMMIT= 0; @@ -5815,7 +5837,8 @@ set @@optimizer_switch= default; set optimizer_switch='subquery_cache=on'; SET optimizer_switch=@save_optimizer_switch; set @@optimizer_switch= default; -SET @orig_debug=@@debug; +# MXS Embedded parser is not aware of the 'debug' variable. +# MXS SET @orig_debug=@@debug; SET GLOBAL debug_dbug="d,subselect_exec_fail"; SET GLOBAL debug_dbug=@orig_debug; set @subselect_mat_cost=@@optimizer_switch; @@ -6886,7 +6909,8 @@ set @@autocommit=0; set @@autocommit=1; set @@global.general_log=@save_general_log; SET TIMESTAMP=10000; -SET @`a b`='hello'; +# MXSTODO qc_sqlite can not parse quoted variables. +# MXSTODO SET @`a b`='hello'; set @var1= "';aaa"; SET @var2=char(ascii('a')); set @a := foo; @@ -6899,7 +6923,8 @@ set @a=_latin2'test'; set @a=_latin2'test' collate latin2_general_ci; set @var= NULL ; set @v1=null, @v2=1, @v3=1.1, @v4=now(); -set session @honk=99; +# 'set session @honk = 99' is not legal. +# MXS set session @honk=99; set @first_var= NULL; set @first_var= cast(NULL as signed integer); set @first_var= NULL; @@ -6928,7 +6953,8 @@ SET @aux = NULL; SET @bug12408412=1; SET @var=NULL; set @var= repeat('a',20000); -set @my_slave_net_timeout =@@global.slave_net_timeout; +# MXS Embedded parser is not aware of the 'global.slave_net_timeout' variable. +# MXS SET set @my_slave_net_timeout =@@global.slave_net_timeout; set global slave_net_timeout=100; set global sql_slave_skip_counter=100; set global slave_net_timeout=default; @@ -6987,10 +7013,13 @@ set @my_max_allowed_packet =@@global.max_allowed_packet; set @my_delay_key_write =@@global.delay_key_write; set @my_join_buffer_size =@@global.join_buffer_size; set @my_log_warnings =@@global.log_warnings; -set @`test`=1; +# MXSTODO qc_sqlite can not parse quoted variables. +# MXSTODO set @`test`=1; set @TEST=2; -set @"tEST"=3; -set @`TeST`=4; +# MXSTODO qc_sqlite can not parse quoted variables. +# MXSTODO set @"tEST"=3; +# MXSTODO qc_sqlite can not parse quoted variables. +# MXSTODO set @`TeST`=4; set @select=2,@t5=1.23456; set @test_int=10,@test_double=1e-10,@test_string="abcdeghi",@test_string2="abcdefghij",@select=NULL; set @test_int="hello",@test_double="hello",@test_string="hello",@test_string2="hello"; diff --git a/server/core/query_classifier.c b/server/core/query_classifier.c index 91dc1df3d..1d0723cbf 100644 --- a/server/core/query_classifier.c +++ b/server/core/query_classifier.c @@ -381,8 +381,14 @@ struct type_name_info type_to_type_name_info(qc_query_type_t type) } break; - /** Not implemented yet */ - //case QUERY_TYPE_USERVAR_WRITE: + case QUERY_TYPE_USERVAR_WRITE: + { + static const char name[] = "QUERY_TYPE_USERVAR_WRITE"; + info.name = name; + info.name_len = sizeof(name) - 1; + } + break; + case QUERY_TYPE_USERVAR_READ: { static const char name[] = "QUERY_TYPE_USERVAR_READ"; @@ -549,8 +555,7 @@ static const qc_query_type_t QUERY_TYPES[] = QUERY_TYPE_WRITE, QUERY_TYPE_MASTER_READ, QUERY_TYPE_SESSION_WRITE, - /** Not implemented yet */ - //QUERY_TYPE_USERVAR_WRITE, + QUERY_TYPE_USERVAR_WRITE, QUERY_TYPE_USERVAR_READ, QUERY_TYPE_SYSVAR_READ, /** Not implemented yet */ diff --git a/server/include/query_classifier.h b/server/include/query_classifier.h index ee434c295..ce60e798a 100644 --- a/server/include/query_classifier.h +++ b/server/include/query_classifier.h @@ -26,8 +26,7 @@ typedef enum QUERY_TYPE_WRITE = 0x000004, /*< Master data will be modified:master */ QUERY_TYPE_MASTER_READ = 0x000008, /*< Read from the master:master */ QUERY_TYPE_SESSION_WRITE = 0x000010, /*< Session data will be modified:master or all */ - /** Not implemented yet */ - //QUERY_TYPE_USERVAR_WRITE = 0x000020, /*< Write a user variable:master or all */ + QUERY_TYPE_USERVAR_WRITE = 0x000020, /*< Write a user variable:master or all */ QUERY_TYPE_USERVAR_READ = 0x000040, /*< Read a user variable:master or any */ QUERY_TYPE_SYSVAR_READ = 0x000080, /*< Read a system variable:master or any */ /** Not implemented yet */ diff --git a/utils/skygw_debug.h b/utils/skygw_debug.h index 15aa1b352..4fd321475 100644 --- a/utils/skygw_debug.h +++ b/utils/skygw_debug.h @@ -146,6 +146,7 @@ typedef enum skygw_chk_t ((t) == QUERY_TYPE_UNKNOWN ? "QUERY_TYPE_UNKNOWN" : \ ((t) == QUERY_TYPE_LOCAL_READ ? "QUERY_TYPE_LOCAL_READ" : \ ((t) == QUERY_TYPE_MASTER_READ ? "QUERY_TYPE_MASTER_READ" : \ + ((t) == QUERY_TYPE_USERVAR_WRITE ? "QUERY_TYPE_USERVAR_WRITE" : \ ((t) == QUERY_TYPE_USERVAR_READ ? "QUERY_TYPE_USERVAR_READ" : \ ((t) == QUERY_TYPE_SYSVAR_READ ? "QUERY_TYPE_SYSVAR_READ" : \ ((t) == QUERY_TYPE_GSYSVAR_READ ? "QUERY_TYPE_GSYSVAR_READ" : \ @@ -162,7 +163,7 @@ typedef enum skygw_chk_t ((t) == QUERY_TYPE_READ_TMP_TABLE ? "QUERY_TYPE_READ_TMP_TABLE" : \ ((t) == QUERY_TYPE_SHOW_DATABASES ? "QUERY_TYPE_SHOW_DATABASES" : \ ((t) == QUERY_TYPE_SHOW_TABLES ? "QUERY_TYPE_SHOW_TABLES" : \ - "Unknown query type")))))))))))))))))))))) + "Unknown query type"))))))))))))))))))))))) #define STRLOGPRIORITYNAME(n)\ ((n) == LOG_EMERG ? "LOG_EMERG" : \ From 5aefd35df9cde96661e9c5ce77f1c9f871a6eaf4 Mon Sep 17 00:00:00 2001 From: Markus Makela Date: Tue, 15 Nov 2016 13:36:28 +0200 Subject: [PATCH 03/29] MXS-969: Detect user variable modifications With the use_sql_variables_in=master option, readwritesplit should route all user variable modifications and reads with user variables to the master. Previously, the modification of user variables was grouped into generic system variables which caused all modifications to system variables to go to the master only. The router requires a finer grained distiction between normal system variable modifications and user variable modifications. With the improvements to the query classifier, readwritesplit now properly routes all user variable operations to the master and other system variable modifications to all servers. --- .../routing/readwritesplit/readwritesplit.c | 63 ++++++++----------- 1 file changed, 27 insertions(+), 36 deletions(-) diff --git a/server/modules/routing/readwritesplit/readwritesplit.c b/server/modules/routing/readwritesplit/readwritesplit.c index c5e33c0d6..62e3cf545 100644 --- a/server/modules/routing/readwritesplit/readwritesplit.c +++ b/server/modules/routing/readwritesplit/readwritesplit.c @@ -1346,9 +1346,10 @@ static route_target_t get_route_target(ROUTER_CLIENT_SES *rses, */ else if (!load_active && (QUERY_IS_TYPE(qtype, QUERY_TYPE_SESSION_WRITE) || - /** Configured to allow writing variables to all nodes */ + /** Configured to allow writing user variables to all nodes */ (use_sql_variables_in == TYPE_ALL && - QUERY_IS_TYPE(qtype, QUERY_TYPE_GSYSVAR_WRITE)) || + QUERY_IS_TYPE(qtype, QUERY_TYPE_USERVAR_WRITE)) || + QUERY_IS_TYPE(qtype, QUERY_TYPE_GSYSVAR_WRITE) || /** enable or disable autocommit are always routed to all */ QUERY_IS_TYPE(qtype, QUERY_TYPE_ENABLE_AUTOCOMMIT) || QUERY_IS_TYPE(qtype, QUERY_TYPE_DISABLE_AUTOCOMMIT))) @@ -1388,44 +1389,29 @@ static route_target_t get_route_target(ROUTER_CLIENT_SES *rses, * Hints may affect on routing of the following queries */ else if (!trx_active && !load_active && + !QUERY_IS_TYPE(qtype, QUERY_TYPE_MASTER_READ) && !QUERY_IS_TYPE(qtype, QUERY_TYPE_WRITE) && - (QUERY_IS_TYPE(qtype, QUERY_TYPE_READ) || /*< any SELECT */ - QUERY_IS_TYPE(qtype, QUERY_TYPE_SHOW_TABLES) || /*< 'SHOW TABLES' */ - 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 */ + (QUERY_IS_TYPE(qtype, QUERY_TYPE_READ) || + QUERY_IS_TYPE(qtype, QUERY_TYPE_SHOW_TABLES) || + QUERY_IS_TYPE(qtype, QUERY_TYPE_USERVAR_READ) || + QUERY_IS_TYPE(qtype, QUERY_TYPE_SYSVAR_READ) || + QUERY_IS_TYPE(qtype, QUERY_TYPE_GSYSVAR_READ))) { - /** First set expected targets before evaluating hints */ - if (!QUERY_IS_TYPE(qtype, QUERY_TYPE_MASTER_READ) && - (QUERY_IS_TYPE(qtype, QUERY_TYPE_READ) || - QUERY_IS_TYPE(qtype, QUERY_TYPE_SHOW_TABLES) || /*< 'SHOW TABLES' */ - /** Configured to allow reading variables from slaves */ - (use_sql_variables_in == TYPE_ALL && - (QUERY_IS_TYPE(qtype, QUERY_TYPE_USERVAR_READ) || - QUERY_IS_TYPE(qtype, QUERY_TYPE_SYSVAR_READ) || - QUERY_IS_TYPE(qtype, QUERY_TYPE_GSYSVAR_READ))))) + if (QUERY_IS_TYPE(qtype, QUERY_TYPE_USERVAR_READ)) + { + if (use_sql_variables_in == TYPE_ALL) + { + target = TARGET_SLAVE; + } + } + else if (QUERY_IS_TYPE(qtype, QUERY_TYPE_READ) || // Normal read + QUERY_IS_TYPE(qtype, QUERY_TYPE_SHOW_TABLES) || // SHOW TABLES + QUERY_IS_TYPE(qtype, QUERY_TYPE_SYSVAR_READ) || // System variable + QUERY_IS_TYPE(qtype, QUERY_TYPE_GSYSVAR_READ)) // Global system variable { target = TARGET_SLAVE; } - 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) || - QUERY_IS_TYPE(qtype, QUERY_TYPE_SYSVAR_READ)))) - { - target = TARGET_MASTER; - } - /** If nothing matches then choose the master */ if ((target & (TARGET_ALL | TARGET_SLAVE | TARGET_MASTER)) == 0) { @@ -1434,7 +1420,7 @@ static route_target_t get_route_target(ROUTER_CLIENT_SES *rses, } else { - ss_dassert(trx_active || + ss_dassert(trx_active || load_active || (QUERY_IS_TYPE(qtype, QUERY_TYPE_WRITE) || QUERY_IS_TYPE(qtype, QUERY_TYPE_MASTER_READ) || QUERY_IS_TYPE(qtype, QUERY_TYPE_SESSION_WRITE) || @@ -1446,6 +1432,8 @@ static route_target_t get_route_target(ROUTER_CLIENT_SES *rses, use_sql_variables_in == TYPE_MASTER) || (QUERY_IS_TYPE(qtype, QUERY_TYPE_GSYSVAR_WRITE) && use_sql_variables_in == TYPE_MASTER) || + (QUERY_IS_TYPE(qtype, QUERY_TYPE_USERVAR_WRITE) && + use_sql_variables_in == TYPE_MASTER) || QUERY_IS_TYPE(qtype, QUERY_TYPE_BEGIN_TRX) || QUERY_IS_TYPE(qtype, QUERY_TYPE_ENABLE_AUTOCOMMIT) || QUERY_IS_TYPE(qtype, QUERY_TYPE_DISABLE_AUTOCOMMIT) || @@ -1454,7 +1442,10 @@ static route_target_t get_route_target(ROUTER_CLIENT_SES *rses, QUERY_IS_TYPE(qtype, QUERY_TYPE_EXEC_STMT) || QUERY_IS_TYPE(qtype, QUERY_TYPE_CREATE_TMP_TABLE) || QUERY_IS_TYPE(qtype, QUERY_TYPE_READ_TMP_TABLE) || - QUERY_IS_TYPE(qtype, QUERY_TYPE_UNKNOWN))); + QUERY_IS_TYPE(qtype, QUERY_TYPE_UNKNOWN)) || + 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)); target = TARGET_MASTER; } From 4e007e87d01b431f82defda90f42f832daab0ebb Mon Sep 17 00:00:00 2001 From: Markus Makela Date: Wed, 16 Nov 2016 22:59:47 +0200 Subject: [PATCH 04/29] Allow stale master status to be assigned via maxadmin If a master once had slaves and is in the stale status, it will not retain this status after a restart. Without storing on-disk information, the stale master status cannot be deduced by looking at the master alone. Because of this, the user should be able to manually enable the stale master status. --- Documentation/Reference/MaxAdmin.md | 4 ++++ server/core/server.c | 1 + 2 files changed, 5 insertions(+) diff --git a/Documentation/Reference/MaxAdmin.md b/Documentation/Reference/MaxAdmin.md index c1da5453e..6120c779e 100644 --- a/Documentation/Reference/MaxAdmin.md +++ b/Documentation/Reference/MaxAdmin.md @@ -520,6 +520,10 @@ The status bit that can be controlled are maintenance The server is in maintenance mode. In this mode no new connections will be established to the server. The monitors will also not monitor servers that are in maintenance mode. + + stale + The server is a stale master server. Read [MySQL Monitor](../Monitors/MySQL-Monitor.md) documentation for more details. + diff --git a/server/core/server.c b/server/core/server.c index 8de58eb5d..1517c566f 100644 --- a/server/core/server.c +++ b/server/core/server.c @@ -990,6 +990,7 @@ static struct { "ndb", SERVER_NDB }, { "maintenance", SERVER_MAINT }, { "maint", SERVER_MAINT }, + { "stale", SERVER_STALE_STATUS }, { NULL, 0 } }; From 72622bc92fb042057e4318ae3b8e1fe3cf57407a Mon Sep 17 00:00:00 2001 From: Markus Makela Date: Wed, 16 Nov 2016 19:16:32 +0200 Subject: [PATCH 05/29] MXS-977: Move common diagnostic code to the core Almost all monitors printed the same diagnostic output inside the modules. This should be a part of the core, not the modules themselves. --- server/core/monitor.c | 42 ++++++++++++++++++- server/modules/monitor/galeramon/galeramon.c | 36 ++-------------- server/modules/monitor/mmmon/mmmon.c | 29 +------------ server/modules/monitor/mysqlmon/mysql_mon.c | 34 +++------------ .../monitor/ndbclustermon/ndbclustermon.c | 32 -------------- 5 files changed, 50 insertions(+), 123 deletions(-) diff --git a/server/core/monitor.c b/server/core/monitor.c index 4708a4c9d..33a4dd2f0 100644 --- a/server/core/monitor.c +++ b/server/core/monitor.c @@ -424,9 +424,46 @@ monitorShowAll(DCB *dcb) void monitorShow(DCB *dcb, MONITOR *monitor) { + const char *state; + + switch (monitor->state) + { + case MONITOR_STATE_RUNNING: + state = "Running"; + break; + case MONITOR_STATE_STOPPING: + state = "Stopping"; + break; + case MONITOR_STATE_STOPPED: + state = "Stopped"; + break; + case MONITOR_STATE_ALLOC: + state = "Allocated"; + break; + default: + state = "Unknown"; + break; + } + + dcb_printf(dcb, "Monitor: %p\n", monitor); + dcb_printf(dcb, "Name: %s\n", monitor->name); + dcb_printf(dcb, "State: %s\n", state); + dcb_printf(dcb, "Sampling interval: %lu milliseconds\n", monitor->interval); + dcb_printf(dcb, "Connect Timeout: %i seconds\n", monitor->connect_timeout); + dcb_printf(dcb, "Read Timeout: %i seconds\n", monitor->read_timeout); + dcb_printf(dcb, "Write Timeout: %i seconds\n", monitor->write_timeout); + dcb_printf(dcb, "Monitored servers: "); + + const char *sep = ""; + + for (MONITOR_SERVERS *db = monitor->databases; db; db = db->next) + { + dcb_printf(dcb, "%s%s:%d", sep, db->server->name, db->server->port); + sep = ", "; + } + + dcb_printf(dcb, "\n"); - dcb_printf(dcb, "Monitor: %p\n", monitor); - dcb_printf(dcb, "\tName: %s\n", monitor->name); if (monitor->handle) { if (monitor->module->diagnostics) @@ -442,6 +479,7 @@ monitorShow(DCB *dcb, MONITOR *monitor) { dcb_printf(dcb, "\tMonitor failed\n"); } + dcb_printf(dcb, "\n"); } /** diff --git a/server/modules/monitor/galeramon/galeramon.c b/server/modules/monitor/galeramon/galeramon.c index 00e33c545..f98f3ff70 100644 --- a/server/modules/monitor/galeramon/galeramon.c +++ b/server/modules/monitor/galeramon/galeramon.c @@ -250,41 +250,11 @@ static void diagnostics(DCB *dcb, const MONITOR *mon) { const GALERA_MONITOR *handle = (const GALERA_MONITOR *) mon->handle; - MONITOR_SERVERS *db; - char *sep; - switch (handle->status) - { - case MONITOR_RUNNING: - dcb_printf(dcb, "\tMonitor running\n"); - break; - case MONITOR_STOPPING: - dcb_printf(dcb, "\tMonitor stopping\n"); - break; - case MONITOR_STOPPED: - dcb_printf(dcb, "\tMonitor stopped\n"); - break; - } - - dcb_printf(dcb, "\tSampling interval:\t%lu milliseconds\n", mon->interval); - dcb_printf(dcb, "\tMaster Failback:\t%s\n", (handle->disableMasterFailback == 1) ? "off" : "on"); - dcb_printf(dcb, "\tAvailable when Donor:\t%s\n", (handle->availableWhenDonor == 1) ? "on" : "off"); - dcb_printf(dcb, "\tMaster Role Setting Disabled:\t%s\n", + dcb_printf(dcb, "Master Failback:\t%s\n", (handle->disableMasterFailback == 1) ? "off" : "on"); + dcb_printf(dcb, "Available when Donor:\t%s\n", (handle->availableWhenDonor == 1) ? "on" : "off"); + dcb_printf(dcb, "Master Role Setting Disabled:\t%s\n", (handle->disableMasterRoleSetting == 1) ? "on" : "off"); - dcb_printf(dcb, "\tConnect Timeout:\t%i seconds\n", mon->connect_timeout); - dcb_printf(dcb, "\tRead Timeout:\t\t%i seconds\n", mon->read_timeout); - dcb_printf(dcb, "\tWrite Timeout:\t\t%i seconds\n", mon->write_timeout); - dcb_printf(dcb, "\tMonitored servers: "); - - db = mon->databases; - sep = ""; - while (db) - { - dcb_printf(dcb, "%s%s:%d", sep, db->server->name, db->server->port); - sep = ", "; - db = db->next; - } - dcb_printf(dcb, "\n"); } /** diff --git a/server/modules/monitor/mmmon/mmmon.c b/server/modules/monitor/mmmon/mmmon.c index ef5ea103f..1826ec328 100644 --- a/server/modules/monitor/mmmon/mmmon.c +++ b/server/modules/monitor/mmmon/mmmon.c @@ -218,35 +218,8 @@ stopMonitor(MONITOR *mon) static void diagnostics(DCB *dcb, const MONITOR *mon) { const MM_MONITOR *handle = (const MM_MONITOR *) mon->handle; - MONITOR_SERVERS *db; - char *sep; - switch (handle->status) - { - case MONITOR_RUNNING: - dcb_printf(dcb, "\tMonitor running\n"); - break; - case MONITOR_STOPPING: - dcb_printf(dcb, "\tMonitor stopping\n"); - break; - case MONITOR_STOPPED: - dcb_printf(dcb, "\tMonitor stopped\n"); - break; - } - - dcb_printf(dcb, "\tSampling interval:\t%lu milliseconds\n", mon->interval); - dcb_printf(dcb, "\tDetect Stale Master:\t%s\n", (handle->detectStaleMaster == 1) ? "enabled" : "disabled"); - dcb_printf(dcb, "\tMonitored servers: "); - - db = mon->databases; - sep = ""; - while (db) - { - dcb_printf(dcb, "%s%s:%d", sep, db->server->name, db->server->port); - sep = ", "; - db = db->next; - } - dcb_printf(dcb, "\n"); + dcb_printf(dcb, "Detect Stale Master:\t%s\n", (handle->detectStaleMaster == 1) ? "enabled" : "disabled"); } /** diff --git a/server/modules/monitor/mysqlmon/mysql_mon.c b/server/modules/monitor/mysqlmon/mysql_mon.c index 75593b583..adb03135f 100644 --- a/server/modules/monitor/mysqlmon/mysql_mon.c +++ b/server/modules/monitor/mysqlmon/mysql_mon.c @@ -403,35 +403,14 @@ stopMonitor(MONITOR *mon) */ static void diagnostics(DCB *dcb, const MONITOR *mon) { - const MYSQL_MONITOR *handle = (const MYSQL_MONITOR *) mon->handle; - MONITOR_SERVERS *db; - char *sep; + const MYSQL_MONITOR *handle = (const MYSQL_MONITOR *)mon->handle; - switch (handle->status) - { - case MONITOR_RUNNING: - dcb_printf(dcb, "\tMonitor running\n"); - break; - case MONITOR_STOPPING: - dcb_printf(dcb, "\tMonitor stopping\n"); - break; - case MONITOR_STOPPED: - dcb_printf(dcb, "\tMonitor stopped\n"); - break; - } + dcb_printf(dcb, "MaxScale MonitorId:\t%lu\n", handle->id); + dcb_printf(dcb, "Replication lag:\t%s\n", (handle->replicationHeartbeat == 1) ? "enabled" : "disabled"); + dcb_printf(dcb, "Detect Stale Master:\t%s\n", (handle->detectStaleMaster == 1) ? "enabled" : "disabled"); + dcb_printf(dcb, "Server information\n\n"); - dcb_printf(dcb, "\tSampling interval:\t%lu milliseconds\n", mon->interval); - dcb_printf(dcb, "\tMaxScale MonitorId:\t%lu\n", handle->id); - dcb_printf(dcb, "\tReplication lag:\t%s\n", (handle->replicationHeartbeat == 1) ? "enabled" : "disabled"); - dcb_printf(dcb, "\tDetect Stale Master:\t%s\n", (handle->detectStaleMaster == 1) ? "enabled" : "disabled"); - dcb_printf(dcb, "\tConnect Timeout:\t%i seconds\n", mon->connect_timeout); - dcb_printf(dcb, "\tRead Timeout:\t\t%i seconds\n", mon->read_timeout); - dcb_printf(dcb, "\tWrite Timeout:\t\t%i seconds\n", mon->write_timeout); - dcb_printf(dcb, "\nMonitored servers\n\n"); - - db = mon->databases; - - while (db) + for (MONITOR_SERVERS *db = mon->databases; db; db = db->next) { MYSQL_SERVER_INFO *serv_info = hashtable_fetch(handle->server_info, db->server->unique_name); dcb_printf(dcb, "Server: %s\n", db->server->unique_name); @@ -450,7 +429,6 @@ static void diagnostics(DCB *dcb, const MONITOR *mon) } dcb_printf(dcb, "\n"); - db = db->next; } } diff --git a/server/modules/monitor/ndbclustermon/ndbclustermon.c b/server/modules/monitor/ndbclustermon/ndbclustermon.c index 54adb6bff..a8cab02b4 100644 --- a/server/modules/monitor/ndbclustermon/ndbclustermon.c +++ b/server/modules/monitor/ndbclustermon/ndbclustermon.c @@ -210,38 +210,6 @@ stopMonitor(MONITOR *mon) static void diagnostics(DCB *dcb, const MONITOR *mon) { - const MYSQL_MONITOR *handle = (const MYSQL_MONITOR *) mon->handle; - MONITOR_SERVERS *db; - char *sep; - - switch (handle->status) - { - case MONITOR_RUNNING: - dcb_printf(dcb, "\tMonitor running\n"); - break; - case MONITOR_STOPPING: - dcb_printf(dcb, "\tMonitor stopping\n"); - break; - case MONITOR_STOPPED: - dcb_printf(dcb, "\tMonitor stopped\n"); - break; - } - - dcb_printf(dcb, "\tSampling interval:\t%lu milliseconds\n", mon->interval); - dcb_printf(dcb, "\tConnect Timeout:\t%i seconds\n", mon->connect_timeout); - dcb_printf(dcb, "\tRead Timeout:\t\t%i seconds\n", mon->read_timeout); - dcb_printf(dcb, "\tWrite Timeout:\t\t%i seconds\n", mon->write_timeout); - dcb_printf(dcb, "\tMonitored servers: "); - - db = mon->databases; - sep = ""; - while (db) - { - dcb_printf(dcb, "%s%s:%d", sep, db->server->name, db->server->port); - sep = ", "; - db = db->next; - } - dcb_printf(dcb, "\n"); } /** From 042491ac072790521ea93b20329e50d5d70b0913 Mon Sep 17 00:00:00 2001 From: Markus Makela Date: Wed, 16 Nov 2016 23:18:40 +0200 Subject: [PATCH 06/29] Rename dbfwfilter objects to more descriptive ones The object names in the dbfwfilter weren't very descriptive and were not that easy to comprehend. The same goes for function names which had somewhat cryptic names. --- server/modules/filter/dbfwfilter/dbfwfilter.c | 353 +++++++++--------- 1 file changed, 178 insertions(+), 175 deletions(-) diff --git a/server/modules/filter/dbfwfilter/dbfwfilter.c b/server/modules/filter/dbfwfilter/dbfwfilter.c index b292286ff..14dfad541 100644 --- a/server/modules/filter/dbfwfilter/dbfwfilter.c +++ b/server/modules/filter/dbfwfilter/dbfwfilter.c @@ -136,108 +136,6 @@ typedef enum RT_CLAUSE /*< WHERE-clause requirement rule */ } ruletype_t; -const char* rule_names[] = -{ - "UNDEFINED", - "COLUMN", - "THROTTLE", - "PERMISSION", - "WILDCARD", - "REGEX", - "CLAUSE" -}; - -/** - * Linked list of strings. - */ -typedef struct strlink_t -{ - struct strlink_t *next; /*< Next node in the list */ - char* value; /*< Value of the current node */ -} STRLINK; - -/** - * A structure defining a range of time - */ -typedef struct timerange_t -{ - struct timerange_t* next; /*< Next node in the list */ - struct tm start; /*< Start of the time range */ - struct tm end; /*< End of the time range */ -} TIMERANGE; - -/** - * Query speed measurement and limitation structure - */ -typedef struct queryspeed_t -{ - time_t first_query; /*< Time when the first query occurred */ - time_t triggered; /*< Time when the limit was exceeded */ - int period; /*< Measurement interval in seconds */ - int cooldown; /*< Time the user is denied access for */ - int count; /*< Number of queries done */ - int limit; /*< Maximum number of queries */ - long id; /*< Unique id of the rule */ - bool active; /*< If the rule has been triggered */ - struct queryspeed_t* next; /*< Next node in the list */ -} QUERYSPEED; - -/** - * A structure used to identify individual rules and to store their contents - * - * Each type of rule has different requirements that are expressed as void pointers. - * This allows to match an arbitrary set of rules against a user. - */ -typedef struct rule_t -{ - void* data; /*< Actual implementation of the rule */ - char* name; /*< Name of the rule */ - ruletype_t type; /*< Type of the rule */ - qc_query_op_t on_queries; /*< Types of queries to inspect */ - int times_matched; /*< Number of times this rule has been matched */ - TIMERANGE* active; /*< List of times when this rule is active */ - struct rule_t *next; -} RULE; - -/** - * Linked list of pointers to a global pool of RULE structs - */ -typedef struct rulelist_t -{ - RULE* rule; /*< The rule structure */ - struct rulelist_t* next; /*< Next node in the list */ -} RULELIST; - -typedef struct user_template -{ - char *name; - enum match_type type; /** Matching type */ - STRLINK *rulenames; /** names of the rules */ - struct user_template *next; -} user_template_t; - -typedef struct user_t -{ - char* name; /*< Name of the user */ - SPINLOCK lock; /*< User spinlock */ - QUERYSPEED* qs_limit; /*< The query speed structure unique to this user */ - RULELIST* rules_or; /*< If any of these rules match the action is triggered */ - RULELIST* rules_and; /*< All of these rules must match for the action to trigger */ - RULELIST* rules_strict_and; /*< rules that skip the rest of the rules if one of them - * fails. This is only for rules paired with 'match strict_all'. */ - -} USER; - -/** - * Linked list of IP adresses and subnet masks - */ -typedef struct iprange_t -{ - struct iprange_t* next; /*< Next node in the list */ - uint32_t ip; /*< IP address */ - uint32_t mask; /*< Network mask */ -} IPRANGE; - /** * Possible actions to take when the query matches a rule */ @@ -258,18 +156,117 @@ enum fw_actions /** Maximum length of the match/nomatch messages */ #define FW_MAX_SQL_LEN 400 +const char* rule_names[] = +{ + "UNDEFINED", + "COLUMN", + "THROTTLE", + "PERMISSION", + "WILDCARD", + "REGEX", + "CLAUSE" +}; + +/** + * Linked list of strings. + */ +typedef struct strlink_t +{ + struct strlink_t *next; /*< Next node in the list */ + char* value; /*< Value of the current node */ +} STRLINK; + +/** + * A structure defining a range of time + */ +typedef struct timerange_t +{ + struct timerange_t* next; /*< Next node in the list */ + struct tm start; /*< Start of the time range */ + struct tm end; /*< End of the time range */ +} TIMERANGE; + +/** + * Query speed measurement and limitation structure + */ +typedef struct queryspeed_t +{ + time_t first_query; /*< Time when the first query occurred */ + time_t triggered; /*< Time when the limit was exceeded */ + int period; /*< Measurement interval in seconds */ + int cooldown; /*< Time the user is denied access for */ + int count; /*< Number of queries done */ + int limit; /*< Maximum number of queries */ + long id; /*< Unique id of the rule */ + bool active; /*< If the rule has been triggered */ + struct queryspeed_t* next; /*< Next node in the list */ +} QUERYSPEED; + +/** + * A structure used to identify individual rules and to store their contents + * + * Each type of rule has different requirements that are expressed as void pointers. + * This allows to match an arbitrary set of rules against a user. + */ +typedef struct rule_t +{ + void* data; /*< Actual implementation of the rule */ + char* name; /*< Name of the rule */ + ruletype_t type; /*< Type of the rule */ + qc_query_op_t on_queries; /*< Types of queries to inspect */ + int times_matched; /*< Number of times this rule has been matched */ + TIMERANGE* active; /*< List of times when this rule is active */ + struct rule_t *next; +} RULE; + +/** + * A set of rules that the filter follows + */ +typedef struct rulebook_t +{ + RULE* rule; /*< The rule structure */ + struct rulebook_t* next; /*< The next rule in the book */ +} RULE_BOOK; + +/** + * A temporary template structure used in the creation of actual users. + * This is also used to link the user definitions with the rules. + * @see struct user_t + */ +typedef struct user_template +{ + char *name; + enum match_type type; /** Matching type */ + STRLINK *rulenames; /** names of the rules */ + struct user_template *next; +} user_template_t; + +/** + * A user definition + */ +typedef struct user_t +{ + char* name; /*< Name of the user */ + SPINLOCK lock; /*< User spinlock */ + QUERYSPEED* qs_limit; /*< The query speed structure unique to this user */ + RULE_BOOK* rules_or; /*< If any of these rules match the action is triggered */ + RULE_BOOK* rules_and; /*< All of these rules must match for the action to trigger */ + RULE_BOOK* rules_strict_and; /*< rules that skip the rest of the rules if one of them + * fails. This is only for rules paired with 'match strict_all'. */ +} USER; + /** * The Firewall filter instance. */ typedef struct { - HASHTABLE* htable; /*< User hashtable */ - RULE* rules; /*< List of all the rules */ - STRLINK* userstrings; /*< Temporary list of raw strings of users */ - enum fw_actions action; /*< Default operation mode, defaults to deny */ - int log_match; /*< Log matching and/or non-matching queries */ - SPINLOCK lock; /*< Instance spinlock */ - int idgen; /*< UID generator */ + HASHTABLE* htable; /*< User hashtable */ + RULE* rules; /*< List of all the rules */ + STRLINK* userstrings; /*< Temporary list of raw strings of users */ + enum fw_actions action; /*< Default operation mode, defaults to deny */ + int log_match; /*< Log matching and/or non-matching queries */ + SPINLOCK lock; /*< Instance spinlock */ + int idgen; /*< UID generator */ } FW_INSTANCE; /** @@ -277,10 +274,10 @@ typedef struct */ typedef struct { - SESSION* session; /*< Client session structure */ - char* errmsg; /*< Rule specific error message */ - DOWNSTREAM down; /*< Next object in the downstream chain */ - UPSTREAM up; /*< Next object in the upstream chain */ + SESSION* session; /*< Client session structure */ + char* errmsg; /*< Rule specific error message */ + DOWNSTREAM down; /*< Next object in the downstream chain */ + UPSTREAM up; /*< Next object in the upstream chain */ } FW_SESSION; bool parse_at_times(const char** tok, char** saveptr, RULE* ruledef); @@ -366,9 +363,15 @@ static STRLINK* strlink_reverse_clone(STRLINK* head) return clone; } -static RULELIST* rulelist_push(RULELIST *head, RULE *rule) +/** + * Add a rule to a rulebook + * @param head + * @param rule + * @return + */ +static RULE_BOOK* rulebook_push(RULE_BOOK *head, RULE *rule) { - RULELIST *rval = MXS_MALLOC(sizeof(RULELIST)); + RULE_BOOK *rval = MXS_MALLOC(sizeof(RULE_BOOK)); if (rval) { @@ -378,16 +381,16 @@ static RULELIST* rulelist_push(RULELIST *head, RULE *rule) return rval; } -static void* rulelist_clone(void* fval) +static void* rulebook_clone(void* fval) { - RULELIST *rule = NULL, - *ptr = (RULELIST*) fval; + RULE_BOOK *rule = NULL, + *ptr = (RULE_BOOK*) fval; while (ptr) { - RULELIST* tmp = (RULELIST*) MXS_MALLOC(sizeof(RULELIST)); + RULE_BOOK* tmp = (RULE_BOOK*) MXS_MALLOC(sizeof(RULE_BOOK)); MXS_ABORT_IF_NULL(tmp); tmp->next = rule; tmp->rule = ptr->rule; @@ -398,25 +401,25 @@ static void* rulelist_clone(void* fval) return (void*) rule; } -static void* rulelist_free(void* fval) +static void* rulebook_free(void* fval) { - RULELIST *ptr = (RULELIST*) fval; + RULE_BOOK *ptr = (RULE_BOOK*) fval; while (ptr) { - RULELIST *tmp = ptr; + RULE_BOOK *tmp = ptr; ptr = ptr->next; MXS_FREE(tmp); } return NULL; } -static void huserfree(void* fval) +static void dbfw_user_free(void* fval) { USER* value = (USER*) fval; - rulelist_free(value->rules_and); - rulelist_free(value->rules_or); - rulelist_free(value->rules_strict_and); + rulebook_free(value->rules_and); + rulebook_free(value->rules_or); + rulebook_free(value->rules_strict_and); MXS_FREE(value->qs_limit); MXS_FREE(value->name); MXS_FREE(value); @@ -710,13 +713,13 @@ void add_users(char* rule, FW_INSTANCE* instance) * * @param instance Filter instance * @param user User name - * @param rulelist List of rules to apply + * @param rulebook List of rules to apply * @param type Matching type, one of FWTOK_MATCH_ANY, FWTOK_MATCH_ALL or FWTOK_MATCH_STRICT_ALL * @return True of the rules were successfully applied. False if memory allocation * fails */ static bool apply_rule_to_user(FW_INSTANCE *instance, char *username, - RULELIST *rulelist, enum match_type type) + RULE_BOOK *rulebook, enum match_type type) { USER* user; ss_dassert(type == FWTOK_MATCH_ANY || type == FWTOK_MATCH_STRICT_ALL || type == FWTOK_MATCH_ALL); @@ -732,8 +735,8 @@ static bool apply_rule_to_user(FW_INSTANCE *instance, char *username, user->name = (char*) MXS_STRDUP_A(username); user->qs_limit = NULL; - RULELIST *tl = (RULELIST*) rulelist_clone(rulelist); - RULELIST *tail = tl; + RULE_BOOK *tl = (RULE_BOOK*) rulebook_clone(rulebook); + RULE_BOOK *tail = tl; while (tail && tail->next) { @@ -1247,19 +1250,19 @@ static bool process_user_templates(FW_INSTANCE *instance, user_template_t *templ } } - RULELIST *foundrules = NULL; + RULE_BOOK *foundrules = NULL; RULE *rule; STRLINK *names = templates->rulenames; while (names && (rule = find_rule_by_name(rules, names->value))) { - foundrules = rulelist_push(foundrules, rule); + foundrules = rulebook_push(foundrules, rule); names = names->next; } if (foundrules) { - RULELIST *tail = foundrules; + RULE_BOOK *tail = foundrules; while (tail->next) { @@ -1389,7 +1392,7 @@ createInstance(const char *name, char **options, FILTER_PARAMETER **params) return NULL; } - hashtable_memory_fns(ht, hashtable_item_strdup, NULL, hashtable_item_free, huserfree); + hashtable_memory_fns(ht, hashtable_item_strdup, NULL, hashtable_item_free, dbfw_user_free); my_instance->htable = ht; my_instance->action = FW_ACTION_BLOCK; @@ -1691,7 +1694,7 @@ static char* create_parse_error(FW_INSTANCE* my_instance, * @param my_instance Fwfilter instance * @param my_session Fwfilter session * @param queue The GWBUF containing the query - * @param rulelist The rule to check + * @param rulebook The rule to check * @param query Pointer to the null-terminated query string * @return true if the query matches the rule */ @@ -1699,7 +1702,7 @@ bool rule_matches(FW_INSTANCE* my_instance, FW_SESSION* my_session, GWBUF *queue, USER* user, - RULELIST *rulelist, + RULE_BOOK *rulebook, char* query) { char *ptr, *msg = NULL; @@ -1735,9 +1738,9 @@ bool rule_matches(FW_INSTANCE* my_instance, if (parse_result != QC_QUERY_PARSED) { - if ((rulelist->rule->type == RT_COLUMN) || - (rulelist->rule->type == RT_WILDCARD) || - (rulelist->rule->type == RT_CLAUSE)) + if ((rulebook->rule->type == RT_COLUMN) || + (rulebook->rule->type == RT_WILDCARD) || + (rulebook->rule->type == RT_CLAUSE)) { switch (optype) { @@ -1762,12 +1765,12 @@ bool rule_matches(FW_INSTANCE* my_instance, is_real = false; } - if (rulelist->rule->on_queries == QUERY_OP_UNDEFINED || - rulelist->rule->on_queries & optype || + if (rulebook->rule->on_queries == QUERY_OP_UNDEFINED || + rulebook->rule->on_queries & optype || (MYSQL_IS_COM_INIT_DB((uint8_t*)GWBUF_DATA(queue)) && - rulelist->rule->on_queries & QUERY_OP_CHANGE_DB)) + rulebook->rule->on_queries & QUERY_OP_CHANGE_DB)) { - switch (rulelist->rule->type) + switch (rulebook->rule->type) { case RT_UNDEFINED: MXS_ERROR("Undefined rule type found."); @@ -1777,11 +1780,11 @@ bool rule_matches(FW_INSTANCE* my_instance, if (query) { pcre2_match_data *mdata = pcre2_match_data_create_from_pattern( - rulelist->rule->data, NULL); + rulebook->rule->data, NULL); if (mdata) { - if (pcre2_match((pcre2_code*) rulelist->rule->data, + if (pcre2_match((pcre2_code*) rulebook->rule->data, (PCRE2_SPTR) query, PCRE2_ZERO_TERMINATED, 0, 0, mdata, NULL) > 0) { @@ -1791,7 +1794,7 @@ bool rule_matches(FW_INSTANCE* my_instance, if (matches) { msg = MXS_STRDUP_A("Permission denied, query matched regular expression."); - MXS_INFO("dbfwfilter: rule '%s': regex matched on query", rulelist->rule->name); + MXS_INFO("dbfwfilter: rule '%s': regex matched on query", rulebook->rule->name); goto queryresolved; } } @@ -1809,7 +1812,7 @@ bool rule_matches(FW_INSTANCE* my_instance, msg = MXS_STRDUP_A("Permission denied at this time."); char buffer[32]; // asctime documentation requires 26 asctime_r(&tm_now, buffer); - MXS_INFO("dbfwfilter: rule '%s': query denied at: %s", rulelist->rule->name, buffer); + MXS_INFO("dbfwfilter: rule '%s': query denied at: %s", rulebook->rule->name, buffer); goto queryresolved; } break; @@ -1825,7 +1828,7 @@ bool rule_matches(FW_INSTANCE* my_instance, { const char* tok = infos[i].column; - STRLINK* strln = (STRLINK*) rulelist->rule->data; + STRLINK* strln = (STRLINK*) rulebook->rule->data; while (strln) { if (strcasecmp(tok, strln->value) == 0) @@ -1834,7 +1837,7 @@ bool rule_matches(FW_INSTANCE* my_instance, sprintf(emsg, "Permission denied to column '%s'.", strln->value); MXS_INFO("dbfwfilter: rule '%s': query targets forbidden column: %s", - rulelist->rule->name, strln->value); + rulebook->rule->name, strln->value); msg = MXS_STRDUP_A(emsg); goto queryresolved; } @@ -1860,7 +1863,7 @@ bool rule_matches(FW_INSTANCE* my_instance, matches = true; msg = MXS_STRDUP_A("Usage of wildcard denied."); MXS_INFO("dbfwfilter: rule '%s': query contains a wildcard.", - rulelist->rule->name); + rulebook->rule->name); goto queryresolved; } } @@ -1873,7 +1876,7 @@ bool rule_matches(FW_INSTANCE* my_instance, * and initialize a new QUERYSPEED struct for this session. */ spinlock_acquire(&my_instance->lock); - rule_qs = (QUERYSPEED*) rulelist->rule->data; + rule_qs = (QUERYSPEED*) rulebook->rule->data; spinlock_release(&my_instance->lock); spinlock_acquire(&user->lock); @@ -1913,7 +1916,7 @@ bool rule_matches(FW_INSTANCE* my_instance, sprintf(emsg, "Queries denied for %f seconds", blocked_for); MXS_INFO("dbfwfilter: rule '%s': user denied for %f seconds", - rulelist->rule->name, blocked_for); + rulebook->rule->name, blocked_for); msg = MXS_STRDUP_A(emsg); matches = true; } @@ -1933,7 +1936,7 @@ bool rule_matches(FW_INSTANCE* my_instance, MXS_INFO("dbfwfilter: rule '%s': query limit triggered (%d queries in %d seconds), " "denying queries from user for %d seconds.", - rulelist->rule->name, + rulebook->rule->name, queryspeed->limit, queryspeed->period, queryspeed->cooldown); @@ -1962,7 +1965,7 @@ bool rule_matches(FW_INSTANCE* my_instance, matches = true; msg = MXS_STRDUP_A("Required WHERE/HAVING clause is missing."); MXS_INFO("dbfwfilter: rule '%s': query has no where/having " - "clause, query is denied.", rulelist->rule->name); + "clause, query is denied.", rulebook->rule->name); } break; @@ -1985,45 +1988,45 @@ queryresolved: if (matches) { - rulelist->rule->times_matched++; + rulebook->rule->times_matched++; } return matches; } /** - * Check if the query matches any of the rules in the user's rulelist. + * Check if the query matches any of the rules in the user's rulebook. * @param my_instance Fwfilter instance * @param my_session Fwfilter session * @param queue The GWBUF containing the query - * @param user The user whose rulelist is checked + * @param user The user whose rulebook is checked * @return True if the query matches at least one of the rules otherwise false */ bool check_match_any(FW_INSTANCE* my_instance, FW_SESSION* my_session, GWBUF *queue, USER* user, char** rulename) { - RULELIST* rulelist; + RULE_BOOK* rulebook; bool rval = false; - if ((rulelist = user->rules_or) && + if ((rulebook = user->rules_or) && (modutil_is_SQL(queue) || modutil_is_SQL_prepare(queue) || MYSQL_IS_COM_INIT_DB((uint8_t*)GWBUF_DATA(queue)))) { char *fullquery = modutil_get_SQL(queue); - while (rulelist) + while (rulebook) { - if (!rule_is_active(rulelist->rule)) + if (!rule_is_active(rulebook->rule)) { - rulelist = rulelist->next; + rulebook = rulebook->next; continue; } - if (rule_matches(my_instance, my_session, queue, user, rulelist, fullquery)) + if (rule_matches(my_instance, my_session, queue, user, rulebook, fullquery)) { - *rulename = MXS_STRDUP_A(rulelist->rule->name); + *rulename = MXS_STRDUP_A(rulebook->rule->name); rval = true; break; } - rulelist = rulelist->next; + rulebook = rulebook->next; } MXS_FREE(fullquery); @@ -2067,11 +2070,11 @@ void append_string(char** dest, size_t* size, const char* src) } /** - * Check if the query matches all rules in the user's rulelist. + * Check if the query matches all rules in the user's rulebook. * @param my_instance Fwfilter instance * @param my_session Fwfilter session * @param queue The GWBUF containing the query - * @param user The user whose rulelist is checked + * @param user The user whose rulebook is checked * @return True if the query matches all of the rules otherwise false */ bool check_match_all(FW_INSTANCE* my_instance, FW_SESSION* my_session, @@ -2079,27 +2082,27 @@ bool check_match_all(FW_INSTANCE* my_instance, FW_SESSION* my_session, { bool rval = false; bool have_active_rule = false; - RULELIST* rulelist = strict_all ? user->rules_strict_and : user->rules_and; + RULE_BOOK* rulebook = strict_all ? user->rules_strict_and : user->rules_and; char *matched_rules = NULL; size_t size = 0; - if (rulelist && (modutil_is_SQL(queue) || modutil_is_SQL_prepare(queue))) + if (rulebook && (modutil_is_SQL(queue) || modutil_is_SQL_prepare(queue))) { char *fullquery = modutil_get_SQL(queue); rval = true; - while (rulelist) + while (rulebook) { - if (!rule_is_active(rulelist->rule)) + if (!rule_is_active(rulebook->rule)) { - rulelist = rulelist->next; + rulebook = rulebook->next; continue; } have_active_rule = true; - if (rule_matches(my_instance, my_session, queue, user, rulelist, fullquery)) + if (rule_matches(my_instance, my_session, queue, user, rulebook, fullquery)) { - append_string(&matched_rules, &size, rulelist->rule->name); + append_string(&matched_rules, &size, rulebook->rule->name); } else { @@ -2110,7 +2113,7 @@ bool check_match_all(FW_INSTANCE* my_instance, FW_SESSION* my_session, } } - rulelist = rulelist->next; + rulebook = rulebook->next; } if (!have_active_rule) From b08e48113740450b8b9265994f05fbaa8713c12f Mon Sep 17 00:00:00 2001 From: Markus Makela Date: Tue, 15 Nov 2016 23:28:38 +0200 Subject: [PATCH 07/29] Fix memory leak on reactivation of old server If a destroyed server was reactivated, it would leak memory. --- server/core/service.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/server/core/service.c b/server/core/service.c index f205338ed..9797bdaf8 100644 --- a/server/core/service.c +++ b/server/core/service.c @@ -802,6 +802,10 @@ serviceAddBackend(SERVICE *service, SERVER *server) atomic_synchronize(); prev->next = new_ref; } + else + { + MXS_FREE(new_ref); + } } else { From ae339f174c56419d033e03782f94db329b4695a8 Mon Sep 17 00:00:00 2001 From: Markus Makela Date: Tue, 15 Nov 2016 23:30:11 +0200 Subject: [PATCH 08/29] Allow servers to be added to multiple objects at one time The maxadmin interface to add servers to objects now allows a maximum of 11 objects to be listed. This will make it simpler to add a server to both a monitor and a service in one command. --- server/modules/routing/debugcli/debugcmd.c | 63 ++++++++++++---------- 1 file changed, 36 insertions(+), 27 deletions(-) diff --git a/server/modules/routing/debugcli/debugcmd.c b/server/modules/routing/debugcli/debugcmd.c index 0b19e3bb3..226e0fd82 100644 --- a/server/modules/routing/debugcli/debugcmd.c +++ b/server/modules/routing/debugcli/debugcmd.c @@ -734,37 +734,42 @@ struct subcommand disableoptions[] = static void telnetdAddUser(DCB *, char *user, char *password); -static void cmd_AddServer(DCB *dcb, void *a, void *b) +static void cmd_AddServer(DCB *dcb, SERVER *server, char *v1, char *v2, char *v3, + char *v4, char *v5, char *v6, char *v7, char *v8, char *v9, + char *v10, char *v11) { - SERVER *server = (SERVER*)a; - char *name = (char*)b; + char *values[11] = {v1, v2, v3, v4, v5, v6, v7, v8, v9, v10, v11}; + const int items = sizeof(values) / sizeof(values[0]); - SERVICE *service = service_find(name); - MONITOR *monitor = monitor_find(name); - - if (service || monitor) + for (int i = 0; i < items && values[i]; i++) { - ss_dassert(service == NULL || monitor == NULL); + SERVICE *service = service_find(values[i]); + MONITOR *monitor = monitor_find(values[i]); - if (service) + if (service || monitor) { - serviceAddBackend(service, server); - service_serialize_servers(service); + ss_dassert(service == NULL || monitor == NULL); + + if (service) + { + serviceAddBackend(service, server); + service_serialize_servers(service); + } + else if (monitor) + { + monitorAddServer(monitor, server); + monitor_serialize_servers(monitor); + } + + const char *target = service ? "service" : "monitor"; + + MXS_NOTICE("Added server '%s' to %s '%s'", server->unique_name, target, values[i]); + dcb_printf(dcb, "Added server '%s' to %s '%s'\n", server->unique_name, target, values[i]); } - else if (monitor) + else { - monitorAddServer(monitor, server); - monitor_serialize_servers(monitor); + dcb_printf(dcb, "No service or monitor with the name '%s'\n", values[i]); } - - const char *target = service ? "service" : "monitor"; - - MXS_NOTICE("Added server '%s' to %s '%s'", server->unique_name, target, name); - dcb_printf(dcb, "Added server '%s' to %s '%s'\n", server->unique_name, target, name); - } - else - { - dcb_printf(dcb, "No service or monitor with the name '%s'\n", name); } } @@ -781,11 +786,15 @@ struct subcommand addoptions[] = {ARG_TYPE_STRING, ARG_TYPE_STRING, 0} }, { - "server", 2, 2, cmd_AddServer, + "server", 2, 12, cmd_AddServer, "Add a new server to a service", - "Usage: add server SERVER TARGET\n" - "The TARGET must be either a service or a monitor", - {ARG_TYPE_SERVER, ARG_TYPE_STRING, 0} + "Usage: add server SERVER TARGET...\n" + "The TARGET must be a list of service and monitor names\n" + "e.g. add server my-db my-service 'Cluster Monitor'\n" + "A server can be assigned to a maximum of 11 objects in one command", + {ARG_TYPE_SERVER, ARG_TYPE_STRING, ARG_TYPE_STRING, ARG_TYPE_STRING, + ARG_TYPE_STRING, ARG_TYPE_STRING, ARG_TYPE_STRING, ARG_TYPE_STRING, + ARG_TYPE_STRING, ARG_TYPE_STRING, ARG_TYPE_STRING, ARG_TYPE_STRING} }, { EMPTY_OPTION} }; From 696d103ed0bd7d1697694694cbbba4e3398864a4 Mon Sep 17 00:00:00 2001 From: Markus Makela Date: Tue, 15 Nov 2016 23:36:59 +0200 Subject: [PATCH 09/29] Allow servers to be removed from multiple objects Since servers can be added to multiple objects, it only makes sense to be able to remove them from multiple objects. --- server/modules/routing/debugcli/debugcmd.c | 138 +++++++++++---------- 1 file changed, 71 insertions(+), 67 deletions(-) diff --git a/server/modules/routing/debugcli/debugcmd.c b/server/modules/routing/debugcli/debugcmd.c index 226e0fd82..8934abf11 100644 --- a/server/modules/routing/debugcli/debugcmd.c +++ b/server/modules/routing/debugcli/debugcmd.c @@ -802,35 +802,41 @@ struct subcommand addoptions[] = static void telnetdRemoveUser(DCB *, char *user, char *password); -static void cmd_RemoveServer(DCB *dcb, void *a, void *b) +static void cmd_RemoveServer(DCB *dcb, SERVER *server, char *v1, char *v2, char *v3, + char *v4, char *v5, char *v6, char *v7, char *v8, char *v9, + char *v10, char *v11) { - SERVER *server = (SERVER*)a; - char *name = (char*)b; - SERVICE *service = service_find(name); - MONITOR *monitor = monitor_find(name); + char *values[11] = {v1, v2, v3, v4, v5, v6, v7, v8, v9, v10, v11}; + const int items = sizeof(values) / sizeof(values[0]); - if (service || monitor) + for (int i = 0; i < items && values[i]; i++) { - ss_dassert(service == NULL || monitor == NULL); + SERVICE *service = service_find(values[i]); + MONITOR *monitor = monitor_find(values[i]); - if (service) + if (service || monitor) { - serviceRemoveBackend(service, server); - service_serialize_servers(service); - } - else if (monitor) - { - monitorRemoveServer(monitor, server); - monitor_serialize_servers(monitor); - } + ss_dassert(service == NULL || monitor == NULL); - const char *target = service ? "service" : "monitor"; - MXS_NOTICE("Removed server '%s' from %s '%s'", server->unique_name, target, name); - dcb_printf(dcb, "Removed server '%s' from %s '%s'\n", server->unique_name, target, name); - } - else - { - dcb_printf(dcb, "No service or monitor with the name '%s'\n", name); + if (service) + { + serviceRemoveBackend(service, server); + service_serialize_servers(service); + } + else if (monitor) + { + monitorRemoveServer(monitor, server); + monitor_serialize_servers(monitor); + } + + const char *target = service ? "service" : "monitor"; + MXS_NOTICE("Removed server '%s' from %s '%s'", server->unique_name, target, values[i]); + dcb_printf(dcb, "Removed server '%s' from %s '%s'\n", server->unique_name, target, values[i]); + } + else + { + dcb_printf(dcb, "No service or monitor with the name '%s'\n", values[i]); + } } } @@ -849,11 +855,15 @@ struct subcommand removeoptions[] = {ARG_TYPE_STRING, ARG_TYPE_STRING} }, { - "server", 2, 2, cmd_RemoveServer, + "server", 2, 12, cmd_RemoveServer, "Remove a server from a service or a monitor", - "Usage: remove server SERVER TARGET\n" - "The TARGET must be either a service or a monitor", - {ARG_TYPE_SERVER, ARG_TYPE_STRING} + "Usage: remove server SERVER TARGET...\n" + "The TARGET must be a list of service and monitor names\n" + "e.g. remove server my-db my-service 'Cluster Monitor'\n" + "A server can be removed from a maximum of 11 objects in one command", + {ARG_TYPE_SERVER, ARG_TYPE_STRING, ARG_TYPE_STRING, ARG_TYPE_STRING, + ARG_TYPE_STRING, ARG_TYPE_STRING, ARG_TYPE_STRING, ARG_TYPE_STRING, + ARG_TYPE_STRING, ARG_TYPE_STRING, ARG_TYPE_STRING, ARG_TYPE_STRING} }, { EMPTY_OPTION @@ -1125,41 +1135,38 @@ static void alterServer(DCB *dcb, SERVER *server, char *v1, char *v2, char *v3, const int items = sizeof(values) / sizeof(values[0]); CONFIG_CONTEXT *obj = NULL; - for (int i = 0; i < items; i++) + for (int i = 0; i < items && values[i]; i++) { - if (values[i]) + char *key = values[i]; + char *value = strchr(key, '='); + + if (value) { - char *key = values[i]; - char *value = strchr(key, '='); + *value++ = '\0'; - if (value) + if (config_is_ssl_parameter(key)) { - *value++ = '\0'; - - if (config_is_ssl_parameter(key)) + /** + * All the required SSL parameters must be defined at once to + * enable SSL for created servers. This removes the problem + * of partial configuration and allows a somewhat atomic + * operation. + */ + if ((obj == NULL && (obj = config_context_create(server->unique_name)) == NULL) || + (!config_add_param(obj, key, value))) { - /** - * All the required SSL parameters must be defined at once to - * enable SSL for created servers. This removes the problem - * of partial configuration and allows a somewhat atomic - * operation. - */ - if ((obj == NULL && (obj = config_context_create(server->unique_name)) == NULL) || - (!config_add_param(obj, key, value))) - { - dcb_printf(dcb, "Internal error, see log for more details\n"); - } - } - else if (!handle_alter_server(server, key, value)) - { - dcb_printf(dcb, "Error: Bad key-value parameter: %s=%s\n", key, value); + dcb_printf(dcb, "Internal error, see log for more details\n"); } } - else + else if (!handle_alter_server(server, key, value)) { - dcb_printf(dcb, "Error: not a key-value parameter: %s\n", values[i]); + dcb_printf(dcb, "Error: Bad key-value parameter: %s=%s\n", key, value); } } + else + { + dcb_printf(dcb, "Error: not a key-value parameter: %s\n", values[i]); + } } if (obj) @@ -1252,27 +1259,24 @@ static void alterMonitor(DCB *dcb, MONITOR *monitor, char *v1, char *v2, char *v char *values[11] = {v1, v2, v3, v4, v5, v6, v7, v8, v9, v10, v11}; const int items = sizeof(values) / sizeof(values[0]); - for (int i = 0; i < items; i++) + for (int i = 0; i < items && values[i]; i++) { - if (values[i]) + char *key = values[i]; + char *value = strchr(key, '='); + + if (value) { - char *key = values[i]; - char *value = strchr(key, '='); + *value++ = '\0'; - if (value) + if (!handle_alter_monitor(monitor, key, value)) { - *value++ = '\0'; - - if (!handle_alter_monitor(monitor, key, value)) - { - dcb_printf(dcb, "Error: Bad key-value parameter: %s=%s\n", key, value); - } - } - else - { - dcb_printf(dcb, "Error: not a key-value parameter: %s\n", values[i]); + dcb_printf(dcb, "Error: Bad key-value parameter: %s=%s\n", key, value); } } + else + { + dcb_printf(dcb, "Error: not a key-value parameter: %s\n", values[i]); + } } } From 8ae76e3ced0d9352e1a8c73b63b4c41874a8d127 Mon Sep 17 00:00:00 2001 From: Markus Makela Date: Wed, 16 Nov 2016 23:43:38 +0200 Subject: [PATCH 10/29] Clean up dbfwfilter Removed unused functions, made function names more consistent. --- server/modules/filter/dbfwfilter/dbfwfilter.c | 124 +++++++++--------- 1 file changed, 59 insertions(+), 65 deletions(-) diff --git a/server/modules/filter/dbfwfilter/dbfwfilter.c b/server/modules/filter/dbfwfilter/dbfwfilter.c index 14dfad541..7d9dcc8e6 100644 --- a/server/modules/filter/dbfwfilter/dbfwfilter.c +++ b/server/modules/filter/dbfwfilter/dbfwfilter.c @@ -253,16 +253,15 @@ typedef struct user_t RULE_BOOK* rules_and; /*< All of these rules must match for the action to trigger */ RULE_BOOK* rules_strict_and; /*< rules that skip the rest of the rules if one of them * fails. This is only for rules paired with 'match strict_all'. */ -} USER; +} DBFW_USER; /** * The Firewall filter instance. */ typedef struct { - HASHTABLE* htable; /*< User hashtable */ + HASHTABLE* users; /*< User hashtable */ RULE* rules; /*< List of all the rules */ - STRLINK* userstrings; /*< Temporary list of raw strings of users */ enum fw_actions action; /*< Default operation mode, defaults to deny */ int log_match; /*< Log matching and/or non-matching queries */ SPINLOCK lock; /*< Instance spinlock */ @@ -367,7 +366,7 @@ static STRLINK* strlink_reverse_clone(STRLINK* head) * Add a rule to a rulebook * @param head * @param rule - * @return + * @return */ static RULE_BOOK* rulebook_push(RULE_BOOK *head, RULE *rule) { @@ -385,7 +384,7 @@ static void* rulebook_clone(void* fval) { RULE_BOOK *rule = NULL, - *ptr = (RULE_BOOK*) fval; + *ptr = (RULE_BOOK*) fval; while (ptr) @@ -415,7 +414,7 @@ static void* rulebook_free(void* fval) static void dbfw_user_free(void* fval) { - USER* value = (USER*) fval; + DBFW_USER* value = (DBFW_USER*) fval; rulebook_free(value->rules_and); rulebook_free(value->rules_or); @@ -697,17 +696,6 @@ FILTER_OBJECT * GetModuleObject() return &MyObject; } -/** - * Adds the given rule string to the list of strings to be parsed for users. - * @param rule The rule string, assumed to be null-terminated - * @param instance The FW_FILTER instance - */ -void add_users(char* rule, FW_INSTANCE* instance) -{ - assert(rule != NULL && instance != NULL); - instance->userstrings = strlink_push(instance->userstrings, rule); -} - /** * Apply a rule set to a user * @@ -721,12 +709,12 @@ void add_users(char* rule, FW_INSTANCE* instance) static bool apply_rule_to_user(FW_INSTANCE *instance, char *username, RULE_BOOK *rulebook, enum match_type type) { - USER* user; + DBFW_USER* user; ss_dassert(type == FWTOK_MATCH_ANY || type == FWTOK_MATCH_STRICT_ALL || type == FWTOK_MATCH_ALL); - if ((user = (USER*) hashtable_fetch(instance->htable, username)) == NULL) + if ((user = (DBFW_USER*) hashtable_fetch(instance->users, username)) == NULL) { /**New user*/ - if ((user = (USER*) MXS_CALLOC(1, sizeof(USER))) == NULL) + if ((user = (DBFW_USER*) MXS_CALLOC(1, sizeof(DBFW_USER))) == NULL) { return false; } @@ -758,7 +746,7 @@ static bool apply_rule_to_user(FW_INSTANCE *instance, char *username, user->rules_and = tl; break; } - hashtable_add(instance->htable, (void *) username, (void *) user); + hashtable_add(instance->users, (void *) username, (void *) user); return true; } @@ -766,7 +754,7 @@ static bool apply_rule_to_user(FW_INSTANCE *instance, char *username, * Free a TIMERANGE struct * @param tr pointer to a TIMERANGE struct */ -void tr_free(TIMERANGE* tr) +void timerange_free(TIMERANGE* tr) { TIMERANGE *node, *tmp; @@ -902,16 +890,14 @@ bool create_rule(void* scanner, const char* name) * Free a list of rules * @param rule Rules to free */ -static void free_rules(RULE* rule) +static void rule_free_all(RULE* rule) { while (rule) { RULE *tmp = rule->next; - while (rule->active) + if (rule->active) { - TIMERANGE *tr = rule->active; - rule->active = rule->active->next; - MXS_FREE(tr); + timerange_free(rule->active); } switch (rule->type) @@ -933,6 +919,7 @@ static void free_rules(RULE* rule) } MXS_FREE(rule->name); + MXS_FREE(rule); rule = tmp; } } @@ -1230,17 +1217,17 @@ static bool process_user_templates(FW_INSTANCE *instance, user_template_t *templ while (templates) { - USER *user = hashtable_fetch(instance->htable, templates->name); + DBFW_USER *user = hashtable_fetch(instance->users, templates->name); if (user == NULL) { - if ((user = MXS_MALLOC(sizeof(USER))) && (user->name = MXS_STRDUP(templates->name))) + if ((user = MXS_MALLOC(sizeof(DBFW_USER))) && (user->name = MXS_STRDUP(templates->name))) { user->rules_and = NULL; user->rules_or = NULL; user->rules_strict_and = NULL; spinlock_init(&user->lock); - hashtable_add(instance->htable, user->name, user); + hashtable_add(instance->users, user->name, user); } else { @@ -1339,7 +1326,7 @@ static bool process_rule_file(const char* filename, FW_INSTANCE* instance) else { rc = 1; - free_rules(pstack.rule); + rule_free_all(pstack.rule); MXS_ERROR("Failed to process rule file '%s'.", filename); } @@ -1358,6 +1345,18 @@ static bool process_rule_file(const char* filename, FW_INSTANCE* instance) return rc == 0; } +HASHTABLE *create_dbfw_users() +{ + HASHTABLE *ht = hashtable_alloc(100, hashtable_item_strhash, hashtable_item_strcmp); + + if (ht) + { + hashtable_memory_fns(ht, hashtable_item_strdup, NULL, hashtable_item_free, dbfw_user_free); + } + + return ht; +} + /** * Create an instance of the filter for a particular service * within MaxScale. @@ -1373,7 +1372,6 @@ createInstance(const char *name, char **options, FILTER_PARAMETER **params) { FW_INSTANCE *my_instance; int i; - HASHTABLE* ht; char *filename = NULL; bool err = false; @@ -1385,19 +1383,15 @@ createInstance(const char *name, char **options, FILTER_PARAMETER **params) spinlock_init(&my_instance->lock); - if ((ht = hashtable_alloc(100, hashtable_item_strhash, hashtable_item_strcmp)) == NULL) + if ((my_instance->users = create_dbfw_user()) == NULL) { MXS_ERROR("Unable to allocate hashtable."); MXS_FREE(my_instance); return NULL; } - hashtable_memory_fns(ht, hashtable_item_strdup, NULL, hashtable_item_free, dbfw_user_free); - - my_instance->htable = ht; my_instance->action = FW_ACTION_BLOCK; my_instance->log_match = FW_LOG_NONE; - my_instance->userstrings = NULL; for (i = 0; params[i]; i++) { @@ -1452,7 +1446,7 @@ createInstance(const char *name, char **options, FILTER_PARAMETER **params) if (err || !process_rule_file(filename, my_instance)) { - hashtable_free(my_instance->htable); + hashtable_free(my_instance->users); MXS_FREE(my_instance); my_instance = NULL; } @@ -1701,7 +1695,7 @@ static char* create_parse_error(FW_INSTANCE* my_instance, bool rule_matches(FW_INSTANCE* my_instance, FW_SESSION* my_session, GWBUF *queue, - USER* user, + DBFW_USER* user, RULE_BOOK *rulebook, char* query) { @@ -1744,17 +1738,17 @@ bool rule_matches(FW_INSTANCE* my_instance, { switch (optype) { - case QUERY_OP_SELECT: - case QUERY_OP_UPDATE: - case QUERY_OP_INSERT: - case QUERY_OP_DELETE: - // In these cases, we have to be able to trust what qc_get_field_info - // returns. Unless the query was parsed completely, we cannot do that. - msg = create_parse_error(my_instance, "parsed completely", query, &matches); - goto queryresolved; + case QUERY_OP_SELECT: + case QUERY_OP_UPDATE: + case QUERY_OP_INSERT: + case QUERY_OP_DELETE: + // In these cases, we have to be able to trust what qc_get_field_info + // returns. Unless the query was parsed completely, we cannot do that. + msg = create_parse_error(my_instance, "parsed completely", query, &matches); + goto queryresolved; - default: - break; + default: + break; } } } @@ -1807,15 +1801,15 @@ bool rule_matches(FW_INSTANCE* my_instance, break; case RT_PERMISSION: - { - matches = true; - msg = MXS_STRDUP_A("Permission denied at this time."); - char buffer[32]; // asctime documentation requires 26 - asctime_r(&tm_now, buffer); - MXS_INFO("dbfwfilter: rule '%s': query denied at: %s", rulebook->rule->name, buffer); - goto queryresolved; - } - break; + { + matches = true; + msg = MXS_STRDUP_A("Permission denied at this time."); + char buffer[32]; // asctime documentation requires 26 + asctime_r(&tm_now, buffer); + MXS_INFO("dbfwfilter: rule '%s': query denied at: %s", rulebook->rule->name, buffer); + goto queryresolved; + } + break; case RT_COLUMN: if (is_sql && is_real) @@ -2003,7 +1997,7 @@ queryresolved: * @return True if the query matches at least one of the rules otherwise false */ bool check_match_any(FW_INSTANCE* my_instance, FW_SESSION* my_session, - GWBUF *queue, USER* user, char** rulename) + GWBUF *queue, DBFW_USER* user, char** rulename) { RULE_BOOK* rulebook; bool rval = false; @@ -2078,7 +2072,7 @@ void append_string(char** dest, size_t* size, const char* src) * @return True if the query matches all of the rules otherwise false */ bool check_match_all(FW_INSTANCE* my_instance, FW_SESSION* my_session, - GWBUF *queue, USER* user, bool strict_all, char** rulename) + GWBUF *queue, DBFW_USER* user, bool strict_all, char** rulename) { bool rval = false; bool have_active_rule = false; @@ -2138,17 +2132,17 @@ bool check_match_all(FW_INSTANCE* my_instance, FW_SESSION* my_session, * @param remote Remove network address * @return The user data or NULL if it was not found */ -USER* find_user_data(HASHTABLE *hash, const char *name, const char *remote) +DBFW_USER* find_user_data(HASHTABLE *hash, const char *name, const char *remote) { char nameaddr[strlen(name) + strlen(remote) + 2]; snprintf(nameaddr, sizeof(nameaddr), "%s@%s", name, remote); - USER* user = (USER*) hashtable_fetch(hash, nameaddr); + DBFW_USER* user = (DBFW_USER*) hashtable_fetch(hash, nameaddr); if (user == NULL) { char *ip_start = strchr(nameaddr, '@') + 1; while (user == NULL && next_ip_class(ip_start)) { - user = (USER*) hashtable_fetch(hash, nameaddr); + user = (DBFW_USER*) hashtable_fetch(hash, nameaddr); } if (user == NULL) @@ -2157,7 +2151,7 @@ USER* find_user_data(HASHTABLE *hash, const char *name, const char *remote) ip_start = strchr(nameaddr, '@') + 1; while (user == NULL && next_ip_class(ip_start)) { - user = (USER*) hashtable_fetch(hash, nameaddr); + user = (DBFW_USER*) hashtable_fetch(hash, nameaddr); } } } @@ -2194,7 +2188,7 @@ routeQuery(FILTER *instance, void *session, GWBUF *queue) } else { - USER *user = find_user_data(my_instance->htable, dcb->user, dcb->remote); + DBFW_USER *user = find_user_data(my_instance->users, dcb->user, dcb->remote); bool query_ok = false; if (user) From a018e732eb6c5576aa4662e57793d0a8ac1fe1fc Mon Sep 17 00:00:00 2001 From: Markus Makela Date: Thu, 17 Nov 2016 07:17:12 +0200 Subject: [PATCH 11/29] Make the dbfwfilter rule check a separate utility The code for the utility is now stored in a separate file. This also removes the need to include testing headers from other directories. Also added a function to reload rules that uses the newly modified rule parsing mechanism. This can be used later on to update the rules at runtime. --- .../modules/filter/dbfwfilter/CMakeLists.txt | 11 +- .../filter/dbfwfilter/dbfw_rule_check.c | 55 ++++++ server/modules/filter/dbfwfilter/dbfwfilter.c | 156 ++++++------------ 3 files changed, 111 insertions(+), 111 deletions(-) create mode 100644 server/modules/filter/dbfwfilter/dbfw_rule_check.c diff --git a/server/modules/filter/dbfwfilter/CMakeLists.txt b/server/modules/filter/dbfwfilter/CMakeLists.txt index ad1b91371..8f03ad2b0 100644 --- a/server/modules/filter/dbfwfilter/CMakeLists.txt +++ b/server/modules/filter/dbfwfilter/CMakeLists.txt @@ -11,12 +11,11 @@ if(BISON_FOUND AND FLEX_FOUND) set_target_properties(dbfwfilter PROPERTIES VERSION "1.0.0") install_module(dbfwfilter core) - if(BUILD_TOOLS) - add_executable(dbfwruleparser dbfwfilter.c ${BISON_ruleparser_OUTPUTS} ${FLEX_token_OUTPUTS}) - target_compile_definitions(dbfwruleparser PUBLIC "BUILD_RULE_PARSER") - target_link_libraries(dbfwruleparser maxscale-common) - install_module(dbfwruleparser core) - endif() + # The offline rule check utility + add_executable(dbfwchk dbfw_rule_check.c ${BISON_ruleparser_OUTPUTS} ${FLEX_token_OUTPUTS}) + target_link_libraries(dbfwchk maxscale-common) + install_executable(dbfwchk core) + else() message(FATAL_ERROR "Could not find Bison or Flex: ${BISON_EXECUTABLE} ${FLEX_EXECUTABLE}") endif() diff --git a/server/modules/filter/dbfwfilter/dbfw_rule_check.c b/server/modules/filter/dbfwfilter/dbfw_rule_check.c new file mode 100644 index 000000000..13f8adf6f --- /dev/null +++ b/server/modules/filter/dbfwfilter/dbfw_rule_check.c @@ -0,0 +1,55 @@ +/* + * Copyright (c) 2016 MariaDB Corporation Ab + * + * Use of this software is governed by the Business Source License included + * in the LICENSE.TXT file and at www.mariadb.com/bsl. + * + * Change Date: 2019-07-01 + * + * On the date above, in accordance with the Business Source License, use + * of this software will be governed by version 2 or later of the General + * Public License. + */ + +#include "dbfwfilter.c" + +int main(int argc, char **argv) +{ + int rval = 1; + + if (argc < 2) + { + printf("Usage: dbfw_rule_check FILE\n"); + } + else + { + mxs_log_init("dbfwfilter_rule_parser", ".", MXS_LOG_TARGET_STDOUT); + + if (access(argv[1], R_OK) == 0) + { + MXS_NOTICE("Parsing rule file: %s", argv[1]); + + RULE* rules; + HASHTABLE *users; + + if (process_rule_file(argv[1], &rules, &users)) + { + MXS_NOTICE("Rule parsing was successful."); + rval = 0; + } + else + { + MXS_ERROR("Failed to parse rules."); + } + } + else + { + MXS_ERROR("Failed to read file '%s': %d, %s", argv[1], errno, strerror(errno)); + } + + mxs_log_finish(); + + } + + return rval; +} diff --git a/server/modules/filter/dbfwfilter/dbfwfilter.c b/server/modules/filter/dbfwfilter/dbfwfilter.c index 7d9dcc8e6..42fcbada9 100644 --- a/server/modules/filter/dbfwfilter/dbfwfilter.c +++ b/server/modules/filter/dbfwfilter/dbfwfilter.c @@ -424,6 +424,18 @@ static void dbfw_user_free(void* fval) MXS_FREE(value); } +HASHTABLE *dbfw_userlist_create() +{ + HASHTABLE *ht = hashtable_alloc(100, hashtable_item_strhash, hashtable_item_strcmp); + + if (ht) + { + hashtable_memory_fns(ht, hashtable_item_strdup, NULL, hashtable_item_free, dbfw_user_free); + } + + return ht; +} + /** * Parses a string that contains an IP address and converts the last octet to '%'. * This modifies the string passed as the parameter. @@ -1204,7 +1216,7 @@ static RULE* find_rule_by_name(RULE* rules, const char* name) * @param rules List of all rules * @return True on success, false on error. */ -static bool process_user_templates(FW_INSTANCE *instance, user_template_t *templates, +static bool process_user_templates(HASHTABLE *users, user_template_t *templates, RULE* rules) { bool rval = true; @@ -1217,7 +1229,7 @@ static bool process_user_templates(FW_INSTANCE *instance, user_template_t *templ while (templates) { - DBFW_USER *user = hashtable_fetch(instance->users, templates->name); + DBFW_USER *user = hashtable_fetch(users, templates->name); if (user == NULL) { @@ -1227,7 +1239,7 @@ static bool process_user_templates(FW_INSTANCE *instance, user_template_t *templ user->rules_or = NULL; user->rules_strict_and = NULL; spinlock_init(&user->lock); - hashtable_add(instance->users, user->name, user); + hashtable_add(users, user->name, user); } else { @@ -1292,7 +1304,7 @@ static bool process_user_templates(FW_INSTANCE *instance, user_template_t *templ * @param instance Filter instance * @return True on success, false on error. */ -static bool process_rule_file(const char* filename, FW_INSTANCE* instance) +static bool process_rule_file(const char* filename, RULE** rules, HASHTABLE **users) { int rc = 1; FILE *file = fopen(filename, "r"); @@ -1318,15 +1330,18 @@ static bool process_rule_file(const char* filename, FW_INSTANCE* instance) dbfw_yy_delete_buffer(buf, scanner); dbfw_yylex_destroy(scanner); fclose(file); + HASHTABLE *new_users = dbfw_userlist_create(); - if (rc == 0 && process_user_templates(instance, pstack.templates, pstack.rule)) + if (rc == 0 && new_users && process_user_templates(new_users, pstack.templates, pstack.rule)) { - instance->rules = pstack.rule; + *rules = pstack.rule; + *users = new_users; } else { rc = 1; rule_free_all(pstack.rule); + hashtable_free(new_users); MXS_ERROR("Failed to process rule file '%s'.", filename); } @@ -1345,16 +1360,39 @@ static bool process_rule_file(const char* filename, FW_INSTANCE* instance) return rc == 0; } -HASHTABLE *create_dbfw_users() -{ - HASHTABLE *ht = hashtable_alloc(100, hashtable_item_strhash, hashtable_item_strcmp); - if (ht) +/** + * @brief Replace the rule file used by this filter instance + * + * This function does no locking. An external lock needs to protect this function + * call to prevent any connections from using the data when it is being replaced. + * + * @param filename File where the rules are located + * @param instance Filter instance + * @return True on success, false on error. If the return value is false, the + * old rules remain active. + */ +bool replace_rule_file(const char* filename, FW_INSTANCE* instance) +{ + bool rval = false; + RULE *rules; + HASHTABLE *users; + + if (process_rule_file(filename, &rules, &users)) { - hashtable_memory_fns(ht, hashtable_item_strdup, NULL, hashtable_item_free, dbfw_user_free); + /** Rules processed successfully, free the old ones */ + rule_free_all(instance->rules); + hashtable_free(instance->users); + instance->rules = rules; + instance->users = users; + rval = true; + } + else + { + MXS_ERROR("Failed to process rule file at '%s', old rules are still active.", filename); } - return ht; + return rval; } /** @@ -1382,14 +1420,6 @@ createInstance(const char *name, char **options, FILTER_PARAMETER **params) } spinlock_init(&my_instance->lock); - - if ((my_instance->users = create_dbfw_user()) == NULL) - { - MXS_ERROR("Unable to allocate hashtable."); - MXS_FREE(my_instance); - return NULL; - } - my_instance->action = FW_ACTION_BLOCK; my_instance->log_match = FW_LOG_NONE; @@ -1444,9 +1474,8 @@ createInstance(const char *name, char **options, FILTER_PARAMETER **params) err = true; } - if (err || !process_rule_file(filename, my_instance)) + if (err || !process_rule_file(filename, &my_instance->rules, &my_instance->users)) { - hashtable_free(my_instance->users); MXS_FREE(my_instance); my_instance = NULL; } @@ -2332,86 +2361,3 @@ static uint64_t getCapabilities(void) { return RCAP_TYPE_STMT_INPUT; } - -#ifdef BUILD_RULE_PARSER -// TODO: Not ok to include file from other component's test directory. -#include "../../../core/test/test_utils.h" - -int main(int argc, char** argv) -{ - char ch; - bool have_icase = false; - char *home; - char cwd[PATH_MAX]; - char* opts[2] = {NULL, NULL}; - FILTER_PARAMETER ruleparam; - FILTER_PARAMETER * paramlist[2]; - - opterr = 0; - while ((ch = getopt(argc, argv, "h?")) != -1) - { - switch (ch) - { - case '?': - case 'h': - printf("Usage: %s [OPTION]... RULEFILE\n" - "Options:\n" - "\t-?\tPrint this information\n", - argv[0]); - return 0; - default: - printf("Unknown option '%c'.\n", ch); - return 1; - } - } - - if (argc < 2) - { - printf("Usage: %s [OPTION]... RULEFILE\n" - "-?\tPrint this information\n", - argv[0]); - return 1; - } - - home = MXS_MALLOC(sizeof(char) * (PATH_MAX + 1)); - MXS_ABORT_IF_NULL(home); - if (getcwd(home, PATH_MAX) == NULL) - { - MXS_FREE(home); - home = NULL; - } - - printf("Log files written to: %s\n", home ? home : "/tpm"); - - int argc_ = 2; - char* argv_[] = - { - "log_manager", - "-o", - NULL - }; - - mxs_log_init(NULL, NULL, MXS_LOG_TARGET_DEFAULT); - - - init_test_env(home); - ruleparam.name = MXS_STRDUP_A("rules"); - ruleparam.value = MXS_STRDUP_A(argv[1]); - paramlist[0] = &ruleparam; - paramlist[1] = NULL; - - if (createInstance(opts, paramlist)) - { - printf("Rule parsing was successful.\n"); - } - else - { - printf("Failed to parse rule. Read the error log for the reason of the failure.\n"); - } - - mxs_log_flush_sync(); - - return 0; -} - -#endif From d64029102314dc63d342d8c32032a7f512100c97 Mon Sep 17 00:00:00 2001 From: Johan Wikman Date: Fri, 18 Nov 2016 16:00:57 +0200 Subject: [PATCH 12/29] Add crashing test-case With 2.0.1 or earlier, if a statement contains a trailing NULL, the statement will inside qc_sqlite.c incorrectly be assumed not to be the one to be classified with a crash being indirectly the result. --- query_classifier/test/CMakeLists.txt | 9 +++- query_classifier/test/crash_qc_sqlite.c | 66 +++++++++++++++++++++++++ 2 files changed, 74 insertions(+), 1 deletion(-) create mode 100644 query_classifier/test/crash_qc_sqlite.c diff --git a/query_classifier/test/CMakeLists.txt b/query_classifier/test/CMakeLists.txt index 565b067f5..ba817cb5e 100644 --- a/query_classifier/test/CMakeLists.txt +++ b/query_classifier/test/CMakeLists.txt @@ -17,9 +17,16 @@ if (BUILD_QC_MYSQLEMBEDDED) endif() add_executable(classify classify.c) - add_executable(compare compare.cc) target_link_libraries(classify maxscale-common) + + add_executable(compare compare.cc) target_link_libraries(compare maxscale-common) + + add_executable(crash_qc_sqlite crash_qc_sqlite.c) + target_link_libraries(crash_qc_sqlite maxscale-common) + + add_test(TestQC_Crash_qcsqlite crash_qc_sqlite) + add_test(TestQC_MySQLEmbedded classify qc_mysqlembedded ${CMAKE_CURRENT_SOURCE_DIR}/input.sql ${CMAKE_CURRENT_SOURCE_DIR}/expected.sql) add_test(TestQC_SqLite classify qc_sqlite ${CMAKE_CURRENT_SOURCE_DIR}/input.sql ${CMAKE_CURRENT_SOURCE_DIR}/expected.sql) diff --git a/query_classifier/test/crash_qc_sqlite.c b/query_classifier/test/crash_qc_sqlite.c new file mode 100644 index 000000000..2f1983f05 --- /dev/null +++ b/query_classifier/test/crash_qc_sqlite.c @@ -0,0 +1,66 @@ +/* + * Copyright (c) 2016 MariaDB Corporation Ab + * + * Use of this software is governed by the Business Source License included + * in the LICENSE.TXT file and at www.mariadb.com/bsl. + * + * Change Date: 2019-01-01 + * + * On the date above, in accordance with the Business Source License, use + * of this software will be governed by version 2 or later of the General + * Public License. + */ + +#include +#include +#include +#include + +#define MYSQL_HEADER_LEN 4 + +GWBUF* create_gwbuf(const char* s, size_t len) +{ + size_t payload_len = len + 1; + size_t gwbuf_len = MYSQL_HEADER_LEN + payload_len; + + GWBUF* gwbuf = gwbuf_alloc(gwbuf_len); + + *((unsigned char*)((char*)GWBUF_DATA(gwbuf))) = payload_len; + *((unsigned char*)((char*)GWBUF_DATA(gwbuf) + 1)) = (payload_len >> 8); + *((unsigned char*)((char*)GWBUF_DATA(gwbuf) + 2)) = (payload_len >> 16); + *((unsigned char*)((char*)GWBUF_DATA(gwbuf) + 3)) = 0x00; + *((unsigned char*)((char*)GWBUF_DATA(gwbuf) + 4)) = 0x03; + memcpy((char*)GWBUF_DATA(gwbuf) + 5, s, len); + + return gwbuf; +} + +int main() +{ + int rv = EXIT_FAILURE; + + set_libdir(strdup("../qc_sqlite")); + + if (qc_init("qc_sqlite", NULL)) + { + const char s[] = "SELECT @@global.max_allowed_packet"; + + GWBUF *stmt = create_gwbuf(s, sizeof(s)); // Include superfluous NULL. + + // In 2.0.1 this crashed due to is_submitted_query() in qc_sqlite.c + // being of the opinion that the statement was not the one to be + // classified and hence an alien parse-tree being passed to sqlite3's + // code generator. + qc_parse(stmt); + + qc_end(); + + rv = EXIT_SUCCESS; + } + else + { + fprintf(stderr, "error: Could not load query classifier."); + } + + return rv; +} From ecb6680e71f064e1f14c316558ab4f688fec9265 Mon Sep 17 00:00:00 2001 From: Johan Wikman Date: Fri, 18 Nov 2016 13:33:44 +0200 Subject: [PATCH 13/29] MXS-976: qc_sqlite: Force initialization of sqlite3 Sqlite3 performs some lazy initialization, during which it internally parses some SQL statements of its own. Earlier there was detection code for noticing that, but it was costly and errorprone. Now, sqlite3 is forced to perform the initialization at startup so that we no longer need any detection code. --- query_classifier/qc_sqlite/qc_sqlite.c | 121 ++++++++----------------- 1 file changed, 38 insertions(+), 83 deletions(-) diff --git a/query_classifier/qc_sqlite/qc_sqlite.c b/query_classifier/qc_sqlite/qc_sqlite.c index f96b4cf57..d44c09949 100644 --- a/query_classifier/qc_sqlite/qc_sqlite.c +++ b/query_classifier/qc_sqlite/qc_sqlite.c @@ -77,6 +77,7 @@ typedef struct qc_sqlite_info size_t database_names_capacity; // The capacity of database_names. int keyword_1; // The first encountered keyword. int keyword_2; // The second encountered keyword. + bool initializing; // Whether we are initializing sqlite3. } QC_SQLITE_INFO; typedef enum qc_log_level @@ -188,7 +189,6 @@ static QC_SQLITE_INFO* info_alloc(void); static void info_finish(QC_SQLITE_INFO* info); static void info_free(QC_SQLITE_INFO* info); static QC_SQLITE_INFO* info_init(QC_SQLITE_INFO* info); -static bool is_submitted_query(const QC_SQLITE_INFO* info, const Parse* pParse); static void log_invalid_data(GWBUF* query, const char* message); static bool parse_query(GWBUF* query); static void parse_query_string(const char* query, size_t len); @@ -381,6 +381,7 @@ static QC_SQLITE_INFO* info_init(QC_SQLITE_INFO* info) info->database_names_capacity = 0; info->keyword_1 = 0; // Sqlite3 starts numbering tokens from 1, so 0 means info->keyword_2 = 0; // that we have not seen a keyword. + info->initializing = false; return info; } @@ -528,76 +529,6 @@ static bool query_is_parsed(GWBUF* query) return query && GWBUF_IS_PARSED(query); } -/* - * Check that the statement being reported about is the one that initially was - * submitted to parse_query_string(...). When sqlite3 is parsing other statements - * it may (right after it's used for the first time) parse selects of its own. - * We need to detect that, so that the internal stuff is not allowed to interfere - * with the parsing of the provided statement. - * - * @param info The thread specific info structure. - * @param pParse Sqlite3's parse context. - * - * @return True, if the current callback relates to the provided query, - * false otherwise. - */ -static bool is_submitted_query(const QC_SQLITE_INFO* info, const Parse* pParse) -{ - bool rv = false; - - if (*pParse->zTail == 0) - { - // Everything has been consumed => the callback relates to the statement - // that was provided to parse_query_string(...). - rv = true; - } - else if (pParse->sLastToken.z && (*pParse->sLastToken.z == ';')) - { - // A ';' has been reached, i.e. everything has been consumed => the callback - // relates to the statement that was provided to parse_query_string(...). - rv = true; - } - else - { - // If info->query contains a trailing ';', pParse->zTail may or may not contain - // one also. We need to cater for the situation that the former has one and the - // latter does not. - const char* i = info->query; - const char* end = i + info->query_len; - const char* j = pParse->zTail; - - // Walk forward as long as neither has reached the end, - // and the characters are the same. - while ((i < end) && (*j != 0) && (*i == *j)) - { - ++i; - ++j; - } - - if (i == end) - { - // If i has reached the end, then if j also has reached the end, - // both are the same. In this case both either have or have not - // a trailing ';'. - rv = (*j == 0); - } - else if (*j == 0) - { - // Else if j has reached the end, then if i points to ';', both - // are the same. In this case info->query contains a trailing ';' - // while pParse->zTail does not. - rv = (*i == ';'); - } - else - { - // Otherwise they are not the same. - rv = false; - } - } - - return rv; -} - /** * Logs information about invalid data. * @@ -1271,7 +1202,7 @@ void mxs_sqlite3EndTable(Parse *pParse, /* Parse context */ QC_SQLITE_INFO* info = this_thread.info; ss_dassert(info); - if (is_submitted_query(info, pParse)) + if (!info->initializing) { if (pSelect) { @@ -1359,10 +1290,7 @@ int mxs_sqlite3Select(Parse* pParse, Select* p, SelectDest* pDest) QC_SQLITE_INFO* info = this_thread.info; ss_dassert(info); - // Check whether the statement being parsed is the one that was passed - // to sqlite3_prepare in parse_query_string(). During inserts, sqlite may - // parse selects of its own. - if (is_submitted_query(info, pParse)) + if (!info->initializing) { info->status = QC_QUERY_PARSED; info->operation = QUERY_OP_SELECT; @@ -1391,7 +1319,7 @@ void mxs_sqlite3StartTable(Parse *pParse, /* Parser context */ QC_SQLITE_INFO* info = this_thread.info; ss_dassert(info); - if (is_submitted_query(info, pParse)) + if (!info->initializing) { info->status = QC_QUERY_PARSED; info->operation = QUERY_OP_CREATE; @@ -2479,14 +2407,13 @@ static bool qc_sqlite_init(const char* args) if (sqlite3_initialize() == 0) { + init_builtin_functions(); + this_unit.initialized = true; + this_unit.log_level = log_level; if (qc_sqlite_thread_init()) { - init_builtin_functions(); - - this_unit.log_level = log_level; - if (log_level != QC_LOG_NOTHING) { const char* message; @@ -2550,10 +2477,38 @@ static bool qc_sqlite_thread_init(void) int rc = sqlite3_open(":memory:", &this_thread.db); if (rc == SQLITE_OK) { - this_thread.initialized = true; - MXS_INFO("qc_sqlite: In-memory sqlite database successfully opened for thread %lu.", (unsigned long) pthread_self()); + + QC_SQLITE_INFO* info = info_alloc(); + + if (info) + { + this_thread.info = info; + + // With this statement we cause sqlite3 to initialize itself, so that it + // is not done as part of the actual classification of data. + const char* s = "CREATE TABLE __maxscale__internal__ (int field UNIQUE)"; + size_t len = strlen(s); + + this_thread.info->query = s; + this_thread.info->query_len = len; + this_thread.info->initializing = true; + parse_query_string(s, len); + this_thread.info->initializing = false; + this_thread.info->query = NULL; + this_thread.info->query_len = 0; + + info_free(this_thread.info); + this_thread.info = NULL; + + this_thread.initialized = true; + } + else + { + sqlite3_close(this_thread.db); + this_thread.db = NULL; + } } else { From 5198c3e45671cdd1d90f078825f1ac5fabf3c471 Mon Sep 17 00:00:00 2001 From: ekorh475 Date: Fri, 18 Nov 2016 10:54:14 +0200 Subject: [PATCH 14/29] Run astyle on httpd.c and maxinfo_exec.c --- server/modules/protocol/httpd.c | 21 +- server/modules/routing/maxinfo/maxinfo_exec.c | 769 ++++++++++-------- 2 files changed, 420 insertions(+), 370 deletions(-) diff --git a/server/modules/protocol/httpd.c b/server/modules/protocol/httpd.c index 8c98d8bba..83407c9ed 100644 --- a/server/modules/protocol/httpd.c +++ b/server/modules/protocol/httpd.c @@ -39,9 +39,9 @@ #include #include - /* @see function load_module in load_utils.c for explanation of the following - * lint directives. - */ +/* @see function load_module in load_utils.c for explanation of the following + * lint directives. +*/ /*lint -e14 */ MODULE_INFO info = { @@ -145,9 +145,9 @@ static int httpd_read_event(DCB* dcb) SESSION *session = dcb->session; int numchars = 1; - char buf[HTTPD_REQUESTLINE_MAXLEN-1] = ""; + char buf[HTTPD_REQUESTLINE_MAXLEN - 1] = ""; char *query_string = NULL; - char method[HTTPD_METHOD_MAXLEN-1] = ""; + char method[HTTPD_METHOD_MAXLEN - 1] = ""; char url[HTTPD_SMALL_BUFFER] = ""; size_t i, j; int headers_read = 0; @@ -163,11 +163,13 @@ static int httpd_read_event(DCB* dcb) numchars = httpd_get_line(dcb->fd, buf, sizeof(buf)); - i = 0; j = 0; + i = 0; + j = 0; while (!ISspace(buf[j]) && (i < sizeof(method) - 1)) { method[i] = buf[j]; - i++; j++; + i++; + j++; } method[i] = '\0'; @@ -190,7 +192,8 @@ static int httpd_read_event(DCB* dcb) while ((j < sizeof(buf) - 1) && !ISspace(buf[j]) && (i < sizeof(url) - 1)) { url[i] = buf[j]; - i++; j++; + i++; + j++; } url[i] = '\0'; @@ -225,7 +228,7 @@ static int httpd_read_event(DCB* dcb) { *value = '\0'; value++; - end = &value[strlen(value) -1]; + end = &value[strlen(value) - 1]; *end = '\0'; if (strncasecmp(buf, "Hostname", 6) == 0) diff --git a/server/modules/routing/maxinfo/maxinfo_exec.c b/server/modules/routing/maxinfo/maxinfo_exec.c index 8f77d2aec..bbe8a782e 100644 --- a/server/modules/routing/maxinfo/maxinfo_exec.c +++ b/server/modules/routing/maxinfo/maxinfo_exec.c @@ -18,8 +18,8 @@ * @verbatim * Revision History * - * Date Who Description - * 17/02/15 Mark Riddoch Initial implementation + * Date Who Description + * 17/02/15 Mark Riddoch Initial implementation * * @endverbatim */ @@ -59,20 +59,20 @@ void maxinfo_send_ok(DCB *dcb); /** * Execute a parse tree and return the result set or runtime error * - * @param dcb The DCB that connects to the client - * @param tree The parse tree for the query + * @param dcb The DCB that connects to the client + * @param tree The parse tree for the query */ void maxinfo_execute(DCB *dcb, MAXINFO_TREE *tree) { - switch (tree->op) - { - case MAXOP_SHOW: - exec_show(dcb, tree); - break; - case MAXOP_SELECT: - exec_select(dcb, tree); - break; + switch (tree->op) + { + case MAXOP_SHOW: + exec_show(dcb, tree); + break; + case MAXOP_SELECT: + exec_select(dcb, tree); + break; case MAXOP_FLUSH: exec_flush(dcb, tree); @@ -90,212 +90,232 @@ maxinfo_execute(DCB *dcb, MAXINFO_TREE *tree) exec_restart(dcb, tree); break; - case MAXOP_TABLE: - case MAXOP_COLUMNS: - case MAXOP_LITERAL: - case MAXOP_PREDICATE: - case MAXOP_LIKE: - case MAXOP_EQUAL: - default: - maxinfo_send_error(dcb, 0, "Unexpected operator in parse tree"); - } + case MAXOP_TABLE: + case MAXOP_COLUMNS: + case MAXOP_LITERAL: + case MAXOP_PREDICATE: + case MAXOP_LIKE: + case MAXOP_EQUAL: + default: + maxinfo_send_error(dcb, 0, "Unexpected operator in parse tree"); + } } /** * Fetch the list of services and stream as a result set * - * @param dcb DCB to which to stream result set - * @param tree Potential like clause (currently unused) + * @param dcb DCB to which to stream result set + * @param tree Potential like clause (currently unused) */ static void exec_show_services(DCB *dcb, MAXINFO_TREE *tree) { -RESULTSET *set; + RESULTSET *set; - if ((set = serviceGetList()) == NULL) - return; - - resultset_stream_mysql(set, dcb); - resultset_free(set); + if ((set = serviceGetList()) == NULL) + { + return; + } + + resultset_stream_mysql(set, dcb); + resultset_free(set); } /** * Fetch the list of listeners and stream as a result set * - * @param dcb DCB to which to stream result set - * @param tree Potential like clause (currently unused) + * @param dcb DCB to which to stream result set + * @param tree Potential like clause (currently unused) */ static void exec_show_listeners(DCB *dcb, MAXINFO_TREE *tree) { -RESULTSET *set; + RESULTSET *set; - if ((set = serviceGetListenerList()) == NULL) - return; - - resultset_stream_mysql(set, dcb); - resultset_free(set); + if ((set = serviceGetListenerList()) == NULL) + { + return; + } + + resultset_stream_mysql(set, dcb); + resultset_free(set); } /** * Fetch the list of sessions and stream as a result set * - * @param dcb DCB to which to stream result set - * @param tree Potential like clause (currently unused) + * @param dcb DCB to which to stream result set + * @param tree Potential like clause (currently unused) */ static void exec_show_sessions(DCB *dcb, MAXINFO_TREE *tree) { -RESULTSET *set; + RESULTSET *set; - if ((set = sessionGetList(SESSION_LIST_ALL)) == NULL) - return; - - resultset_stream_mysql(set, dcb); - resultset_free(set); + if ((set = sessionGetList(SESSION_LIST_ALL)) == NULL) + { + return; + } + + resultset_stream_mysql(set, dcb); + resultset_free(set); } /** * Fetch the list of client sessions and stream as a result set * - * @param dcb DCB to which to stream result set - * @param tree Potential like clause (currently unused) + * @param dcb DCB to which to stream result set + * @param tree Potential like clause (currently unused) */ static void exec_show_clients(DCB *dcb, MAXINFO_TREE *tree) { -RESULTSET *set; + RESULTSET *set; - if ((set = sessionGetList(SESSION_LIST_CONNECTION)) == NULL) - return; - - resultset_stream_mysql(set, dcb); - resultset_free(set); + if ((set = sessionGetList(SESSION_LIST_CONNECTION)) == NULL) + { + return; + } + + resultset_stream_mysql(set, dcb); + resultset_free(set); } /** * Fetch the list of servers and stream as a result set * - * @param dcb DCB to which to stream result set - * @param tree Potential like clause (currently unused) + * @param dcb DCB to which to stream result set + * @param tree Potential like clause (currently unused) */ static void exec_show_servers(DCB *dcb, MAXINFO_TREE *tree) { -RESULTSET *set; + RESULTSET *set; - if ((set = serverGetList()) == NULL) - return; - - resultset_stream_mysql(set, dcb); - resultset_free(set); + if ((set = serverGetList()) == NULL) + { + return; + } + + resultset_stream_mysql(set, dcb); + resultset_free(set); } /** * Fetch the list of modules and stream as a result set * - * @param dcb DCB to which to stream result set - * @param tree Potential like clause (currently unused) + * @param dcb DCB to which to stream result set + * @param tree Potential like clause (currently unused) */ static void exec_show_modules(DCB *dcb, MAXINFO_TREE *tree) { -RESULTSET *set; + RESULTSET *set; - if ((set = moduleGetList()) == NULL) - return; - - resultset_stream_mysql(set, dcb); - resultset_free(set); + if ((set = moduleGetList()) == NULL) + { + return; + } + + resultset_stream_mysql(set, dcb); + resultset_free(set); } /** * Fetch the list of monitors and stream as a result set * - * @param dcb DCB to which to stream result set - * @param tree Potential like clause (currently unused) + * @param dcb DCB to which to stream result set + * @param tree Potential like clause (currently unused) */ static void exec_show_monitors(DCB *dcb, MAXINFO_TREE *tree) { -RESULTSET *set; + RESULTSET *set; - if ((set = monitorGetList()) == NULL) - return; - - resultset_stream_mysql(set, dcb); - resultset_free(set); + if ((set = monitorGetList()) == NULL) + { + return; + } + + resultset_stream_mysql(set, dcb); + resultset_free(set); } /** * Fetch the event times data * - * @param dcb DCB to which to stream result set - * @param tree Potential like clause (currently unused) + * @param dcb DCB to which to stream result set + * @param tree Potential like clause (currently unused) */ static void exec_show_eventTimes(DCB *dcb, MAXINFO_TREE *tree) { -RESULTSET *set; + RESULTSET *set; - if ((set = eventTimesGetList()) == NULL) - return; - - resultset_stream_mysql(set, dcb); - resultset_free(set); + if ((set = eventTimesGetList()) == NULL) + { + return; + } + + resultset_stream_mysql(set, dcb); + resultset_free(set); } /** * The table of show commands that are supported */ -static struct { - char *name; - void (*func)(DCB *, MAXINFO_TREE *); -} show_commands[] = { - { "variables", exec_show_variables }, - { "status", exec_show_status }, - { "services", exec_show_services }, - { "listeners", exec_show_listeners }, - { "sessions", exec_show_sessions }, - { "clients", exec_show_clients }, - { "servers", exec_show_servers }, - { "modules", exec_show_modules }, - { "monitors", exec_show_monitors }, - { "eventTimes", exec_show_eventTimes }, - { NULL, NULL } +static struct +{ + char *name; + void (*func)(DCB *, MAXINFO_TREE *); +} show_commands[] = +{ + { "variables", exec_show_variables }, + { "status", exec_show_status }, + { "services", exec_show_services }, + { "listeners", exec_show_listeners }, + { "sessions", exec_show_sessions }, + { "clients", exec_show_clients }, + { "servers", exec_show_servers }, + { "modules", exec_show_modules }, + { "monitors", exec_show_monitors }, + { "eventTimes", exec_show_eventTimes }, + { NULL, NULL } }; /** * Execute a show command parse tree and return the result set or runtime error * - * @param dcb The DCB that connects to the client - * @param tree The parse tree for the query + * @param dcb The DCB that connects to the client + * @param tree The parse tree for the query */ static void exec_show(DCB *dcb, MAXINFO_TREE *tree) { -int i; -char errmsg[120]; + int i; + char errmsg[120]; - for (i = 0; show_commands[i].name; i++) - { - if (strcasecmp(show_commands[i].name, tree->value) == 0) - { - (*show_commands[i].func)(dcb, tree->right); - return; - } - } - if (strlen(tree->value) > 80) // Prevent buffer overrun - tree->value[80] = 0; - sprintf(errmsg, "Unsupported show command '%s'", tree->value); - maxinfo_send_error(dcb, 0, errmsg); - MXS_NOTICE("%s", errmsg); + for (i = 0; show_commands[i].name; i++) + { + if (strcasecmp(show_commands[i].name, tree->value) == 0) + { + (*show_commands[i].func)(dcb, tree->right); + return; + } + } + if (strlen(tree->value) > 80) // Prevent buffer overrun + { + tree->value[80] = 0; + } + sprintf(errmsg, "Unsupported show command '%s'", tree->value); + maxinfo_send_error(dcb, 0, errmsg); + MXS_NOTICE("%s", errmsg); } /** * Flush all logs to disk and rotate them. - * @param dcb The DCB that connects to the client - * @param tree The parse tree for the query + * @param dcb The DCB that connects to the client + * @param tree The parse tree for the query */ void exec_flush_logs(DCB *dcb, MAXINFO_TREE *tree) { @@ -310,7 +330,8 @@ static struct { char *name; void (*func)(DCB *, MAXINFO_TREE *); -} flush_commands[] = { +} flush_commands[] = +{ { "logs", exec_flush_logs}, { NULL, NULL} }; @@ -318,8 +339,8 @@ static struct /** * Execute a flush command parse tree and return the result set or runtime error * - * @param dcb The DCB that connects to the client - * @param tree The parse tree for the query + * @param dcb The DCB that connects to the client + * @param tree The parse tree for the query */ static void exec_flush(DCB *dcb, MAXINFO_TREE *tree) @@ -390,7 +411,8 @@ static struct { char *name; void (*func)(DCB *, MAXINFO_TREE *); -} set_commands[] = { +} set_commands[] = +{ { "server", exec_set_server}, { NULL, NULL} }; @@ -398,8 +420,8 @@ static struct /** * Execute a set command parse tree and return the result set or runtime error * - * @param dcb The DCB that connects to the client - * @param tree The parse tree for the query + * @param dcb The DCB that connects to the client + * @param tree The parse tree for the query */ static void exec_set(DCB *dcb, MAXINFO_TREE *tree) @@ -470,7 +492,8 @@ static struct { char *name; void (*func)(DCB *, MAXINFO_TREE *); -} clear_commands[] = { +} clear_commands[] = +{ { "server", exec_clear_server}, { NULL, NULL} }; @@ -478,8 +501,8 @@ static struct /** * Execute a clear command parse tree and return the result set or runtime error * - * @param dcb The DCB that connects to the client - * @param tree The parse tree for the query + * @param dcb The DCB that connects to the client + * @param tree The parse tree for the query */ static void exec_clear(DCB *dcb, MAXINFO_TREE *tree) @@ -590,7 +613,8 @@ static struct { char *name; void (*func)(DCB *, MAXINFO_TREE *); -} shutdown_commands[] = { +} shutdown_commands[] = +{ { "maxscale", exec_shutdown_maxscale}, { "monitor", exec_shutdown_monitor}, { "service", exec_shutdown_service}, @@ -600,8 +624,8 @@ static struct /** * Execute a shutdown command parse tree and return OK or runtime error * - * @param dcb The DCB that connects to the client - * @param tree The parse tree for the query + * @param dcb The DCB that connects to the client + * @param tree The parse tree for the query */ static void exec_shutdown(DCB *dcb, MAXINFO_TREE *tree) @@ -699,7 +723,8 @@ static struct { char *name; void (*func)(DCB *, MAXINFO_TREE *); -} restart_commands[] = { +} restart_commands[] = +{ { "monitor", exec_restart_monitor}, { "service", exec_restart_service}, { NULL, NULL} @@ -708,8 +733,8 @@ static struct /** * Execute a restart command parse tree and return OK or runtime error * - * @param dcb The DCB that connects to the client - * @param tree The parse tree for the query + * @param dcb The DCB that connects to the client + * @param tree The parse tree for the query */ static void exec_restart(DCB *dcb, MAXINFO_TREE *tree) @@ -742,7 +767,7 @@ exec_restart(DCB *dcb, MAXINFO_TREE *tree) static char * getVersion() { - return MAXSCALE_VERSION; + return MAXSCALE_VERSION; } static char *versionComment = "MariaDB MaxScale"; @@ -754,7 +779,7 @@ static char *versionComment = "MariaDB MaxScale"; static char * getVersionComment() { - return versionComment; + return versionComment; } /** @@ -765,109 +790,116 @@ getVersionComment() static char * getMaxScaleHome() { - return getenv("MAXSCALE_HOME"); + return getenv("MAXSCALE_HOME"); } /* The various methods to fetch the variables */ -#define VT_STRING 1 -#define VT_INT 2 +#define VT_STRING 1 +#define VT_INT 2 typedef void *(*STATSFUNC)(); /** * Variables that may be sent in a show variables */ -static struct { - char *name; - int type; - STATSFUNC func; -} variables[] = { - { "version", VT_STRING, (STATSFUNC)getVersion }, - { "version_comment", VT_STRING, (STATSFUNC)getVersionComment }, - { "basedir", VT_STRING, (STATSFUNC)getMaxScaleHome}, - { "MAXSCALE_VERSION", VT_STRING, (STATSFUNC)getVersion }, - { "MAXSCALE_THREADS", VT_INT, (STATSFUNC)config_threadcount }, - { "MAXSCALE_NBPOLLS", VT_INT, (STATSFUNC)config_nbpolls }, - { "MAXSCALE_POLLSLEEP", VT_INT, (STATSFUNC)config_pollsleep }, - { "MAXSCALE_UPTIME", VT_INT, (STATSFUNC)maxscale_uptime }, - { "MAXSCALE_SESSIONS", VT_INT, (STATSFUNC)serviceSessionCountAll }, - { NULL, 0, NULL } +static struct +{ + char *name; + int type; + STATSFUNC func; +} variables[] = +{ + { "version", VT_STRING, (STATSFUNC)getVersion }, + { "version_comment", VT_STRING, (STATSFUNC)getVersionComment }, + { "basedir", VT_STRING, (STATSFUNC)getMaxScaleHome}, + { "MAXSCALE_VERSION", VT_STRING, (STATSFUNC)getVersion }, + { "MAXSCALE_THREADS", VT_INT, (STATSFUNC)config_threadcount }, + { "MAXSCALE_NBPOLLS", VT_INT, (STATSFUNC)config_nbpolls }, + { "MAXSCALE_POLLSLEEP", VT_INT, (STATSFUNC)config_pollsleep }, + { "MAXSCALE_UPTIME", VT_INT, (STATSFUNC)maxscale_uptime }, + { "MAXSCALE_SESSIONS", VT_INT, (STATSFUNC)serviceSessionCountAll }, + { NULL, 0, NULL } }; -typedef struct { - int index; - char *like; +typedef struct +{ + int index; + char *like; } VARCONTEXT; /** * Callback function to populate rows of the show variable * command * - * @param data The context point - * @return The next row or NULL if end of rows + * @param data The context point + * @return The next row or NULL if end of rows */ static RESULT_ROW * variable_row(RESULTSET *result, void *data) { -VARCONTEXT *context = (VARCONTEXT *)data; -RESULT_ROW *row; -char buf[80]; + VARCONTEXT *context = (VARCONTEXT *)data; + RESULT_ROW *row; + char buf[80]; - if (variables[context->index].name) - { - if (context->like && - maxinfo_pattern_match(context->like, - variables[context->index].name)) - { - context->index++; - return variable_row(result, data); - } - row = resultset_make_row(result); - resultset_row_set(row, 0, variables[context->index].name); - switch (variables[context->index].type) - { - case VT_STRING: - resultset_row_set(row, 1, - (char *)(*variables[context->index].func)()); - break; - case VT_INT: - snprintf(buf, 80, "%ld", - (long)(*variables[context->index].func)()); - resultset_row_set(row, 1, buf); - break; - } - context->index++; - return row; - } - return NULL; + if (variables[context->index].name) + { + if (context->like && + maxinfo_pattern_match(context->like, + variables[context->index].name)) + { + context->index++; + return variable_row(result, data); + } + row = resultset_make_row(result); + resultset_row_set(row, 0, variables[context->index].name); + switch (variables[context->index].type) + { + case VT_STRING: + resultset_row_set(row, 1, + (char *)(*variables[context->index].func)()); + break; + case VT_INT: + snprintf(buf, 80, "%ld", + (long)(*variables[context->index].func)()); + resultset_row_set(row, 1, buf); + break; + } + context->index++; + return row; + } + return NULL; } /** * Execute a show variables command applying an optional filter * - * @param dcb The DCB connected to the client - * @param filter A potential like clause or NULL + * @param dcb The DCB connected to the client + * @param filter A potential like clause or NULL */ static void exec_show_variables(DCB *dcb, MAXINFO_TREE *filter) { -RESULTSET *result; -VARCONTEXT context; + RESULTSET *result; + VARCONTEXT context; - if (filter) - context.like = filter->value; - else - context.like = NULL; - context.index = 0; + if (filter) + { + context.like = filter->value; + } + else + { + context.like = NULL; + } + context.index = 0; - if ((result = resultset_create(variable_row, &context)) == NULL) - { - maxinfo_send_error(dcb, 0, "No resources available"); - return; - } - resultset_add_column(result, "Variable_name", 40, COL_TYPE_VARCHAR); - resultset_add_column(result, "Value", 40, COL_TYPE_VARCHAR); - resultset_stream_mysql(result, dcb); - resultset_free(result); + if ((result = resultset_create(variable_row, &context)) == NULL) + { + maxinfo_send_error(dcb, 0, "No resources available"); + return; + } + resultset_add_column(result, "Variable_name", 40, COL_TYPE_VARCHAR); + resultset_add_column(result, "Value", 40, COL_TYPE_VARCHAR); + resultset_stream_mysql(result, dcb); + resultset_free(result); } /** @@ -878,19 +910,19 @@ VARCONTEXT context; RESULTSET * maxinfo_variables() { -RESULTSET *result; -static VARCONTEXT context; + RESULTSET *result; + static VARCONTEXT context; - context.like = NULL; - context.index = 0; + context.like = NULL; + context.index = 0; - if ((result = resultset_create(variable_row, &context)) == NULL) - { - return NULL; - } - resultset_add_column(result, "Variable_name", 40, COL_TYPE_VARCHAR); - resultset_add_column(result, "Value", 40, COL_TYPE_VARCHAR); - return result; + if ((result = resultset_create(variable_row, &context)) == NULL) + { + return NULL; + } + resultset_add_column(result, "Variable_name", 40, COL_TYPE_VARCHAR); + resultset_add_column(result, "Value", 40, COL_TYPE_VARCHAR); + return result; } /** @@ -899,7 +931,7 @@ static VARCONTEXT context; static int maxinfo_all_dcbs() { - return dcb_count_by_usage(DCB_USAGE_ALL); + return dcb_count_by_usage(DCB_USAGE_ALL); } /** @@ -908,7 +940,7 @@ maxinfo_all_dcbs() static int maxinfo_client_dcbs() { - return dcb_count_by_usage(DCB_USAGE_CLIENT); + return dcb_count_by_usage(DCB_USAGE_CLIENT); } /** @@ -917,7 +949,7 @@ maxinfo_client_dcbs() static int maxinfo_listener_dcbs() { - return dcb_count_by_usage(DCB_USAGE_LISTENER); + return dcb_count_by_usage(DCB_USAGE_LISTENER); } /** @@ -926,7 +958,7 @@ maxinfo_listener_dcbs() static int maxinfo_backend_dcbs() { - return dcb_count_by_usage(DCB_USAGE_BACKEND); + return dcb_count_by_usage(DCB_USAGE_BACKEND); } /** @@ -935,7 +967,7 @@ maxinfo_backend_dcbs() static int maxinfo_internal_dcbs() { - return dcb_count_by_usage(DCB_USAGE_INTERNAL); + return dcb_count_by_usage(DCB_USAGE_INTERNAL); } /** @@ -944,7 +976,7 @@ maxinfo_internal_dcbs() static int maxinfo_zombie_dcbs() { - return dcb_count_by_usage(DCB_USAGE_ZOMBIE); + return dcb_count_by_usage(DCB_USAGE_ZOMBIE); } /** @@ -953,7 +985,7 @@ maxinfo_zombie_dcbs() static int maxinfo_read_events() { - return poll_get_stat(POLL_STAT_READ); + return poll_get_stat(POLL_STAT_READ); } /** @@ -962,7 +994,7 @@ maxinfo_read_events() static int maxinfo_write_events() { - return poll_get_stat(POLL_STAT_WRITE); + return poll_get_stat(POLL_STAT_WRITE); } /** @@ -971,7 +1003,7 @@ maxinfo_write_events() static int maxinfo_error_events() { - return poll_get_stat(POLL_STAT_ERROR); + return poll_get_stat(POLL_STAT_ERROR); } /** @@ -980,7 +1012,7 @@ maxinfo_error_events() static int maxinfo_hangup_events() { - return poll_get_stat(POLL_STAT_HANGUP); + return poll_get_stat(POLL_STAT_HANGUP); } /** @@ -989,7 +1021,7 @@ maxinfo_hangup_events() static int maxinfo_accept_events() { - return poll_get_stat(POLL_STAT_ACCEPT); + return poll_get_stat(POLL_STAT_ACCEPT); } /** @@ -998,7 +1030,7 @@ maxinfo_accept_events() static int maxinfo_event_queue_length() { - return poll_get_stat(POLL_STAT_EVQ_LEN); + return poll_get_stat(POLL_STAT_EVQ_LEN); } /** @@ -1007,7 +1039,7 @@ maxinfo_event_queue_length() static int maxinfo_event_pending_queue_length() { - return poll_get_stat(POLL_STAT_EVQ_PENDING); + return poll_get_stat(POLL_STAT_EVQ_PENDING); } /** @@ -1016,7 +1048,7 @@ maxinfo_event_pending_queue_length() static int maxinfo_max_event_queue_length() { - return poll_get_stat(POLL_STAT_EVQ_MAX); + return poll_get_stat(POLL_STAT_EVQ_MAX); } /** @@ -1025,7 +1057,7 @@ maxinfo_max_event_queue_length() static int maxinfo_max_event_queue_time() { - return poll_get_stat(POLL_STAT_MAX_QTIME); + return poll_get_stat(POLL_STAT_MAX_QTIME); } /** @@ -1034,112 +1066,118 @@ maxinfo_max_event_queue_time() static int maxinfo_max_event_exec_time() { - return poll_get_stat(POLL_STAT_MAX_EXECTIME); + return poll_get_stat(POLL_STAT_MAX_EXECTIME); } /** * Variables that may be sent in a show status */ -static struct { - char *name; - int type; - STATSFUNC func; -} status[] = { - { "Uptime", VT_INT, (STATSFUNC)maxscale_uptime }, - { "Uptime_since_flush_status", VT_INT, (STATSFUNC)maxscale_uptime }, - { "Threads_created", VT_INT, (STATSFUNC)config_threadcount }, - { "Threads_running", VT_INT, (STATSFUNC)config_threadcount }, - { "Threadpool_threads", VT_INT, (STATSFUNC)config_threadcount }, - { "Threads_connected", VT_INT, (STATSFUNC)serviceSessionCountAll }, - { "Connections", VT_INT, (STATSFUNC)maxinfo_all_dcbs }, - { "Client_connections", VT_INT, (STATSFUNC)maxinfo_client_dcbs }, - { "Backend_connections", VT_INT, (STATSFUNC)maxinfo_backend_dcbs }, - { "Listeners", VT_INT, (STATSFUNC)maxinfo_listener_dcbs }, - { "Zombie_connections", VT_INT, (STATSFUNC)maxinfo_zombie_dcbs }, - { "Internal_descriptors", VT_INT, (STATSFUNC)maxinfo_internal_dcbs }, - { "Read_events", VT_INT, (STATSFUNC)maxinfo_read_events }, - { "Write_events", VT_INT, (STATSFUNC)maxinfo_write_events }, - { "Hangup_events", VT_INT, (STATSFUNC)maxinfo_hangup_events }, - { "Error_events", VT_INT, (STATSFUNC)maxinfo_error_events }, - { "Accept_events", VT_INT, (STATSFUNC)maxinfo_accept_events }, - { "Event_queue_length", VT_INT, (STATSFUNC)maxinfo_event_queue_length }, - { "Pending_events", VT_INT, (STATSFUNC)maxinfo_event_pending_queue_length }, - { "Max_event_queue_length", VT_INT, (STATSFUNC)maxinfo_max_event_queue_length }, - { "Max_event_queue_time", VT_INT, (STATSFUNC)maxinfo_max_event_queue_time }, - { "Max_event_execution_time", VT_INT, (STATSFUNC)maxinfo_max_event_exec_time }, - { NULL, 0, NULL } +static struct +{ + char *name; + int type; + STATSFUNC func; +} status[] = +{ + { "Uptime", VT_INT, (STATSFUNC)maxscale_uptime }, + { "Uptime_since_flush_status", VT_INT, (STATSFUNC)maxscale_uptime }, + { "Threads_created", VT_INT, (STATSFUNC)config_threadcount }, + { "Threads_running", VT_INT, (STATSFUNC)config_threadcount }, + { "Threadpool_threads", VT_INT, (STATSFUNC)config_threadcount }, + { "Threads_connected", VT_INT, (STATSFUNC)serviceSessionCountAll }, + { "Connections", VT_INT, (STATSFUNC)maxinfo_all_dcbs }, + { "Client_connections", VT_INT, (STATSFUNC)maxinfo_client_dcbs }, + { "Backend_connections", VT_INT, (STATSFUNC)maxinfo_backend_dcbs }, + { "Listeners", VT_INT, (STATSFUNC)maxinfo_listener_dcbs }, + { "Zombie_connections", VT_INT, (STATSFUNC)maxinfo_zombie_dcbs }, + { "Internal_descriptors", VT_INT, (STATSFUNC)maxinfo_internal_dcbs }, + { "Read_events", VT_INT, (STATSFUNC)maxinfo_read_events }, + { "Write_events", VT_INT, (STATSFUNC)maxinfo_write_events }, + { "Hangup_events", VT_INT, (STATSFUNC)maxinfo_hangup_events }, + { "Error_events", VT_INT, (STATSFUNC)maxinfo_error_events }, + { "Accept_events", VT_INT, (STATSFUNC)maxinfo_accept_events }, + { "Event_queue_length", VT_INT, (STATSFUNC)maxinfo_event_queue_length }, + { "Pending_events", VT_INT, (STATSFUNC)maxinfo_event_pending_queue_length }, + { "Max_event_queue_length", VT_INT, (STATSFUNC)maxinfo_max_event_queue_length }, + { "Max_event_queue_time", VT_INT, (STATSFUNC)maxinfo_max_event_queue_time }, + { "Max_event_execution_time", VT_INT, (STATSFUNC)maxinfo_max_event_exec_time }, + { NULL, 0, NULL } }; /** * Callback function to populate rows of the show variable * command * - * @param data The context point - * @return The next row or NULL if end of rows + * @param data The context point + * @return The next row or NULL if end of rows */ static RESULT_ROW * status_row(RESULTSET *result, void *data) { -VARCONTEXT *context = (VARCONTEXT *)data; -RESULT_ROW *row; -char buf[80]; + VARCONTEXT *context = (VARCONTEXT *)data; + RESULT_ROW *row; + char buf[80]; - if (status[context->index].name) - { - if (context->like && - maxinfo_pattern_match(context->like, - status[context->index].name)) - { - context->index++; - return status_row(result, data); - } - row = resultset_make_row(result); - resultset_row_set(row, 0, status[context->index].name); - switch (status[context->index].type) - { - case VT_STRING: - resultset_row_set(row, 1, - (char *)(*status[context->index].func)()); - break; - case VT_INT: - snprintf(buf, 80, "%ld", - (long)(*status[context->index].func)()); - resultset_row_set(row, 1, buf); - break; - } - context->index++; - return row; - } - return NULL; + if (status[context->index].name) + { + if (context->like && + maxinfo_pattern_match(context->like, + status[context->index].name)) + { + context->index++; + return status_row(result, data); + } + row = resultset_make_row(result); + resultset_row_set(row, 0, status[context->index].name); + switch (status[context->index].type) + { + case VT_STRING: + resultset_row_set(row, 1, + (char *)(*status[context->index].func)()); + break; + case VT_INT: + snprintf(buf, 80, "%ld", + (long)(*status[context->index].func)()); + resultset_row_set(row, 1, buf); + break; + } + context->index++; + return row; + } + return NULL; } /** * Execute a show status command applying an optional filter * - * @param dcb The DCB connected to the client - * @param filter A potential like clause or NULL + * @param dcb The DCB connected to the client + * @param filter A potential like clause or NULL */ static void exec_show_status(DCB *dcb, MAXINFO_TREE *filter) { -RESULTSET *result; -VARCONTEXT context; + RESULTSET *result; + VARCONTEXT context; - if (filter) - context.like = filter->value; - else - context.like = NULL; - context.index = 0; + if (filter) + { + context.like = filter->value; + } + else + { + context.like = NULL; + } + context.index = 0; - if ((result = resultset_create(status_row, &context)) == NULL) - { - maxinfo_send_error(dcb, 0, "No resources available"); - return; - } - resultset_add_column(result, "Variable_name", 40, COL_TYPE_VARCHAR); - resultset_add_column(result, "Value", 40, COL_TYPE_VARCHAR); - resultset_stream_mysql(result, dcb); - resultset_free(result); + if ((result = resultset_create(status_row, &context)) == NULL) + { + maxinfo_send_error(dcb, 0, "No resources available"); + return; + } + resultset_add_column(result, "Variable_name", 40, COL_TYPE_VARCHAR); + resultset_add_column(result, "Value", 40, COL_TYPE_VARCHAR); + resultset_stream_mysql(result, dcb); + resultset_free(result); } /** @@ -1150,19 +1188,19 @@ VARCONTEXT context; RESULTSET * maxinfo_status() { -RESULTSET *result; -static VARCONTEXT context; + RESULTSET *result; + static VARCONTEXT context; - context.like = NULL; - context.index = 0; + context.like = NULL; + context.index = 0; - if ((result = resultset_create(status_row, &context)) == NULL) - { - return NULL; - } - resultset_add_column(result, "Variable_name", 40, COL_TYPE_VARCHAR); - resultset_add_column(result, "Value", 40, COL_TYPE_VARCHAR); - return result; + if ((result = resultset_create(status_row, &context)) == NULL) + { + return NULL; + } + resultset_add_column(result, "Variable_name", 40, COL_TYPE_VARCHAR); + resultset_add_column(result, "Value", 40, COL_TYPE_VARCHAR); + return result; } @@ -1170,57 +1208,65 @@ static VARCONTEXT context; * Execute a select command parse tree and return the result set * or runtime error * - * @param dcb The DCB that connects to the client - * @param tree The parse tree for the query + * @param dcb The DCB that connects to the client + * @param tree The parse tree for the query */ static void exec_select(DCB *dcb, MAXINFO_TREE *tree) { - maxinfo_send_error(dcb, 0, "Select not yet implemented"); + maxinfo_send_error(dcb, 0, "Select not yet implemented"); } /** * Perform a "like" pattern match. Only works for leading and trailing % * - * @param pattern Pattern to match - * @param str String to match against pattern - * @return Zero on match + * @param pattern Pattern to match + * @param str String to match against pattern + * @return Zero on match */ static int maxinfo_pattern_match(char *pattern, char *str) { -int anchor = 0, len, trailing; -char *fixed; -extern char *strcasestr(); + int anchor = 0, len, trailing; + char *fixed; + extern char *strcasestr(); - if (*pattern != '%') - { - fixed = pattern; - anchor = 1; - } - else - { - fixed = &pattern[1]; - } - len = strlen(fixed); - if (fixed[len - 1] == '%') - trailing = 1; - else - trailing = 0; - if (anchor == 1 && trailing == 0) // No wildcard - return strcasecmp(pattern, str); - else if (anchor == 1) - return strncasecmp(str, pattern, len - trailing); - else - { - char *portion = malloc(len + 1); - int rval; - strncpy(portion, fixed, len - trailing); - portion[len - trailing] = 0; - rval = (strcasestr(str, portion) != NULL ? 0 : 1); - free(portion); - return rval; - } + if (*pattern != '%') + { + fixed = pattern; + anchor = 1; + } + else + { + fixed = &pattern[1]; + } + len = strlen(fixed); + if (fixed[len - 1] == '%') + { + trailing = 1; + } + else + { + trailing = 0; + } + if (anchor == 1 && trailing == 0) // No wildcard + { + return strcasecmp(pattern, str); + } + else if (anchor == 1) + { + return strncasecmp(str, pattern, len - trailing); + } + else + { + char *portion = malloc(len + 1); + int rval; + strncpy(portion, fixed, len - trailing); + portion[len - trailing] = 0; + rval = (strcasestr(str, portion) != NULL ? 0 : 1); + free(portion); + return rval; + } } /** @@ -1229,7 +1275,8 @@ extern char *strcasestr(); */ void maxinfo_send_ok(DCB *dcb) { - static const char ok_packet[] ={ + static const char ok_packet[] = + { 0x07, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, From de4ea067cffbce9c93bf4ba84d2f78af64821be3 Mon Sep 17 00:00:00 2001 From: ekorh475 Date: Fri, 18 Nov 2016 11:30:02 +0200 Subject: [PATCH 15/29] Fix for MXS-968 This commit adds a free() to null_auth_free_client_data, which plugs the memory leak in maxinfo. Also, this commit fixes some segfaults when multiple threads are running status_row() or variable_row(). The functions use statically allocated index variables, which often go out-of-bounds in concurrent use. This fix changes the indexes to thread-specific variables, with allocating and deallocating. This does seem to slow the functions down somewhat. --- server/modules/authenticator/null_auth.c | 6 ++- server/modules/routing/maxinfo/maxinfo_exec.c | 49 ++++++++++++------- 2 files changed, 37 insertions(+), 18 deletions(-) diff --git a/server/modules/authenticator/null_auth.c b/server/modules/authenticator/null_auth.c index 6819e1d06..c4ca31f08 100644 --- a/server/modules/authenticator/null_auth.c +++ b/server/modules/authenticator/null_auth.c @@ -150,4 +150,8 @@ null_auth_is_client_ssl_capable(DCB *dcb) * @param dcb Request handler DCB connected to the client */ static void -null_auth_free_client_data(DCB *dcb) {} +null_auth_free_client_data(DCB *dcb) +{ + free(dcb->data); + dcb->data = NULL; +} diff --git a/server/modules/routing/maxinfo/maxinfo_exec.c b/server/modules/routing/maxinfo/maxinfo_exec.c index bbe8a782e..884d65e42 100644 --- a/server/modules/routing/maxinfo/maxinfo_exec.c +++ b/server/modules/routing/maxinfo/maxinfo_exec.c @@ -825,7 +825,6 @@ typedef struct int index; char *like; } VARCONTEXT; - /** * Callback function to populate rows of the show variable * command @@ -836,9 +835,9 @@ typedef struct static RESULT_ROW * variable_row(RESULTSET *result, void *data) { - VARCONTEXT *context = (VARCONTEXT *)data; - RESULT_ROW *row; - char buf[80]; + VARCONTEXT *context = (VARCONTEXT *) data; + RESULT_ROW *row; + char buf[80]; if (variables[context->index].name) { @@ -862,10 +861,14 @@ variable_row(RESULTSET *result, void *data) (long)(*variables[context->index].func)()); resultset_row_set(row, 1, buf); break; + default: + ss_dassert(!true); } context->index++; return row; } + // We only get to this point once all variables have been printed + free(data); return NULL; } @@ -910,16 +913,20 @@ exec_show_variables(DCB *dcb, MAXINFO_TREE *filter) RESULTSET * maxinfo_variables() { - RESULTSET *result; - static VARCONTEXT context; - - context.like = NULL; - context.index = 0; - - if ((result = resultset_create(variable_row, &context)) == NULL) + RESULTSET *result; + VARCONTEXT *context; + if ((context = malloc(sizeof(VARCONTEXT))) == NULL) { return NULL; } + context->like = NULL; + context->index = 0; + + if ((result = resultset_create(variable_row, context)) == NULL) + { + free(context); + return NULL; + } resultset_add_column(result, "Variable_name", 40, COL_TYPE_VARCHAR); resultset_add_column(result, "Value", 40, COL_TYPE_VARCHAR); return result; @@ -1140,10 +1147,14 @@ status_row(RESULTSET *result, void *data) (long)(*status[context->index].func)()); resultset_row_set(row, 1, buf); break; + default: + ss_dassert(!true); } context->index++; return row; } + // We only get to this point once all status elements have been printed + free(data); return NULL; } @@ -1189,15 +1200,19 @@ RESULTSET * maxinfo_status() { RESULTSET *result; - static VARCONTEXT context; - - context.like = NULL; - context.index = 0; - - if ((result = resultset_create(status_row, &context)) == NULL) + VARCONTEXT *context; + if ((context = malloc(sizeof(VARCONTEXT))) == NULL) { return NULL; } + context->like = NULL; + context->index = 0; + + if ((result = resultset_create(status_row, context)) == NULL) + { + free(context); + return NULL; + } resultset_add_column(result, "Variable_name", 40, COL_TYPE_VARCHAR); resultset_add_column(result, "Value", 40, COL_TYPE_VARCHAR); return result; From 0aee4ac02097248780a4185b3e38fbf856fe3447 Mon Sep 17 00:00:00 2001 From: Markus Makela Date: Fri, 18 Nov 2016 15:41:07 +0200 Subject: [PATCH 16/29] Differentiate active and inactive servers in services The code that checked whether a server was added to a service did not check whether the server reference was active. This caused problems when an old server was added again to a service that once had used it. --- server/core/service.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/server/core/service.c b/server/core/service.c index 9797bdaf8..9380d7c44 100644 --- a/server/core/service.c +++ b/server/core/service.c @@ -832,7 +832,7 @@ void serviceRemoveBackend(SERVICE *service, const SERVER *server) for (SERVER_REF *ref = service->dbref; ref; ref = ref->next) { - if (ref->server == server) + if (ref->server == server && ref->active) { ref->active = false; service->n_dbref--; @@ -856,8 +856,12 @@ serviceHasBackend(SERVICE *service, SERVER *server) spinlock_acquire(&service->spin); ptr = service->dbref; - while (ptr && ptr->server != server) + while (ptr) { + if (ptr->server == server && ptr->active) + { + break; + } ptr = ptr->next; } spinlock_release(&service->spin); From 7ef10860406242c6e4d266c31cc1bffe83ea4710 Mon Sep 17 00:00:00 2001 From: Markus Makela Date: Fri, 18 Nov 2016 16:29:18 +0200 Subject: [PATCH 17/29] Add case label block indentation to astylerc Blocks after case labels are now indented. --- astylerc | 1 + 1 file changed, 1 insertion(+) diff --git a/astylerc b/astylerc index 90562db93..c3c67f2e6 100644 --- a/astylerc +++ b/astylerc @@ -1,6 +1,7 @@ --style=allman --indent=spaces=4 --indent-switches +--indent-cases --indent-labels --min-conditional-indent=0 --pad-oper From 6a68338c7274e4d754df39b531acc7346ec1f530 Mon Sep 17 00:00:00 2001 From: Johan Wikman Date: Mon, 21 Nov 2016 09:29:14 +0200 Subject: [PATCH 18/29] Update version to 2.0.2 --- VERSION.cmake | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/VERSION.cmake b/VERSION.cmake index 0693bfd8b..cb7413725 100644 --- a/VERSION.cmake +++ b/VERSION.cmake @@ -5,7 +5,7 @@ set(MAXSCALE_VERSION_MAJOR "2" CACHE STRING "Major version") set(MAXSCALE_VERSION_MINOR "0" CACHE STRING "Minor version") -set(MAXSCALE_VERSION_PATCH "1" CACHE STRING "Patch version") +set(MAXSCALE_VERSION_PATCH "2" CACHE STRING "Patch version") # This should only be incremented if a package is rebuilt set(MAXSCALE_BUILD_NUMBER 1 CACHE STRING "Release number") From ef4fe8785f1144946e73f175c7293602a59dae02 Mon Sep 17 00:00:00 2001 From: Johan Wikman Date: Mon, 21 Nov 2016 10:01:42 +0200 Subject: [PATCH 19/29] Update ChangeLog --- Documentation/Changelog.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Documentation/Changelog.md b/Documentation/Changelog.md index 264a2038a..e7818829e 100644 --- a/Documentation/Changelog.md +++ b/Documentation/Changelog.md @@ -11,7 +11,9 @@ as JSON objects (beta level functionality). For more details, please refer to: +* [MariaDB MaxScale 2.0.2 Release Notes](Release-Notes/MaxScale-2.0.2-Release-Notes.md) * [MariaDB MaxScale 2.0.1 Release Notes](Release-Notes/MaxScale-2.0.1-Release-Notes.md) +* [MariaDB MaxScale 2.0.0 Release Notes](Release-Notes/MaxScale-2.0.0-Release-Notes.md) ## MariaDB MaxScale 1.4 * Authentication now allows table level resolution of grants. MaxScale service From 3b30d5c8100dbfc75fdbb83b1272102c7d99f75a Mon Sep 17 00:00:00 2001 From: Johan Wikman Date: Mon, 21 Nov 2016 09:55:24 +0200 Subject: [PATCH 20/29] Update 2.0.2 release notes --- .../MaxScale-2.0.2-Release-Notes.md | 62 +++++++++++++++++++ 1 file changed, 62 insertions(+) create mode 100644 Documentation/Release-Notes/MaxScale-2.0.2-Release-Notes.md diff --git a/Documentation/Release-Notes/MaxScale-2.0.2-Release-Notes.md b/Documentation/Release-Notes/MaxScale-2.0.2-Release-Notes.md new file mode 100644 index 000000000..e50c25a00 --- /dev/null +++ b/Documentation/Release-Notes/MaxScale-2.0.2-Release-Notes.md @@ -0,0 +1,62 @@ +# MariaDB MaxScale 2.0.2 Release Notes + +Release 2.0.2 is a GA release. + +This document describes the changes in release 2.0.2, when compared to +release [2.0.1](MaxScale-2.0.1-Release-Notes.md). + +If you are upgrading from release 1.4.4, please also read the release +notes of release [2.0.0](./MaxScale-2.0.0-Release-Notes.md) and +release [2.0.1](./MaxScale-2.0.1-Release-Notes.md). + +For any problems you encounter, please submit a bug report at +[Jira](https://jira.mariadb.org). + +## Updated Features + +### [MXS-978] (https://jira.mariadb.org/browse/MXS-978) Support for stale master in case of restart + +In case where replication monitor gets a stale master status (slaves are down) +and maxscale gets restarted, master loses the stale master status and no writes +can happen. + +To cater for this situation there is now a `set server stale` command. + +## Bug fixes + +[Here is a list of bugs fixed since the release of MaxScale 2.0.1.](https://jira.mariadb.org/browse/MXS-976?jql=project%20%3D%20MXS%20AND%20issuetype%20%3D%20Bug%20AND%20status%20%3D%20Closed%20AND%20fixVersion%20%3D%202.0.2) + +* [MXS-1018](https://jira.mariadb.org/browse/MXS-1018): Internal connections don't use TLS +* [MXS-976](https://jira.mariadb.org/browse/MXS-976): Crash in libqc_sqlite +* [MXS-975](https://jira.mariadb.org/browse/MXS-975): TCP backlog is capped at 1280 +* [MXS-970](https://jira.mariadb.org/browse/MXS-970): A fatal problem with maxscale automatically shut down +* [MXS-969](https://jira.mariadb.org/browse/MXS-969): use_sql_variables_in=master can break functionality of important session variables +* [MXS-967](https://jira.mariadb.org/browse/MXS-967): setting connection_timeout=value cause error : Not a boolean value +* [MXS-965](https://jira.mariadb.org/browse/MXS-965): galeramon erlaubt keine TLS verschlüsselte Verbindung +* [MXS-960](https://jira.mariadb.org/browse/MXS-960): MaxScale Binlog Server does not allow comma to be in password +* [MXS-957](https://jira.mariadb.org/browse/MXS-957): Temporary table creation from another temporary table isn't detected +* [MXS-955](https://jira.mariadb.org/browse/MXS-955): MaxScale 2.0.1 doesn't recognize user and passwd options in .maxadmin file +* [MXS-953](https://jira.mariadb.org/browse/MXS-953): Charset error when server configued in utf8mb4 +* [MXS-942](https://jira.mariadb.org/browse/MXS-942): describe table query not routed to shard that contains the schema +* [MXS-917](https://jira.mariadb.org/browse/MXS-917): False error message about master not being in use + +## Known Issues and Limitations + +There are some limitations and known issues within this version of MaxScale. +For more information, please refer to the [Limitations](../About/Limitations.md) document. + +## Packaging + +RPM and Debian packages are provided for the Linux distributions supported +by MariaDB Enterprise. + +Packages can be downloaded [here](https://mariadb.com/resources/downloads). + +## Source Code + +The source code of MaxScale is tagged at GitHub with a tag, which is derived +from the version of MaxScale. For instance, the tag of version `X.Y.Z` of MaxScale +is `maxscale-X.Y.Z`. + +The source code is available [here](https://github.com/mariadb-corporation/MaxScale). + From 97b039689f34072affa87de69da5af6f7487ff3e Mon Sep 17 00:00:00 2001 From: Johan Wikman Date: Mon, 21 Nov 2016 21:16:27 +0200 Subject: [PATCH 21/29] Disable write-ahead log Since it's a cache, we do not need to retain the data from one MaxScale invocation to the next. Consequently, we can turn off write ahead logging completely if we simply wipe the RocksDB database at each startup. In addition, the location of the cache directory can now be specified explicitly, so it can be placed, for instance, on a RAM disk. --- Documentation/Filters/Cache.md | 24 +- server/modules/filter/cache/cache.c | 58 +++- server/modules/filter/cache/cache.h | 1 + .../storage/storage_rocksdb/rocksdbstorage.cc | 295 ++++++++++++------ .../storage/storage_rocksdb/rocksdbstorage.h | 11 + 5 files changed, 280 insertions(+), 109 deletions(-) diff --git a/Documentation/Filters/Cache.md b/Documentation/Filters/Cache.md index 8679f58c3..058bf7e22 100644 --- a/Documentation/Filters/Cache.md +++ b/Documentation/Filters/Cache.md @@ -381,4 +381,26 @@ regardless of what host the `admin` user comes from. # Storage -## Storage RocksDB +## `storage_rocksdb` + +This storage module uses RocksDB database for storing the cached data. The +directory where the RocksDB database will be created is by default created +into the _MaxScale cache_ directory, which usually is not on a RAM disk. For +maximum performance, you may want to explicitly place the RocksDB database +on a RAM disk. + +### Parameters + +#### `cache_directory` + +Specifies the directory under which the filter instance specific RocksDB +databases will be placed. Note that at startup, each RocksDB database will +be deleted and recreated. That is, cache content will not be retained across +MaxScale restarts. + +``` +storage_options=cache_directory=/mnt/maxscale-cache +``` + +With the above setting a directory `/mnt/macscale-cache/storage_rocksdb` will +created, under which the actual instance specific cache directories are created. diff --git a/server/modules/filter/cache/cache.c b/server/modules/filter/cache/cache.c index b7519fc3f..302b40a5e 100644 --- a/server/modules/filter/cache/cache.c +++ b/server/modules/filter/cache/cache.c @@ -12,6 +12,7 @@ */ #define MXS_MODULE_NAME "cache" +#include "cache.h" #include #include #include @@ -20,7 +21,6 @@ #include #include #include -#include "cache.h" #include "rules.h" #include "storage.h" @@ -95,9 +95,11 @@ typedef struct cache_config { uint32_t max_resultset_rows; uint32_t max_resultset_size; - const char* rules; + const char *rules; const char *storage; - const char *storage_options; + char *storage_options; + char **storage_argv; + int storage_argc; uint32_t ttl; uint32_t debug; } CACHE_CONFIG; @@ -109,6 +111,8 @@ static const CACHE_CONFIG DEFAULT_CONFIG = NULL, NULL, NULL, + NULL, + 0, CACHE_DEFAULT_TTL, CACHE_DEFAULT_DEBUG }; @@ -214,7 +218,11 @@ static FILTER *createInstance(const char *name, char **options, FILTER_PARAMETER if (module) { - CACHE_STORAGE *storage = module->api->createInstance(name, config.ttl, 0, NULL); + uint32_t ttl = config.ttl; + int argc = config.storage_argc; + char** argv = config.storage_argv; + + CACHE_STORAGE *storage = module->api->createInstance(name, ttl, argc, argv); if (storage) { @@ -976,7 +984,47 @@ static bool process_params(char **options, FILTER_PARAMETER **params, CACHE_CONF } else if (strcmp(param->name, "storage_options") == 0) { - config->storage_options = param->value; + config->storage_options = MXS_STRDUP(param->value); + + if (config->storage_options) + { + int argc = 1; + char *arg = config->storage_options; + + while ((arg = strchr(config->storage_options, ','))) + { + ++argc; + } + + config->storage_argv = (char**) MXS_MALLOC((argc + 1) * sizeof(char*)); + + if (config->storage_argv) + { + config->storage_argc = argc; + + int i = 0; + arg = config->storage_options; + config->storage_argv[i++] = arg; + + while ((arg = strchr(config->storage_options, ','))) + { + *arg = 0; + ++arg; + config->storage_argv[i++] = arg; + } + + config->storage_argv[i] = NULL; + } + else + { + MXS_FREE(config->storage_options); + config->storage_options = NULL; + } + } + else + { + error = true; + } } else if (strcmp(param->name, "storage") == 0) { diff --git a/server/modules/filter/cache/cache.h b/server/modules/filter/cache/cache.h index 93c1261e3..5b725f147 100644 --- a/server/modules/filter/cache/cache.h +++ b/server/modules/filter/cache/cache.h @@ -14,6 +14,7 @@ * Public License. */ +#include #include MXS_BEGIN_DECLS diff --git a/server/modules/filter/cache/storage/storage_rocksdb/rocksdbstorage.cc b/server/modules/filter/cache/storage/storage_rocksdb/rocksdbstorage.cc index a57675594..966bfdf8b 100644 --- a/server/modules/filter/cache/storage/storage_rocksdb/rocksdbstorage.cc +++ b/server/modules/filter/cache/storage/storage_rocksdb/rocksdbstorage.cc @@ -15,6 +15,7 @@ #include #include #include +#include #include #include #include @@ -37,8 +38,6 @@ using std::unique_ptr; namespace { -string u_storageDirectory; - const size_t ROCKSDB_KEY_LENGTH = 2 * SHA512_DIGEST_LENGTH; #if ROCKSDB_KEY_LENGTH > CACHE_KEY_MAXLEN @@ -85,8 +84,116 @@ string toString(const StorageRocksDBVersion& version) const char STORAGE_ROCKSDB_VERSION_KEY[] = "MaxScale_Storage_RocksDB_Version"; +/** + * Deletes a path, irrespective of whether it represents a file, a directory + * or a directory hierarchy. If the path does not exist, then the path is + * considered to have been removed. + * + * @param path A path (file or directory). + * + * @return True if the path could be deleted, false otherwise. + */ +bool deletePath(const string& path) +{ + int rv = false; + + struct stat st; + + if (stat(path.c_str(), &st) == -1) + { + if (errno == ENOENT) + { + rv = true; + } + else + { + char errbuf[MXS_STRERROR_BUFLEN]; + MXS_ERROR("Could not stat: %s", strerror_r(errno, errbuf, sizeof(errbuf))); + } + } + else + { + MXS_NOTICE("Deleting cache storage at '%s'.", path.c_str()); + + rv = true; + + char* files[] = { (char *) path.c_str(), NULL }; + + // FTS_NOCHDIR - Do not change CWD while traversing. + // FTS_PHYSICAL - Don't follow symlinks. + // FTS_XDEV - Don't cross filesystem boundaries + FTS *pFts = fts_open(files, FTS_NOCHDIR | FTS_PHYSICAL | FTS_XDEV, NULL); + + if (pFts) { + FTSENT* pCurrent; + while ((pCurrent = fts_read(pFts))) + { + switch (pCurrent->fts_info) + { + case FTS_NS: + case FTS_DNR: + case FTS_ERR: + { + char errbuf[MXS_STRERROR_BUFLEN]; + MXS_ERROR("Error while traversing %s: %s", + pCurrent->fts_accpath, + strerror_r(pCurrent->fts_errno, errbuf, sizeof(errbuf))); + rv = false; + } + break; + + case FTS_DC: + case FTS_DOT: + case FTS_NSOK: + // Not reached unless FTS_LOGICAL, FTS_SEEDOT, or FTS_NOSTAT were + // passed to fts_open() + break; + + case FTS_D: + // Do nothing. Need depth-first search, so directories are deleted + // in FTS_DP + break; + + case FTS_DP: + case FTS_F: + case FTS_SL: + case FTS_SLNONE: + case FTS_DEFAULT: + if (remove(pCurrent->fts_accpath) < 0) + { + char errbuf[MXS_STRERROR_BUFLEN]; + MXS_ERROR("Could not remove '%s', the cache directory may need to " + "be deleted manually: %s", + pCurrent->fts_accpath, + strerror_r(errno, errbuf, sizeof(errbuf))); + rv = false; + } + break; + + default: + ss_dassert(!true); + } + } + + if (rv) + { + MXS_NOTICE("Deleted cache storage at '%s'.", path.c_str()); + } + + if (pFts) { + fts_close(pFts); + } + } + } + + return rv; } +} + +//private +rocksdb::WriteOptions RocksDBStorage::s_writeOptions; + //private RocksDBStorage::RocksDBStorage(unique_ptr& sDb, const string& name, @@ -106,69 +213,98 @@ RocksDBStorage::~RocksDBStorage() //static bool RocksDBStorage::Initialize() { - bool initialized = true; + auto pEnv = rocksdb::Env::Default(); + pEnv->SetBackgroundThreads(ROCKSDB_N_LOW_THREADS, rocksdb::Env::LOW); + pEnv->SetBackgroundThreads(ROCKSDB_N_HIGH_THREADS, rocksdb::Env::HIGH); - u_storageDirectory = get_cachedir(); - u_storageDirectory += "/storage_rocksdb"; + // No logging; the database will always be deleted at startup, so there's + // no reason for usinf space and processing for writing the write ahead log. + s_writeOptions.disableWAL = true; - if (mkdir(u_storageDirectory.c_str(), S_IRWXU) == 0) - { - MXS_NOTICE("Created storage directory %s.", u_storageDirectory.c_str()); - } - else if (errno != EEXIST) - { - initialized = false; - char errbuf[MXS_STRERROR_BUFLEN]; - - MXS_ERROR("Failed to create storage directory %s: %s", - u_storageDirectory.c_str(), - strerror_r(errno, errbuf, sizeof(errbuf))); - } - else - { - auto pEnv = rocksdb::Env::Default(); - pEnv->SetBackgroundThreads(ROCKSDB_N_LOW_THREADS, rocksdb::Env::LOW); - pEnv->SetBackgroundThreads(ROCKSDB_N_HIGH_THREADS, rocksdb::Env::HIGH); - } - - return initialized; + return true; } - //static RocksDBStorage* RocksDBStorage::Create(const char* zName, uint32_t ttl, int argc, char* argv[]) { ss_dassert(zName); - string path(u_storageDirectory); + string storageDirectory = get_cachedir(); - path += "/"; - path += zName; - - rocksdb::Options options; - options.env = rocksdb::Env::Default(); - options.max_background_compactions = ROCKSDB_N_LOW_THREADS; - options.max_background_flushes = ROCKSDB_N_HIGH_THREADS; - - rocksdb::DBWithTTL* pDb; - rocksdb::Status status; - rocksdb::Slice key(STORAGE_ROCKSDB_VERSION_KEY); - - do + for (int i = 0; i < argc; ++i) { - // Try to open existing. - options.create_if_missing = false; - options.error_if_exists = false; + size_t len = strlen(argv[i]); + char arg[len + 1]; + strcpy(arg, argv[i]); - status = rocksdb::DBWithTTL::Open(options, path, &pDb, ttl); + const char* zValue = NULL; + char *zEq = strchr(arg, '='); - if (status.IsInvalidArgument()) // Did not exist + if (zEq) { - MXS_NOTICE("Database \"%s\" does not exist, creating.", path.c_str()); + *zEq = 0; + zValue = trim(zEq + 1); + } + + const char* zKey = trim(arg); + + if (strcmp(zKey, "cache_directory") == 0) + { + if (zValue) + { + storageDirectory = zValue; + } + else + { + MXS_WARNING("No value specified for '%s', using default '%s' instead.", + zKey, get_cachedir()); + } + } + else + { + MXS_WARNING("Unknown argument '%s'.", zKey); + } + } + + storageDirectory += "/storage_rocksdb"; + + return Create(storageDirectory, zName, ttl); +} + +// static +RocksDBStorage* RocksDBStorage::Create(const string& storageDirectory, const char* zName, uint32_t ttl) +{ + RocksDBStorage* pStorage = nullptr; + + if (mkdir(storageDirectory.c_str(), S_IRWXU) == 0) + { + MXS_NOTICE("Created storage directory %s.", storageDirectory.c_str()); + } + else if (errno != EEXIST) + { + char errbuf[MXS_STRERROR_BUFLEN]; + MXS_ERROR("Failed to create storage directory %s: %s", + storageDirectory.c_str(), + strerror_r(errno, errbuf, sizeof(errbuf))); + } + else + { + string path(storageDirectory + "/" + zName); + + if (deletePath(path)) + { + rocksdb::Options options; + options.env = rocksdb::Env::Default(); + options.max_background_compactions = ROCKSDB_N_LOW_THREADS; + options.max_background_flushes = ROCKSDB_N_HIGH_THREADS; options.create_if_missing = true; options.error_if_exists = true; + rocksdb::DBWithTTL* pDb; + rocksdb::Status status; + rocksdb::Slice key(STORAGE_ROCKSDB_VERSION_KEY); + status = rocksdb::DBWithTTL::Open(options, path, &pDb, ttl); if (status.ok()) @@ -179,7 +315,7 @@ RocksDBStorage* RocksDBStorage::Create(const char* zName, uint32_t ttl, int argc rocksdb::Slice value(reinterpret_cast(&STORAGE_ROCKSDB_VERSION), sizeof(STORAGE_ROCKSDB_VERSION)); - status = pDb->Put(rocksdb::WriteOptions(), key, value); + status = pDb->Put(writeOptions(), key, value); if (!status.ok()) { @@ -188,36 +324,6 @@ RocksDBStorage* RocksDBStorage::Create(const char* zName, uint32_t ttl, int argc path.c_str(), status.ToString().c_str()); } - } - } - } - while (status.IsInvalidArgument()); - - RocksDBStorage* pStorage = nullptr; - - if (status.ok()) - { - std::string value; - - status = pDb->Get(rocksdb::ReadOptions(), key, &value); - - if (status.ok()) - { - const StorageRocksDBVersion* pVersion = - reinterpret_cast(value.data()); - - // When the version is bumped, it needs to be decided what if any - // backward compatibility is provided. After all, it's a cache, so - // you should be able to delete it at any point and pay a small - // price while the cache is rebuilt. - if ((pVersion->major == STORAGE_ROCKSDB_MAJOR) && - (pVersion->minor == STORAGE_ROCKSDB_MINOR) && - (pVersion->correction == STORAGE_ROCKSDB_CORRECTION)) - { - MXS_NOTICE("Version of \"%s\" is %s, version of storage_rocksdb is %s.", - path.c_str(), - toString(*pVersion).c_str(), - toString(STORAGE_ROCKSDB_VERSION).c_str()); unique_ptr sDb(pDb); @@ -225,31 +331,14 @@ RocksDBStorage* RocksDBStorage::Create(const char* zName, uint32_t ttl, int argc } else { - MXS_ERROR("Version of RocksDB database \"%s\" is %s, while version required " - "is %s. You need to delete the database and restart.", - path.c_str(), - toString(*pVersion).c_str(), - toString(STORAGE_ROCKSDB_VERSION).c_str()); - delete pDb; - } - } - else - { - MXS_ERROR("Could not read version information from RocksDB database %s. " - "You may need to delete the database and retry. RocksDB error: \"%s\"", - path.c_str(), - status.ToString().c_str()); - delete pDb; - } - } - else - { - MXS_ERROR("Could not open/initialize RocksDB database %s. RocksDB error: \"%s\"", - path.c_str(), status.ToString().c_str()); + MXS_ERROR("Could not create RocksDB database %s. RocksDB error: \"%s\"", + path.c_str(), status.ToString().c_str()); - if (status.IsIOError()) - { - MXS_ERROR("Is an other MaxScale process running?"); + if (status.IsIOError()) + { + MXS_ERROR("Is an other MaxScale process running?"); + } + } } } @@ -374,7 +463,7 @@ cache_result_t RocksDBStorage::putValue(const char* pKey, const GWBUF* pValue) rocksdb::Slice key(pKey, ROCKSDB_KEY_LENGTH); rocksdb::Slice value(static_cast(GWBUF_DATA(pValue)), GWBUF_LENGTH(pValue)); - rocksdb::Status status = m_sDb->Put(rocksdb::WriteOptions(), key, value); + rocksdb::Status status = m_sDb->Put(writeOptions(), key, value); return status.ok() ? CACHE_RESULT_OK : CACHE_RESULT_ERROR; } diff --git a/server/modules/filter/cache/storage/storage_rocksdb/rocksdbstorage.h b/server/modules/filter/cache/storage/storage_rocksdb/rocksdbstorage.h index 0967fb86b..a3bf85ba7 100644 --- a/server/modules/filter/cache/storage/storage_rocksdb/rocksdbstorage.h +++ b/server/modules/filter/cache/storage/storage_rocksdb/rocksdbstorage.h @@ -42,11 +42,22 @@ private: RocksDBStorage(const RocksDBStorage&) = delete; RocksDBStorage& operator = (const RocksDBStorage&) = delete; + static RocksDBStorage* Create(const std::string& storageDirectory, + const char* zName, + uint32_t ttl); + + static const rocksdb::WriteOptions& writeOptions() + { + return s_writeOptions; + } + private: std::unique_ptr m_sDb; std::string m_name; std::string m_path; uint32_t m_ttl; + + static rocksdb::WriteOptions s_writeOptions; }; #endif From 4dbb68cbfee49ec4022a6464adbd8ee72b505520 Mon Sep 17 00:00:00 2001 From: Johan Wikman Date: Thu, 17 Nov 2016 16:44:54 +0200 Subject: [PATCH 22/29] Compile gateway.c as C++ When C and C++ are mixed in a project, main() should be compiled as C++ to ensure that all C++ static initializations are performed properly. That may not be strictly true anymore, depending on the used compiler and environment, but better to do that to be on the safe side. --- server/core/CMakeLists.txt | 2 +- server/core/{gateway.c => gateway.cc} | 116 ++++++++++++++------------ 2 files changed, 63 insertions(+), 55 deletions(-) rename server/core/{gateway.c => gateway.cc} (95%) diff --git a/server/core/CMakeLists.txt b/server/core/CMakeLists.txt index 6131c33b6..4e6207424 100644 --- a/server/core/CMakeLists.txt +++ b/server/core/CMakeLists.txt @@ -12,7 +12,7 @@ add_dependencies(maxscale-common pcre2 connector-c) set_target_properties(maxscale-common PROPERTIES VERSION "1.0.0") install_module(maxscale-common core) -add_executable(maxscale gateway.c) +add_executable(maxscale gateway.cc) add_dependencies(maxscale pcre2) if(WITH_JEMALLOC) diff --git a/server/core/gateway.c b/server/core/gateway.cc similarity index 95% rename from server/core/gateway.c rename to server/core/gateway.cc index 10bacd28d..2293e6917 100644 --- a/server/core/gateway.c +++ b/server/core/gateway.cc @@ -159,7 +159,7 @@ static void write_footer(void); static int ntfw_cb(const char*, const struct stat*, int, struct FTW*); static bool file_is_readable(const char* absolute_pathname); static bool file_is_writable(const char* absolute_pathname); -bool handle_path_arg(char** dest, const char* path, char* arg, bool rd, bool wr); +bool handle_path_arg(char** dest, const char* path, const char* arg, bool rd, bool wr); static void set_log_augmentation(const char* value); static void usage(void); static char* get_expanded_pathname( @@ -218,7 +218,8 @@ struct CRYPTO_dynlock_value */ static struct CRYPTO_dynlock_value *ssl_create_dynlock(const char* file, int line) { - struct CRYPTO_dynlock_value* lock = MXS_MALLOC(sizeof(struct CRYPTO_dynlock_value)); + struct CRYPTO_dynlock_value* lock = + (struct CRYPTO_dynlock_value*) MXS_MALLOC(sizeof(struct CRYPTO_dynlock_value)); if (lock) { spinlock_init(&lock->lock); @@ -1298,6 +1299,8 @@ int main(int argc, char **argv) bool config_check = false; bool to_stdout = false; void (*exitfunp[4])(void) = { mxs_log_finish, cleanup_process_datadir, write_footer, NULL }; + GATEWAY_CONF* cnf = NULL; + int numlocks = 0; *syslog_enabled = 1; *maxlog_enabled = 1; @@ -1321,7 +1324,7 @@ int main(int argc, char **argv) if (l != 0) { - char* fprerr = "Failed to register exit functions for MaxScale"; + const char* fprerr = "Failed to register exit functions for MaxScale"; print_log_n_stderr(false, true, NULL, fprerr, 0); rc = MAXSCALE_INTERNALERROR; goto return_main; @@ -1351,11 +1354,12 @@ int main(int argc, char **argv) } if (cnf_file_arg == NULL) { - char* logerr = "Configuration file argument " - "identifier \'-f\' was specified but " - "the argument didn't specify\n a valid " - "configuration file or the argument " - "was missing."; + const char* logerr = + "Configuration file argument " + "identifier \'-f\' was specified but " + "the argument didn't specify\n a valid " + "configuration file or the argument " + "was missing."; print_log_n_stderr(true, true, logerr, logerr, 0); usage(); succp = false; @@ -1391,11 +1395,12 @@ int main(int argc, char **argv) } else { - char* logerr = "Configuration file argument " - "identifier \'-l\' was specified but " - "the argument didn't specify\n a valid " - "configuration file or the argument " - "was missing."; + const char* logerr = + "Configuration file argument " + "identifier \'-l\' was specified but " + "the argument didn't specify\n a valid " + "configuration file or the argument " + "was missing."; print_log_n_stderr(true, true, logerr, logerr, 0); usage(); succp = false; @@ -1618,14 +1623,14 @@ int main(int argc, char **argv) if (nread == -1) { - char* logerr = "Failed to read data from child process pipe."; + const char* logerr = "Failed to read data from child process pipe."; print_log_n_stderr(true, true, logerr, logerr, errno); exit(MAXSCALE_INTERNALERROR); } else if (nread == 0) { /** Child process has exited or closed write pipe */ - char* logerr = "No data read from child process pipe."; + const char* logerr = "No data read from child process pipe."; print_log_n_stderr(true, true, logerr, logerr, 0); exit(MAXSCALE_INTERNALERROR); } @@ -1652,8 +1657,7 @@ int main(int argc, char **argv) if (eno != 0) { - char* logerr = "Failed to initialise signal mask for MaxScale. " - "Exiting."; + const char* logerr = "Failed to initialise signal mask for MaxScale. Exiting."; print_log_n_stderr(true, true, logerr, logerr, eno); rc = MAXSCALE_INTERNALERROR; goto return_main; @@ -1661,7 +1665,7 @@ int main(int argc, char **argv) if (!utils_init()) { - char* logerr = "Failed to initialise utility library."; + const char* logerr = "Failed to initialise utility library."; print_log_n_stderr(true, true, logerr, logerr, eno); rc = MAXSCALE_INTERNALERROR; goto return_main; @@ -1670,7 +1674,7 @@ int main(int argc, char **argv) /** OpenSSL initialization */ if (!HAVE_OPENSSL_THREADS) { - char* logerr = "OpenSSL library does not support multi-threading"; + const char* logerr = "OpenSSL library does not support multi-threading"; print_log_n_stderr(true, true, logerr, logerr, eno); rc = MAXSCALE_INTERNALERROR; goto return_main; @@ -1679,8 +1683,8 @@ int main(int argc, char **argv) SSL_load_error_strings(); OPENSSL_add_all_algorithms_noconf(); - int numlocks = CRYPTO_num_locks(); - if ((ssl_locks = MXS_MALLOC(sizeof(SPINLOCK) * (numlocks + 1))) == NULL) + numlocks = CRYPTO_num_locks(); + if ((ssl_locks = (SPINLOCK*)MXS_MALLOC(sizeof(SPINLOCK) * (numlocks + 1))) == NULL) { rc = MAXSCALE_INTERNALERROR; goto return_main; @@ -1705,10 +1709,12 @@ int main(int argc, char **argv) if (l != 0) { - char* fprerr = "Failed to register exit function for\n* " - "embedded MySQL library.\n* Exiting."; - char* logerr = "Failed to register exit function libmysql_done " - "for MaxScale. Exiting."; + const char* fprerr = + "Failed to register exit function for\n* " + "embedded MySQL library.\n* Exiting."; + const char* logerr = + "Failed to register exit function libmysql_done " + "for MaxScale. Exiting."; print_log_n_stderr(true, true, logerr, fprerr, 0); rc = MAXSCALE_INTERNALERROR; goto return_main; @@ -1837,7 +1843,7 @@ int main(int argc, char **argv) if (!config_load(cnf_file_path)) { - char* fprerr = + const char* fprerr = "Failed to open, read or process the MaxScale configuration " "file. Exiting. See the error log for details."; print_log_n_stderr(false, true, fprerr, fprerr, 0); @@ -1848,12 +1854,12 @@ int main(int argc, char **argv) goto return_main; } - GATEWAY_CONF* cnf = config_get_global_options(); + cnf = config_get_global_options(); ss_dassert(cnf); if (!qc_init(cnf->qc_name, cnf->qc_args)) { - char* logerr = "Failed to initialise query classifier library."; + const char* logerr = "Failed to initialise query classifier library."; print_log_n_stderr(true, true, logerr, logerr, eno); rc = MAXSCALE_INTERNALERROR; goto return_main; @@ -1870,8 +1876,7 @@ int main(int argc, char **argv) { if (!daemon_mode) { - char* fprerr = "Failed to initialise the MySQL library. " - "Exiting."; + const char* fprerr = "Failed to initialise the MySQL library. Exiting."; print_log_n_stderr(false, true, fprerr, fprerr, 0); if (mysql_errno(NULL) == 2000) @@ -1950,7 +1955,7 @@ int main(int argc, char **argv) if (n_services == 0) { - char* logerr = "Failed to start all MaxScale services. Exiting."; + const char* logerr = "Failed to start all MaxScale services. Exiting."; print_log_n_stderr(true, true, logerr, logerr, 0); rc = MAXSCALE_NOSERVICES; goto return_main; @@ -1962,7 +1967,7 @@ int main(int argc, char **argv) if (thread_start(&log_flush_thr, log_flush_cb, (void *) &log_flush_timeout_ms) == NULL) { - char* logerr = "Failed to start log flushing thread."; + const char* logerr = "Failed to start log flushing thread."; print_log_n_stderr(true, true, logerr, logerr, 0); rc = MAXSCALE_INTERNALERROR; goto return_main; @@ -1973,7 +1978,7 @@ int main(int argc, char **argv) */ if (!hkinit()) { - char* logerr = "Failed to start housekeeper thread."; + const char* logerr = "Failed to start housekeeper thread."; print_log_n_stderr(true, true, logerr, logerr, 0); rc = MAXSCALE_INTERNALERROR; goto return_main; @@ -1993,7 +1998,7 @@ int main(int argc, char **argv) if (thread_start(&threads[thread_id], worker_thread_main, (void *)(thread_id + 1)) == NULL) { - char* logerr = "Failed to start worker thread."; + const char* logerr = "Failed to start worker thread."; print_log_n_stderr(true, true, logerr, logerr, 0); rc = MAXSCALE_INTERNALERROR; goto return_main; @@ -2133,7 +2138,7 @@ static void unlock_pidfile() if (flock(pidfd, LOCK_UN | LOCK_NB) != 0) { char logbuf[STRING_BUFFER_SIZE + PATH_MAX]; - char* logerr = "Failed to unlock PID file '%s'."; + const char* logerr = "Failed to unlock PID file '%s'."; snprintf(logbuf, sizeof(logbuf), logerr, pidfile); print_log_n_stderr(true, true, logbuf, logbuf, errno); } @@ -2189,7 +2194,7 @@ bool pid_file_exists() if ((fd = open(pathbuf, O_RDWR)) == -1) { - char* logerr = "Failed to open PID file '%s'."; + const char* logerr = "Failed to open PID file '%s'."; snprintf(logbuf, sizeof(logbuf), logerr, pathbuf); print_log_n_stderr(true, true, logbuf, logbuf, errno); return true; @@ -2198,7 +2203,7 @@ bool pid_file_exists() { if (errno != EWOULDBLOCK) { - char* logerr = "Failed to lock PID file '%s'."; + const char* logerr = "Failed to lock PID file '%s'."; snprintf(logbuf, sizeof(logbuf), logerr, pathbuf); print_log_n_stderr(true, true, logbuf, logbuf, errno); close(fd); @@ -2212,7 +2217,7 @@ bool pid_file_exists() if (b == -1) { - char* logerr = "Failed to read from PID file '%s'."; + const char* logerr = "Failed to read from PID file '%s'."; snprintf(logbuf, sizeof(logbuf), logerr, pathbuf); print_log_n_stderr(true, true, logbuf, logbuf, errno); unlock_pidfile(); @@ -2221,24 +2226,26 @@ bool pid_file_exists() else if (b == 0) { /** Empty file */ - char* logerr = "PID file read from '%s'. File was empty.\n" - "If the file is the correct PID file and no other MaxScale processes " - "are running, please remove it manually and start MaxScale again."; + const char* logerr = + "PID file read from '%s'. File was empty.\n" + "If the file is the correct PID file and no other MaxScale processes " + "are running, please remove it manually and start MaxScale again."; snprintf(logbuf, sizeof(logbuf), logerr, pathbuf); print_log_n_stderr(true, true, logbuf, logbuf, errno); unlock_pidfile(); return true; } - pidbuf[b < sizeof(pidbuf) ? b : sizeof(pidbuf) - 1] = '\0'; + pidbuf[(size_t)b < sizeof(pidbuf) ? (size_t)b : sizeof(pidbuf) - 1] = '\0'; pid = strtol(pidbuf, NULL, 0); if (pid < 1) { /** Bad PID */ - char* logerr = "PID file read from '%s'. File contents not valid.\n" - "If the file is the correct PID file and no other MaxScale processes " - "are running, please remove it manually and start MaxScale again."; + const char* logerr = + "PID file read from '%s'. File contents not valid.\n" + "If the file is the correct PID file and no other MaxScale processes " + "are running, please remove it manually and start MaxScale again."; snprintf(logbuf, sizeof(logbuf), logerr, pathbuf); print_log_n_stderr(true, true, logbuf, logbuf, errno); unlock_pidfile(); @@ -2252,7 +2259,7 @@ bool pid_file_exists() /** no such process, old PID file */ if (lock_failed) { - char* logerr = + const char* logerr = "Locking the PID file '%s' failed. " "Read PID from file and no process found with PID %d. " "Confirm that no other process holds the lock on the PID file."; @@ -2264,7 +2271,7 @@ bool pid_file_exists() } else { - char* logerr = "Failed to check the existence of process %d read from file '%s'"; + const char* logerr = "Failed to check the existence of process %d read from file '%s'"; snprintf(logbuf, sizeof(logbuf), logerr, pid, pathbuf); print_log_n_stderr(true, true, logbuf, logbuf, errno); unlock_pidfile(); @@ -2272,7 +2279,7 @@ bool pid_file_exists() } else { - char* logerr = + const char* logerr = "MaxScale is already running. Process id: %d. " "Use another location for the PID file to run multiple " "instances of MaxScale on the same machine."; @@ -2283,8 +2290,9 @@ bool pid_file_exists() } else { - char* logerr = "Cannot open PID file '%s', no read permissions. " - "Please confirm that the user running MaxScale has read permissions on the file."; + const char* logerr = + "Cannot open PID file '%s', no read permissions. " + "Please confirm that the user running MaxScale has read permissions on the file."; snprintf(logbuf, sizeof(logbuf), logerr, pathbuf); print_log_n_stderr(true, true, logbuf, logbuf, errno); } @@ -2313,7 +2321,7 @@ static int write_pid_file() fd = open(pidfile, O_WRONLY | O_CREAT, 0777); if (fd == -1) { - char* logerr = "Failed to open PID file '%s'."; + const char* logerr = "Failed to open PID file '%s'."; snprintf(logbuf, sizeof(logbuf), logerr, pidfile); print_log_n_stderr(true, true, logbuf, logbuf, errno); return -1; @@ -2342,7 +2350,7 @@ static int write_pid_file() /* truncate pidfile content */ if (ftruncate(pidfd, 0)) { - char* logerr = "MaxScale failed to truncate PID file '%s'."; + const char* logerr = "MaxScale failed to truncate PID file '%s'."; snprintf(logbuf, sizeof(logbuf), logerr, pidfile); print_log_n_stderr(true, true, logbuf, logbuf, errno); unlock_pidfile(); @@ -2353,7 +2361,7 @@ static int write_pid_file() if (pwrite(pidfd, pidstr, strlen(pidstr), 0) != (ssize_t)strlen(pidstr)) { - char* logerr = "MaxScale failed to write into PID file '%s'."; + const char* logerr = "MaxScale failed to write into PID file '%s'."; snprintf(logbuf, sizeof(logbuf), logerr, pidfile); print_log_n_stderr(true, true, logbuf, logbuf, errno); unlock_pidfile(); @@ -2364,7 +2372,7 @@ static int write_pid_file() return 0; } -bool handle_path_arg(char** dest, const char* path, char* arg, bool rd, bool wr) +bool handle_path_arg(char** dest, const char* path, const char* arg, bool rd, bool wr) { char pathbuffer[PATH_MAX + 2]; char* errstr; From 0c128e65986b698ae4863006ae835b835f5d84d8 Mon Sep 17 00:00:00 2001 From: Markus Makela Date: Fri, 4 Nov 2016 15:19:22 +0200 Subject: [PATCH 23/29] Convert README into Markdown Converting the README into Markdown format makes it a lot easier to comprehent. Also cleaned up the formatting. --- README => README.md | 30 +++++++++++++++++------------- 1 file changed, 17 insertions(+), 13 deletions(-) rename README => README.md (57%) diff --git a/README b/README.md similarity index 57% rename from README rename to README.md index fb0611c14..40fadbd18 100644 --- a/README +++ b/README.md @@ -1,5 +1,7 @@ # MaxScale by MariaDB Corporation +[![Build Status](https://travis-ci.org/mariadb-corporation/MaxScale.svg?branch=develop)](https://travis-ci.org/mariadb-corporation/MaxScale) + The MariaDB Corporation MaxScale is an intelligent proxy that allows forwarding of database statements to one or more database servers using complex rules, a semantic understanding of the database statements and the @@ -26,23 +28,25 @@ as external shared objects and are referred to as routing modules. An Google Group exists for MaxScale that can be used to discuss ideas, issues and communicate with the MaxScale community. - Email: maxscale@googlegroups.com - Forum: http://groups.google.com/forum/#!forum/maxscale +- Email: maxscale@googlegroups.com +- Forum: http://groups.google.com/forum/#!forum/maxscale -Bugs can be reported in the MariaDB Corporation bugs database: -https://jira.mariadb.org/projects/MXS/issues +Please report all feature requests, improvements and bugs in the [MariaDB Jira](https://jira.mariadb.org/projects/MXS/issues). # Documentation For information about installing and using MaxScale, please refer to the -documentation. It is in Markdown format. +documentation. The official documentation can be found on the +[MariaDB Knowledge Base](https://mariadb.com/kb/en/mariadb-enterprise/maxscale/). -You can point your browser to the MaxScale project at GitHub. Look -inside the "Documentation" directory, where you will find a file named -Documentation-Contents.md. Click on that, and GitHub will show the -documentation in its intended display format. The contents page lists -the available documents and has links to them. +- [MariaDB MaxScale 2.0 Documentation](https://mariadb.com/kb/en/mariadb-enterprise/mariadb-maxscale-20-contents/) +- [MariaDB MaxScale 1.4 Documentation](https://mariadb.com/kb/en/mariadb-enterprise/mariadb-maxscale-14/maxscale-maxscale-contents/) -If you do not want to rely on the internet, then clone the project -from GitHub and point your browser to the Documentation-Contents.md -file in your local file system and proceed as above. +The module and configuration documentation can be found in the _Documentation_ +directory of the source tree. + +# Contributing Code + +Read the [Contributing](https://github.com/mariadb-corporation/MaxScale/wiki/Contributing) +page on the wiki for more information on how to do pull request and where to do +them. From bddcee6f7ed59bdeea8a41332519bb70fc81cb18 Mon Sep 17 00:00:00 2001 From: Johan Wikman Date: Tue, 22 Nov 2016 10:38:35 +0200 Subject: [PATCH 24/29] Add space between literal and string macro --- server/core/gateway.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/server/core/gateway.cc b/server/core/gateway.cc index 2293e6917..9d6a2e28d 100644 --- a/server/core/gateway.cc +++ b/server/core/gateway.cc @@ -384,9 +384,9 @@ sigfatal_handler(int i) } fatal_handling = 1; GATEWAY_CONF* cnf = config_get_global_options(); - fprintf(stderr, "\n\nMaxScale "MAXSCALE_VERSION" received fatal signal %d\n", i); + fprintf(stderr, "\n\nMaxScale " MAXSCALE_VERSION " received fatal signal %d\n", i); - MXS_ALERT("Fatal: MaxScale "MAXSCALE_VERSION" received fatal signal %d. Attempting backtrace.", i); + MXS_ALERT("Fatal: MaxScale " MAXSCALE_VERSION " received fatal signal %d. Attempting backtrace.", i); MXS_ALERT("Commit ID: %s System name: %s " "Release string: %s", From 4603e719873ccbb555f82a19fdacf6cc4a51bdab Mon Sep 17 00:00:00 2001 From: Markus Makela Date: Fri, 18 Nov 2016 14:15:59 +0200 Subject: [PATCH 25/29] MXS-929: Add domain function registration A module can register a function to a domain. These function can then be called by external actors enabling the modules to expand their functionality beyond the module API. Each module should use its own domain e.g the library name. Currently, the functions do not return results. The possible next step would be to alter the function entry point to return a result set of sorts. This would allow the modules to convey structured information to the client information which would handle the formatting of the result. Although this sounds good, it is not required for the implementation of MXS-929. --- include/maxscale/modulecmd.h | 171 ++++++++++++++ server/core/CMakeLists.txt | 2 +- server/core/modulecmd.c | 389 +++++++++++++++++++++++++++++++ server/core/test/CMakeLists.txt | 3 + server/core/test/testmodulecmd.c | 185 +++++++++++++++ 5 files changed, 749 insertions(+), 1 deletion(-) create mode 100644 include/maxscale/modulecmd.h create mode 100644 server/core/modulecmd.c create mode 100644 server/core/test/testmodulecmd.c diff --git a/include/maxscale/modulecmd.h b/include/maxscale/modulecmd.h new file mode 100644 index 000000000..7c1b0088c --- /dev/null +++ b/include/maxscale/modulecmd.h @@ -0,0 +1,171 @@ +#pragma once +/* + * Copyright (c) 2016 MariaDB Corporation Ab + * + * Use of this software is governed by the Business Source License included + * in the LICENSE.TXT file and at www.mariadb.com/bsl. + * + * Change Date: 2019-07-01 + * + * On the date above, in accordance with the Business Source License, use + * of this software will be governed by version 2 or later of the General + * Public License. + */ + +/** + * @file module_command.h Module driven commands + * + * This header describes the structures and functions used to register new + * functions for modules. It allows modules to introduce custom commands that + * are registered into a module specific domain. These commands can then be + * accessed from multiple different client interfaces without implementing the + * same functionality again. + */ + +#include +#include +#include +#include +#include +#include +#include + +MXS_BEGIN_DECLS + +/** + * The argument type + * + * First 8 bits are reserved for argument type, bits 9 through 32 are reserved + * for argument options and bits 33 through 64 are reserved for future use. + */ +typedef uint64_t modulecmd_arg_type_t; + +/** + * Argument types for the registered functions, the first 8 bits of + * the modulecmd_arg_type_t type. An argument can be of only one type. + */ +#define MODULECMD_ARG_NONE 0 +#define MODULECMD_ARG_STRING 1 +#define MODULECMD_ARG_BOOLEAN 2 +#define MODULECMD_ARG_SERVICE 3 +#define MODULECMD_ARG_SERVER 4 +#define MODULECMD_ARG_SESSION 5 +#define MODULECMD_ARG_DCB 6 +#define MODULECMD_ARG_MONITOR 7 +#define MODULECMD_ARG_FILTER 8 + +/** + * Options for arguments, bits 9 through 32 + */ +#define MODULECMD_ARG_OPTIONAL (1 << 8) /**< The argument is optional */ + +/** + * Helper macros + */ +#define MODULECMD_GET_TYPE(type) ((type & 0xff)) +#define MODULECMD_ARG_IS_REQUIRED(type) ((type & MODULECMD_ARG_OPTIONAL) == 0) + +/** Argument list node */ +struct arg_node +{ + modulecmd_arg_type_t type; + union + { + char *string; + bool boolean; + SERVICE *service; + SERVER *server; + SESSION *session; + DCB *dcb; + MONITOR *monitor; + FILTER_DEF *filter; + } value; +}; + +/** Argument list */ +typedef struct +{ + int argc; + struct arg_node *argv; +} MODULECMD_ARG; + +/** + * The function signature for the module commands. + * + * The number of arguments will always be the maximum number of arguments the + * module requested. If an argument had the MODULECMD_ARG_OPTIONAL flag, and + * the argument was not provided, the type of the argument will be + * MODULECMD_ARG_NONE. + * + * @param argv Argument list + * @return True on success, false on error + */ +typedef bool (*MODULECMDFN)(const MODULECMD_ARG *argv); + +/** + * A registered command + */ +typedef struct modulecmd +{ + char *identifier; /**< Unique identifier */ + char *domain; /**< Command domain */ + MODULECMDFN func; /**< The registered function */ + int arg_count; /**< Number of arguments */ + modulecmd_arg_type_t *arg_types; /**< Argument types */ + struct modulecmd *next; /**< Next command */ +} MODULECMD; + +/** + * @brief Register a new command + * + * This function registers a new command into the domain. + * + * @param domain Command domain + * @param identifier The unique identifier for this command + * @param entry_point The actual entry point function + * @param argc Maximum number of arguments + * @param argv Array of argument types of size @c argc + * @return True if the module was successfully registered, false on error + */ +bool modulecmd_register_command(const char *domain, const char *identifier, + MODULECMDFN entry_point, int argc, modulecmd_arg_type_t *argv); + +/** + * @brief Find a registered command + * + * @param domain Command domain + * @param identifier Command identifier + * @return Registered command or NULL if no command was found + */ +const MODULECMD* modulecmd_find_command(const char *domain, const char *identifier); + +/** + * @brief Parse arguments for a command + * + * @param cmd Command for which the parameters are parsed + * @param argc Number of arguments + * @param argv Argument list in string format of size @c argc + * @return Parsed arguments or NULL on error + */ +MODULECMD_ARG* modulecmd_arg_parse(const MODULECMD *cmd, int argc, const char **argv); + +/** + * @brief Free parsed arguments returned by modulecmd_arg_parse + * @param arg Arguments to free + */ +void modulecmd_arg_free(MODULECMD_ARG *arg); + +/** + * @brief Call a registered command + * + * This calls a registered command in a specific domain. There are no guarantees + * on the length of the call or whether it will block. All of this depends on the + * module and what the command does. + * + * @param cmd Command to call + * @param args Parsed command arguments + * @return True on success, false on error + */ +bool modulecmd_call_command(const MODULECMD *cmd, const MODULECMD_ARG *args); + +MXS_END_DECLS diff --git a/server/core/CMakeLists.txt b/server/core/CMakeLists.txt index 4e6207424..d232fe559 100644 --- a/server/core/CMakeLists.txt +++ b/server/core/CMakeLists.txt @@ -1,4 +1,4 @@ -add_library(maxscale-common SHARED adminusers.c alloc.c authenticator.c atomic.c buffer.c config.c dcb.c filter.c externcmd.c gwbitmask.c gwdirs.c hashtable.c hint.c housekeeper.c listmanager.c load_utils.c log_manager.cc maxscale_pcre2.c memlog.c misc.c mlist.c modutil.c monitor.c queuemanager.c query_classifier.c poll.c random_jkiss.c resultset.c secrets.c server.c service.c session.c spinlock.c thread.c users.c utils.c skygw_utils.cc statistics.c listener.c gw_ssl.c mysql_utils.c mysql_binlog.c) +add_library(maxscale-common SHARED adminusers.c alloc.c authenticator.c atomic.c buffer.c config.c dcb.c filter.c externcmd.c gwbitmask.c gwdirs.c hashtable.c hint.c housekeeper.c listmanager.c load_utils.c log_manager.cc maxscale_pcre2.c memlog.c misc.c mlist.c modutil.c monitor.c queuemanager.c query_classifier.c poll.c random_jkiss.c resultset.c secrets.c server.c service.c session.c spinlock.c thread.c users.c utils.c skygw_utils.cc statistics.c listener.c gw_ssl.c mysql_utils.c mysql_binlog.c modulecmd.c) target_link_libraries(maxscale-common ${MARIADB_CONNECTOR_LIBRARIES} ${LZMA_LINK_FLAGS} ${PCRE2_LIBRARIES} ${CURL_LIBRARIES} ssl pthread crypt dl crypto inih z rt m stdc++) diff --git a/server/core/modulecmd.c b/server/core/modulecmd.c new file mode 100644 index 000000000..cf5d4515e --- /dev/null +++ b/server/core/modulecmd.c @@ -0,0 +1,389 @@ +/* + * Copyright (c) 2016 MariaDB Corporation Ab + * + * Use of this software is governed by the Business Source License included + * in the LICENSE.TXT file and at www.mariadb.com/bsl. + * + * Change Date: 2019-07-01 + * + * On the date above, in accordance with the Business Source License, use + * of this software will be governed by version 2 or later of the General + * Public License. + */ + +#include +#include +#include +#include + +/** + * A registered domain + */ +typedef struct modulecmd_domain +{ + char *domain; /**< The domain */ + MODULECMD *commands; /**< List of registered commands */ + struct modulecmd_domain *next; /**< Next domain */ +} MODULECMD_DOMAIN; + +/** + * Internal functions + */ + +/** The global list of registered domains */ +static MODULECMD_DOMAIN *modulecmd_domains = NULL; +static SPINLOCK modulecmd_lock = SPINLOCK_INIT; + +static MODULECMD_DOMAIN* domain_create(const char *domain) +{ + MODULECMD_DOMAIN *rval = MXS_MALLOC(sizeof(*rval)); + char *dm = MXS_STRDUP(domain); + + if (rval && dm) + { + rval->domain = dm; + rval->commands = NULL; + rval->next = NULL; + } + else + { + MXS_FREE(rval); + MXS_FREE(dm); + rval = NULL; + } + + return rval; +} + +static void domain_free(MODULECMD_DOMAIN *dm) +{ + if (dm) + { + MXS_FREE(dm->domain); + MXS_FREE(dm); + } +} + +static MODULECMD_DOMAIN* get_or_create_domain(const char *domain) +{ + + MODULECMD_DOMAIN *dm; + + for (dm = modulecmd_domains; dm; dm = dm->next) + { + if (strcmp(dm->domain, domain) == 0) + { + return dm; + } + } + + if ((dm = domain_create(domain))) + { + dm->next = modulecmd_domains; + modulecmd_domains = dm; + } + + return dm; +} + +static MODULECMD* command_create(const char *identifier, const char *domain, + MODULECMDFN entry_point, int argc, + modulecmd_arg_type_t* argv) +{ + MODULECMD *rval = MXS_MALLOC(sizeof(*rval)); + char *id = MXS_STRDUP(identifier); + char *dm = MXS_STRDUP(domain); + modulecmd_arg_type_t *types = MXS_MALLOC(sizeof(*types) * argc); + + if (rval && id && dm && types) + { + for (int i = 0; i < argc; i++) + { + types[i] = argv[i]; + } + + rval->func = entry_point; + rval->identifier = id; + rval->domain = dm; + rval->arg_types = types; + rval->arg_count = argc; + rval->next = NULL; + } + else + { + MXS_FREE(rval); + MXS_FREE(id); + MXS_FREE(dm); + MXS_FREE(types); + rval = NULL; + } + + return rval; +} + +static void command_free(MODULECMD *cmd) +{ + if (cmd) + { + MXS_FREE(cmd->identifier); + MXS_FREE(cmd->domain); + MXS_FREE(cmd->arg_types); + MXS_FREE(cmd); + } +} + +static bool domain_has_command(MODULECMD_DOMAIN *dm, const char *id) +{ + for (MODULECMD *cmd = dm->commands; cmd; cmd = cmd->next) + { + if (strcmp(cmd->identifier, id) == 0) + { + return true; + } + } + return false; +} + +static bool process_argument(modulecmd_arg_type_t type, const char* value, + struct arg_node *arg) +{ + bool rval = false; + + if (!MODULECMD_ARG_IS_REQUIRED(type) && value == NULL) + { + arg->type = MODULECMD_ARG_NONE; + rval = true; + } + else if (value) + { + switch (MODULECMD_GET_TYPE(type)) + { + case MODULECMD_ARG_NONE: + arg->type = MODULECMD_ARG_NONE; + rval = true; + break; + + case MODULECMD_ARG_STRING: + if ((arg->value.string = MXS_STRDUP(value))) + { + arg->type = MODULECMD_ARG_STRING; + rval = true; + } + break; + + case MODULECMD_ARG_BOOLEAN: + { + int truthval = config_truth_value((char*)value); + if (truthval != -1) + { + arg->value.boolean = truthval; + arg->type = MODULECMD_ARG_BOOLEAN; + rval = true; + } + } + break; + + case MODULECMD_ARG_SERVICE: + if ((arg->value.service = service_find((char*)value))) + { + arg->type = MODULECMD_ARG_SERVICE; + rval = true; + } + break; + + case MODULECMD_ARG_SERVER: + if ((arg->value.server = server_find_by_unique_name(value))) + { + arg->type = MODULECMD_ARG_SERVER; + rval = true; + } + break; + + case MODULECMD_ARG_SESSION: + // TODO: Implement this + break; + + case MODULECMD_ARG_DCB: + // TODO: Implement this + break; + + case MODULECMD_ARG_MONITOR: + if ((arg->value.monitor = monitor_find((char*)value))) + { + arg->type = MODULECMD_ARG_MONITOR; + rval = true; + } + break; + + case MODULECMD_ARG_FILTER: + if ((arg->value.filter = filter_find((char*)value))) + { + arg->type = MODULECMD_ARG_FILTER; + rval = true; + } + break; + + default: + ss_dassert(false); + MXS_ERROR("Undefined argument type: %0lx", type); + break; + } + } + + return rval; +} + +static MODULECMD_ARG* modulecmd_arg_create(int argc) +{ + MODULECMD_ARG* arg = MXS_MALLOC(sizeof(*arg)); + struct arg_node *argv = MXS_CALLOC(argc, sizeof(*argv)); + + if (arg && argv) + { + arg->argc = argc; + arg->argv = argv; + } + else + { + MXS_FREE(argv); + MXS_FREE(arg); + arg = NULL; + } + + return arg; +} + +static void free_argument(struct arg_node *arg) +{ + switch (arg->type) + { + case MODULECMD_ARG_STRING: + MXS_FREE(arg->value.string); + break; + + default: + break; + } +} + +/** + * Public functions declared in modulecmd.h + */ + +bool modulecmd_register_command(const char *domain, const char *identifier, + MODULECMDFN entry_point, int argc, modulecmd_arg_type_t *argv) +{ + bool rval = false; + spinlock_acquire(&modulecmd_lock); + + MODULECMD_DOMAIN *dm = get_or_create_domain(domain); + + if (dm) + { + if (domain_has_command(dm, identifier)) + { + MXS_ERROR("Command '%s' in domain '%s' was registered more than once.", + identifier, domain); + } + else + { + MODULECMD *cmd = command_create(identifier, domain, entry_point, argc, argv); + + if (cmd) + { + cmd->next = dm->commands; + dm->commands = cmd; + rval = true; + } + } + } + + spinlock_release(&modulecmd_lock); + + return rval; +} + +const MODULECMD* modulecmd_find_command(const char *domain, const char *identifier) +{ + MODULECMD *rval = NULL; + spinlock_acquire(&modulecmd_lock); + + for (MODULECMD_DOMAIN *dm = modulecmd_domains; dm; dm = dm->next) + { + if (strcmp(domain, dm->domain) == 0) + { + for (MODULECMD *cmd = dm->commands; cmd; cmd = cmd->next) + { + if (strcmp(cmd->identifier, identifier) == 0) + { + rval = cmd; + break; + } + } + break; + } + } + + spinlock_release(&modulecmd_lock); + return rval; +} + +MODULECMD_ARG* modulecmd_arg_parse(const MODULECMD *cmd, int argc, const char **argv) +{ + int argc_min = 0; + + for (int i = 0; i < cmd->arg_count; i++) + { + if (MODULECMD_ARG_IS_REQUIRED(cmd->arg_types[i])) + { + argc_min++; + } + } + + MODULECMD_ARG* arg = NULL; + + if (argc >= argc_min && argc <= cmd->arg_count) + { + arg = modulecmd_arg_create(cmd->arg_count); + bool error = false; + + if (arg) + { + for (int i = 0; i < cmd->arg_count && i < argc; i++) + { + if (!process_argument(cmd->arg_types[i], argv[i], &arg->argv[i])) + { + MXS_ERROR("Failed to parse argument %d: %s", i + 1, argv[i] ? argv[i] : "NULL"); + error = true; + } + } + + if (error) + { + modulecmd_arg_free(arg); + arg = NULL; + } + } + } + + return arg; +} + +void modulecmd_arg_free(MODULECMD_ARG* arg) +{ + if (arg) + { + for (int i = 0; i < arg->argc; i++) + { + free_argument(&arg->argv[i]); + } + + MXS_FREE(arg->argv); + MXS_FREE(arg); + } +} + +bool modulecmd_call_command(const MODULECMD *cmd, const MODULECMD_ARG *args) +{ + return cmd->func(args); +} diff --git a/server/core/test/CMakeLists.txt b/server/core/test/CMakeLists.txt index f12f22ca5..1dd858aa6 100644 --- a/server/core/test/CMakeLists.txt +++ b/server/core/test/CMakeLists.txt @@ -18,6 +18,7 @@ add_executable(test_users testusers.c) add_executable(testfeedback testfeedback.c) add_executable(testmaxscalepcre2 testmaxscalepcre2.c) add_executable(testmemlog testmemlog.c) +add_executable(testmodulecmd testmodulecmd.c) target_link_libraries(test_adminusers maxscale-common) target_link_libraries(test_buffer maxscale-common) target_link_libraries(test_dcb maxscale-common) @@ -38,6 +39,7 @@ target_link_libraries(test_users maxscale-common) target_link_libraries(testfeedback maxscale-common) target_link_libraries(testmaxscalepcre2 maxscale-common) target_link_libraries(testmemlog maxscale-common) +target_link_libraries(testmodulecmd maxscale-common) add_test(TestAdminUsers test_adminusers) add_test(TestBuffer test_buffer) add_test(TestDCB test_dcb) @@ -58,6 +60,7 @@ add_test(TestServer test_server) add_test(TestService test_service) add_test(TestSpinlock test_spinlock) add_test(TestUsers test_users) +add_test(TestModulecmd testmodulecmd) # This test requires external dependencies and thus cannot be run # as a part of the core test set diff --git a/server/core/test/testmodulecmd.c b/server/core/test/testmodulecmd.c new file mode 100644 index 000000000..7a1832aa8 --- /dev/null +++ b/server/core/test/testmodulecmd.c @@ -0,0 +1,185 @@ +/* + * Copyright (c) 2016 MariaDB Corporation Ab + * + * Use of this software is governed by the Business Source License included + * in the LICENSE.TXT file and at www.mariadb.com/bsl. + * + * Change Date: 2019-07-01 + * + * On the date above, in accordance with the Business Source License, use + * of this software will be governed by version 2 or later of the General + * Public License. + */ + +/** + * Test modulecmd.h functionality + */ + +#include + +#define TEST(a, b) do{if (!(a)){printf("%s:%d "b"\n", __FILE__, __LINE__);return 1;}}while(false) + +static bool ok = false; + +bool test_fn(const MODULECMD_ARG *arg) +{ + + ok = (arg->argc == 2 && strcmp(arg->argv[0].value.string, "Hello") == 0 && + arg->argv[1].value.boolean); + + return true; +} + +int test_arguments() +{ + const char *params1[] = {"Hello", "true"}; + const char *params2[] = {"Hello", "1"}; + + const char *wrong_params1[] = {"Hi", "true"}; + const char *wrong_params2[] = {"Hello", "false"}; + + const char *bad_params1[] = {"Hello", "World!"}; + const char *bad_params2[] = {"Hello", NULL}; + const char *bad_params3[] = {NULL, NULL}; + const char *bad_params4[] = {NULL, "World!"}; + + const char *ns = "test_arguments"; + const char *id = "test_arguments"; + modulecmd_arg_type_t args1[] = {MODULECMD_ARG_STRING, MODULECMD_ARG_BOOLEAN}; + + /** + * Test command creation + */ + + TEST(modulecmd_find_command(ns, id) == NULL, "The registered command should not yet be found"); + + TEST(modulecmd_register_command(ns, id, test_fn, 2, args1), + "Registering a command should succeed"); + + TEST(!modulecmd_register_command(ns, id, test_fn, 2, args1), + "Registering the command a second time should fail"); + + const MODULECMD *cmd = modulecmd_find_command(ns, id); + TEST(cmd, "The registered command should be found"); + + /** + * Test bad arguments + */ + + TEST(modulecmd_arg_parse(cmd, 0, NULL) == NULL, "Passing no arguments should fail"); + TEST(modulecmd_arg_parse(cmd, 1, params1) == NULL, "Passing one argument should fail"); + TEST(modulecmd_arg_parse(cmd, 3, params1) == NULL, "Passing three arguments should fail"); + + TEST(modulecmd_arg_parse(cmd, 2, bad_params1) == NULL, "Passing bad arguments should fail"); + TEST(modulecmd_arg_parse(cmd, 2, bad_params2) == NULL, "Passing bad arguments should fail"); + TEST(modulecmd_arg_parse(cmd, 2, bad_params3) == NULL, "Passing bad arguments should fail"); + TEST(modulecmd_arg_parse(cmd, 2, bad_params4) == NULL, "Passing bad arguments should fail"); + + /** + * Test valid arguments + */ + + MODULECMD_ARG* alist = modulecmd_arg_parse(cmd, 2, params1); + TEST(alist, "Arguments should be parsed"); + + TEST(modulecmd_call_command(cmd, alist), "Module call should be successful"); + TEST(ok, "Function should receive right parameters"); + + ok = false; + + TEST(modulecmd_call_command(cmd, alist), "Second Module call should be successful"); + TEST(ok, "Function should receive right parameters"); + + + ok = false; + modulecmd_arg_free(alist); + + TEST((alist = modulecmd_arg_parse(cmd, 2, params2)), "Arguments should be parsed"); + + TEST(modulecmd_call_command(cmd, alist), "Module call should be successful"); + TEST(ok, "Function should receive right parameters"); + + modulecmd_arg_free(alist); + + /** + * Test valid but wrong arguments + */ + TEST((alist = modulecmd_arg_parse(cmd, 2, wrong_params1)), "Arguments should be parsed"); + TEST(modulecmd_call_command(cmd, alist), "Module call should be successful"); + TEST(!ok, "Function should receive wrong parameters"); + modulecmd_arg_free(alist); + + TEST((alist = modulecmd_arg_parse(cmd, 2, wrong_params2)), "Arguments should be parsed"); + TEST(modulecmd_call_command(cmd, alist), "Module call should be successful"); + TEST(!ok, "Function should receive wrong parameters"); + modulecmd_arg_free(alist); + + return 0; +} + +bool test_fn2(const MODULECMD_ARG *arg) +{ + return true; +} + +int test_optional_arguments() +{ + const char *params1[] = {"Hello", "true"}; + const char *params2[] = {NULL, "true"}; + const char *params3[] = {"Hello", NULL}; + const char *params4[] = {NULL, NULL}; + + const char *ns = "test_optional_arguments"; + const char *id = "test_optional_arguments"; + modulecmd_arg_type_t args1[] = + { + MODULECMD_ARG_STRING | MODULECMD_ARG_OPTIONAL, + MODULECMD_ARG_BOOLEAN | MODULECMD_ARG_OPTIONAL + }; + + TEST(modulecmd_register_command(ns, id, test_fn2, 2, args1), + "Registering a command should succeed"); + + const MODULECMD *cmd = modulecmd_find_command(ns, id); + TEST(cmd, "The registered command should be found"); + + MODULECMD_ARG *arg = modulecmd_arg_parse(cmd, 2, params1); + TEST(arg, "Parsing arguments should succeed"); + modulecmd_arg_free(arg); + + arg = modulecmd_arg_parse(cmd, 2, params2); + TEST(arg, "Parsing arguments should succeed"); + modulecmd_arg_free(arg); + + arg = modulecmd_arg_parse(cmd, 2, params3); + TEST(arg, "Parsing arguments should succeed"); + modulecmd_arg_free(arg); + + arg = modulecmd_arg_parse(cmd, 2, params4); + TEST(arg, "Parsing arguments should succeed"); + modulecmd_arg_free(arg); + + arg = modulecmd_arg_parse(cmd, 1, params1); + TEST(arg, "Parsing arguments should succeed"); + modulecmd_arg_free(arg); + + arg = modulecmd_arg_parse(cmd, 1, params2); + TEST(arg, "Parsing arguments should succeed"); + modulecmd_arg_free(arg); + + arg = modulecmd_arg_parse(cmd, 0, params1); + TEST(arg, "Parsing arguments should succeed"); + modulecmd_arg_free(arg); + + return 0; +} + +int main(int argc, char **argv) +{ + int rc = 0; + + rc += test_arguments(); + rc += test_optional_arguments(); + + return rc; +} From 4a142b1ca9258c38aa1fee5783ea1b57390ee7fc Mon Sep 17 00:00:00 2001 From: Markus Makela Date: Sat, 19 Nov 2016 01:43:16 +0200 Subject: [PATCH 26/29] MXS-929: Add errors to the modulecmd system The modules can now return human-readable error messages to the caller of the function. The internals of the modulecmd system also use this to return errors to the users. The error messages can be retrieved with a common error function which should make it easy to use in various clients. --- include/maxscale/modulecmd.h | 21 +++- server/core/config.c | 2 +- server/core/modulecmd.c | 175 ++++++++++++++++++++++++++----- server/core/test/testmodulecmd.c | 65 +++++++++++- 4 files changed, 233 insertions(+), 30 deletions(-) diff --git a/include/maxscale/modulecmd.h b/include/maxscale/modulecmd.h index 7c1b0088c..bfa8d6d5e 100644 --- a/include/maxscale/modulecmd.h +++ b/include/maxscale/modulecmd.h @@ -110,7 +110,8 @@ typedef struct modulecmd char *identifier; /**< Unique identifier */ char *domain; /**< Command domain */ MODULECMDFN func; /**< The registered function */ - int arg_count; /**< Number of arguments */ + int arg_count_min; /**< Minimum number of arguments */ + int arg_count_max; /**< Maximum number of arguments */ modulecmd_arg_type_t *arg_types; /**< Argument types */ struct modulecmd *next; /**< Next command */ } MODULECMD; @@ -168,4 +169,22 @@ void modulecmd_arg_free(MODULECMD_ARG *arg); */ bool modulecmd_call_command(const MODULECMD *cmd, const MODULECMD_ARG *args); +/** + * @brief Set the current error message + * + * Modules that register commands should use this function to report errors. + * This will overwrite the existing error message. + * + * @param format Format string + * @param ... Format string arguments + */ +void modulecmd_set_error(const char *format, ...); + +/** + * @brief Get the latest error generated by the modulecmd system + * + * @return Human-readable error message + */ +const char* modulecmd_get_error(); + MXS_END_DECLS diff --git a/server/core/config.c b/server/core/config.c index f2aec6ab3..3e62de294 100644 --- a/server/core/config.c +++ b/server/core/config.c @@ -2010,7 +2010,7 @@ config_truth_value(char *str) { return 0; } - MXS_ERROR("Not a boolean value: %s", str); + return -1; } diff --git a/server/core/modulecmd.c b/server/core/modulecmd.c index cf5d4515e..045f0b2e7 100644 --- a/server/core/modulecmd.c +++ b/server/core/modulecmd.c @@ -14,8 +14,18 @@ #include #include #include +#include #include +/** Size of the error buffer */ +#define MODULECMD_ERRBUF_SIZE 512 + +/** Thread local error buffer */ +thread_local char *errbuf = NULL; + +/** Parameter passed to functions that do not always expect arguments */ +static const MODULECMD_ARG MODULECMD_NO_ARGUMENTS = {0, NULL}; + /** * A registered domain */ @@ -34,6 +44,41 @@ typedef struct modulecmd_domain static MODULECMD_DOMAIN *modulecmd_domains = NULL; static SPINLOCK modulecmd_lock = SPINLOCK_INIT; +static inline void prepare_error() +{ + if (errbuf == NULL) + { + errbuf = MXS_MALLOC(MODULECMD_ERRBUF_SIZE); + MXS_ABORT_IF_NULL(errbuf); + errbuf[0] = '\0'; + } +} + +/** + * @brief Reset error message + * + * This should be the first function called in every API function that can + * generate errors. + */ +static void reset_error() +{ + prepare_error(); + errbuf[0] = '\0'; +} + +static void report_argc_mismatch(const MODULECMD *cmd, int argc) +{ + if (cmd->arg_count_min == cmd->arg_count_max) + { + modulecmd_set_error("Expected %d arguments, got %d.", cmd->arg_count_min, argc); + } + else + { + modulecmd_set_error("Expected between %d and %d arguments, got %d.", cmd->arg_count_min, cmd->arg_count_max, + argc); + } +} + static MODULECMD_DOMAIN* domain_create(const char *domain) { MODULECMD_DOMAIN *rval = MXS_MALLOC(sizeof(*rval)); @@ -97,8 +142,14 @@ static MODULECMD* command_create(const char *identifier, const char *domain, if (rval && id && dm && types) { + int argc_min = 0; + for (int i = 0; i < argc; i++) { + if (MODULECMD_ARG_IS_REQUIRED(argv[i])) + { + argc_min++; + } types[i] = argv[i]; } @@ -106,7 +157,8 @@ static MODULECMD* command_create(const char *identifier, const char *domain, rval->identifier = id; rval->domain = dm; rval->arg_types = types; - rval->arg_count = argc; + rval->arg_count_min = argc_min; + rval->arg_count_max = argc; rval->next = NULL; } else @@ -145,7 +197,7 @@ static bool domain_has_command(MODULECMD_DOMAIN *dm, const char *id) } static bool process_argument(modulecmd_arg_type_t type, const char* value, - struct arg_node *arg) + struct arg_node *arg, const char **err) { bool rval = false; @@ -169,19 +221,27 @@ static bool process_argument(modulecmd_arg_type_t type, const char* value, arg->type = MODULECMD_ARG_STRING; rval = true; } + else + { + *err = "memory allocation failed"; + } break; case MODULECMD_ARG_BOOLEAN: - { - int truthval = config_truth_value((char*)value); - if (truthval != -1) { - arg->value.boolean = truthval; - arg->type = MODULECMD_ARG_BOOLEAN; - rval = true; + int truthval = config_truth_value((char*)value); + if (truthval != -1) + { + arg->value.boolean = truthval; + arg->type = MODULECMD_ARG_BOOLEAN; + rval = true; + } + else + { + *err = "not a boolean value"; + } } - } - break; + break; case MODULECMD_ARG_SERVICE: if ((arg->value.service = service_find((char*)value))) @@ -189,6 +249,10 @@ static bool process_argument(modulecmd_arg_type_t type, const char* value, arg->type = MODULECMD_ARG_SERVICE; rval = true; } + else + { + *err = "service not found"; + } break; case MODULECMD_ARG_SERVER: @@ -197,6 +261,10 @@ static bool process_argument(modulecmd_arg_type_t type, const char* value, arg->type = MODULECMD_ARG_SERVER; rval = true; } + else + { + *err = "server not found"; + } break; case MODULECMD_ARG_SESSION: @@ -213,6 +281,10 @@ static bool process_argument(modulecmd_arg_type_t type, const char* value, arg->type = MODULECMD_ARG_MONITOR; rval = true; } + else + { + *err = "monitor not found"; + } break; case MODULECMD_ARG_FILTER: @@ -221,14 +293,23 @@ static bool process_argument(modulecmd_arg_type_t type, const char* value, arg->type = MODULECMD_ARG_FILTER; rval = true; } + else + { + *err = "filter not found"; + } break; default: ss_dassert(false); MXS_ERROR("Undefined argument type: %0lx", type); + *err = "internal error"; break; } } + else + { + *err = "required argument"; + } return rval; } @@ -273,6 +354,7 @@ static void free_argument(struct arg_node *arg) bool modulecmd_register_command(const char *domain, const char *identifier, MODULECMDFN entry_point, int argc, modulecmd_arg_type_t *argv) { + reset_error(); bool rval = false; spinlock_acquire(&modulecmd_lock); @@ -282,8 +364,8 @@ bool modulecmd_register_command(const char *domain, const char *identifier, { if (domain_has_command(dm, identifier)) { - MXS_ERROR("Command '%s' in domain '%s' was registered more than once.", - identifier, domain); + modulecmd_set_error("Command registered more than once: %s::%s", domain, identifier); + MXS_ERROR("Command registered more than once: %s::%s", domain, identifier); } else { @@ -305,6 +387,7 @@ bool modulecmd_register_command(const char *domain, const char *identifier, const MODULECMD* modulecmd_find_command(const char *domain, const char *identifier) { + reset_error(); MODULECMD *rval = NULL; spinlock_acquire(&modulecmd_lock); @@ -325,36 +408,37 @@ const MODULECMD* modulecmd_find_command(const char *domain, const char *identifi } spinlock_release(&modulecmd_lock); + + if (rval == NULL) + { + modulecmd_set_error("Command not found: %s::%s", domain, identifier); + } + return rval; } MODULECMD_ARG* modulecmd_arg_parse(const MODULECMD *cmd, int argc, const char **argv) { - int argc_min = 0; - - for (int i = 0; i < cmd->arg_count; i++) - { - if (MODULECMD_ARG_IS_REQUIRED(cmd->arg_types[i])) - { - argc_min++; - } - } + reset_error(); MODULECMD_ARG* arg = NULL; - if (argc >= argc_min && argc <= cmd->arg_count) + if (argc >= cmd->arg_count_min && argc <= cmd->arg_count_max) { - arg = modulecmd_arg_create(cmd->arg_count); + arg = modulecmd_arg_create(cmd->arg_count_max); bool error = false; if (arg) { - for (int i = 0; i < cmd->arg_count && i < argc; i++) + for (int i = 0; i < cmd->arg_count_max && i < argc; i++) { - if (!process_argument(cmd->arg_types[i], argv[i], &arg->argv[i])) + const char *err = ""; + + if (!process_argument(cmd->arg_types[i], argv[i], &arg->argv[i], &err)) { - MXS_ERROR("Failed to parse argument %d: %s", i + 1, argv[i] ? argv[i] : "NULL"); error = true; + modulecmd_set_error("Argument %d, %s: %s", i + 1, err, argv[i] ? argv[i] : "No argument given"); + break; } } @@ -365,6 +449,10 @@ MODULECMD_ARG* modulecmd_arg_parse(const MODULECMD *cmd, int argc, const char ** } } } + else + { + report_argc_mismatch(cmd, argc); + } return arg; } @@ -385,5 +473,38 @@ void modulecmd_arg_free(MODULECMD_ARG* arg) bool modulecmd_call_command(const MODULECMD *cmd, const MODULECMD_ARG *args) { - return cmd->func(args); + bool rval = false; + reset_error(); + + if (cmd->arg_count_min > 0 && args == NULL) + { + report_argc_mismatch(cmd, 0); + } + else + { + if (args == NULL) + { + args = &MODULECMD_NO_ARGUMENTS; + } + + rval = cmd->func(args); + } + + return rval; +} + +void modulecmd_set_error(const char *format, ...) +{ + prepare_error(); + + va_list list; + va_start(list, format); + vsnprintf(errbuf, MODULECMD_ERRBUF_SIZE, format, list); + va_end(list); +} + +const char* modulecmd_get_error() +{ + prepare_error(); + return errbuf; } diff --git a/server/core/test/testmodulecmd.c b/server/core/test/testmodulecmd.c index 7a1832aa8..be4a0a498 100644 --- a/server/core/test/testmodulecmd.c +++ b/server/core/test/testmodulecmd.c @@ -47,17 +47,21 @@ int test_arguments() const char *id = "test_arguments"; modulecmd_arg_type_t args1[] = {MODULECMD_ARG_STRING, MODULECMD_ARG_BOOLEAN}; + TEST(strlen(modulecmd_get_error()) == 0, "Error message should be empty"); + /** * Test command creation */ TEST(modulecmd_find_command(ns, id) == NULL, "The registered command should not yet be found"); + TEST(strlen(modulecmd_get_error()), "Error message should not be empty"); TEST(modulecmd_register_command(ns, id, test_fn, 2, args1), "Registering a command should succeed"); TEST(!modulecmd_register_command(ns, id, test_fn, 2, args1), "Registering the command a second time should fail"); + TEST(strlen(modulecmd_get_error()), "Error message should not be empty"); const MODULECMD *cmd = modulecmd_find_command(ns, id); TEST(cmd, "The registered command should be found"); @@ -67,13 +71,20 @@ int test_arguments() */ TEST(modulecmd_arg_parse(cmd, 0, NULL) == NULL, "Passing no arguments should fail"); + TEST(strlen(modulecmd_get_error()), "Error message should not be empty"); TEST(modulecmd_arg_parse(cmd, 1, params1) == NULL, "Passing one argument should fail"); + TEST(strlen(modulecmd_get_error()), "Error message should not be empty"); TEST(modulecmd_arg_parse(cmd, 3, params1) == NULL, "Passing three arguments should fail"); + TEST(strlen(modulecmd_get_error()), "Error message should not be empty"); TEST(modulecmd_arg_parse(cmd, 2, bad_params1) == NULL, "Passing bad arguments should fail"); + TEST(strlen(modulecmd_get_error()), "Error message should not be empty"); TEST(modulecmd_arg_parse(cmd, 2, bad_params2) == NULL, "Passing bad arguments should fail"); + TEST(strlen(modulecmd_get_error()), "Error message should not be empty"); TEST(modulecmd_arg_parse(cmd, 2, bad_params3) == NULL, "Passing bad arguments should fail"); + TEST(strlen(modulecmd_get_error()), "Error message should not be empty"); TEST(modulecmd_arg_parse(cmd, 2, bad_params4) == NULL, "Passing bad arguments should fail"); + TEST(strlen(modulecmd_get_error()), "Error message should not be empty"); /** * Test valid arguments @@ -81,6 +92,7 @@ int test_arguments() MODULECMD_ARG* alist = modulecmd_arg_parse(cmd, 2, params1); TEST(alist, "Arguments should be parsed"); + TEST(strlen(modulecmd_get_error()) == 0, "Error message should be empty"); TEST(modulecmd_call_command(cmd, alist), "Module call should be successful"); TEST(ok, "Function should receive right parameters"); @@ -95,7 +107,7 @@ int test_arguments() modulecmd_arg_free(alist); TEST((alist = modulecmd_arg_parse(cmd, 2, params2)), "Arguments should be parsed"); - + TEST(strlen(modulecmd_get_error()) == 0, "Error message should be empty"); TEST(modulecmd_call_command(cmd, alist), "Module call should be successful"); TEST(ok, "Function should receive right parameters"); @@ -105,11 +117,14 @@ int test_arguments() * Test valid but wrong arguments */ TEST((alist = modulecmd_arg_parse(cmd, 2, wrong_params1)), "Arguments should be parsed"); + TEST(strlen(modulecmd_get_error()) == 0, "Error message should be empty"); TEST(modulecmd_call_command(cmd, alist), "Module call should be successful"); + TEST(strlen(modulecmd_get_error()) == 0, "Error message should be empty"); TEST(!ok, "Function should receive wrong parameters"); modulecmd_arg_free(alist); TEST((alist = modulecmd_arg_parse(cmd, 2, wrong_params2)), "Arguments should be parsed"); + TEST(strlen(modulecmd_get_error()) == 0, "Error message should be empty"); TEST(modulecmd_call_command(cmd, alist), "Module call should be successful"); TEST(!ok, "Function should receive wrong parameters"); modulecmd_arg_free(alist); @@ -145,32 +160,79 @@ int test_optional_arguments() MODULECMD_ARG *arg = modulecmd_arg_parse(cmd, 2, params1); TEST(arg, "Parsing arguments should succeed"); + TEST(strlen(modulecmd_get_error()) == 0, "Error message should be empty"); + TEST(modulecmd_call_command(cmd, arg), "Module call should be successful"); + TEST(strlen(modulecmd_get_error()) == 0, "Error message should be empty"); modulecmd_arg_free(arg); arg = modulecmd_arg_parse(cmd, 2, params2); TEST(arg, "Parsing arguments should succeed"); + TEST(strlen(modulecmd_get_error()) == 0, "Error message should be empty"); + TEST(modulecmd_call_command(cmd, arg), "Module call should be successful"); + TEST(strlen(modulecmd_get_error()) == 0, "Error message should be empty"); modulecmd_arg_free(arg); arg = modulecmd_arg_parse(cmd, 2, params3); TEST(arg, "Parsing arguments should succeed"); + TEST(strlen(modulecmd_get_error()) == 0, "Error message should be empty"); + TEST(modulecmd_call_command(cmd, arg), "Module call should be successful"); + TEST(strlen(modulecmd_get_error()) == 0, "Error message should be empty"); modulecmd_arg_free(arg); arg = modulecmd_arg_parse(cmd, 2, params4); TEST(arg, "Parsing arguments should succeed"); + TEST(strlen(modulecmd_get_error()) == 0, "Error message should be empty"); + TEST(modulecmd_call_command(cmd, arg), "Module call should be successful"); + TEST(strlen(modulecmd_get_error()) == 0, "Error message should be empty"); modulecmd_arg_free(arg); arg = modulecmd_arg_parse(cmd, 1, params1); TEST(arg, "Parsing arguments should succeed"); + TEST(strlen(modulecmd_get_error()) == 0, "Error message should be empty"); + TEST(modulecmd_call_command(cmd, arg), "Module call should be successful"); + TEST(strlen(modulecmd_get_error()) == 0, "Error message should be empty"); modulecmd_arg_free(arg); arg = modulecmd_arg_parse(cmd, 1, params2); TEST(arg, "Parsing arguments should succeed"); + TEST(strlen(modulecmd_get_error()) == 0, "Error message should be empty"); + TEST(modulecmd_call_command(cmd, arg), "Module call should be successful"); + TEST(strlen(modulecmd_get_error()) == 0, "Error message should be empty"); modulecmd_arg_free(arg); arg = modulecmd_arg_parse(cmd, 0, params1); TEST(arg, "Parsing arguments should succeed"); + TEST(strlen(modulecmd_get_error()) == 0, "Error message should be empty"); + TEST(modulecmd_call_command(cmd, arg), "Module call should be successful"); + TEST(strlen(modulecmd_get_error()) == 0, "Error message should be empty"); modulecmd_arg_free(arg); + TEST(modulecmd_call_command(cmd, NULL), "Module call should be successful"); + TEST(strlen(modulecmd_get_error()) == 0, "Error message should be empty"); + + return 0; +} + +bool test_fn3(const MODULECMD_ARG *arg) +{ + modulecmd_set_error("Something went wrong!"); + return false; +} + +int test_module_errors() +{ + const char *ns = "test_module_errors"; + const char *id = "test_module_errors"; + + TEST(modulecmd_register_command(ns, id, test_fn3, 0, NULL), + "Registering a command should succeed"); + + const MODULECMD *cmd = modulecmd_find_command(ns, id); + TEST(cmd, "The registered command should be found"); + + TEST(!modulecmd_call_command(cmd, NULL), "Module call should fail"); + TEST(strlen(modulecmd_get_error()), "Error message should not be empty"); + return 0; } @@ -180,6 +242,7 @@ int main(int argc, char **argv) rc += test_arguments(); rc += test_optional_arguments(); + rc += test_module_errors(); return rc; } From d68172260d72ed43a6a8956e80998168f48f7db7 Mon Sep 17 00:00:00 2001 From: Markus Makela Date: Sat, 19 Nov 2016 04:29:03 +0200 Subject: [PATCH 27/29] MXS-929: Add mapping function for module commands The modulecmd_foreach function allows commands to be iterated without having to manage the locking of the system. This allows the commands to be easily iterated and gathered into filtered lists without having to build it into the module command system itself. --- include/maxscale/modulecmd.h | 29 ++++++++++++++- server/core/modulecmd.c | 56 ++++++++++++++++++++++++++++ server/core/test/testmodulecmd.c | 63 ++++++++++++++++++++++++++++++++ 3 files changed, 147 insertions(+), 1 deletion(-) diff --git a/include/maxscale/modulecmd.h b/include/maxscale/modulecmd.h index bfa8d6d5e..ebd18541a 100644 --- a/include/maxscale/modulecmd.h +++ b/include/maxscale/modulecmd.h @@ -173,7 +173,7 @@ bool modulecmd_call_command(const MODULECMD *cmd, const MODULECMD_ARG *args); * @brief Set the current error message * * Modules that register commands should use this function to report errors. - * This will overwrite the existing error message. + * This will overwrite any existing error message. * * @param format Format string * @param ... Format string arguments @@ -187,4 +187,31 @@ void modulecmd_set_error(const char *format, ...); */ const char* modulecmd_get_error(); + +/** + * @brief Call a function for each command + * + * Calls a function for each matching command in the matched domains. The filters + * for the domain and identifier are PCRE2 expressions that are matched against + * the domain and identifier. These are optional and both @c domain and @c ident + * can be NULL. + * + * @param domain_re Command domain filter, NULL for all domains + * + * @param ident_re Command identifier filter, NULL for all commands + * + * @param fn Function that is called for every command. The first parameter is + * the current command. The second parameter is the value of @c data. + * The function should return true to continue iteration or false to + * stop iteration early. The function must not call any of the functions + * declared in modulecmd.h. + * + * @param data User defined data passed as the second parameter to @c fn + * + * @return True on success, false on PCRE2 error. Use modulecmd_get_error() + * to retrieve the error. + */ +bool modulecmd_foreach(const char *domain_re, const char *ident_re, + bool(*fn)(const MODULECMD *cmd, void *data), void *data); + MXS_END_DECLS diff --git a/server/core/modulecmd.c b/server/core/modulecmd.c index 045f0b2e7..09801764f 100644 --- a/server/core/modulecmd.c +++ b/server/core/modulecmd.c @@ -14,6 +14,7 @@ #include #include #include +#include #include #include @@ -508,3 +509,58 @@ const char* modulecmd_get_error() prepare_error(); return errbuf; } + +bool modulecmd_foreach(const char *domain_re, const char *ident_re, + bool(*fn)(const MODULECMD *cmd, void *data), void *data) +{ + bool rval = true; + bool stop = false; + spinlock_acquire(&modulecmd_lock); + + for (MODULECMD_DOMAIN *domain = modulecmd_domains; domain && rval && !stop; domain = domain->next) + { + int err; + mxs_pcre2_result_t d_res = domain_re ? + mxs_pcre2_simple_match(domain_re, domain->domain, 0, &err) : + MXS_PCRE2_MATCH; + + if (d_res == MXS_PCRE2_MATCH) + { + for (MODULECMD *cmd = domain->commands; cmd && rval; cmd = cmd->next) + { + mxs_pcre2_result_t i_res = ident_re ? + mxs_pcre2_simple_match(ident_re, cmd->identifier, 0, &err) : + MXS_PCRE2_MATCH; + + if (i_res == MXS_PCRE2_MATCH) + { + if (!fn(cmd, data)) + { + stop = true; + break; + } + } + else if (i_res == MXS_PCRE2_ERROR) + { + PCRE2_UCHAR errbuf[MXS_STRERROR_BUFLEN]; + pcre2_get_error_message(err, errbuf, sizeof(errbuf)); + MXS_ERROR("Failed to match command identifier with '%s': %s", ident_re, errbuf); + modulecmd_set_error("Failed to match command identifier with '%s': %s", ident_re, errbuf); + rval = false; + } + + } + } + else if (d_res == MXS_PCRE2_ERROR) + { + PCRE2_UCHAR errbuf[MXS_STRERROR_BUFLEN]; + pcre2_get_error_message(err, errbuf, sizeof(errbuf)); + MXS_ERROR("Failed to match command domain with '%s': %s", domain_re, errbuf); + modulecmd_set_error("Failed to match command domain with '%s': %s", domain_re, errbuf); + rval = false; + } + } + + spinlock_release(&modulecmd_lock); + return rval; +} diff --git a/server/core/test/testmodulecmd.c b/server/core/test/testmodulecmd.c index be4a0a498..96b9fd419 100644 --- a/server/core/test/testmodulecmd.c +++ b/server/core/test/testmodulecmd.c @@ -236,6 +236,68 @@ int test_module_errors() return 0; } +bool test_fn_map(const MODULECMD_ARG *args) +{ + return true; +} + +const char *map_dom = "test_map"; + +bool mapfn(const MODULECMD *cmd, void *data) +{ + int *i = (int*)data; + (*i)++; + return true; +} + +int test_map() +{ + for (int i = 0; i < 10; i++) + { + char id[200]; + sprintf(id, "test_map%d", i + 1); + TEST(modulecmd_register_command(map_dom, id, test_fn_map, 0, NULL), + "Registering a command should succeed"); + } + + int n = 0; + TEST(modulecmd_foreach(NULL, NULL, mapfn, &n), "Mapping function should succeed"); + TEST(strlen(modulecmd_get_error()) == 0, "Error message should be empty"); + TEST(n == 10, "Every registered command should be called"); + + n = 0; + TEST(modulecmd_foreach("test_map", NULL, mapfn, &n), "Mapping function should succeed"); + TEST(strlen(modulecmd_get_error()) == 0, "Error message should be empty"); + TEST(n == 10, "Every registered command should be called"); + + n = 0; + TEST(modulecmd_foreach(NULL, "test_map", mapfn, &n), "Mapping function should succeed"); + TEST(strlen(modulecmd_get_error()) == 0, "Error message should be empty"); + TEST(n == 10, "Every registered command should be called"); + + n = 0; + TEST(modulecmd_foreach("test_map", "test_map", mapfn, &n), "Mapping function should succeed"); + TEST(strlen(modulecmd_get_error()) == 0, "Error message should be empty"); + TEST(n == 10, "Every registered command should be called"); + + n = 0; + TEST(modulecmd_foreach("wrong domain", "test_map", mapfn, &n), "Mapping function should succeed"); + TEST(strlen(modulecmd_get_error()) == 0, "Error message should be empty"); + TEST(n == 0, "No registered command should be called"); + + n = 0; + TEST(modulecmd_foreach("test_map", "test_map[2-4]", mapfn, &n), "Mapping function should succeed"); + TEST(strlen(modulecmd_get_error()) == 0, "Error message should be empty"); + TEST(n == 3, "Three registered commands should be called"); + + n = 0; + TEST(!modulecmd_foreach("(", NULL, mapfn, &n), "Mapping function should fail"); + TEST(strlen(modulecmd_get_error()), "Error message should not be empty"); + TEST(n == 0, "No registered command should be called"); + + return 0; +} + int main(int argc, char **argv) { int rc = 0; @@ -243,6 +305,7 @@ int main(int argc, char **argv) rc += test_arguments(); rc += test_optional_arguments(); rc += test_module_errors(); + rc += test_map(); return rc; } From 4137d58dd5fc9a2577888df26726fc3afaec1ad6 Mon Sep 17 00:00:00 2001 From: Markus Makela Date: Mon, 21 Nov 2016 06:54:56 +0200 Subject: [PATCH 28/29] MXS-929: Implement DCB and SESSION pointer handling DCBs and SESSIONs can be passed either as raw pointers or as the string representations of them. The preferred way to pass them is to use the raw pointer types. This removes the need to convert the pointer to string form and back. --- include/maxscale/modulecmd.h | 22 +++++---- server/core/modulecmd.c | 28 ++++++++--- server/core/test/testmodulecmd.c | 84 ++++++++++++++++++++++++++------ 3 files changed, 103 insertions(+), 31 deletions(-) diff --git a/include/maxscale/modulecmd.h b/include/maxscale/modulecmd.h index ebd18541a..0c184ab1b 100644 --- a/include/maxscale/modulecmd.h +++ b/include/maxscale/modulecmd.h @@ -44,15 +44,17 @@ typedef uint64_t modulecmd_arg_type_t; * Argument types for the registered functions, the first 8 bits of * the modulecmd_arg_type_t type. An argument can be of only one type. */ -#define MODULECMD_ARG_NONE 0 -#define MODULECMD_ARG_STRING 1 -#define MODULECMD_ARG_BOOLEAN 2 -#define MODULECMD_ARG_SERVICE 3 -#define MODULECMD_ARG_SERVER 4 -#define MODULECMD_ARG_SESSION 5 -#define MODULECMD_ARG_DCB 6 -#define MODULECMD_ARG_MONITOR 7 -#define MODULECMD_ARG_FILTER 8 +#define MODULECMD_ARG_NONE 0 +#define MODULECMD_ARG_STRING 1 +#define MODULECMD_ARG_BOOLEAN 2 +#define MODULECMD_ARG_SERVICE 3 +#define MODULECMD_ARG_SERVER 4 +#define MODULECMD_ARG_SESSION 5 +#define MODULECMD_ARG_SESSION_PTR 6 +#define MODULECMD_ARG_DCB 7 +#define MODULECMD_ARG_DCB_PTR 8 +#define MODULECMD_ARG_MONITOR 9 +#define MODULECMD_ARG_FILTER 10 /** * Options for arguments, bits 9 through 32 @@ -148,7 +150,7 @@ const MODULECMD* modulecmd_find_command(const char *domain, const char *identifi * @param argv Argument list in string format of size @c argc * @return Parsed arguments or NULL on error */ -MODULECMD_ARG* modulecmd_arg_parse(const MODULECMD *cmd, int argc, const char **argv); +MODULECMD_ARG* modulecmd_arg_parse(const MODULECMD *cmd, int argc, const void **argv); /** * @brief Free parsed arguments returned by modulecmd_arg_parse diff --git a/server/core/modulecmd.c b/server/core/modulecmd.c index 09801764f..fca0bbd84 100644 --- a/server/core/modulecmd.c +++ b/server/core/modulecmd.c @@ -197,7 +197,7 @@ static bool domain_has_command(MODULECMD_DOMAIN *dm, const char *id) return false; } -static bool process_argument(modulecmd_arg_type_t type, const char* value, +static bool process_argument(modulecmd_arg_type_t type, const void* value, struct arg_node *arg, const char **err) { bool rval = false; @@ -217,7 +217,7 @@ static bool process_argument(modulecmd_arg_type_t type, const char* value, break; case MODULECMD_ARG_STRING: - if ((arg->value.string = MXS_STRDUP(value))) + if ((arg->value.string = MXS_STRDUP((char*)value))) { arg->type = MODULECMD_ARG_STRING; rval = true; @@ -257,7 +257,7 @@ static bool process_argument(modulecmd_arg_type_t type, const char* value, break; case MODULECMD_ARG_SERVER: - if ((arg->value.server = server_find_by_unique_name(value))) + if ((arg->value.server = server_find_by_unique_name((char*)value))) { arg->type = MODULECMD_ARG_SERVER; rval = true; @@ -269,11 +269,27 @@ static bool process_argument(modulecmd_arg_type_t type, const char* value, break; case MODULECMD_ARG_SESSION: - // TODO: Implement this + arg->type = MODULECMD_ARG_SESSION; + arg->value.session = (SESSION*)strtol((char*)value, NULL, 0); + rval = true; + break; + + case MODULECMD_ARG_SESSION_PTR: + arg->type = MODULECMD_ARG_SESSION_PTR; + arg->value.session = (SESSION*)value; + rval = true; break; case MODULECMD_ARG_DCB: - // TODO: Implement this + arg->type = MODULECMD_ARG_DCB; + arg->value.dcb = (DCB*)strtol((char*)value, NULL, 0); + rval = true; + break; + + case MODULECMD_ARG_DCB_PTR: + arg->type = MODULECMD_ARG_DCB_PTR; + arg->value.dcb = (DCB*)value; + rval = true; break; case MODULECMD_ARG_MONITOR: @@ -418,7 +434,7 @@ const MODULECMD* modulecmd_find_command(const char *domain, const char *identifi return rval; } -MODULECMD_ARG* modulecmd_arg_parse(const MODULECMD *cmd, int argc, const char **argv) +MODULECMD_ARG* modulecmd_arg_parse(const MODULECMD *cmd, int argc, const void **argv) { reset_error(); diff --git a/server/core/test/testmodulecmd.c b/server/core/test/testmodulecmd.c index 96b9fd419..a28cf4a38 100644 --- a/server/core/test/testmodulecmd.c +++ b/server/core/test/testmodulecmd.c @@ -15,7 +15,9 @@ * Test modulecmd.h functionality */ +#include #include +#include #define TEST(a, b) do{if (!(a)){printf("%s:%d "b"\n", __FILE__, __LINE__);return 1;}}while(false) @@ -32,16 +34,16 @@ bool test_fn(const MODULECMD_ARG *arg) int test_arguments() { - const char *params1[] = {"Hello", "true"}; - const char *params2[] = {"Hello", "1"}; + const void *params1[] = {"Hello", "true"}; + const void *params2[] = {"Hello", "1"}; - const char *wrong_params1[] = {"Hi", "true"}; - const char *wrong_params2[] = {"Hello", "false"}; + const void *wrong_params1[] = {"Hi", "true"}; + const void *wrong_params2[] = {"Hello", "false"}; - const char *bad_params1[] = {"Hello", "World!"}; - const char *bad_params2[] = {"Hello", NULL}; - const char *bad_params3[] = {NULL, NULL}; - const char *bad_params4[] = {NULL, "World!"}; + const void *bad_params1[] = {"Hello", "World!"}; + const void *bad_params2[] = {"Hello", NULL}; + const void *bad_params3[] = {NULL, NULL}; + const void *bad_params4[] = {NULL, "World!"}; const char *ns = "test_arguments"; const char *id = "test_arguments"; @@ -139,13 +141,13 @@ bool test_fn2(const MODULECMD_ARG *arg) int test_optional_arguments() { - const char *params1[] = {"Hello", "true"}; - const char *params2[] = {NULL, "true"}; - const char *params3[] = {"Hello", NULL}; - const char *params4[] = {NULL, NULL}; + const void *params1[] = {"Hello", "true"}; + const void *params2[] = {NULL, "true"}; + const void *params3[] = {"Hello", NULL}; + const void *params4[] = {NULL, NULL}; - const char *ns = "test_optional_arguments"; - const char *id = "test_optional_arguments"; + const void *ns = "test_optional_arguments"; + const void *id = "test_optional_arguments"; modulecmd_arg_type_t args1[] = { MODULECMD_ARG_STRING | MODULECMD_ARG_OPTIONAL, @@ -263,7 +265,7 @@ int test_map() int n = 0; TEST(modulecmd_foreach(NULL, NULL, mapfn, &n), "Mapping function should succeed"); TEST(strlen(modulecmd_get_error()) == 0, "Error message should be empty"); - TEST(n == 10, "Every registered command should be called"); + TEST(n >= 10, "Every registered command should be called"); n = 0; TEST(modulecmd_foreach("test_map", NULL, mapfn, &n), "Mapping function should succeed"); @@ -298,6 +300,57 @@ int test_map() return 0; } +static DCB my_dcb; +static SESSION my_session; + +bool ptrfn(const MODULECMD_ARG *argv) +{ + bool rval = false; + + if (argv->argc == 4 && argv->argv[0].value.dcb == &my_dcb && + argv->argv[1].value.dcb == &my_dcb && + argv->argv[2].value.session == &my_session && + argv->argv[3].value.session == &my_session) + { + rval = true; + } + + return rval; +} + +int test_pointers() +{ + const char *ns = "test_pointers"; + const char *id = "test_pointers"; + + modulecmd_arg_type_t args[] = {MODULECMD_ARG_DCB, MODULECMD_ARG_DCB_PTR, + MODULECMD_ARG_SESSION, MODULECMD_ARG_SESSION_PTR + }; + + TEST(modulecmd_register_command(ns, id, ptrfn, 4, args), "Registering a command should succeed"); + TEST(strlen(modulecmd_get_error()) == 0, "Error message should be empty"); + + const MODULECMD *cmd = modulecmd_find_command(ns, id); + TEST(cmd, "The registered command should be found"); + char dcb_str[200]; + char session_str[200]; + + sprintf(dcb_str, "%p", &my_dcb); + sprintf(session_str, "%p", &my_session); + + const void* params[] = {dcb_str, &my_dcb, session_str, &my_session}; + + MODULECMD_ARG *arg = modulecmd_arg_parse(cmd, 4, params); + TEST(arg, "Parsing arguments should succeed"); + TEST(strlen(modulecmd_get_error()) == 0, "Error message should be empty"); + + TEST(modulecmd_call_command(cmd, arg), "Module call should be successful"); + TEST(strlen(modulecmd_get_error()) == 0, "Error message should be empty"); + + modulecmd_arg_free(arg); + return 0; +} + int main(int argc, char **argv) { int rc = 0; @@ -306,6 +359,7 @@ int main(int argc, char **argv) rc += test_optional_arguments(); rc += test_module_errors(); rc += test_map(); + rc += test_pointers(); return rc; } From 3b5b616edff8363545b216330589c601a4ba3491 Mon Sep 17 00:00:00 2001 From: Markus Makela Date: Mon, 21 Nov 2016 12:54:56 +0200 Subject: [PATCH 29/29] MXS-929: Add descriptions to module command arguments Each argument now has a description describing what it does and what it's used for. --- include/maxscale/modulecmd.h | 69 +++++++++++++----- server/core/modulecmd.c | 119 ++++++++++++++++++++++++++----- server/core/test/testmodulecmd.c | 20 ++++-- 3 files changed, 168 insertions(+), 40 deletions(-) diff --git a/include/maxscale/modulecmd.h b/include/maxscale/modulecmd.h index 0c184ab1b..10f772bb8 100644 --- a/include/maxscale/modulecmd.h +++ b/include/maxscale/modulecmd.h @@ -35,26 +35,37 @@ MXS_BEGIN_DECLS /** * The argument type * - * First 8 bits are reserved for argument type, bits 9 through 32 are reserved - * for argument options and bits 33 through 64 are reserved for future use. + * First 8 bits of @c value are reserved for argument type, bits 9 through + * 32 are reserved for argument options and bits 33 through 64 are reserved + * for future use. + * + * @c description should be a human-readable description of the argument. */ -typedef uint64_t modulecmd_arg_type_t; +typedef struct +{ + uint64_t type; /**< The argument type */ + const char *description; /**< The argument description */ +} modulecmd_arg_type_t; /** * Argument types for the registered functions, the first 8 bits of - * the modulecmd_arg_type_t type. An argument can be of only one type. + * the modulecmd_arg_type_t type's @c value member. An argument can be of + * only one type. */ -#define MODULECMD_ARG_NONE 0 -#define MODULECMD_ARG_STRING 1 -#define MODULECMD_ARG_BOOLEAN 2 -#define MODULECMD_ARG_SERVICE 3 -#define MODULECMD_ARG_SERVER 4 -#define MODULECMD_ARG_SESSION 5 -#define MODULECMD_ARG_SESSION_PTR 6 -#define MODULECMD_ARG_DCB 7 -#define MODULECMD_ARG_DCB_PTR 8 -#define MODULECMD_ARG_MONITOR 9 -#define MODULECMD_ARG_FILTER 10 +#define MODULECMD_ARG_NONE 0 /**< Empty argument */ +#define MODULECMD_ARG_STRING 1 /**< String */ +#define MODULECMD_ARG_BOOLEAN 2 /**< Boolean value */ +#define MODULECMD_ARG_SERVICE 3 /**< Service name */ +#define MODULECMD_ARG_SERVER 4 /**< Server name */ +#define MODULECMD_ARG_SESSION 5 /**< SESSION pointer in string format */ +#define MODULECMD_ARG_SESSION_PTR 6 /**< Raw SESSION pointer */ +#define MODULECMD_ARG_DCB 7 /**< DCB pointer in string format*/ +#define MODULECMD_ARG_DCB_PTR 8 /**< Raw DCB pointer*/ +#define MODULECMD_ARG_MONITOR 9 /**< Monitor name */ +#define MODULECMD_ARG_FILTER 10 /**< Filter name */ +#define MODULECMD_ARG_OUTPUT 11 /**< DCB suitable for writing results to. + This should always be the first argument + if the function requires an output DCB. */ /** * Options for arguments, bits 9 through 32 @@ -64,8 +75,9 @@ typedef uint64_t modulecmd_arg_type_t; /** * Helper macros */ -#define MODULECMD_GET_TYPE(type) ((type & 0xff)) -#define MODULECMD_ARG_IS_REQUIRED(type) ((type & MODULECMD_ARG_OPTIONAL) == 0) +#define MODULECMD_GET_TYPE(t) ((t)->type & 0xff) +#define MODULECMD_ARG_IS_REQUIRED(t) (((t)->type & MODULECMD_ARG_OPTIONAL) == 0) +#define MODULECMD_ARG_PRESENT(t) (MODULECMD_GET_TYPE(t) != MODULECMD_ARG_NONE) /** Argument list node */ struct arg_node @@ -158,6 +170,18 @@ MODULECMD_ARG* modulecmd_arg_parse(const MODULECMD *cmd, int argc, const void ** */ void modulecmd_arg_free(MODULECMD_ARG *arg); +/** + * @brief Check if an optional argument was defined + * + * This function looks the argument list @c arg at an offset of @c idx and + * checks if the argument list contains a value for an optional argument. + * + * @param arg Argument list + * @param idx Index of the argument, starts at 0 + * @return True if the optional argument is present + */ +bool modulecmd_arg_is_present(const MODULECMD_ARG *arg, int idx); + /** * @brief Call a registered command * @@ -216,4 +240,15 @@ const char* modulecmd_get_error(); bool modulecmd_foreach(const char *domain_re, const char *ident_re, bool(*fn)(const MODULECMD *cmd, void *data), void *data); +/** + * @brief Return argument type as string + * + * This returns the string type of @c type. The returned string must be freed + * by the called. + * + * @param type Type to convert + * @return New string or NULL on memory allocation error + */ +char* modulecmd_argtype_to_str(modulecmd_arg_type_t *type); + MXS_END_DECLS diff --git a/server/core/modulecmd.c b/server/core/modulecmd.c index fca0bbd84..b3e3c870c 100644 --- a/server/core/modulecmd.c +++ b/server/core/modulecmd.c @@ -147,7 +147,7 @@ static MODULECMD* command_create(const char *identifier, const char *domain, for (int i = 0; i < argc; i++) { - if (MODULECMD_ARG_IS_REQUIRED(argv[i])) + if (MODULECMD_ARG_IS_REQUIRED(&argv[i])) { argc_min++; } @@ -197,14 +197,14 @@ static bool domain_has_command(MODULECMD_DOMAIN *dm, const char *id) return false; } -static bool process_argument(modulecmd_arg_type_t type, const void* value, +static bool process_argument(modulecmd_arg_type_t *type, const void* value, struct arg_node *arg, const char **err) { bool rval = false; if (!MODULECMD_ARG_IS_REQUIRED(type) && value == NULL) { - arg->type = MODULECMD_ARG_NONE; + arg->type.type = MODULECMD_ARG_NONE; rval = true; } else if (value) @@ -212,14 +212,14 @@ static bool process_argument(modulecmd_arg_type_t type, const void* value, switch (MODULECMD_GET_TYPE(type)) { case MODULECMD_ARG_NONE: - arg->type = MODULECMD_ARG_NONE; + arg->type.type = MODULECMD_ARG_NONE; rval = true; break; case MODULECMD_ARG_STRING: if ((arg->value.string = MXS_STRDUP((char*)value))) { - arg->type = MODULECMD_ARG_STRING; + arg->type.type = MODULECMD_ARG_STRING; rval = true; } else @@ -234,7 +234,7 @@ static bool process_argument(modulecmd_arg_type_t type, const void* value, if (truthval != -1) { arg->value.boolean = truthval; - arg->type = MODULECMD_ARG_BOOLEAN; + arg->type.type = MODULECMD_ARG_BOOLEAN; rval = true; } else @@ -247,7 +247,7 @@ static bool process_argument(modulecmd_arg_type_t type, const void* value, case MODULECMD_ARG_SERVICE: if ((arg->value.service = service_find((char*)value))) { - arg->type = MODULECMD_ARG_SERVICE; + arg->type.type = MODULECMD_ARG_SERVICE; rval = true; } else @@ -259,7 +259,7 @@ static bool process_argument(modulecmd_arg_type_t type, const void* value, case MODULECMD_ARG_SERVER: if ((arg->value.server = server_find_by_unique_name((char*)value))) { - arg->type = MODULECMD_ARG_SERVER; + arg->type.type = MODULECMD_ARG_SERVER; rval = true; } else @@ -269,25 +269,25 @@ static bool process_argument(modulecmd_arg_type_t type, const void* value, break; case MODULECMD_ARG_SESSION: - arg->type = MODULECMD_ARG_SESSION; + arg->type.type = MODULECMD_ARG_SESSION; arg->value.session = (SESSION*)strtol((char*)value, NULL, 0); rval = true; break; case MODULECMD_ARG_SESSION_PTR: - arg->type = MODULECMD_ARG_SESSION_PTR; + arg->type.type = MODULECMD_ARG_SESSION_PTR; arg->value.session = (SESSION*)value; rval = true; break; case MODULECMD_ARG_DCB: - arg->type = MODULECMD_ARG_DCB; + arg->type.type = MODULECMD_ARG_DCB; arg->value.dcb = (DCB*)strtol((char*)value, NULL, 0); rval = true; break; case MODULECMD_ARG_DCB_PTR: - arg->type = MODULECMD_ARG_DCB_PTR; + arg->type.type = MODULECMD_ARG_DCB_PTR; arg->value.dcb = (DCB*)value; rval = true; break; @@ -295,7 +295,7 @@ static bool process_argument(modulecmd_arg_type_t type, const void* value, case MODULECMD_ARG_MONITOR: if ((arg->value.monitor = monitor_find((char*)value))) { - arg->type = MODULECMD_ARG_MONITOR; + arg->type.type = MODULECMD_ARG_MONITOR; rval = true; } else @@ -307,7 +307,7 @@ static bool process_argument(modulecmd_arg_type_t type, const void* value, case MODULECMD_ARG_FILTER: if ((arg->value.filter = filter_find((char*)value))) { - arg->type = MODULECMD_ARG_FILTER; + arg->type.type = MODULECMD_ARG_FILTER; rval = true; } else @@ -316,9 +316,15 @@ static bool process_argument(modulecmd_arg_type_t type, const void* value, } break; + case MODULECMD_ARG_OUTPUT: + arg->type.type = MODULECMD_ARG_OUTPUT; + arg->value.dcb = (DCB*)value; + rval = true; + break; + default: ss_dassert(false); - MXS_ERROR("Undefined argument type: %0lx", type); + MXS_ERROR("Undefined argument type: %0lx", type->type); *err = "internal error"; break; } @@ -353,7 +359,7 @@ static MODULECMD_ARG* modulecmd_arg_create(int argc) static void free_argument(struct arg_node *arg) { - switch (arg->type) + switch (arg->type.type) { case MODULECMD_ARG_STRING: MXS_FREE(arg->value.string); @@ -451,7 +457,7 @@ MODULECMD_ARG* modulecmd_arg_parse(const MODULECMD *cmd, int argc, const void ** { const char *err = ""; - if (!process_argument(cmd->arg_types[i], argv[i], &arg->argv[i], &err)) + if (!process_argument(&cmd->arg_types[i], argv[i], &arg->argv[i], &err)) { error = true; modulecmd_set_error("Argument %d, %s: %s", i + 1, err, argv[i] ? argv[i] : "No argument given"); @@ -580,3 +586,82 @@ bool modulecmd_foreach(const char *domain_re, const char *ident_re, spinlock_release(&modulecmd_lock); return rval; } + +char* modulecmd_argtype_to_str(modulecmd_arg_type_t *type) +{ + const char *strtype = "UNKNOWN"; + + switch (MODULECMD_GET_TYPE(type)) + { + case MODULECMD_ARG_NONE: + strtype = "NONE"; + break; + + case MODULECMD_ARG_STRING: + strtype = "STRING"; + break; + + case MODULECMD_ARG_BOOLEAN: + strtype = "BOOLEAN"; + break; + + case MODULECMD_ARG_SERVICE: + strtype = "SERVICE"; + break; + + case MODULECMD_ARG_SERVER: + strtype = "SERVER"; + break; + + case MODULECMD_ARG_SESSION: + strtype = "SESSION"; + break; + + case MODULECMD_ARG_SESSION_PTR: + strtype = "SESSION_PTR"; + break; + + case MODULECMD_ARG_DCB: + strtype = "DCB"; + break; + + case MODULECMD_ARG_DCB_PTR: + strtype = "DCB_PTR"; + break; + + case MODULECMD_ARG_MONITOR: + strtype = "MONITOR"; + break; + + case MODULECMD_ARG_FILTER: + strtype = "FILTER"; + break; + + case MODULECMD_ARG_OUTPUT: + strtype = "OUTPUT"; + break; + + default: + ss_dassert(false); + MXS_ERROR("Unknown type"); + break; + } + + size_t slen = strlen(strtype); + size_t extra = MODULECMD_ARG_IS_REQUIRED(type) ? 0 : 2; + char *rval = MXS_MALLOC(slen + extra + 1); + + if (rval) + { + const char *fmtstr = extra ? "[%s]" : "%s"; + sprintf(rval, fmtstr, strtype); + } + + return rval; +} + +bool modulecmd_arg_is_present(const MODULECMD_ARG *arg, int idx) +{ + return arg->argc > idx && + MODULECMD_GET_TYPE(&arg->argv[idx].type) != MODULECMD_ARG_NONE; +} diff --git a/server/core/test/testmodulecmd.c b/server/core/test/testmodulecmd.c index a28cf4a38..6b6628e2f 100644 --- a/server/core/test/testmodulecmd.c +++ b/server/core/test/testmodulecmd.c @@ -47,7 +47,11 @@ int test_arguments() const char *ns = "test_arguments"; const char *id = "test_arguments"; - modulecmd_arg_type_t args1[] = {MODULECMD_ARG_STRING, MODULECMD_ARG_BOOLEAN}; + modulecmd_arg_type_t args1[] = + { + {MODULECMD_ARG_STRING, ""}, + {MODULECMD_ARG_BOOLEAN, ""} + }; TEST(strlen(modulecmd_get_error()) == 0, "Error message should be empty"); @@ -150,8 +154,8 @@ int test_optional_arguments() const void *id = "test_optional_arguments"; modulecmd_arg_type_t args1[] = { - MODULECMD_ARG_STRING | MODULECMD_ARG_OPTIONAL, - MODULECMD_ARG_BOOLEAN | MODULECMD_ARG_OPTIONAL + {MODULECMD_ARG_STRING | MODULECMD_ARG_OPTIONAL, ""}, + {MODULECMD_ARG_BOOLEAN | MODULECMD_ARG_OPTIONAL, ""} }; TEST(modulecmd_register_command(ns, id, test_fn2, 2, args1), @@ -323,9 +327,13 @@ int test_pointers() const char *ns = "test_pointers"; const char *id = "test_pointers"; - modulecmd_arg_type_t args[] = {MODULECMD_ARG_DCB, MODULECMD_ARG_DCB_PTR, - MODULECMD_ARG_SESSION, MODULECMD_ARG_SESSION_PTR - }; + modulecmd_arg_type_t args[] = + { + {MODULECMD_ARG_DCB, ""}, + {MODULECMD_ARG_DCB_PTR, ""}, + {MODULECMD_ARG_SESSION, ""}, + {MODULECMD_ARG_SESSION_PTR, ""} + }; TEST(modulecmd_register_command(ns, id, ptrfn, 4, args), "Registering a command should succeed"); TEST(strlen(modulecmd_get_error()) == 0, "Error message should be empty");