Merge into libdrizzle-redux : misc : Code : Drizzle Client & Protocol Library

Status:	Merged
Approved by:	Andrew Hutchings on 2013-04-24
Approved revision:	115
Merged at revision:	114
Proposed branch:	lp://qastaging/~wiml-omni/libdrizzle/misc
Merge into:	lp://qastaging/libdrizzle
Diff against target:	378 lines (+82/-89) 8 files modified libdrizzle-5.1/statement.h (+1/-1) libdrizzle/binlog.cc (+1/-0) libdrizzle/common.h (+1/-0) libdrizzle/conn.cc (+62/-84) libdrizzle/conn_uds.cc (+0/-1) libdrizzle/field.cc (+2/-0) libdrizzle/statement_local.h (+1/-1) libdrizzle/statement_param.cc (+14/-2)
To merge this branch:	bzr merge lp://qastaging/~wiml-omni/libdrizzle/misc
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Andrew Hutchings		2013-04-24	Approve on 2013-04-24
Review via email: mp+160736@code.qastaging.launchpad.net

Description of the change

Miscellaneous improvements I have made while working with the library.

Revision history for this message

Andrew Hutchings (linuxjedi) wrote on 2013-04-24:

#

"#if defined(EWOULDBLOCK) && (EWOULDBLOCK != EAGAIN)" could probably remove the "(EWOULDBLOCK != EAGAIN)" bit because you are removing the thing that makes them equal. But it should work fine anyway.

Thanks for the cleanups.

review: Approve

Revision history for this message

Wim Lewis (wiml-omni) wrote on 2013-04-24:

#

I was thinking of the case where the system header helpfully #defines EWOULDBLOCK to EAGAIN (or the reverse) --- (in fact, checking it just now, sys/errno.h on MacOSX does exactly that :) )

Revision history for this message

Andrew Hutchings (linuxjedi) wrote on 2013-04-24:

#

ah, cool. Nice catch then :)

Revision history for this message

Brian Aker (brianaker) wrote on 2013-04-24:

#

Download full text (12.0 KiB)

Why not use cinttypes?

It would be better to do this as the first include. inttypes is a bit annoying because whatever calls it determines its behavior.

On Apr 24, 2013, at 11:11 AM, Wim Lewis <email address hidden> wrote:

> Wim Lewis has proposed merging lp:~wiml-omni/libdrizzle/misc into lp:libdrizzle.
>
> Requested reviews:
> Drizzle Trunk (drizzle-trunk)
>
> For more details, see:
> https://code.launchpad.net/~wiml-omni/libdrizzle/misc/+merge/160736
>
> Miscellaneous improvements I have made while working with the library.
> --
> https://code.launchpad.net/~wiml-omni/libdrizzle/misc/+merge/160736
> Your team Drizzle Trunk is requested to review the proposed merge of lp:~wiml-omni/libdrizzle/misc into lp:libdrizzle.
> === modified file 'libdrizzle-5.1/statement.h'
> --- libdrizzle-5.1/statement.h 2013-01-27 11:55:48 +0000
> +++ libdrizzle-5.1/statement.h 2013-04-24 18:10:30 +0000
> @@ -96,7 +96,7 @@
> drizzle_return_t drizzle_stmt_set_float(drizzle_stmt_st *stmt, uint16_t param_num, float value);
>
> DRIZZLE_API
> -drizzle_return_t drizzle_stmt_set_string(drizzle_stmt_st *stmt, uint16_t param_num, char *value, size_t length);
> +drizzle_return_t drizzle_stmt_set_string(drizzle_stmt_st *stmt, uint16_t param_num, const char *value, size_t length);
>
> DRIZZLE_API
> drizzle_return_t drizzle_stmt_set_null(drizzle_stmt_st *stmt, uint16_t param_num);
>
> === modified file 'libdrizzle/binlog.cc'
> --- libdrizzle/binlog.cc 2013-03-06 18:09:39 +0000
> +++ libdrizzle/binlog.cc 2013-04-24 18:10:30 +0000
> @@ -39,6 +39,7 @@
> #include "libdrizzle/common.h"
>
> #include <zlib.h>
> +#include <inttypes.h>
>
> drizzle_binlog_st *drizzle_binlog_init(drizzle_st *con,
> drizzle_binlog_fn *binlog_fn,
>
> === modified file 'libdrizzle/common.h'
> --- libdrizzle/common.h 2013-01-27 11:55:48 +0000
> +++ libdrizzle/common.h 2013-04-24 18:10:30 +0000
> @@ -76,6 +76,7 @@
>
> #include <stddef.h>
> #include <stdarg.h>
> +#include <stdint.h>
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
>
> === modified file 'libdrizzle/conn.cc'
> --- libdrizzle/conn.cc 2013-03-11 17:33:00 +0000
> +++ libdrizzle/conn.cc 2013-04-24 18:10:30 +0000
> @@ -45,34 +45,10 @@
> #include "config.h"
> #include "libdrizzle/common.h"
>
> -#ifndef SOCK_CLOEXEC
> -# define SOCK_CLOEXEC 0
> -#endif
> -
> -#ifndef SOCK_NONBLOCK
> -# define SOCK_NONBLOCK 0
> -#endif
> -
> -#ifndef FD_CLOEXEC
> -# define FD_CLOEXEC 0
> -#endif
> -
> -#ifndef F_GETFD
> -# define F_GETFD 0
> -#endif
> -
> -#ifndef MSG_DONTWAIT
> -# define MSG_DONTWAIT 0
> -#endif
> -
> #ifndef MSG_NOSIGNAL
> # define MSG_NOSIGNAL 0
> #endif
>
> -#ifndef EWOULDBLOCK
> -# define EWOULDBLOCK EAGAIN
> -#endif
> -
> #include <cerrno>
>
> /**
> @@ -1044,16 +1020,16 @@
>
> {
> int type= con->addrinfo_next->ai_socktype;
> - if (SOCK_CLOEXEC)
> - {
> - type|= SOCK_CLOEXEC;
> - }
> -
> - if (SOCK_NONBLOCK)
> - {
> - type|= SOCK_NONBLOCK;
> - }
> -
> +
> + /* Linuxisms to set some fd flags at the same time as creating the socket. */
> +#ifdef SOCK_CLOEXEC
> + type|= SOCK_CLOEXEC;
> +#endif
> +
...

Why not use cinttypes?

It would be better to do this as the first include. inttypes is a bit annoying because whatever calls it determines its behavior.

On Apr 24, 2013, at 11:11 AM, Wim Lewis <wiml@omnigroup.com> wrote:

> Wim Lewis has proposed merging lp:~wiml-omni/libdrizzle/misc into lp:libdrizzle.
> 
> Requested reviews:
>  Drizzle Trunk (drizzle-trunk)
> 
> For more details, see:
> https://code.launchpad.net/~wiml-omni/libdrizzle/misc/+merge/160736
> 
> Miscellaneous improvements I have made while working with the library.
> -- 
> https://code.launchpad.net/~wiml-omni/libdrizzle/misc/+merge/160736
> Your team Drizzle Trunk is requested to review the proposed merge of lp:~wiml-omni/libdrizzle/misc into lp:libdrizzle.
> === modified file 'libdrizzle-5.1/statement.h'
> --- libdrizzle-5.1/statement.h	2013-01-27 11:55:48 +0000
> +++ libdrizzle-5.1/statement.h	2013-04-24 18:10:30 +0000
> @@ -96,7 +96,7 @@
> drizzle_return_t drizzle_stmt_set_float(drizzle_stmt_st *stmt, uint16_t param_num, float value);
> 
> DRIZZLE_API
> -drizzle_return_t drizzle_stmt_set_string(drizzle_stmt_st *stmt, uint16_t param_num, char *value, size_t length);
> +drizzle_return_t drizzle_stmt_set_string(drizzle_stmt_st *stmt, uint16_t param_num, const char *value, size_t length);
> 
> DRIZZLE_API
> drizzle_return_t drizzle_stmt_set_null(drizzle_stmt_st *stmt, uint16_t param_num);
> 
> === modified file 'libdrizzle/binlog.cc'
> --- libdrizzle/binlog.cc	2013-03-06 18:09:39 +0000
> +++ libdrizzle/binlog.cc	2013-04-24 18:10:30 +0000
> @@ -39,6 +39,7 @@
> #include "libdrizzle/common.h"
> 
> #include <zlib.h>
> +#include <inttypes.h>
> 
> drizzle_binlog_st *drizzle_binlog_init(drizzle_st *con,
>                                        drizzle_binlog_fn *binlog_fn,
> 
> === modified file 'libdrizzle/common.h'
> --- libdrizzle/common.h	2013-01-27 11:55:48 +0000
> +++ libdrizzle/common.h	2013-04-24 18:10:30 +0000
> @@ -76,6 +76,7 @@
> 
> #include <stddef.h>
> #include <stdarg.h>
> +#include <stdint.h>
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
> 
> === modified file 'libdrizzle/conn.cc'
> --- libdrizzle/conn.cc	2013-03-11 17:33:00 +0000
> +++ libdrizzle/conn.cc	2013-04-24 18:10:30 +0000
> @@ -45,34 +45,10 @@
> #include "config.h"
> #include "libdrizzle/common.h"
> 
> -#ifndef SOCK_CLOEXEC 
> -# define SOCK_CLOEXEC 0
> -#endif
> -
> -#ifndef SOCK_NONBLOCK 
> -# define SOCK_NONBLOCK 0
> -#endif
> -
> -#ifndef FD_CLOEXEC
> -# define FD_CLOEXEC 0
> -#endif
> -
> -#ifndef F_GETFD
> -# define F_GETFD 0
> -#endif
> -
> -#ifndef MSG_DONTWAIT
> -# define MSG_DONTWAIT 0
> -#endif
> -
> #ifndef MSG_NOSIGNAL
> # define MSG_NOSIGNAL 0
> #endif
> 
> -#ifndef EWOULDBLOCK
> -# define EWOULDBLOCK EAGAIN
> -#endif
> -
> #include <cerrno>
> 
> /**
> @@ -1044,16 +1020,16 @@
> 
>     {
>       int type= con->addrinfo_next->ai_socktype;
> -      if (SOCK_CLOEXEC)
> -      { 
> -        type|= SOCK_CLOEXEC;
> -      }
> -
> -      if (SOCK_NONBLOCK)
> -      { 
> -        type|= SOCK_NONBLOCK;
> -      }
> -
> +      
> +      /* Linuxisms to set some fd flags at the same time as creating the socket. */ 
> +#ifdef SOCK_CLOEXEC
> +      type|= SOCK_CLOEXEC;
> +#endif
> +      
> +#ifdef SOCK_NONBLOCK
> +      type|= SOCK_NONBLOCK;
> +#endif
> +      
>       con->fd= socket(con->addrinfo_next->ai_family,
>                       type,
>                       con->addrinfo_next->ai_protocol);
> @@ -1171,7 +1147,11 @@
>     }
> 
>     ret= drizzle_set_events(con, POLLOUT);
> -    if (con->options.non_blocking)
> +    if (ret != DRIZZLE_RETURN_OK)
> +    {
> +      return ret;
> +    }
> +    else if (con->options.non_blocking)
>     {
>       return DRIZZLE_RETURN_IO_WAIT;
>     }
> @@ -1220,6 +1200,8 @@
> 
>     return DRIZZLE_RETURN_IO_WAIT;
>   }
> +  
> +  assert(con->buffer_allocation > 0); /* Appease static analyzer */
> 
>   while (1)
>   {
> @@ -1287,7 +1269,7 @@
>       switch (errno)
>       {
>       case EAGAIN:
> -#if EWOULDBLOCK != EAGAIN
> +#if defined(EWOULDBLOCK) && (EWOULDBLOCK != EAGAIN)
>       case EWOULDBLOCK:
> #endif
>         {
> @@ -1463,32 +1445,25 @@
>     return DRIZZLE_RETURN_INVALID_ARGUMENT;
>   }
> 
> -#ifdef HAVE_FCNTL
> -  if (HAVE_FCNTL)
> +#if HAVE_FCNTL && !defined(SOCK_CLOEXEC) && defined(FD_CLOEXEC)
>   {
> -    if (SOCK_CLOEXEC == 0)
> -    {
> -      if (FD_CLOEXEC and F_GETFD)
> +    int flags;
> +    do
> +    {
> +      flags= fcntl(con->fd, F_GETFD, 0);
> +    } while (flags == -1 and (errno == EINTR or errno == EAGAIN));
> +    
> +    if (flags != -1)
> +    {
> +      int rval;
> +      do
>       {
> -        int flags;
> -        do
> -        {
> -          flags= fcntl(con->fd, F_GETFD, 0);
> -        } while (flags == -1 and (errno == EINTR or errno == EAGAIN));
> -
> -        if (flags != -1)
> -        { 
> -          int rval;
> -          do
> -          { 
> -            rval= fcntl (con->fd, F_SETFD, flags | FD_CLOEXEC);
> -          } while (rval == -1 && (errno == EINTR or errno == EAGAIN));
> -          // we currently ignore the case where rval is -1
> -        }
> -      }
> +        rval= fcntl (con->fd, F_SETFD, flags | FD_CLOEXEC);
> +      } while (rval == -1 && (errno == EINTR or errno == EAGAIN));
> +      // we currently ignore the case where rval is -1
>     }
>   }
> -#endif // HAVE_FCNTL
> +#endif // HAVE_FCNTL but not SOCK_CLOEXEC
> 
> 
>   int ret= 1;
> @@ -1579,16 +1554,20 @@
>   }
> 
> #if defined(SO_NOSIGPIPE)
> -  if (SO_NOSIGPIPE)
> -  {
> -    int ret= 1;
> -    ret= setsockopt(con->fd, SOL_SOCKET, SO_NOSIGPIPE, static_cast<void *>(&ret), sizeof(int));
> -
> -    if (ret == -1)
> -    {
> -      drizzle_set_error(con, __func__, "setsockopt(SO_NOSIGPIPE): %s", strerror(errno));
> -      return DRIZZLE_RETURN_ERRNO;
> -    }
> +  ret= setsockopt(con->fd, SOL_SOCKET, SO_NOSIGPIPE, static_cast<void *>(&ret), sizeof(int));
> +  
> +  if (ret == -1)
> +  {
> +    drizzle_set_error(con, __func__, "setsockopt(SO_NOSIGPIPE): %s", strerror(errno));
> +    return DRIZZLE_RETURN_ERRNO;
> +  }
> +#elif HAVE_FCNTL && defined(F_SETNOSIGPIPE)
> +  ret= fcntl(con->fd, F_SETNOSIGPIPE, 1);
> +  
> +  if (ret == -1)
> +  {
> +    drizzle_set_error(con, __func__, "fcntl:F_SETNOSIGPIPE:%s", strerror(errno));
> +    return DRIZZLE_RETURN_ERRNO;
>   }
> #endif
> 
> @@ -1602,22 +1581,21 @@
>   // @todo find out why this can't be non-blocking for SSL
>   if (!con->ssl)
>   {
> -    if (SOCK_NONBLOCK == 0)
> -    {
> -      ret= fcntl(con->fd, F_GETFL, 0);
> -      if (ret == -1)
> -      {
> -        drizzle_set_error(con, __func__, "fcntl:F_GETFL:%s", strerror(errno));
> -        return DRIZZLE_RETURN_ERRNO;
> -      }
> -
> -      ret= fcntl(con->fd, F_SETFL, ret | O_NONBLOCK);
> -      if (ret == -1)
> -      {
> -        drizzle_set_error(con, __func__, "fcntl:F_SETFL:%s", strerror(errno));
> -        return DRIZZLE_RETURN_ERRNO;
> -      }
> -    }
> +#if HAVE_FCNTL && defined(O_NONBLOCK) && !defined(SOCK_NONBLOCK)
> +    ret= fcntl(con->fd, F_GETFL, 0);
> +    if (ret == -1)
> +    {
> +      drizzle_set_error(con, __func__, "fcntl:F_GETFL:%s", strerror(errno));
> +      return DRIZZLE_RETURN_ERRNO;
> +    }
> +    
> +    ret= fcntl(con->fd, F_SETFL, ret | O_NONBLOCK);
> +    if (ret == -1)
> +    {
> +      drizzle_set_error(con, __func__, "fcntl:F_SETFL:%s", strerror(errno));
> +      return DRIZZLE_RETURN_ERRNO;
> +    }
> +#endif
>   }
> #endif
> 
> 
> === modified file 'libdrizzle/conn_uds.cc'
> --- libdrizzle/conn_uds.cc	2013-01-27 11:55:48 +0000
> +++ libdrizzle/conn_uds.cc	2013-04-24 18:10:30 +0000
> @@ -77,7 +77,6 @@
> 
>   if (uds == NULL)
>   {
> -    uds= "";
>     con->socket.uds.path_buffer[0]= 0;
>   }
>   else
> 
> === modified file 'libdrizzle/field.cc'
> --- libdrizzle/field.cc	2013-04-22 18:32:01 +0000
> +++ libdrizzle/field.cc	2013-04-24 18:10:30 +0000
> @@ -44,6 +44,8 @@
> #include "config.h"
> #include "libdrizzle/common.h"
> 
> +#include <inttypes.h>
> +
> /*
>  * Client definitions
>  */
> 
> === modified file 'libdrizzle/statement_local.h'
> --- libdrizzle/statement_local.h	2013-01-27 11:55:48 +0000
> +++ libdrizzle/statement_local.h	2013-04-24 18:10:30 +0000
> @@ -41,7 +41,7 @@
> extern "C" {
> #endif
> 
> -drizzle_return_t drizzle_stmt_set_param(drizzle_stmt_st *stmt, uint16_t param_num, drizzle_column_type_t type, void *data, uint32_t length, bool is_unsigned);
> +drizzle_return_t drizzle_stmt_set_param(drizzle_stmt_st *stmt, uint16_t param_num, drizzle_column_type_t type, const void *data, size_t length, bool is_unsigned);
> 
> char *long_to_string(drizzle_bind_st *param, uint32_t val);
> 
> 
> === modified file 'libdrizzle/statement_param.cc'
> --- libdrizzle/statement_param.cc	2013-04-23 20:36:28 +0000
> +++ libdrizzle/statement_param.cc	2013-04-24 18:10:30 +0000
> @@ -38,8 +38,12 @@
> #include "config.h"
> #include "libdrizzle/common.h"
> 
> +#include <inttypes.h>
> +
> +#define CHECK_PARAM_NUM do { if (param_num > stmt->param_count) return DRIZZLE_RETURN_INVALID_ARGUMENT; } while(0)
> +
> /* Internal function */
> -drizzle_return_t drizzle_stmt_set_param(drizzle_stmt_st *stmt, uint16_t param_num, drizzle_column_type_t type, void *data, uint32_t length, bool is_unsigned)
> +drizzle_return_t drizzle_stmt_set_param(drizzle_stmt_st *stmt, uint16_t param_num, drizzle_column_type_t type, const void *data, size_t length, bool is_unsigned)
> {
>   if ((stmt == NULL) || (param_num >= stmt->param_count))
>   {
> @@ -64,6 +68,7 @@
> drizzle_return_t drizzle_stmt_set_tiny(drizzle_stmt_st *stmt, uint16_t param_num, uint8_t value, bool is_unsigned)
> {
>   uint8_t *val;
> +  CHECK_PARAM_NUM;
>   val= (uint8_t*) stmt->query_params[param_num].data_buffer;
>   *val= value;
> 
> @@ -72,6 +77,7 @@
> drizzle_return_t drizzle_stmt_set_short(drizzle_stmt_st *stmt, uint16_t param_num, uint16_t value, bool is_unsigned)
> {
>   uint16_t *val;
> +  CHECK_PARAM_NUM;
>   val= (uint16_t*) stmt->query_params[param_num].data_buffer;
>   *val= value;
> 
> @@ -81,6 +87,7 @@
> drizzle_return_t drizzle_stmt_set_int(drizzle_stmt_st *stmt, uint16_t param_num, uint32_t value, bool is_unsigned)
> {
>   uint32_t *val;
> +  CHECK_PARAM_NUM;
>   val= (uint32_t*) stmt->query_params[param_num].data_buffer;
>   *val= value;
> 
> @@ -90,6 +97,7 @@
> drizzle_return_t drizzle_stmt_set_bigint(drizzle_stmt_st *stmt, uint16_t param_num, uint64_t value, bool is_unsigned)
> {
>   uint64_t *val;
> +  CHECK_PARAM_NUM;
>   val= (uint64_t*) stmt->query_params[param_num].data_buffer;
>   *val= value;
> 
> @@ -99,6 +107,7 @@
> drizzle_return_t drizzle_stmt_set_double(drizzle_stmt_st *stmt, uint16_t param_num, double value)
> {
>   double *val;
> +  CHECK_PARAM_NUM;
>   val= (double*) stmt->query_params[param_num].data_buffer;
>   *val= value;
> 
> @@ -108,13 +117,14 @@
> drizzle_return_t drizzle_stmt_set_float(drizzle_stmt_st *stmt, uint16_t param_num, float value)
> {
>   float *val;
> +  CHECK_PARAM_NUM;
>   val= (float*) stmt->query_params[param_num].data_buffer;
>   *val= value;
> 
>   return drizzle_stmt_set_param(stmt, param_num, DRIZZLE_COLUMN_TYPE_FLOAT, val, 4, false);
> }
> 
> -drizzle_return_t drizzle_stmt_set_string(drizzle_stmt_st *stmt, uint16_t param_num, char *value, size_t length)
> +drizzle_return_t drizzle_stmt_set_string(drizzle_stmt_st *stmt, uint16_t param_num, const char *value, size_t length)
> {
>   return drizzle_stmt_set_param(stmt, param_num, DRIZZLE_COLUMN_TYPE_STRING, value, length, false);
> }
> @@ -127,6 +137,7 @@
> drizzle_return_t drizzle_stmt_set_time(drizzle_stmt_st *stmt, uint16_t param_num, uint32_t days, uint8_t hours, uint8_t minutes, uint8_t seconds, uint32_t microseconds, bool is_negative)
> {
>   drizzle_datetime_st *time;
> +  CHECK_PARAM_NUM;
>   time= (drizzle_datetime_st*) stmt->query_params[param_num].data_buffer;
> 
>   memset(time, 0, sizeof(*time));
> @@ -145,6 +156,7 @@
> drizzle_return_t drizzle_stmt_set_timestamp(drizzle_stmt_st *stmt, uint16_t param_num, uint16_t year, uint8_t month, uint8_t day, uint8_t hours, uint8_t minutes, uint8_t seconds, uint32_t microseconds)
> {
>   drizzle_datetime_st *timestamp;
> +  CHECK_PARAM_NUM;
>   timestamp= (drizzle_datetime_st*) stmt->query_params[param_num].data_buffer;
> 
>   memset(timestamp, 0, sizeof(*timestamp));
>

Revision history for this message

Wim Lewis (wiml-omni) wrote on 2013-04-24:

#

You're right, <cinttypes> would be better here. (I'm mostly a C, not C++, programmer.)

Drizzle Client & Protocol Library

Merge lp://qastaging/~wiml-omni/libdrizzle/misc into lp://qastaging/libdrizzle

Commit message

Description of the change

Preview Diff

Subscribers