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

@@ -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 hashed : 1;
-   Eina_Bool togc;
+   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
+      of that global. access to the hash should be considered global.
-      lock will be needed to make multiple gc run at the same safely.
     */
    return current_cow_size;
 }
@@ -131,39 +153,45 @@ _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 */
+   /* eina_cow_gc is not supposed to be thread safe */
-   if (ref->hashed)
+   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 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
 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,66 +321,68 @@ 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;
-     }
 
-   ref = (Eina_Cow_Ptr*)(((unsigned char*) *data) + cow->struct_size);
+   ref = EINA_COW_PTR_GET(*data);
 
    if (ref->refcount == 1)
      {
-        _eina_cow_hash_del(cow, *data, ref);
+        if (ref->writing)
-        return (void *) *data;
+          {
+             ERR("Request writing on an pointer that is already in a writing process %p\n", data);
+             return NULL;
           }
 
-   src = *data;
+        _eina_cow_hash_del(cow, *data, ref);
+        goto end;
+     }
 
  allocate:
-   r = eina_mempool_malloc(cow->pool,
+   ref = eina_mempool_malloc(cow->pool,
-                           cow->struct_size + sizeof (Eina_Cow_Ptr));
+                             cow->struct_size + EINA_COW_PTR_SIZE);
-   memcpy(r, src, cow->struct_size);
-
-   ref = (Eina_Cow_Ptr*)(((unsigned char*) r) + cow->struct_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)
+   if (ref->togc) return ;
-     {
-        Eina_Cow_GC *gc;
 
    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);
+   cow->togc = eina_list_prepend(cow->togc, gc);
    ref->togc = EINA_TRUE;
 }
-}
 
 EAPI void
 eina_cow_memcpy(Eina_Cow *cow, const void * const *dst, const void *src)
@@ -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++;
      }

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_write(Eina_Cow *cow, const void * const *src) EINA_WARN_UNUSED_RESULT;
-EAPI void eina_cow_commit(Eina_Cow *cow, const void * const *dst, const void *data);
+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);

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);
 }