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
_______________________________________________
developers mailing list -- developers@lists.mariadb.org
To unsubscribe send an email to developers-le...@lists.mariadb.org
