maria-developers team mailing list archive

[Michael Widenius <monty@xxxxxxxxxxxx>]

Hi!

>>>>> "sanja" == sanja  <sanja@xxxxxxxxxxxxxxxx> writes:

sanja> At file:///home/bell/maria/bzr/work-maria-5.2-lpb956079/
sanja> ------------------------------------------------------------
sanja> revno: 3126
sanja> revision-id: sanja@xxxxxxxxxxxxxxxx-20120319122824-7tnmdh8bq7cbnvni
sanja> parent: sanja@xxxxxxxxxxxxxxxx-20120314100903-a6txqk4l73smh189
sanja> committer: sanja@xxxxxxxxxxxxxxxx
sanja> branch nick: work-maria-5.2-lpb956079
sanja> timestamp: Mon 2012-03-19 14:28:24 +0200
sanja> message:
sanja>   FIX for LP bug#956079
  
sanja>   Always set all callbacks for page cache.

<cut>

> --- a/storage/maria/ma_pagecache.c	2011-08-10 19:44:39 +0000
> +++ b/storage/maria/ma_pagecache.c	2012-03-19 12:28:24 +0000
> @@ -633,7 +633,6 @@ static my_bool pagecache_fwrite(PAGECACH
>  {
>    DBUG_ENTER("pagecache_fwrite");
>    DBUG_ASSERT(type != PAGECACHE_READ_UNKNOWN_PAGE);
> -
>  #ifdef EXTRA_DEBUG_BITMAP
>    /*
>      This code is very good when debugging changes in bitmaps or dirty lists
> @@ -654,6 +653,12 @@ static my_bool pagecache_fwrite(PAGECACH
>    }
>  #endif
>  
> +  DBUG_ASSERT(filedesc->read_callback);
> +  DBUG_ASSERT(filedesc->write_callback);
> +  DBUG_ASSERT(filedesc->write_fail);
> +  DBUG_ASSERT(filedesc->flush_log_callback);

Aren't the above callbacks guranteed to be set by the init code?
If thats the case, there is no reason to test for these.

There is no reason to test for read_callback or write_callback as
we will always get a crash if these are not set (a few lines later).

As we already test this in ma_pagecache.h, I think we can remove the
DBUG_ASSERT's from ma_pagecache.c

> === modified file 'storage/maria/ma_pagecache.h'
> --- a/storage/maria/ma_pagecache.h	2011-02-28 17:39:30 +0000
> +++ b/storage/maria/ma_pagecache.h	2012-03-19 12:28:24 +0000
> @@ -271,6 +271,10 @@ extern void pagecache_set_write_on_delet
>      (F).read_callback= (RC); (F).write_callback= (WC); \
>      (F).write_fail= (WF); \
>      (F).flush_log_callback= (GLC); (F).callback_data= (uchar*)(D); \
> +    DBUG_ASSERT((F).read_callback); \
> +    DBUG_ASSERT((F).write_callback); \
> +    DBUG_ASSERT((F).write_fail); \
> +    DBUG_ASSERT((F).flush_log_callback); \
>    } while(0)

The above is good!

It would of course be good to always use the function
pagecache_file_init() to set all callbacks instead of setting these
directly.

To do this, you could change ma_open.c::
_ma_set_data_pagecache_callbacks() to use the above function instead
of setting these directly.

This way we would know for sure that either none or all are set and we
don't need to do any DBUG_ASSERT() of these elements in the code.

Regards,
Monty