maria-developers team mailing list archive

[Alexander Barkov <bar@xxxxxxxxxxx>]

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 <wlad@xxxxxxxxxxx>
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 <wlad@xxxxxxxxxxx>
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 <wlad@xxxxxxxxxxx>
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 <wlad@xxxxxxxxxxx>
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 <wlad@xxxxxxxxxxx>
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-code-page

    - 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 <wlad@xxxxxxxxxxx>
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 <wlad@xxxxxxxxxxx>
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 <wlad@xxxxxxxxxxx>
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 <wlad@xxxxxxxxxxx>
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 <wlad@xxxxxxxxxxx>
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 <wlad@xxxxxxxxxxx>
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 <wlad@xxxxxxxxxxx>
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 <wlad@xxxxxxxxxxx>
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.