List:Commits« Previous MessageNext Message »
From:Paul DuBois Date:April 4 2011 1:41pm
Subject:Re: bzr commit into mysql-5.5 branch (rafal.somla:3376)
View as plain text  
On Apr 2, 2011, at 3:41 PM, Sergei Golubchik wrote:

> Hi, Rafal!
> 
> A couple of comments
> 
> On Mar 30, Rafal Somla wrote:
>> 3376 Rafal Somla	2011-03-30
>>      Heap allocation service - only for review purposes.
>> 
>>    added:
>>      include/mysql/service_heap_alloc.h
>>      libservices/heap_alloc_service.c
>>    modified:
>>      include/mysql/services.h
>>      include/service_versions.h
>>      libservices/CMakeLists.txt
>>      sql/sql_plugin_services.h
>> === added file 'include/mysql/service_heap_alloc.h'
>> --- a/include/mysql/service_heap_alloc.h	1970-01-01 00:00:00 +0000
>> +++ b/include/mysql/service_heap_alloc.h	2011-03-30 14:22:50 +0000
>> +/**
>> +  @file
>> +  This service provdes functions to allocate memory on server's heap.
> 
> This is supposed to be the complete documentation of the service.  But

Just to add to that:

http://dev.mysql.com/doc/refman/5.5/en/plugin-services.html says:

"For developers who wish to modify the server to add a new service, see MySQL Services for
Plugins in the MySQL Internals Manual."

And that part of the Internals manual says:

http://forge.mysql.com/wiki/MySQL_Internals_Support_for_Plug-Ins#MySQL_Services_for_Plugins

"When you write a service, be sure to provide complete instructions on how to use it. A
service named xxx will have a file named service_xxx.h in the include/mysql directory.
For the benefit of plugin developers who use your service, this file should include
comments that fully document the service interface:
	• Its purpose
	• Any applicable guidelines, including limitations or restrictions
	• For each function, a description of what it does, its calling sequence, and return
value
The goal for documentation in this file is that plugin developers should be able to look
at the file and completely understand how to use the service.When you write a service, be
sure to provide complete instructions on how to use it. A service named xxx will have a
file named service_xxx.h in the include/mysql directory. For the benefit of plugin
developers who use your service, this file should include comments that fully document
the service interface:
	• Its purpose
	• Any applicable guidelines, including limitations or restrictions
	• For each function, a description of what it does, its calling sequence, and return
value
The goal for documentation in this file is that plugin developers should be able to look
at the file and completely understand how to use the service."

> you don't describe flags, for example. And don't even say that these
> functions are like normal malloc/realloc/free. And why one should use
> them instead of OS versions.
> 
>> +*/
>> +
>> +
>> +#ifdef __cplusplus
>> +extern "C" {
>> +#endif
>> +
>> +typedef int myf;
>> +
>> +extern struct heap_alloc_service_st {
>> +  void* (*malloc)(size_t size, myf MyFlags);
>> +  void  (*free)(void* ptr);
>> +  void* (*realloc)(void* oldpoint, size_t size, myf MyFlags);
> 
> I'd suggest to avoid using malloc/free/realloc names here, use something
> more unique. There's no guarantee that system headers don't have macros
> for malloc, for example. I remember seeing read() implemented as a macro
> on some redhat version :)
> 
>> +} *heap_alloc_service;
>> +
>> +#ifdef MYSQL_DYNAMIC_PLUGIN
>> +
>> +#define MY_ZEROFILL       32  /* malloc(): fill array with zero */
>> +#define MY_ALLOW_ZERO_PTR 64  /* realloc(): zero ptr -> malloc */
>> +#define MY_FREE_ON_ERROR 128  /* realloc(): Free old ptr on error */
>> +#define MY_HOLD_ON_ERROR 256  /* realloc(): Return old ptr on error */
>> +
>> +#define heap_malloc(size, flags) (heap_alloc_service->malloc((size),
> myf(flags)))
>> +#define heap_free(ptr)           (heap_alloc_service->free((ptr)))
>> +#define heap_realloc(old, size, flags) (heap_alloc_service->realloc((old),
> (size), myf(flags)))
> 
> These macros should be called my_malloc, my_free, my_realloc. That is,
> they should have exactly the same names as functions below - because
> when the plugin is compiled dynamically, it uses these macors, but when
> it's compiled statically it uses functions. So, the names must match.
> 
>> +#else
>> +
>> +extern void* my_malloc(size_t Size,myf MyFlags);
>> +extern void* my_realloc(void *oldpoint, size_t Size, myf MyFlags);
>> +extern void  my_free(void *ptr);
>> +
>> +#endif
>> +
>> +#ifdef __cplusplus
>> +}
>> +#endif
>> +
>> +#define MYSQL_SERVICE_HEAP_ALLOC_INCLUDED
>> +#endif
>> +
> Regards,
> Sergei
> 
> -- 
> MySQL Code Commits Mailing List
> For list archives: http://lists.mysql.com/commits
> To unsubscribe:    http://lists.mysql.com/commits?unsub=1
> 

-- 
Paul DuBois
Oracle Corporation / MySQL Documentation Team
Madison, Wisconsin, USA
www.mysql.com

Thread
bzr commit into mysql-5.5 branch (rafal.somla:3376) Rafal Somla30 Mar
  • Re: bzr commit into mysql-5.5 branch (rafal.somla:3376)Sergei Golubchik2 Apr
    • Re: bzr commit into mysql-5.5 branch (rafal.somla:3376)Paul DuBois4 Apr