[Maria-developers] Review: bb-10.8-wlad-MDEV-26713

Hi Wlad, You've done a great piece of work. Thanks and congrats. I generally like all the changes. Have a few comments only, please see below. commit 1411d5f40ceaf2eecabbcf68588743439003f4a2 Author: Vladislav Vaintroub Date: Fri Nov 19 11:42:46 2021 +0100 Windows : incorrect handling of non-ASCIIs in get_tty_password Why no MDEV? + wchar_t wbuf[80]; + char to[80*3]; /* there is at most 3 bytes per wchar, in any codepage */ + wchar_t *pos=wbuf,*end=wbuf + array_elements(wbuf)-1; Isn't it "at most 4 bytes per char"? ############################################################# commit 7af691ca11c95a385aff4889a56870d70843e78d Author: Vladislav Vaintroub Date: Fri Nov 19 12:03:48 2021 +0100 MDEV-27090: Windows client - ReadConsoleA does not work correctly with UTF8 codepage Corresponding Windows bug https://github.com/microsoft/terminal/issues/4551 Use ReadConsoleW instead and convert to console's input codepage, to workaround. Also, disable VT sequences in the console output, as we do not knows what type of data comes with SELECT, we do not want VT escapes there. Remove my_cgets() + while (nchars > 0) + { + if (wstrbuf[nchars - 1] != '\n' && wstrbuf[nchars - 1] != '\r') + break; + wstrbuf[--nchars]= 0; + } + + wstrbuf[nchars]= 0; The inner assignment wstrbuf[--nchars]=0 inside every iteration is not needed. I'd write it this way: for ( ; nchars > 0 ; nchars--) { if (wstrbuf[nchars - 1] != '\n' && wstrbuf[nchars - 1] != '\r') break; } wstrbuf[nchars]= 0; But the outer assignment to 0 does not seem to be needed either. Why do it? my_convert() does not need the trailing zero. It uses only sizes passed. So this should be enough: + strbuf[len]= 0; - pthread_auto_mutex_decl(my_conio_cs); - - /* lock the console for the current process*/ - if (pthread_auto_mutex_lock(my_conio_cs, GetCurrentProcessId(), INFINITE)) - { There was a mutex lock inside my_cgets(). Why was it needed before the patch and why is it not needed any more? ##################################################### commit 5c09e9ce4f922fe9674f3f85329742c0c5498a28 Author: Vladislav Vaintroub Date: Fri Nov 19 14:03:51 2021 +0100 MDEV-27092 Windows - services that have non-ASCII characters do not work with activeCodePage=UTF8 CreateServiceA, OpenServiceA, and couple of other functions do not work correctly with non-ASCII character, in the special case where application has defined activeCodePage=UTF8. ... - Copyright (c) 2011, 2012, Monty Program Ab + Copyright (c) 2011, 2021 Monty Program Ab MariaDB Corporation? This code: + AWSTRDUP(w_ServiceName,lpServiceName); + AWSTRDUP(w_DisplayName,lpDisplayName); + AWSTRDUP(w_BinaryPathName, lpBinaryPathName); + AWSTRDUP(w_LoadOrderGroup, lpLoadOrderGroup); + AWSTRDUP(w_Dependencies, lpDependencies); + AWSTRDUP(w_ServiceStartName, lpServiceStartName); + AWSTRDUP(w_Password, lpPassword); + + free(w_ServiceName); + free(w_DisplayName); + free(w_BinaryPathName); + free(w_LoadOrderGroup); + free(w_Dependencies); + free(w_ServiceStartName); + free(w_Password); duplicates two times. Consider wrapping these variables into a class and reuse the class in both cases. ########################################################## commit d4056a28ec5ae0c9a0c3144a8e90357dc574723f Author: Vladislav Vaintroub Date: Fri Nov 19 14:14:38 2021 +0100 MDEV-27093 Do not pass root password in clear text from mariadb-install-db.exe to bootstrap Pass the hash instead, so that bootstrap would not misinterpret it for string with different charset. The password was passed using hex encoding. But the comment says it was clear text. Wrong comment? I did not understand the change. Reading the MDEV did not make it clearer. What was wrong? How does the patch fix the problem? Can we discuss on slack? ######################################################## commit fa29a64de918d2045df3cce1d4a546de5597c0bd Author: Vladislav Vaintroub Date: Mon Nov 22 12:29:15 2021 +0100 MDEV-26713 set activeCodePage=UTF8 for windows programs - Use corresponding entry in the manifest, as described in https://docs.microsoft.com/en-us/windows/apps/design/globalizing/use-utf8-co... - Set console codepage(s) to UTF8, if ANSI codepage is UTF8 This effectively causes UTF8 to be used as default client encoding, for Windows 1903 and later - Allow UTF8 on the command line for MTR, for the affected Windows versions Looks OK for me. ######################################################## commit b88d54132369b938ba041c0d3f97a5a212210b37 Author: Vladislav Vaintroub Date: Mon Nov 22 12:34:10 2021 +0100 MDEV-26713 : Treat codepage 65001 as utf8mb4, not just utf8 Also, fix the "UTF8" option in MSI, which is responsible for character-set-server setting Looks OK for me. ######################################################## commit b9c38470ed6e04185f7f84a056034b7254336d51 Author: Vladislav Vaintroub Date: Mon Nov 22 13:22:05 2021 +0100 MDEV-26713 utf8 support on Windows, mysql_upgrade_service part Handle upgrade - on Windows that is capable of UTF8 codepage, convert entries in my.ini from ANSI to UTF8, during upgrade Reason is that paths must now be utf8. For the client , user name, database etc should be in UTF8, too, as this is now the default charset. Should not we add an UTF8 BOM to the beginning of the file, so text editors do not break it later. The patch looks too large for me. It seems it does more than the comments above tell. Looks like convenience refactoring, but it's hard to follow. It would be easier to understand changes if you split it into two parts: 1. the refactoring that adds functions like upgrade_config_file(), fix_section(), is_mariadb_section(), fix_and_check_datadir(), I guess this refactoring is a non-functional change that should not change the behaviour, right? 2. the actual conversion part on top of it ######################################################## commit 0700393b570c6b9a7b4347d3381a7cf05d1499ba Author: Vladislav Vaintroub Date: Tue Nov 23 12:28:28 2021 +0100 mysqltest, Windows - support background execution in 'exec' command If last character in command is ampersand, do not use my_popen/my_pclose in "exec", instead do CreateProcess() without a window. I suggest you add an MDEV for this change and explain in there why we did not need this feature earlier and why we need it now and how it relates to MDEV-26713. ######################################################## commit 7d50d83b3bcab5a3be761f98722e33ed0b01ab23 Author: Vladislav Vaintroub Date: Tue Nov 23 13:05:25 2021 +0100 MDEV-26713 utf8 support on Windows , add mysql_install_db tests Add mysql_install_db test with some i18n, for data dir and root password Looks OK for me. ######################################################## commit b2cc09fe76f526bd4cd76131a52263b13b3828a5 Author: Vladislav Vaintroub Date: Wed Nov 24 10:15:11 2021 +0100 MDEV-26713 Add test for mysql_install_db creating service, with i18 Looks OK for me. ######################################################## commit 51ce069376fec45d340563a4f7630f8a155afd64 Author: Vladislav Vaintroub Date: Fri Nov 26 18:41:35 2021 +0100 MDEV-26713 Introduce --password-codepage client setting I'm not sure we need this new options. Looks like over-engineering a bit. Is the scenario it covers really deserves documenting, learning, etc? I'd better disallow non-ASCII passwords in the future, and right now just add a warning: You're using a non-ASCII password, which is not portable. If you have problems connecting to the database, please ask the administrator to change the password. ######################################################## commit b155c4036cfb3ffdaa7c272048ac9d32e3182d78 Author: Vladislav Vaintroub Date: Mon Nov 29 19:47:36 2021 +0100 MDEV-26713 set console codepage to what user set in --default-character-set I prefer to keep the comment that you removed: - if (!fs_cset_cache) - { - char buf[10]= "cp"; - GetLocaleInfo(LOCALE_SYSTEM_DEFAULT, LOCALE_IDEFAULTANSICODEPAGE, - buf+2, sizeof(buf)-3); - /* - We cannot call get_charset_by_name here - because fs_character_set() is executed before - LOCK_THD_charset mutex initialization, which - is used inside get_charset_by_name. - As we're now interested in cp932 only, - let's just detect it using strcmp(). - */ - fs_cset_cache= - #ifdef HAVE_CHARSET_cp932 - !strcmp(buf, "cp932") ? &my_charset_cp932_japanese_ci : - #endif - &my_charset_bin; This function uses both default_charset and charset_info->cs_name.str: +static void adjust_console_codepage() +{ +#ifdef _WIN32 + const char *name= charset_info->cs_name.str; + if (my_set_console_cp(default_charset) < 0) And it seems the former is set to the latter just before the function call: default_charset= (char *)charset_info->cs_name.str; <------ HERE put_info("Charset changed", INFO_INFO); adjust_console_codepage(); I find this hard to follow. Can you please add a parameter to adjust_console_codepage() and pass either charset_info->cs_name.str (or default_charset) when calling? Also, I think this function should only be called when default_charset was some explicit name, not MYSQL_AUTODETECT_CHARSET_NAME. I think the logic should be: 1. If --default-character-set=auto, then detect it from the console. 2. If --default-character-set=csname, then set the console CP to csname. Now in case of N1 it seems to detect the character set from the console, then sets the console to what has been detected. Is the last step really needed in case of auto-detection? ######################################################## commit b38c341cc392630c798eb754f87338d37ec95cdf Author: Vladislav Vaintroub Date: Sun Dec 5 17:08:18 2021 +0100 Fix race condition in test. Do not change console codepage in a possibly shared console. Start new console for chcp. Can you extend the comment and tell what actually happened before this change? Greetings.