← Back to team overview

maria-developers team mailing list archive

Re: [Commits] cae1945: MDEV-9117: Client Server capability negotiation for MariaDB specific functionality

 

On 17.01.2016 11:14, Sergei Golubchik wrote:
Hi, Oleksandr!

On Jan 17, Oleksandr Byelkin wrote:
On 16.01.2016 21:56, Sergei Golubchik wrote:
On Nov 25, OleksandrByelkin wrote:
diff --git a/include/mysql_com.h b/include/mysql_com.h
index aa6ab0f..57092dc 100644
--- a/include/mysql_com.h
+++ b/include/mysql_com.h
@@ -215,7 +216,8 @@ enum enum_server_command
   /* Don't close the connection for a connection with expired password. */
   #define CLIENT_CAN_HANDLE_EXPIRED_PASSWORDS (1UL << 22)
-#define CLIENT_PROGRESS (1UL << 29) /* Client support progress indicator */
+#define CLIENT_PROGRESS_OBSOLETE  (1UL << 29)
ok

+#define CLIENT_PROGRESS           0
why is that?
Because old flag will not work with the new server.
Right, but for binaries (already compiled libraries and clients), the
flag is (1UL << 29) - I mean, the number, not the "CLIENT_PROGRESS"
name. And I suggest it to continue to work, as I wrote in the review,
with something like

   if (client_capabilities & CLIENT_PROGRESS_OBSOLETE)
     client_capabilities|= MARIADB_CLIENT_PROGRESS;

As for the *name* CLIENT_PROGRESS - it only affects clients/libraries
that are compiled with the new MariaDB. And I'd rather see a compilation
failure in this case ("unknown symbol CLIENT_PROGRESS") than a silently
disabled feature.

So, I think, you don't need to keep old CLIENT_PROGRESS symbol, you can
safely remove it.
OK (but check last comment please).
+/* Client support mariaDB extended flags */
+#define MARIADB_CLIENT_EXTENDED_FLAGS (1ULL << 63)
how can this work, if client_flag and server_capabilities are
only 32-bit variables?
it is not anymore (on the server) on non-connector client it will be
lost but who cares if it doesnot support it ((I removed support by your
request).
Oh, yes, right. This is only the server part of the patch.
Sorry.

diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc
index 1240d5d..d07e0a4 100644
--- a/sql/sql_acl.cc
+++ b/sql/sql_acl.cc
@@ -11729,18 +11731,38 @@ static ulong parse_client_handshake_packet(MPVIO_EXT *mpvio,
+    if (!(client_capabilities & CLIENT_MYSQL))
+    {
+      // it is client with mariadb extensions
+      client_capabilities|= CLIENT_MYSQL;
Wait, why is this?
Jast to return it "as it was" if you think that it is not needed then I remove it.

+      ulonglong ext_client_capabilities=
+        (((ulonglong)uint4korr(net->read_pos + 28)) << 32);
+      if (ext_client_capabilities & MARIADB_CLIENT_EXTENDED_FLAGS)
+        client_capabilities|= ext_client_capabilities;
+      else
+      {
+        DBUG_PRINT("error", ("CLIENT_PROTOCOL_41: on, "
+                             "CLIENT_LONG_PASSWORD/CLIENT_MYSQL off, "
+                             "but MARIADB_CLIENT_EXTENDED_FLAGS is off. "
+                             "flags: %llx ext flags %llx",
+                             client_capabilities, ext_client_capabilities));
+        return packet_error;
Why do you need that (MARIADB_CLIENT_EXTENDED_FLAGS and the check)?
Server read or do not read more flags depend on it.
But !(client_capabilities & CLIENT_MYSQL) already tells you that.
It's a MariaDB client = it has extended flags.
As you saw it was not.

diff --git a/tests/mysql_client_test.c b/tests/mysql_client_test.c
index a1a52e8..31fca0f 100644
--- a/tests/mysql_client_test.c
+++ b/tests/mysql_client_test.c
@@ -15336,7 +15336,7 @@ static void test_mysql_insert_id()
myheader("test_mysql_insert_id"); - rc= mysql_query(mysql, "drop table if exists t1");
+  rc= mysql_query(mysql, "drop table if exists t1,t2");
separate commit, please
ok, but it looks too late now, sorry.
If you haven't pushed to 10.2 yet, it's not late.
I'll ping you on irc and explain how to do it.
I'll check what is pushed and what is not.

     myquery(rc);
     /* table without auto_increment column */
     rc= mysql_query(mysql, "create table t1 (f1 int, f2 varchar(255), key(f1))");
@@ -18700,7 +18700,8 @@ static void test_progress_reporting()
     myheader("test_progress_reporting");
conn= client_connect(CLIENT_PROGRESS, MYSQL_PROTOCOL_TCP, 0);
-  DIE_UNLESS(conn->client_flag & CLIENT_PROGRESS);
+  if (!(conn->client_flag & CLIENT_PROGRESS))
+    return;
what are you testing here?
yes, I'll fix it.
I was only asking a question :)
I don't know what "it" is and what's wrong with it.
wrong (as I thought before) was that CLIENT_PROGRESS is defined as 0 now.

OK, maybe after some thinking: it is why I made CLIENT_PROGRESS defined as 0 old clients thinks that server just not support progress if it is not rewritten for new flags support.

Regards,
Sergei
Chief Architect MariaDB
and security@xxxxxxxxxxx



Follow ups

References