-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Stewart Smith wrote:
> review: needs-fixing

>> +#include <drizzled/server_includes.h>
>> +#include <drizzled/sql_udf.h>
> 
> I'm not sure if we strictly need the sql_udf.h include as it is a bit of
> a mix of things. Not that concerned about it, but perhaps is something
> to look into. 

In fact, I'm totally changing what headers you need here at the moment,
so don't lose too much sleep over this.

>> +String *MemcachedAdd::val_str(String *str)
>> +{
>> +  memcached_return rc;
>> +  time_t expiration= 0;
>> +  String *key;
>> +  String *value;
>> +
>> +  if ((arg_count != 2 && arg_count != 3) ||
>> +      ! (key= args[0]->val_str(str)) ||
>> +      ! (value= args[1]->val_str(str)) ||
>> +      ! memc)
>> +  {
>> +    return &failure_buff;
>> +  }
>> +
>> +  if (arg_count == 3)
>> +  {
>> +    String *tmp_exp= args[2]->val_str(str);;
>> +
>> +    expiration= (time_t)atoi(tmp_exp->c_ptr());
>> +  }
> 
> This logic for argument count checking should be in
> check_argument_count().
> 
>> +
>> +  rc= memcached_add(memc, key->c_ptr(), key->length(),
>> +                    value->c_ptr(), value->length(),
>> +                    expiration, (uint16_t) 0);
>> +
>> +  if (rc != MEMCACHED_SUCCESS)
>> +  {
>> +    return &failure_buff;
>> +  }
>> +
>> +  return &success_buff;
>> +}
>> +
> 
> We should probably also push a warning if there is failure.
> 
>> === added file 'plugin/memcached_udf/memc_add.h'
>> --- plugin/memcached_udf/memc_add.h	1970-01-01 00:00:00 +0000
>> +++ plugin/memcached_udf/memc_add.h	2009-07-19 08:26:01 +0000
>> @@ -0,0 +1,56 @@
>> +/* implements memc_add */
> 
> I don't think the comment adds anything :)

++

>> +class MemcachedAdd: public Item_str_func
>> +{
>> +public:
>> +  MemcachedAdd()
>> +    :
>> +      Item_str_func(),
>> +      failure_buff("0", &my_charset_bin),
>> +      success_buff("1", &my_charset_bin)
>> +  {}
> 
> Returning a 0 or 1 string instead of integer is a little strange.
> Probably best to either return an integer or maybe even key/value to
> make query writing a bit easier.
> 
>> === added file 'plugin/memcached_udf/memc_add_by_key.cc'
>> --- plugin/memcached_udf/memc_add_by_key.cc	1970-01-01 00:00:00 +0000
>> +++ plugin/memcached_udf/memc_add_by_key.cc	2009-07-18 11:13:08 +0000
>> @@ -0,0 +1,71 @@
>> +String *MemcachedAddByKey::val_str(String *str)
>> +{
>> +  memcached_return rc;
>> +  time_t expiration= 0;
>> +  String *master_key;
>> +  String *key;
>> +  String *value;
>> +
>> +  if ((arg_count != 3 && arg_count != 4) ||
>> +      ! (master_key= args[0]->val_str(str)) ||
>> +      ! (key= args[1]->val_str(str)) ||
>> +      ! (value= args[2]->val_str(str)) ||
>> +      ! memc)
>> +  {
>> +    return &failure_buff;
>> +  }
>> +
>> +  if (arg_count == 4)
>> +  {
>> +    String *tmp_exp= args[3]->val_str(str);;
>> +
>> +    expiration= (time_t)atoi(tmp_exp->c_ptr());
>> +  }
> 
> same as above with arg count
> 
>> +  rc= memcached_add_by_key(memc,
>> +                           master_key->c_ptr(), master_key->length(),
>> +                           key->c_ptr(), key->length(),
>> +                           value->c_ptr(), value->length(),
>> +                           expiration, (uint16_t) 0);
>> +
>> +  if (rc != MEMCACHED_SUCCESS)
>> +  {
>> +    return &failure_buff;
>> +  }
>> +
>> +  return &success_buff;
>> +}
>> +
> 
> as above.
> 
>> === added file 'plugin/memcached_udf/memc_add_by_key.h'
>> --- plugin/memcached_udf/memc_add_by_key.h	1970-01-01 00:00:00 +0000
>> +++ plugin/memcached_udf/memc_add_by_key.h	2009-07-18 11:13:08 +0000
>> @@ -0,0 +1,56 @@
> 
> as above
> 
>> === added file 'plugin/memcached_udf/memc_append.cc'
>> --- plugin/memcached_udf/memc_append.cc	1970-01-01 00:00:00 +0000
>> +++ plugin/memcached_udf/memc_append.cc	2009-07-12 04:13:35 +0000
>> @@ -0,0 +1,57 @@
> 
> also as above
> 
>> === added file 'plugin/memcached_udf/memc_append.h'
>> --- plugin/memcached_udf/memc_append.h	1970-01-01 00:00:00 +0000
>> +++ plugin/memcached_udf/memc_append.h	2009-07-18 11:13:08 +0000
>> @@ -0,0 +1,56 @@
> 
> also as above
> 
>> === added file 'plugin/memcached_udf/memc_append_by_key.cc'
>> --- plugin/memcached_udf/memc_append_by_key.cc	1970-01-01 00:00:00 +0000
>> +++ plugin/memcached_udf/memc_append_by_key.cc	2009-07-18 11:13:08 +0000
>> @@ -0,0 +1,62 @@
> 
> also here
> 
>> === added file 'plugin/memcached_udf/memc_append_by_key.h'
>> --- plugin/memcached_udf/memc_append_by_key.h	1970-01-01 00:00:00 +0000
>> +++ plugin/memcached_udf/memc_append_by_key.h	2009-07-18 11:13:08 +0000
>> @@ -0,0 +1,56 @@
> 
> here too
> 
>> === added file 'plugin/memcached_udf/memc_behavior_get.cc'
>> --- plugin/memcached_udf/memc_behavior_get.cc	1970-01-01 00:00:00 +0000
>> +++ plugin/memcached_udf/memc_behavior_get.cc	2009-08-09 19:15:52 +0000
>> @@ -0,0 +1,148 @@
> 
>> +void MemcachedBehaviorGet::setFailureString(const char *error)
>> +{
>> +  size_t size= strlen(error);
>> +  failure_buff.realloc(size);
>> +  failure_buff.length(size);
>> +  memcpy(failure_buff.ptr(), error, size);
>> +}
>> +
>> +String *MemcachedBehaviorGet::val_str(String *str)
>> +{
>> +  memcached_behavior mbehavior;
>> +  uint64_t isetting;
>> +  map<const string, memcached_behavior>::iterator it;
>> +  String *tmp_behavior;
>> +
>> +  if (arg_count != 1 ||
>> +      ! (tmp_behavior= args[0]->val_str(str)) ||
>> +      ! memc)
>> +  {
>> +    setFailureString("USAGE: memc_behavior_get('<behavior type>')");
>> +    return &failure_buff;
>> +  }
>> +
>> +  string behavior(tmp_behavior->c_ptr());
>> +
>> +  /*
>> +   * We don't want the user to have to type in all input in upper
>> +   * case so we transform the input strings to upper case here.
>> +   */
>> +  std::transform(behavior.begin(), behavior.end(),
>> +                 behavior.begin(), ::toupper);
>> +
>> +  it = behavior_map.find(behavior);
>> +  if (it == behavior_map.end()) 
>> +  {
>> +    setFailureString("UNKNOWN BEHAVIOR TYPE!");
>> +    return &failure_buff;
>> +  }
>> +	
>> +  mbehavior= behavior_map[behavior];
>> +
>> +  /*if (mbehavior == -1)
>> +  {
>> +    setFailureString("UNKNOWN BEHAVIOR TYPE!");
>> +    return &failure_buff;
>> +  }
>> +*/
>> +
>> +  isetting= memcached_behavior_get(memc, mbehavior);
>> +
>> +  switch (mbehavior)
>> +  {
>> +  case MEMCACHED_BEHAVIOR_SUPPORT_CAS:
>> +  case MEMCACHED_BEHAVIOR_NO_BLOCK:
>> +  case MEMCACHED_BEHAVIOR_BUFFER_REQUESTS:
>> +  case MEMCACHED_BEHAVIOR_USER_DATA:
>> +  case MEMCACHED_BEHAVIOR_SORT_HOSTS:
>> +  case MEMCACHED_BEHAVIOR_VERIFY_KEY:
>> +  case MEMCACHED_BEHAVIOR_TCP_NODELAY:
>> +  case MEMCACHED_BEHAVIOR_KETAMA:
>> +  case MEMCACHED_BEHAVIOR_CACHE_LOOKUPS:
>> +    if (isetting == 1)
>> +      return_buff.append("1");
>> +    else if (isetting == 0)
>> +      return_buff.append("0");
>> +    else
>> +    {
>> +      setFailureString("INVALID VALUE FOR BEHAVIOR - MUST be 1 OR 0!");
>> +      return &failure_buff;
>> +    }
>> +    break;
>> +  case MEMCACHED_BEHAVIOR_DISTRIBUTION:
>> +    {
>> +      string setting(dist_settings_reverse_map[isetting]);
>> +      return_buff.append(setting.c_str());
>> +    }
>> +    break;
>> +  case MEMCACHED_BEHAVIOR_HASH:
>> +    {
>> +      string setting(hash_settings_reverse_map[isetting]);
>> +      return_buff.append(setting.c_str());
>> +    }
>> +    break;
>> +  case MEMCACHED_BEHAVIOR_KETAMA_HASH:
>> +    {
>> +      string setting(ketama_hash_settings_reverse_map[isetting]);
>> +      return_buff.append(setting.c_str());
>> +    }
>> +    break;
>> +  case MEMCACHED_BEHAVIOR_SOCKET_SEND_SIZE:
>> +  case MEMCACHED_BEHAVIOR_SOCKET_RECV_SIZE:
>> +  case MEMCACHED_BEHAVIOR_POLL_TIMEOUT:
>> +  case MEMCACHED_BEHAVIOR_CONNECT_TIMEOUT:
>> +  case MEMCACHED_BEHAVIOR_RETRY_TIMEOUT:
>> +  case MEMCACHED_BEHAVIOR_IO_MSG_WATERMARK:
>> +  case MEMCACHED_BEHAVIOR_IO_BYTES_WATERMARK:
>> +    {
>> +      size_t setting_len= 0;
>> +      char tmp_buff[10];
>> +
>> +      sprintf(tmp_buff, "%"PRIu64, isetting);
>> +      setting_len= strlen(tmp_buff);
>> +      return_buff.realloc(setting_len);
>> +      return_buff.length(setting_len);
>> +      memcpy(return_buff.ptr(),tmp_buff, setting_len);
>> +    }
>> +    break;
>> +  default:
>> +    break;
>> +  }
>> +
>> +  return &return_buff;
>> +}
>>
>> === added file 'plugin/memcached_udf/memc_behavior_get.h'
>> --- plugin/memcached_udf/memc_behavior_get.h	1970-01-01 00:00:00 +0000
>> +++ plugin/memcached_udf/memc_behavior_get.h	2009-07-13 14:02:39 +0000
>> @@ -0,0 +1,215 @@
>> +/* implements memc_behavior_get */
>> +class MemcachedBehaviorGet : public Item_str_func
>> +{
>> +public:
>> +  MemcachedBehaviorGet()
>> +    : 
>> +      Item_str_func(),
>> +      failure_buff("FAILURE", &my_charset_bin),
>> +      return_buff("", &my_charset_bin),
>> +      behavior_map(),
>> +      behavior_reverse_map(),
>> +      dist_settings_reverse_map(),
>> +      hash_settings_reverse_map(),
>> +      ketama_hash_settings_reverse_map()
>> +  {
>> +    behavior_map.insert(std::pair<const std::string, memcached_behavior>
>> +      ("MEMCACHED_BEHAVIOR_SUPPORT_CAS",
> MEMCACHED_BEHAVIOR_SUPPORT_CAS));
> 
> wondering if we should just have a global that's initialised with the
> base behaviour_map and copy it to each function (if in fact we need to
> copy it) as this code is creating the whole behaviour_map for each call
> to memcached_behaviour_get.
> 
>> === added file 'plugin/memcached_udf/memc_behavior_set.cc'
>> --- plugin/memcached_udf/memc_behavior_set.cc	1970-01-01 00:00:00 +0000
>> +++ plugin/memcached_udf/memc_behavior_set.cc	2009-08-09 14:01:57 +0000
>> @@ -0,0 +1,160 @@
>> +String *MemcachedBehaviorSet::val_str(String *str)
>> +{
>> +  memcached_return rc;
>> +  memcached_behavior mbehavior;
>> +  uint64_t isetting= 0;
>> +  map<const string, memcached_behavior>::iterator it_behav;
>> +  map<const string, uint64_t>::iterator it_hash;
>> +  map<const string, uint64_t>::iterator it_dist;
>> +  String *tmp_behavior;
>> +  String *tmp_setting;
>> +
>> +  if (arg_count != 2 ||
>> +      ! (tmp_behavior= args[0]->val_str(str)) ||
>> +      ! (tmp_setting= args[1]->val_str(str)) ||
>> +      ! memc)
>> +  {
>> +    setFailureString("USAGE: memc_behavior_set('<behavior type>','<value>')");
>> +    return &failure_buff;
>> +  }
> 
> same argument checking as before.
> 
>> === added file 'plugin/memcached_udf/memc_behavior_set.h'
>> --- plugin/memcached_udf/memc_behavior_set.h	1970-01-01 00:00:00 +0000
>> +++ plugin/memcached_udf/memc_behavior_set.h	2009-07-28 16:44:11 +0000
>> @@ -0,0 +1,164 @@
>> +class MemcachedBehaviorSet : public Item_str_func
>> +{
>> +public:
>> +  MemcachedBehaviorSet()
>> +    : 
>> +      Item_str_func(),
>> +      failure_buff("0", &my_charset_bin),
>> +      success_buff("1", &my_charset_bin),
>> +      behavior_map(),
>> +      dist_settings_map(),
>> +      hash_settings_map(),
>> +      ketama_hash_settings_map()
>> +  {
>> +    behavior_map.insert(std::pair<const std::string, memcached_behavior>
>> +      ("MEMCACHED_BEHAVIOR_SUPPORT_CAS", MEMCACHED_BEHAVIOR_SUPPORT_CAS));
> 
> same comment as previous bahaviour_map
> 
>> === added file 'plugin/memcached_udf/memc_cas.cc'
>> --- plugin/memcached_udf/memc_cas.cc	1970-01-01 00:00:00 +0000
>> +++ plugin/memcached_udf/memc_cas.cc	2009-07-18 11:13:08 +0000
>> @@ -0,0 +1,70 @@
> 
> see above comments
> 
>> === added file 'plugin/memcached_udf/memc_cas_by_key.cc'
>> --- plugin/memcached_udf/memc_cas_by_key.cc	1970-01-01 00:00:00 +0000
>> +++ plugin/memcached_udf/memc_cas_by_key.cc	2009-07-27 17:00:52 +0000
>> @@ -0,0 +1,74 @@
> 
> same as previous comments.
> 
>> === added file 'plugin/memcached_udf/memc_cas_by_key.h'
>> --- plugin/memcached_udf/memc_cas_by_key.h	1970-01-01 00:00:00 +0000
>> +++ plugin/memcached_udf/memc_cas_by_key.h	2009-07-18 11:13:08 +0000
>> @@ -0,0 +1,56 @@
> 
>> === added file 'plugin/memcached_udf/memc_decrement.cc'
>> --- plugin/memcached_udf/memc_decrement.cc	1970-01-01 00:00:00 +0000
>> +++ plugin/memcached_udf/memc_decrement.cc	2009-08-09 19:15:52 +0000
>> @@ -0,0 +1,80 @@
> 
>> +String *MemcachedDecrement::val_str(String *str)
>> +{
> 
> Should be a int function, not a string func
> 
>> +  memcached_return rc;
>> +  int64_t offset;
>> +  uint64_t value;
>> +  size_t val_len;
>> +  char tmp_buff[32]= "";
>> +  String *key;
>> +  String *dec_str;
>> +
>> +  if ((arg_count != 1 && arg_count != 2) ||
>> +      ! (key= args[0]->val_str(str)) ||
>> +      ! memc)
>> +  {
>> +    return &failure_buff;
>> +  }
>> +
>> +  if (arg_count == 2) {
>> +    dec_str= args[1]->val_str(str);
>> +    offset= atoi(dec_str->c_ptr());
>> +  }
>> +  else
>> +  {
>> +    offset= 1;
>> +  }
> 
> as above - arg count checking
> 
>> +  rc= memcached_decrement(memc,
>> +                    key->c_ptr(),
>> +                    key->length(),
>> +                    offset,
>> +                    &value);
>> +
>> +  sprintf(tmp_buff, "%"PRIu64, value);
>> +  val_len= strlen(tmp_buff);
>> +
>> +  if (rc != MEMCACHED_SUCCESS)
>> +  {
>> +    return &failure_buff;
>> +  }
>> +
>> +  buffer.realloc(val_len);
>> +  buffer.length(val_len);
>> +  memcpy(buffer.ptr(), tmp_buff, val_len);
>> +
>> +  return &buffer;
>> +}
>> +
> 
>> === added file 'plugin/memcached_udf/memc_decrement.h'
>> --- plugin/memcached_udf/memc_decrement.h	1970-01-01 00:00:00 +0000
>> +++ plugin/memcached_udf/memc_decrement.h	2009-07-15 01:52:12 +0000
>> @@ -0,0 +1,55 @@
> 
>> +class MemcachedDecrement : public Item_str_func
> 
> As mentioned above, should be Int func.
> 
>> +{
>> +public:
>> +  MemcachedDecrement()
>> +    : 
>> +      Item_str_func(),
>> +      failure_buff("FAILURE", &my_charset_bin)
>> +  {}
>> +
>> +  const char *func_name() const
>> +  {
>> +    return "memc_decrement";
>> +  }
>> +
>> +  String *val_str(String *);
>> +
>> +  void fix_length_and_dec()
>> +  {
>> +    max_length= 32;
>> +  }
>> +
>> +private:
>> +  String failure_buff;
>> +  String buffer;
>> +};
>> +
>> +#endif /* MEMC_DECREMENT_H */
>>
>> === added file 'plugin/memcached_udf/memc_delete.cc'
>> --- plugin/memcached_udf/memc_delete.cc	1970-01-01 00:00:00 +0000
>> +++ plugin/memcached_udf/memc_delete.cc	2009-07-19 08:26:01 +0000
>> @@ -0,0 +1,55 @@
> 
> as above - arg checking
> 
>> === added file 'plugin/memcached_udf/memc_increment.cc'
>> --- plugin/memcached_udf/memc_increment.cc	1970-01-01 00:00:00 +0000
>> +++ plugin/memcached_udf/memc_increment.cc	2009-08-09 19:15:52 +0000
>> @@ -0,0 +1,81 @@
> 
>> +String *MemcachedIncrement::val_str(String *str)
> 
> also should be an int func.
> 
>> === added file 'plugin/memcached_udf/memc_stats.cc'
>> --- plugin/memcached_udf/memc_stats.cc	1970-01-01 00:00:00 +0000
>> +++ plugin/memcached_udf/memc_stats.cc	2009-07-22 21:47:12 +0000
>> @@ -0,0 +1,120 @@
> 
>> +String *MemcachedStats::val_str(String *str)
>> +{
>> +  memcached_return rc;
>> +  unsigned int count;
>> +  char buff[100];
>> +  memcached_stat_st *stat;
>> +  memcached_server_st *servers;
>> +  memcached_server_st *server_list;
>> +  String *server_names;
>> +
>> +
>> +  if (arg_count != 1 ||
>> +      ! (server_names= args[0]->val_str(str)) ||
>> +      ! memc)
>> +  {
>> +    setFailureString("USAGE: memc_stats('<server list>')");
>> +    return &failure_buff;
>> +  }
> 
> should be pushed as warning/error and not into return value.
> 
>> +
>> +  servers= memcached_servers_parse(server_names->c_ptr());
>> +  if (servers == NULL)
>> +  {
>> +    setFailureString(" ERROR: unable to parse servers string!");
>> +    return &failure_buff;
>> +  }
>> +  memcached_server_push(memc, servers);
>> +  memcached_server_list_free(servers);
>> +
>> +  stat= memcached_stat(memc, NULL, &rc);
>> +
>> +  if (rc != MEMCACHED_SUCCESS && rc != MEMCACHED_SOME_ERRORS)
>> +  {
>> +    sprintf(buff, "Failure to communicate with servers (%s)\n",
>> +            memcached_strerror(memc, rc));
>> +
>> +    setFailureString(buff);
>> +    return &failure_buff;
>> +  }
> 
> never use sprintf. always snprintf.
> 
>> +  server_list= memcached_server_list(memc);
>> +
>> +  results_buff.length(0);
>> +  sprintf(buff, "Listing %u Server\n\n", memcached_server_count(memc));
>> +  results_buff.append(buff);
> 
> as above, r.e. snprintf
> 
>> +
>> +  for (count= 0; count < memcached_server_count(memc); count++)
>> +  {
>> +    char **list;
>> +    char **ptr;
>> +
>> +    list= memcached_stat_get_keys(memc, &stat[count], &rc);
>> +
>> +    sprintf(buff, "Server: %s (%u)\n",
>> +            memcached_server_name(memc, server_list[count]),
>> +            memcached_server_port(memc, server_list[count]));
> 
> snprintf
> 
>> +    results_buff.append(buff);
>> +
>> +    for (ptr= list; *ptr; ptr++)
>> +    {
>> +      char *value= memcached_stat_get_value(memc, &stat[count], *ptr, &rc);
>> +
>> +      sprintf(buff, "\t %s: %s\n", *ptr, value);
>> +      free(value);
>> +      results_buff.append(buff);
> 
> snprintf
> 
>> === added directory 'tests/suite/memcached_udf/t'
>> === added file 'tests/suite/memcached_udf/t/memc.opt'
>> --- tests/suite/memcached_udf/t/memc.opt	1970-01-01 00:00:00 +0000
>> +++ tests/suite/memcached_udf/t/memc.opt	2009-07-14 03:42:55 +0000
>> @@ -0,0 +1,1 @@
>> +--scheduler=multi-thread
> 
> we shouldn't require this, so the memc.opt file shouldn't need to exist.
> we should test with whichever scheduler was loaded.
> 
> 
> I skipped a few of the functions that i'd say the same comments as the
> previous ones.
> 
> hope this helps,
> stewart

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkqKQFQACgkQ2Jv7/VK1RgFZHACgulOWNthnuehnSrqCTykZ9E/j
/9kAn0vIEqA/4AOa9pLjEUPUe9CoV60z
=qfJb
-----END PGP SIGNATURE-----