Hi, Yuchen, Right. This is pretty much the whole feature. Mostly ok, few minor comments below. On Sep 08, Yuchen Pei wrote:
revision-id: 8ac17273b14 (mariadb-11.0.1-104-g8ac17273b14) parent(s): 365faf7229f author: Yuchen Pei committer: Yuchen Pei timestamp: 2023-09-08 14:46:50 +1000 message:
MDEV-15935 Adding global/session system var redirect_url
Adding a global/session var `redirect_url' of string type. The initial value is empty. Can be supplied in mysqld with --redirect-url or set in --init-connect. A valid redirect_url should be of the format
{mysql,mariadb}://host[:port]
where <host> is an arbitrary string not containing colons, and <port> is a number between 0 and 65535 inclusive.
The variable will be used by the server to notify clients that they should connect to another server, specified by the value of the variable, if not empty.
The notification is done by the inclusion of the variable in session_track_system_variable.
diff --git a/mysql-test/suite/sys_vars/r/sysvars_server_embedded.result b/mysql-test/suite/sys_vars/r/sysvars_server_embedded.result index fa3bf6b4354..85dc924fe5f 100644 --- a/mysql-test/suite/sys_vars/r/sysvars_server_embedded.result +++ b/mysql-test/suite/sys_vars/r/sysvars_server_embedded.result @@ -3232,6 +3232,16 @@ NUMERIC_BLOCK_SIZE 1 ENUM_VALUE_LIST NULL READ_ONLY NO COMMAND_LINE_ARGUMENT REQUIRED +VARIABLE_NAME REDIRECT_URL +VARIABLE_SCOPE SESSION +VARIABLE_TYPE VARCHAR +VARIABLE_COMMENT URL to redirect to, if not empty
see if you can make it a bit more descriptive. if not, ok as is. hmm, and a second thought. it's in the sysvars_server_embedded test. it doesn't make sense there (embedded doesn't accept connections, and doesn't have session tracking, iirc), so, may be hide it under #ifndef EMBEDDED_LIBRARY ?
+NUMERIC_MIN_VALUE NULL +NUMERIC_MAX_VALUE NULL +NUMERIC_BLOCK_SIZE NULL +ENUM_VALUE_LIST NULL +READ_ONLY NO +COMMAND_LINE_ARGUMENT REQUIRED VARIABLE_NAME REQUIRE_SECURE_TRANSPORT VARIABLE_SCOPE GLOBAL VARIABLE_TYPE BOOLEAN diff --git a/sql/sql_class.h b/sql/sql_class.h index cf2b5b96a22..c86a76f6a08 100644 --- a/sql/sql_class.h +++ b/sql/sql_class.h @@ -878,6 +878,7 @@ typedef struct system_variables
Time_zone *time_zone; char *session_track_system_variables; + char *redirect_url;
/* Some wsrep variables */ ulonglong wsrep_trx_fragment_size; diff --git a/sql/sql_plugin.cc b/sql/sql_plugin.cc index 4724e2200e6..70aba9cc4d5 100644 --- a/sql/sql_plugin.cc +++ b/sql/sql_plugin.cc @@ -3307,6 +3309,10 @@ void plugin_thdvar_init(THD *thd) #ifndef EMBEDDED_LIBRARY thd->session_tracker.sysvars.init(thd); #endif + thd->variables.redirect_url= + my_strdup(PSI_INSTRUMENT_ME, global_system_variables.redirect_url,
and here, key_memory_Sys_var_charptr_value
+ MYF(MY_WME | MY_THREAD_SPECIFIC)); + DBUG_VOID_RETURN; }
@@ -3375,6 +3381,8 @@ void plugin_thdvar_cleanup(THD *thd) #ifndef EMBEDDED_LIBRARY thd->session_tracker.sysvars.deinit(thd); #endif + my_free(thd->variables.redirect_url); + thd->variables.redirect_url= 0;
also, perhaps in cleanup_variables() ?
my_free((char*) thd->variables.default_master_connection.str); thd->variables.default_master_connection.str= 0; thd->variables.default_master_connection.length= 0; diff --git a/sql/sys_vars.cc b/sql/sys_vars.cc index 064c60ab2b0..a93e68f40d7 100644 --- a/sql/sys_vars.cc +++ b/sql/sys_vars.cc @@ -1343,6 +1343,61 @@ Sys_init_connect( DEFAULT(""), &PLock_sys_init_connect, NOT_IN_BINLOG, ON_CHECK(check_init_string));
+/** + Validate a redirect_url string. + + A valid string is either empty, or of the format mysql://host[:port], + where host is an arbitrary string without any colon ':'. + + @param str A string to validate + @param len Length of the string + @retval false The string is valid + @retval true The string is invalid +*/ +static bool sysvar_validate_redirect_url(sys_var *, THD *, set_var *var) +{ + char *str= var->save_result.string_value.str; + size_t len= var->save_result.string_value.length; + /* Empty string is valid */ + if (len == 0) + return false; + const char* end= str + len; + if (!strncmp(str, "mysql://", strlen("mysql://")))
use !strncmp(str, STRING_WITH_LEN("mysql://")) avoids no strlen and no need to write the string twice. same below.
+ str+= strlen("mysql://");
ah. Then, I'd suggest instead LEX_CSTRING mysql_prefix={STRING_WITH_LEN("mysql://")}; if (!strncmp(str, mysql_prefix.str, mysql_prefix.length)) str+= mysql_prefix.length;
+ else if (!strncmp(str, "mariadb://", strlen("mariadb://"))) + str+= strlen("mariadb://"); + else + return true; + /* Host name cannot be empty */ + if (str == end) + return true; + /* Find the colon, if any */ + while (str < end && *str != ':') + str++; + /* Found colon */ + if (str < end) + { + /* Should have at least one number after the colon */ + if (str + 1 == end) + return true; + int p= 0; + while (str < end && isdigit(*++str)) + p= p * 10 + (*str - '0'); + /* Should be all numbers after the colon */ + if (str < end) + return true; + /* The port number should be between 0 and 65535 */ + if (p > 65535) + return true;
check for overflow too. you can either put the p>65535 check inside the loop or at the end check that, like, (str - colon_ptr) < 7.
+ } + return false; +} + +static Sys_var_charptr Sys_redirect_url( + "redirect_url", "URL to redirect to, if not empty", + SESSION_VAR(redirect_url), CMD_LINE(REQUIRED_ARG), DEFAULT(""), + NO_MUTEX_GUARD, NOT_IN_BINLOG, sysvar_validate_redirect_url, 0, 0);
By convention, write ON_CHECK(sysvar_validate_redirect_url). And you don't need 0's at the end, so just NO_MUTEX_GUARD, NOT_IN_BINLOG, ON_CHECK(sysvar_validate_redirect_url)); Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org