Re: bk commit into 6.0 tree (cmiller:1.2703) BUG#13174

[Magnus Svensson]

cmiller@stripped skrev:
> Quoting Magnus Svensson <msvensson@stripped>:
> 
>> -----BEGIN PGP SIGNED MESSAGE-----
>> Hash: SHA1
>>
>> Hi Chad,
>>
>> see my comments marked with "magnus:" below.
> 
> Thanks.  Three answers below. "chad-reply0:"
> 
>>
>>
>> Chad MILLER wrote:
> [...]
>>> diff -Nrup a/include/mysql.h b/include/mysql.h
>>> --- a/include/mysql.h    2007-08-03 10:54:34 -04:00
>>> +++ b/include/mysql.h    2007-12-03 12:39:54 -05:00
>>> @@ -177,11 +177,13 @@ struct st_mysql_options {
>>>    char *host,*user,*password,*unix_socket,*db;
>>>    struct st_dynamic_array *init_commands;
>>>    char *my_cnf_file,*my_cnf_group, *charset_dir, *charset_name;
>>> +#if defined(HAVE_OPENSSL) && !defined(EMBEDDED_LIBRARY)
>>>    char *ssl_key;                /* PEM key file */
>>>    char *ssl_cert;                /* PEM cert file */
>>>    char *ssl_ca;                    /* PEM CA file */
>>>    char *ssl_capath;                /* PEM directory of CA-s? */
>>>    char *ssl_cipher;                /* cipher to use */
>>> +#endif
>>
>> magnus: No, you can't have ifdefs in these structs since it's part of
>> our external interface. It should not change depending on how you
>> compiled the lib or the client application.
> 
> chad-reply0: Uh oh.  And there are already an #ifdef in it:
> #if !defined(CHECK_EMBEDDED_DIFFERENCES) || defined(EMBEDDED_LIBRARY)
>   my_bool separate_thread;
> #endif
> I removed mine.

magnus1: Yes, I've seen that one. Who knows what it does? ;)

> 
> 
>>> diff -Nrup a/mysql-test/t/openssl_1.test b/mysql-test/t/openssl_1.test
>>> --- a/mysql-test/t/openssl_1.test    2007-04-11 17:43:56 -04:00
>>> +++ b/mysql-test/t/openssl_1.test    2007-12-03 12:39:54 -05:00
>>> @@ -2,6 +2,7 @@
>>>  # with support for SSL.
>>>
>>>  -- source include/have_ssl.inc
>>> +-- source include/not_embedded.inc
>>
>> magnus: But why does the embedded server says it has SSL support?
>>
>> The include/have_openssl.inc would ask the server if it has SSL and
>> shouldn't it say no?
>>
>> Maybe we need to split the check for this feature in two. One that is
>> "ssl_connections" and the other for "ssl_functions"(or 
>> "crypto_functions"?
>>
>> Just think about it, does not block you patch.
>>
> 
> chad-reply0: Yes, I also prefer the idea of testing for functionality 
> instead of for the existence of a library.  I don't see what you're 
> recommending here and earlier with catching warnings from  SHAnnn()  
> though.

magnus1:
If you run
mysql> select SHA2(...);
against a server that hasn't got support for SHA2, it will return NULL 
and lso produce warnings saying that it's not supported(you need to 
./configure --with-ssl).

Those warnings could be checked in the test as an indication that the 
functionality is not supported and thus the particular test should be 
skipped.

If you look at mysql-test/t/func_encrypt.test we have the same situation 
  where we look at "have_ssl" instead of testing for the existence of 
the particular function.

Like below:

if (!`select des_encrypt("test")`){
   if (`select @@warning_count > 0`){
     # Make sure it's the correct warning
     let $code= query_get_value(show warnings, Code, 1);
     if (`select $code != 1289`){
       die wrong warning code after des_encrypt=NULL;
     }

     # Could not use des_encrypt, check that des_decrypt
     # can't be used either
     if (`select des_decrypt("test")`){
       die could use des_decrypt;
     }

     if (!`select @@warning_count`){
       die did not get any warning when using des_decrypt;
     }
     # Make sure it's the correct warning
     let $code= query_get_value(show warnings, Code, 1);
     if (`select $code != 1289`){
       die wrong warning code after des_decrypt=NULL;
     }

     skip Need des_encrypt/des_decrypt;
   }
}



> 
>>> diff -Nrup a/sql/sql_connect.cc b/sql/sql_connect.cc
>>> --- a/sql/sql_connect.cc    2007-11-14 11:29:58 -05:00
>>> +++ b/sql/sql_connect.cc    2007-12-03 12:39:55 -05:00
>>> @@ -20,7 +20,7 @@
>>>
>>>  #include "mysql_priv.h"
>>>
>>> -#ifdef HAVE_OPENSSL
>>> +#if defined(HAVE_OPENSSL) && !defined(EMBEDDED_LIBRARY)
>>>  /*
>>>    Without SSL the handshake consists of one packet. This packet
>>>    has both client capabilites and scrambled password.
>>> @@ -36,7 +36,7 @@
>>>  #define MIN_HANDSHAKE_SIZE      2
>>>  #else
>>>  #define MIN_HANDSHAKE_SIZE      6
>>> -#endif /* HAVE_OPENSSL */
>>> +#endif /* HAVE_OPENSSL && !EMBEDDED_LIBRARY */
>>>
>>>  #ifdef __WIN__
>>>  static void  test_signal(int sig_ptr)
>>> @@ -647,6 +647,7 @@ bool init_new_connection_handler_thread(
>>>    return 0;
>>>  }
>>>
>>> +#ifndef EMBEDDED_LIBRARY
>>>  /*
>>>    Perform handshake, authorize client and update thd ACL variables.
>>>
>>> @@ -660,7 +661,6 @@ bool init_new_connection_handler_thread(
>>>     > 0  error code (not sent to user)
>>>  */
>>>
>>> -#ifndef EMBEDDED_LIBRARY
>>
>> magnus: ^ I don't understand why the 'check_connection' function is
>> needed in the embedded server?
> 
> 
> chad-reply0:  It's not.  That's if-not-defined.  The exclusion area 
> expands to contain more than  check_connection .

magnus1:
OK, so you only moved that ifndef to before the comment, I see.

But then you wouldn't need to do ifdef's inside the function below?

> 
>>
>>>  static int check_connection(THD *thd)
>>>  {
>>>    uint connect_errors= 0;
>>> @@ -738,10 +738,10 @@ static int check_connection(THD *thd)
>>>  #ifdef HAVE_COMPRESS
>>>      client_flags |= CLIENT_COMPRESS;
>>>  #endif /* HAVE_COMPRESS */
>>> -#ifdef HAVE_OPENSSL
>>> +#if defined(HAVE_OPENSSL) && !defined(EMBEDDED_LIBRARY)
magnus1:                          ^^^^ Not needed