Hi Sergei, Thanks for the comments. Please see below my replies, and the JIRA ticket for the updated commits. On Fri 2023-09-08 21:15:47 +0200, Sergei Golubchik wrote:
[... 31 lines elided]
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.
Done.
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 ?
Done. Moved it to before the session track group.
[... 19 lines elided]
/* 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
Done.
[... 5 lines elided]
@@ -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() ?
See my reply to the same comment in the review of the other commit.
[... 23 lines elided]
+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;
Done.
+ 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.
Done.
+ } + 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));
Done.
[... 5 lines elided]
Best, Yuchen