List:Commits« Previous MessageNext Message »
From:Sergei Golubchik Date:April 2 2011 8:41pm
Subject:Re: bzr commit into mysql-5.5 branch (rafal.somla:3376)
View as plain text  
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
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
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