[Asterisk-code-review] core: Remove ABI effects of LOW MEMORY. (asterisk[master])

Joshua Colp asteriskteam at digium.com
Fri Sep 30 06:49:34 CDT 2016 

Joshua Colp has submitted this change and it was merged.

Change subject: core: Remove ABI effects of LOW_MEMORY.
......................................................................


core: Remove ABI effects of LOW_MEMORY.

This allows asterisk to compiled with LOW_MEMORY to load modules built
without LOW_MEMORY.

ASTERISK-26398 #close

Change-Id: I24b78ac9493ab933b11087a8b6794f3c96d4872d
---
M CHANGES
M UPGRADE.txt
M build_tools/make_buildopts_h
M include/asterisk.h
M include/asterisk/inline_api.h
M include/asterisk/lock.h
M include/asterisk/utils.h
M main/asterisk.c
M main/utils.c
M utils/ael_main.c
M utils/astman.c
M utils/check_expr.c
M utils/conf2ael.c
13 files changed, 85 insertions(+), 71 deletions(-)

Approvals:
  Mark Michelson: Looks good to me, but someone else must approve
  Joshua Colp: Looks good to me, approved; Verified



diff --git a/CHANGES b/CHANGES
index da732a7..be27987 100644
--- a/CHANGES
+++ b/CHANGES
@@ -12,6 +12,13 @@
 --- Functionality changes from Asterisk 14 to Asterisk 15 --------------------
 ------------------------------------------------------------------------------
 
+Build System
+------------------
+ * LOW_MEMORY no longer has an effect on Asterisk ABI.  Symbols that were
+   previously suppressed by LOW_MEMORY are now replaced by stub functions.
+   Asterisk built with LOW_MEMORY can now successfully load binary modules
+   built without LOW_MEMORY and vice versa.
+
 chan_sip
 ------------------
  * If an offer is received with optional SRTP (a media stream with RTP/AVP but
diff --git a/UPGRADE.txt b/UPGRADE.txt
index ad03edf..7acc7a8 100644
--- a/UPGRADE.txt
+++ b/UPGRADE.txt
@@ -22,3 +22,7 @@
 === UPGRADE-13.txt  -- Upgrade info for 12 to 13
 === UPGRADE-14.txt  -- Upgrade info for 13 to 14
 ===========================================================
+
+Build System:
+ - The LOW_MEMORY compile option no longer disables inline API.  To disable
+   inline API you must use the DISABLE_INLINE option.
diff --git a/build_tools/make_buildopts_h b/build_tools/make_buildopts_h
index c96e508..900e984 100755
--- a/build_tools/make_buildopts_h
+++ b/build_tools/make_buildopts_h
@@ -24,6 +24,7 @@
 			-o "${x}" = "BETTER_BACKTRACES" \
 			-o "${x}" = "LOTS_OF_SPANS" \
 			-o "${x}" = "BUILD_NATIVE" \
+			-o "${x}" = "LOW_MEMORY" \
 			-o "${x}" = "REF_DEBUG" \
 			-o "${x}" = "AO2_DEBUG" \
 			-o "${x}" = "REBUILD_PARSERS" \
diff --git a/include/asterisk.h b/include/asterisk.h
index 93c7cfb..f5ed091 100644
--- a/include/asterisk.h
+++ b/include/asterisk.h
@@ -152,7 +152,6 @@
  */
 int ast_shutdown_final(void);
 
-#if !defined(LOW_MEMORY)
 /*!
  * \brief Register the version of a source code file with the core.
  * \param file the source file name
@@ -225,11 +224,7 @@
 		__ast_unregister_file(__FILE__); \
 	}
 #endif /* !MTX_PROFILE */
-#else /* LOW_MEMORY */
-#define ASTERISK_REGISTER_FILE()
-#endif /* LOW_MEMORY */
 
-#if !defined(LOW_MEMORY)
 /*!
  * \brief support for event profiling
  *
@@ -248,11 +243,6 @@
 int ast_add_profile(const char *, uint64_t scale);
 int64_t ast_profile(int, int64_t);
 int64_t ast_mark(int, int start1_stop0);
-#else /* LOW_MEMORY */
-#define ast_add_profile(a, b) 0
-#define ast_profile(a, b) do { } while (0)
-#define ast_mark(a, b) do { } while (0)
-#endif /* LOW_MEMORY */
 
 /*! \brief
  * Definition of various structures that many asterisk files need,
diff --git a/include/asterisk/inline_api.h b/include/asterisk/inline_api.h
index 291a838..5f7a43b 100644
--- a/include/asterisk/inline_api.h
+++ b/include/asterisk/inline_api.h
@@ -33,7 +33,7 @@
 	copies of the function body are not built in different modules.
 	However, since this doesn't work for clang, we go with 'static'
 	anyway and hope for the best!
-	- when LOW_MEMORY is defined, inlining should be disabled
+	- when DISABLE_INLINE is defined, inlining should be disabled
 	completely, even if the compiler is configured to support it
 
   The AST_INLINE_API macro allows this to happen automatically, when
@@ -45,7 +45,7 @@
   including the header file
  */
 
-#if !defined(LOW_MEMORY) && !defined(DISABLE_INLINE)
+#if !defined(DISABLE_INLINE)
 
 #if !defined(AST_API_MODULE)
 #if defined(__clang__) || defined(__GNUC_STDC_INLINE__)
@@ -57,7 +57,7 @@
 #define AST_INLINE_API(hdr, body) hdr; hdr body
 #endif
 
-#else /* defined(LOW_MEMORY) */
+#else /* defined(DISABLE_INLINE) */
 
 #if !defined(AST_API_MODULE)
 #define AST_INLINE_API(hdr, body) hdr;
diff --git a/include/asterisk/lock.h b/include/asterisk/lock.h
index 652ca13..5b6817f 100644
--- a/include/asterisk/lock.h
+++ b/include/asterisk/lock.h
@@ -240,7 +240,6 @@
  * lock info struct.  The lock is marked as pending as the thread is waiting
  * on the lock.  ast_mark_lock_acquired() will mark it as held by this thread.
  */
-#if !defined(LOW_MEMORY)
 #ifdef HAVE_BKTR
 void ast_store_lock_info(enum ast_lock_type type, const char *filename,
 	int line_num, const char *func, const char *lock_name, void *lock_addr, struct ast_bt *bt);
@@ -249,32 +248,15 @@
 	int line_num, const char *func, const char *lock_name, void *lock_addr);
 #endif /* HAVE_BKTR */
 
-#else
-
-#ifdef HAVE_BKTR
-#define ast_store_lock_info(I,DONT,CARE,ABOUT,THE,PARAMETERS,BUD)
-#else
-#define ast_store_lock_info(I,DONT,CARE,ABOUT,THE,PARAMETERS)
-#endif /* HAVE_BKTR */
-#endif /* !defined(LOW_MEMORY) */
-
 /*!
  * \brief Mark the last lock as acquired
  */
-#if !defined(LOW_MEMORY)
 void ast_mark_lock_acquired(void *lock_addr);
-#else
-#define ast_mark_lock_acquired(ignore)
-#endif
 
 /*!
  * \brief Mark the last lock as failed (trylock)
  */
-#if !defined(LOW_MEMORY)
 void ast_mark_lock_failed(void *lock_addr);
-#else
-#define ast_mark_lock_failed(ignore)
-#endif
 
 /*!
  * \brief remove lock info for the current thread
@@ -282,7 +264,6 @@
  * this gets called by ast_mutex_unlock so that information on the lock can
  * be removed from the current thread's lock info struct.
  */
-#if !defined(LOW_MEMORY)
 #ifdef HAVE_BKTR
 void ast_remove_lock_info(void *lock_addr, struct ast_bt *bt);
 #else
@@ -290,15 +271,6 @@
 #endif /* HAVE_BKTR */
 void ast_suspend_lock_info(void *lock_addr);
 void ast_restore_lock_info(void *lock_addr);
-#else
-#ifdef HAVE_BKTR
-#define ast_remove_lock_info(ignore,me)
-#else
-#define ast_remove_lock_info(ignore)
-#endif /* HAVE_BKTR */
-#define ast_suspend_lock_info(ignore);
-#define ast_restore_lock_info(ignore);
-#endif /* !defined(LOW_MEMORY) */
 
 /*!
  * \brief log info for the current lock with ast_log().
@@ -333,11 +305,7 @@
  * this gets called during deadlock avoidance, so that the information may
  * be preserved as to what location originally acquired the lock.
  */
-#if !defined(LOW_MEMORY)
 int ast_find_lock_info(void *lock_addr, char *filename, size_t filename_size, int *lineno, char *func, size_t func_size, char *mutex_name, size_t mutex_name_size);
-#else
-#define ast_find_lock_info(a,b,c,d,e,f,g,h) -1
-#endif
 
 /*!
  * \brief Unlock a lock briefly
diff --git a/include/asterisk/utils.h b/include/asterisk/utils.h
index 9789d34..f9608f5 100644
--- a/include/asterisk/utils.h
+++ b/include/asterisk/utils.h
@@ -423,13 +423,12 @@
  * Thread management support (should be moved to lock.h or a different header)
  */
 
-#define AST_STACKSIZE (((sizeof(void *) * 8 * 8) - 16) * 1024)
+#define AST_STACKSIZE     (((sizeof(void *) * 8 * 8) - 16) * 1024)
+#define AST_STACKSIZE_LOW (((sizeof(void *) * 8 * 2) - 16) * 1024)
 
-#if defined(LOW_MEMORY)
-#define AST_BACKGROUND_STACKSIZE (((sizeof(void *) * 8 * 2) - 16) * 1024)
-#else
-#define AST_BACKGROUND_STACKSIZE AST_STACKSIZE
-#endif
+int ast_background_stacksize(void);
+
+#define AST_BACKGROUND_STACKSIZE ast_background_stacksize()
 
 void ast_register_thread(char *name);
 void ast_unregister_thread(void *id);
diff --git a/main/asterisk.c b/main/asterisk.c
index 94481ee..7fc0912 100644
--- a/main/asterisk.c
+++ b/main/asterisk.c
@@ -494,9 +494,11 @@
 };
 
 static AST_RWLIST_HEAD_STATIC(registered_files, registered_file);
+#endif /* ! LOW_MEMORY */
 
 void __ast_register_file(const char *file)
 {
+#if !defined(LOW_MEMORY)
 	struct registered_file *reg;
 
 	reg = ast_calloc(1, sizeof(*reg));
@@ -508,10 +510,12 @@
 	AST_RWLIST_WRLOCK(&registered_files);
 	AST_RWLIST_INSERT_HEAD(&registered_files, reg, list);
 	AST_RWLIST_UNLOCK(&registered_files);
+#endif /* ! LOW_MEMORY */
 }
 
 void __ast_unregister_file(const char *file)
 {
+#if !defined(LOW_MEMORY)
 	struct registered_file *find;
 
 	AST_RWLIST_WRLOCK(&registered_files);
@@ -527,10 +531,12 @@
 	if (find) {
 		ast_free(find);
 	}
+#endif /* ! LOW_MEMORY */
 }
 
 char *ast_complete_source_filename(const char *partial, int n)
 {
+#if !defined(LOW_MEMORY)
 	struct registered_file *find;
 	size_t len = strlen(partial);
 	int count = 0;
@@ -545,8 +551,12 @@
 	}
 	AST_RWLIST_UNLOCK(&registered_files);
 	return res;
+#else /* if defined(LOW_MEMORY) */
+	return NULL;
+#endif
 }
 
+#if !defined(LOW_MEMORY)
 struct thread_list_t {
 	AST_RWLIST_ENTRY(thread_list_t) list;
 	char *name;
@@ -872,12 +882,14 @@
 };
 
 static struct profile_data *prof_data;
+#endif /* ! LOW_MEMORY */
 
 /*! \brief allocates a counter with a given name and scale.
  * \return Returns the identifier of the counter.
  */
 int ast_add_profile(const char *name, uint64_t scale)
 {
+#if !defined(LOW_MEMORY)
 	int l = sizeof(struct profile_data);
 	int n = 10;	/* default entries */
 
@@ -904,10 +916,14 @@
 	prof_data->e[n].mark = 0;
 	prof_data->e[n].scale = scale;
 	return n;
+#else /* if defined(LOW_MEMORY) */
+	return 0;
+#endif
 }
 
 int64_t ast_profile(int i, int64_t delta)
 {
+#if !defined(LOW_MEMORY)
 	if (!prof_data || i < 0 || i > prof_data->entries)	/* invalid index */
 		return 0;
 	if (prof_data->e[i].scale > 1)
@@ -915,8 +931,12 @@
 	prof_data->e[i].value += delta;
 	prof_data->e[i].events++;
 	return prof_data->e[i].value;
+#else /* if defined(LOW_MEMORY) */
+	return 0;
+#endif
 }
 
+#if !defined(LOW_MEMORY)
 /* The RDTSC instruction was introduced on the Pentium processor and is not
  * implemented on certain clones, like the Cyrix 586. Hence, the previous
  * expectation of __i386__ was in error. */
@@ -940,9 +960,11 @@
 	return 0;
 }
 #endif
+#endif /* ! LOW_MEMORY */
 
 int64_t ast_mark(int i, int startstop)
 {
+#if !defined(LOW_MEMORY)
 	if (!prof_data || i < 0 || i > prof_data->entries) /* invalid index */
 		return 0;
 	if (startstop == 1)
@@ -955,8 +977,12 @@
 		prof_data->e[i].events++;
 	}
 	return prof_data->e[i].mark;
+#else /* if defined(LOW_MEMORY) */
+	return 0;
+#endif
 }
 
+#if !defined(LOW_MEMORY)
 #define DEFINE_PROFILE_MIN_MAX_VALUES min = 0; \
 	max = prof_data->entries;\
 	if  (a->argc > 3) { /* specific entries */ \
diff --git a/main/utils.c b/main/utils.c
index 73b1c87..c8ede91 100644
--- a/main/utils.c
+++ b/main/utils.c
@@ -614,10 +614,9 @@
 #undef pthread_create /* For ast_pthread_create function only */
 #endif /* !__linux__ */
 
-#if !defined(LOW_MEMORY)
-
 #ifdef DEBUG_THREADS
 
+#if !defined(LOW_MEMORY)
 /*! \brief A reasonable maximum number of locks a thread would be holding ... */
 #define AST_MAX_LOCKS 64
 
@@ -720,6 +719,8 @@
  * \brief The thread storage key for per-thread lock info
  */
 AST_THREADSTORAGE_CUSTOM(thread_lock_info, NULL, lock_info_destroy);
+#endif /* ! LOW_MEMORY */
+
 #ifdef HAVE_BKTR
 void ast_store_lock_info(enum ast_lock_type type, const char *filename,
 	int line_num, const char *func, const char *lock_name, void *lock_addr, struct ast_bt *bt)
@@ -728,6 +729,7 @@
 	int line_num, const char *func, const char *lock_name, void *lock_addr)
 #endif
 {
+#if !defined(LOW_MEMORY)
 	struct thr_lock_info *lock_info;
 	int i;
 
@@ -778,10 +780,12 @@
 	lock_info->num_locks++;
 
 	pthread_mutex_unlock(&lock_info->lock);
+#endif /* ! LOW_MEMORY */
 }
 
 void ast_mark_lock_acquired(void *lock_addr)
 {
+#if !defined(LOW_MEMORY)
 	struct thr_lock_info *lock_info;
 
 	if (!(lock_info = ast_threadstorage_get(&thread_lock_info, sizeof(*lock_info))))
@@ -792,10 +796,12 @@
 		lock_info->locks[lock_info->num_locks - 1].pending = 0;
 	}
 	pthread_mutex_unlock(&lock_info->lock);
+#endif /* ! LOW_MEMORY */
 }
 
 void ast_mark_lock_failed(void *lock_addr)
 {
+#if !defined(LOW_MEMORY)
 	struct thr_lock_info *lock_info;
 
 	if (!(lock_info = ast_threadstorage_get(&thread_lock_info, sizeof(*lock_info))))
@@ -807,10 +813,12 @@
 		lock_info->locks[lock_info->num_locks - 1].times_locked--;
 	}
 	pthread_mutex_unlock(&lock_info->lock);
+#endif /* ! LOW_MEMORY */
 }
 
 int ast_find_lock_info(void *lock_addr, char *filename, size_t filename_size, int *lineno, char *func, size_t func_size, char *mutex_name, size_t mutex_name_size)
 {
+#if !defined(LOW_MEMORY)
 	struct thr_lock_info *lock_info;
 	int i = 0;
 
@@ -838,10 +846,14 @@
 	pthread_mutex_unlock(&lock_info->lock);
 
 	return 0;
+#else /* if defined(LOW_MEMORY) */
+	return -1;
+#endif
 }
 
 void ast_suspend_lock_info(void *lock_addr)
 {
+#if !defined(LOW_MEMORY)
 	struct thr_lock_info *lock_info;
 	int i = 0;
 
@@ -865,10 +877,12 @@
 	lock_info->locks[i].suspended = 1;
 
 	pthread_mutex_unlock(&lock_info->lock);
+#endif /* ! LOW_MEMORY */
 }
 
 void ast_restore_lock_info(void *lock_addr)
 {
+#if !defined(LOW_MEMORY)
 	struct thr_lock_info *lock_info;
 	int i = 0;
 
@@ -891,6 +905,7 @@
 	lock_info->locks[i].suspended = 0;
 
 	pthread_mutex_unlock(&lock_info->lock);
+#endif /* ! LOW_MEMORY */
 }
 
 
@@ -900,6 +915,7 @@
 void ast_remove_lock_info(void *lock_addr)
 #endif
 {
+#if !defined(LOW_MEMORY)
 	struct thr_lock_info *lock_info;
 	int i = 0;
 
@@ -937,8 +953,10 @@
 	lock_info->num_locks--;
 
 	pthread_mutex_unlock(&lock_info->lock);
+#endif /* ! LOW_MEMORY */
 }
 
+#if !defined(LOW_MEMORY)
 static const char *locktype2str(enum ast_lock_type type)
 {
 	switch (type) {
@@ -1017,7 +1035,7 @@
 	}
 	ast_reentrancy_unlock(lt);
 }
-
+#endif /* ! LOW_MEMORY */
 
 /*! This function can help you find highly temporal locks; locks that happen for a
     short time, but at unexpected times, usually at times that create a deadlock,
@@ -1040,6 +1058,7 @@
 */
 void ast_log_show_lock(void *this_lock_addr)
 {
+#if !defined(LOW_MEMORY)
 	struct thr_lock_info *lock_info;
 	struct ast_str *str;
 
@@ -1066,11 +1085,13 @@
 	}
 	pthread_mutex_unlock(&lock_infos_lock.mutex);
 	ast_free(str);
+#endif /* ! LOW_MEMORY */
 }
 
 
 struct ast_str *ast_dump_locks(void)
 {
+#if !defined(LOW_MEMORY)
 	struct thr_lock_info *lock_info;
 	struct ast_str *str;
 
@@ -1137,8 +1158,12 @@
 	               "\n");
 
 	return str;
+#else /* if defined(LOW_MEMORY) */
+	return NULL;
+#endif
 }
 
+#if !defined(LOW_MEMORY)
 static char *handle_show_locks(struct ast_cli_entry *e, int cmd, struct ast_cli_args *a)
 {
 	struct ast_str *str;
@@ -1172,9 +1197,10 @@
 static struct ast_cli_entry utils_cli[] = {
 	AST_CLI_DEFINE(handle_show_locks, "Show which locks are held by which thread"),
 };
-
+#endif /* ! LOW_MEMORY */
 #endif /* DEBUG_THREADS */
 
+#if !defined(LOW_MEMORY)
 /*
  * support for 'show threads'. The start routine is wrapped by
  * dummy_start(), so that ast_register_thread() and
@@ -1236,6 +1262,15 @@
 
 #endif /* !LOW_MEMORY */
 
+int ast_background_stacksize(void)
+{
+#if !defined(LOW_MEMORY)
+	return AST_STACKSIZE;
+#else
+	return AST_STACKSIZE_LOW;
+#endif
+}
+
 int ast_pthread_create_stack(pthread_t *thread, pthread_attr_t *attr, void *(*start_routine)(void *),
 			     void *data, size_t stacksize, const char *file, const char *caller,
 			     int line, const char *start_fn)
diff --git a/utils/ael_main.c b/utils/ael_main.c
index d2c815b..1801b12 100644
--- a/utils/ael_main.c
+++ b/utils/ael_main.c
@@ -32,10 +32,8 @@
 
 int option_debug = 0;
 int option_verbose = 0;
-#if !defined(LOW_MEMORY)
 void __ast_register_file(const char *file) { }
 void __ast_unregister_file(const char *file) { }
-#endif
 
 /*** MODULEINFO
   	<depend>res_ael_share</depend>
@@ -180,7 +178,6 @@
 	return 0; /* in "standalone" mode, functions are just not avail */
 }
 
-#if !defined(LOW_MEMORY)
 int ast_add_profile(const char *x, uint64_t scale)
 {
 	if (!no_comp)
@@ -188,7 +185,6 @@
 
 	return 0;
 }
-#endif
 
 int ast_loader_register(int (*updater)(void))
 {
@@ -606,7 +602,6 @@
 }
 
 #ifdef DEBUG_THREADS
-#if !defined(LOW_MEMORY)
 void ast_mark_lock_acquired(void *lock_addr)
 {
 }
@@ -652,5 +647,4 @@
 void ast_restore_lock_info(void *lock_addr)
 {
 }
-#endif /* !defined(LOW_MEMORY) */
 #endif /* DEBUG_THREADS */
diff --git a/utils/astman.c b/utils/astman.c
index 50e6a89..a6803c8 100644
--- a/utils/astman.c
+++ b/utils/astman.c
@@ -98,7 +98,6 @@
 {
 }
 
-#if !defined(LOW_MEMORY)
 int ast_add_profile(const char *, uint64_t scale);
 int ast_add_profile(const char *s, uint64_t scale)
 {
@@ -115,7 +114,6 @@
 {
 	return 0;
 }
-#endif /* LOW_MEMORY */
 
 /* end of dummy functions */
 
diff --git a/utils/check_expr.c b/utils/check_expr.c
index 36ff132..abfb91d 100644
--- a/utils/check_expr.c
+++ b/utils/check_expr.c
@@ -56,7 +56,6 @@
 #endif
 
 #ifdef DEBUG_THREADS
-#if !defined(LOW_MEMORY)
 #ifdef HAVE_BKTR
 void ast_store_lock_info(enum ast_lock_type type, const char *filename,
 		        int line_num, const char *func, const char *lock_name, void *lock_addr, struct ast_bt *bt);
@@ -117,7 +116,6 @@
 {
     /* not a lot to do in a standalone w/o threading! */
 }
-#endif
 #endif /* DEBUG_THREADS */
 
 
@@ -161,9 +159,7 @@
 
 void __ast_register_file(const char *file);
 void __ast_register_file(const char *file) { }
-#if !defined(LOW_MEMORY)
 int ast_add_profile(const char *x, uint64_t scale) { return 0;}
-#endif
 int ast_atomic_fetchadd_int_slow(volatile int *p, int v)
 {
         int ret;
diff --git a/utils/conf2ael.c b/utils/conf2ael.c
index 3136fe3..3ebc56e 100644
--- a/utils/conf2ael.c
+++ b/utils/conf2ael.c
@@ -88,9 +88,7 @@
 void __ast_unregister_file(const char *file)
 {
 }
-#if !defined(LOW_MEMORY)
 int ast_add_profile(const char *x, uint64_t scale) { return 0;}
-#endif
 /* Our own version of ast_log, since the expr parser uses it. -- stolen from utils/check_expr.c */
 void ast_log(int level, const char *file, int line, const char *function, const char *fmt, ...) __attribute__((format(printf,5,6)));
 
@@ -708,7 +706,6 @@
 }
 
 #ifdef DEBUG_THREADS
-#if !defined(LOW_MEMORY)
 void ast_mark_lock_acquired(void *lock_addr)
 {
 }
@@ -755,5 +752,4 @@
 void ast_restore_lock_info(void *lock_addr)
 {
 }
-#endif /* !defined(LOW_MEMORY) */
 #endif /* DEBUG_THREADS */

-- 
To view, visit https://gerrit.asterisk.org/3960
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I24b78ac9493ab933b11087a8b6794f3c96d4872d
Gerrit-PatchSet: 3
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>

More information about the asterisk-code-review mailing list