From c7aead6fdc8d29932b090857a6e47581b3c5f20c Mon Sep 17 00:00:00 2001 From: Cedric BAIL Date: Mon, 14 Jan 2013 03:34:19 +0000 Subject: [PATCH] efl: fix cow after review by k-s. NOTE: I didn't find a way to tell valgrind that some memory is read only. SVN revision: 82742 --- src/lib/eina/eina_cow.c | 150 ++++++++++++++++++++------------- src/lib/eina/eina_cow.h | 8 +- src/tests/eina/eina_test_cow.c | 61 +++++++++++++- 3 files changed, 152 insertions(+), 67 deletions(-) diff --git a/src/lib/eina/eina_cow.c b/src/lib/eina/eina_cow.c index e891b5c6c2..9513420682 100644 --- a/src/lib/eina/eina_cow.c +++ b/src/lib/eina/eina_cow.c @@ -1,4 +1,4 @@ -/* EINA - EFL data type library +/* Eina - EFL data type library * Copyright (C) 2013 Cedric Bail * * This library is free software; you can redistribute it and/or @@ -21,6 +21,8 @@ #endif #include "eina_config.h" +#include "eina_private.h" +#include "eina_log.h" #include "eina_mempool.h" #include "eina_types.h" #include "eina_safety_checks.h" @@ -38,13 +40,13 @@ struct _Eina_Cow_Ptr { int refcount; - Eina_Bool hashed; - Eina_Bool togc; + Eina_Bool hashed : 1; + Eina_Bool togc : 1; + Eina_Bool writing : 1; }; struct _Eina_Cow_GC { - Eina_List *togc; Eina_Cow_Ptr *ref; const void * const *dst; }; @@ -70,6 +72,27 @@ typedef int (*Eina_Cow_Hash)(const void *, int); EINA_MAGIC_FAIL((d), EINA_COW_MAGIC); \ } while (0); +#define EINA_COW_PTR_SIZE \ + (sizeof (Eina_Cow_Ptr) < sizeof (void*) ? sizeof (void*) : sizeof (Eina_Cow_Ptr)) + +#define EINA_COW_PTR_GET(d) \ + (Eina_Cow_Ptr*) (((unsigned char *)d) - EINA_COW_PTR_SIZE) + +#define EINA_COW_DATA_GET(d) \ + (((unsigned char *)d) + EINA_COW_PTR_SIZE) + +static int _eina_cow_log_dom = -1; + +#ifdef ERR +#undef ERR +#endif +#define ERR(...) EINA_LOG_DOM_ERR(_eina_cow_log_dom, __VA_ARGS__) + +#ifdef DBG +#undef DBG +#endif +#define DBG(...) EINA_LOG_DOM_DBG(_eina_cow_log_dom, __VA_ARGS__) + static Eina_Mempool *gc_pool = NULL; static inline int @@ -113,8 +136,7 @@ static unsigned int _eina_cow_length(const void *key EINA_UNUSED) { /* nasty hack, has only gc need to access the hash, he will be in charge - of that global. access to the hash should be considered global. so a - lock will be needed to make multiple gc run at the same safely. + of that global. access to the hash should be considered global. */ return current_cow_size; } @@ -131,32 +153,31 @@ _eina_cow_hash_del(Eina_Cow *cow, const void *data, Eina_Cow_Ptr *ref) { - /* if eina_cow_gc is supposed to be thread safe, lock the cow here */ - if (ref->hashed) - { - current_cow_size = cow->struct_size; - eina_hash_del(cow->match, data, ref); - ref->hashed = EINA_FALSE; - } + /* eina_cow_gc is not supposed to be thread safe */ + if (!ref->hashed) return ; + + current_cow_size = cow->struct_size; + eina_hash_del(cow->match, data, ref); + ref->hashed = EINA_FALSE; } static void _eina_cow_togc_del(Eina_Cow *cow, Eina_Cow_Ptr *ref) { - /* if eina_cow_gc is supposed to be thread safe, lock the cow here */ - if (ref->togc) - { - Eina_Cow_GC *gc; - Eina_List *l; + Eina_Cow_GC *gc; + Eina_List *l; - EINA_LIST_FOREACH(cow->togc, l, gc) - if (gc->ref == ref) - { - cow->togc = eina_list_remove_list(cow->togc, l); - break; - } - ref->togc = EINA_FALSE; - } + /* eina_cow_gc is not supposed to be thread safe */ + if (!ref->togc) return ; + + EINA_LIST_FOREACH(cow->togc, l, gc) + if (gc->ref == ref) + { + cow->togc = eina_list_remove_list(cow->togc, l); + eina_mempool_free(gc_pool, gc); + break; + } + ref->togc = EINA_FALSE; } Eina_Bool @@ -164,6 +185,13 @@ eina_cow_init(void) { const char *choice, *tmp; + _eina_cow_log_dom = eina_log_domain_register("eina_cow", EINA_LOG_COLOR_DEFAULT); + if (_eina_cow_log_dom < 0) + { + EINA_LOG_ERR("Could not register log domain: eina_cow"); + return EINA_FALSE; + } + #ifdef EINA_DEFAULT_MEMPOOL choice = "pass_through"; #else @@ -176,7 +204,7 @@ eina_cow_init(void) gc_pool = eina_mempool_add(choice, "gc", NULL, sizeof (Eina_Cow_GC), 32); if (!gc_pool) { - /* ERR("ERROR: Mempool for cow '%s' cannot be allocated in eina_cow_new.", name); */ + ERR("Mempool for cow gc cannot be allocated."); return EINA_FALSE; } return EINA_TRUE; @@ -213,10 +241,10 @@ eina_cow_add(const char *name, unsigned int struct_size, unsigned int step, cons cow->pool = eina_mempool_add(choice, name, NULL, - struct_size + sizeof (Eina_Cow_Ptr), step); + struct_size + EINA_COW_PTR_SIZE, step); if (!cow->pool) { - /* ERR("ERROR: Mempool for cow '%s' cannot be allocated in eina_cow_new.", name); */ + ERR("Mempool for cow '%s' cannot be allocated.", name); goto on_error; } @@ -279,7 +307,7 @@ eina_cow_free(Eina_Cow *cow, const void *data) if (!data) return ; if (cow->default_value == data) return ; - ref = (Eina_Cow_Ptr*)(((unsigned char*) data) + cow->struct_size); + ref = EINA_COW_PTR_GET(data); ref->refcount--; if (ref->refcount > 0) return ; @@ -293,65 +321,67 @@ EAPI void * eina_cow_write(Eina_Cow *cow, const void * const *data) { Eina_Cow_Ptr *ref; - const void *src; void *r; EINA_COW_MAGIC_CHECK(cow); if (!*data) return NULL; /* cow pointer is always != NULL */ if (*data == cow->default_value) - { - src = cow->default_value; - goto allocate; - } + goto allocate; - ref = (Eina_Cow_Ptr*)(((unsigned char*) *data) + cow->struct_size); + ref = EINA_COW_PTR_GET(*data); if (ref->refcount == 1) { + if (ref->writing) + { + ERR("Request writing on an pointer that is already in a writing process %p\n", data); + return NULL; + } + _eina_cow_hash_del(cow, *data, ref); - return (void *) *data; + goto end; } - src = *data; - allocate: - r = eina_mempool_malloc(cow->pool, - cow->struct_size + sizeof (Eina_Cow_Ptr)); - memcpy(r, src, cow->struct_size); - - ref = (Eina_Cow_Ptr*)(((unsigned char*) r) + cow->struct_size); + ref = eina_mempool_malloc(cow->pool, + cow->struct_size + EINA_COW_PTR_SIZE); ref->refcount = 1; ref->hashed = EINA_FALSE; ref->togc = EINA_FALSE; + r = EINA_COW_DATA_GET(ref); + memcpy(r, *data, cow->struct_size); *((void**) data) = r; - return r; + end: + ref->writing = EINA_TRUE; + return (void *) *data; } EAPI void -eina_cow_commit(Eina_Cow *cow, const void * const * dst, const void *data) +eina_cow_done(Eina_Cow *cow, const void * const * dst, const void *data) { Eina_Cow_Ptr *ref; + Eina_Cow_GC *gc; EINA_COW_MAGIC_CHECK(cow); - ref = (Eina_Cow_Ptr*)(((unsigned char*) data) + cow->struct_size); + ref = EINA_COW_PTR_GET(data); + if (!ref->writing) + ERR("Pointer %p is not in a writable state !", dst); + ref->writing = EINA_FALSE; /* needed if we want to make cow gc safe */ - if (!ref->togc) - { - Eina_Cow_GC *gc; + if (ref->togc) return ; - gc = eina_mempool_malloc(gc_pool, sizeof (Eina_Cow_GC)); - if (!gc) return ; /* That one will not get gced this time */ + gc = eina_mempool_malloc(gc_pool, sizeof (Eina_Cow_GC)); + if (!gc) return ; /* That one will not get gced this time */ - gc->ref = ref; - gc->dst = dst; - cow->togc = gc->togc = eina_list_prepend(cow->togc, gc); - ref->togc = EINA_TRUE; - } + gc->ref = ref; + gc->dst = dst; + cow->togc = eina_list_prepend(cow->togc, gc); + ref->togc = EINA_TRUE; } EAPI void @@ -363,7 +393,7 @@ eina_cow_memcpy(Eina_Cow *cow, const void * const *dst, const void *src) eina_cow_free(cow, *dst); - ref = (Eina_Cow_Ptr*)(((unsigned char*) src) + cow->struct_size); + ref = EINA_COW_PTR_GET(src); ref->refcount++; *((const void**)dst) = src; @@ -384,7 +414,7 @@ eina_cow_gc(Eina_Cow *cow) /* Do handle hash and all funky merge think here */ gc = eina_list_data_get(eina_list_last(cow->togc)); - data = ((unsigned char*) gc->ref) - cow->struct_size; + data = EINA_COW_DATA_GET(gc->ref); gc->ref->togc = EINA_FALSE; cow->togc = eina_list_remove_list(cow->togc, eina_list_last(cow->togc)); @@ -395,7 +425,7 @@ eina_cow_gc(Eina_Cow *cow) { eina_cow_free(cow, data); - ref = (Eina_Cow_Ptr*)(((unsigned char*) match) + cow->struct_size); + ref = EINA_COW_PTR_GET(match); *((void**)gc->dst) = match; ref->refcount++; } diff --git a/src/lib/eina/eina_cow.h b/src/lib/eina/eina_cow.h index 03d3c5c0c8..710c4de8ae 100644 --- a/src/lib/eina/eina_cow.h +++ b/src/lib/eina/eina_cow.h @@ -21,14 +21,14 @@ typedef struct _Eina_Cow Eina_Cow; -EAPI Eina_Cow *eina_cow_add(const char *name, unsigned int struct_size, unsigned int step, const void *default_value); +EAPI Eina_Cow *eina_cow_add(const char *name, unsigned int struct_size, unsigned int step, const void *default_value) EINA_WARN_UNUSED_RESULT; EAPI void eina_cow_del(Eina_Cow *cow); -EAPI const void *eina_cow_alloc(Eina_Cow *cow); +EAPI const void *eina_cow_alloc(Eina_Cow *cow) EINA_WARN_UNUSED_RESULT; EAPI void eina_cow_free(Eina_Cow *cow, const void *data); -EAPI void *eina_cow_write(Eina_Cow *cow, const void * const *src); -EAPI void eina_cow_commit(Eina_Cow *cow, const void * const *dst, const void *data); +EAPI void *eina_cow_write(Eina_Cow *cow, const void * const *src) EINA_WARN_UNUSED_RESULT; +EAPI void eina_cow_done(Eina_Cow *cow, const void * const *dst, const void *data); EAPI void eina_cow_memcpy(Eina_Cow *cow, const void * const *dst, const void *src); EAPI Eina_Bool eina_cow_gc(Eina_Cow *cow); diff --git a/src/tests/eina/eina_test_cow.c b/src/tests/eina/eina_test_cow.c index 4d4d8e8f8a..cefba43f18 100644 --- a/src/tests/eina/eina_test_cow.c +++ b/src/tests/eina/eina_test_cow.c @@ -31,6 +31,55 @@ struct _Eina_Cow_Test void *d; }; +static void +_eina_test_log(const Eina_Log_Domain *d EINA_UNUSED, + Eina_Log_Level level EINA_UNUSED, const char *file EINA_UNUSED, const char *fnc EINA_UNUSED, int line EINA_UNUSED, + const char *fmt EINA_UNUSED, void *data, va_list args EINA_UNUSED) +{ + Eina_Bool *bol = data; + + *bol = EINA_TRUE; +} + +START_TEST(eina_cow_bad) +{ + const Eina_Cow_Test *cur; + Eina_Cow_Test *write; + Eina_Cow *cow; + Eina_Bool over_commit = EINA_FALSE; + Eina_Bool over_writing = EINA_FALSE; + Eina_Cow_Test default_value = { 7, 42, NULL }; + + cow = eina_cow_add("COW Test", sizeof (Eina_Cow_Test), 16, &default_value); + fail_if(cow == NULL); + + cur = eina_cow_alloc(cow); + fail_if(cur == NULL); + + write = eina_cow_write(cow, &cur); + fail_if(write == NULL || write == &default_value); + + write->i = 7; + eina_cow_done(cow, &cur, write); + fail_if(cur->i != 7 || default_value.i != 42); + + eina_log_print_cb_set(_eina_test_log, &over_commit); + eina_cow_done(cow, &cur, write); /* Testing over commit */ + fail_if(!over_commit); + + write = eina_cow_write(cow, &cur); + fail_if(write == NULL || write == &default_value); + + eina_log_print_cb_set(_eina_test_log, &over_writing); + write = eina_cow_write(cow, &cur); /* Testing over writing */ + fail_if(write != NULL || !over_writing); + + eina_cow_free(cow, cur); + + eina_cow_del(cow); +} +END_TEST + START_TEST(eina_cow) { const Eina_Cow_Test *prev; @@ -50,7 +99,7 @@ START_TEST(eina_cow) fail_if(write == NULL || write == &default_value); write->i = 7; - eina_cow_commit(cow, &cur, write); + eina_cow_done(cow, &cur, write); fail_if(cur->i != 7 || prev->i != 0); eina_cow_memcpy(cow, &prev, cur); @@ -61,7 +110,7 @@ START_TEST(eina_cow) fail_if(write == NULL || write == &default_value); write->i = 42; write->c = 5; - eina_cow_commit(cow, &cur, write); + eina_cow_done(cow, &cur, write); fail_if(cur->i != 42 || cur->c != 5 || prev->i != 7 || prev->c != 42 || default_value.c != 42 || default_value.i != 0); @@ -71,10 +120,15 @@ START_TEST(eina_cow) write = eina_cow_write(cow, &cur); write->i = 7; write->c = 42; - eina_cow_commit(cow, &cur, write); + eina_cow_done(cow, &cur, write); fail_if(eina_cow_gc(cow) == EINA_FALSE); fail_if(cur != prev); + + eina_cow_free(cow, cur); + eina_cow_free(cow, prev); + + eina_cow_del(cow); } END_TEST @@ -82,4 +136,5 @@ void eina_test_cow(TCase *tc) { tcase_add_test(tc, eina_cow); + tcase_add_test(tc, eina_cow_bad); }