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

[cmiller]

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.


>> 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.

>> 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 .

>
>>  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)