From 27c61ce8f6ee66d910507f8a40ae5497287c943e Mon Sep 17 00:00:00 2001 From: Philippe Proulx Date: Mon, 11 Nov 2019 14:36:58 -0500 Subject: [PATCH] lib: make bt_value_map_foreach_entry_{const_}func() return a status code This patch makes the bt_value_map_foreach_entry_func() and bt_value_map_foreach_entry_const_func() types return status codes instead of `bt_bool`. The available status codes are `OK` (continue the loop, like returning `BT_TRUE` before this patch), `ERROR`, `MEMORY_ERROR`, and `INTERRUPT` (break the loop, like returning `BT_FALSE` before this patch). When the user function returns `BT_VALUE_MAP_FOREACH_ENTRY_FUNC_STATUS_ERROR`, bt_value_map_foreach_entry() returns `BT_VALUE_MAP_FOREACH_ENTRY_STATUS_USER_ERROR` (set to the new global status code `__BT_FUNC_STATUS_USER_ERROR`). This makes it possible to distinguish between an internal library error and a user function error. The purpose of this patch is to make it possible for a user function to append an error cause to the current thread's error the same way other user functions do: append the cause and return an error status. For example, consider this scenario: 1. User calls bt_value_map_foreach_entry() with a user function and user data which contains a status member. 2. User function calls a library function which fails. The library function appends a cause to the current thread's error. 3. User function appends a cause to the current thread's error. 4. User function sets the user data's status member to the failing library function's status and returns `BT_FALSE` to interrupt the loop. 5. bt_value_map_foreach_entry() returns `BT_VALUE_MAP_FOREACH_ENTRY_STATUS_INTERRUPTED` (not an error status). 6. The caller of bt_value_map_foreach_entry() interprets `BT_VALUE_MAP_FOREACH_ENTRY_STATUS_INTERRUPTED` as an error because the user data's status member has an error value. With this patch, it becomes: 1. User calls bt_value_map_foreach_entry() with a user function. 2. User function calls a library function which fails. The library function appends a cause to the current thread's error. 3. User function appends a cause to the current thread's error. 4. User function returns `BT_VALUE_MAP_FOREACH_ENTRY_FUNC_STATUS_ERROR` or `BT_VALUE_MAP_FOREACH_ENTRY_FUNC_STATUS_MEMORY_ERROR`, depending on the failing library function's status, which breaks the loop. 5. bt_value_map_foreach_entry() appends a cause to the current thread's error and returns `BT_VALUE_MAP_FOREACH_ENTRY_STATUS_USER_ERROR` or `BT_VALUE_MAP_FOREACH_ENTRY_STATUS_MEMORY_ERROR`. The latter seems more natural to me. In Python, nothing wraps bt_value_map_foreach_entry() directly, so I just converted bt_value_map_get_keys_cb() to return the appropriate status instead of `BT_TRUE` or `BT_FALSE`. In other words, bt2.utils._handle_func_status() does not need any change because it will never receive `native_bt.__BT_FUNC_STATUS_USER_ERROR`. Signed-off-by: Philippe Proulx Change-Id: I235d7957003b51630f4a2f72c1ccdef89d6173e8 Reviewed-on: https://review.lttng.org/c/babeltrace/+/2365 Reviewed-by: Francis Deslauriers Reviewed-by: Simon Marchi Tested-by: jenkins --- include/babeltrace2/babeltrace.h | 1 + include/babeltrace2/func-status.h | 5 ++ include/babeltrace2/value-const.h | 15 +++- include/babeltrace2/value.h | 15 +++- .../python/bt2/bt2/native_bt_value.i.h | 13 ++-- src/cli/babeltrace2.c | 5 +- src/common/common.h | 2 + src/lib/func-status.h | 1 + src/lib/value.c | 78 ++++++++++++------- .../param-validation/param-validation.c | 8 +- src/plugins/text/details/write.c | 6 +- tests/lib/test_bt_values.c | 35 +++++++-- 12 files changed, 130 insertions(+), 54 deletions(-) diff --git a/include/babeltrace2/babeltrace.h b/include/babeltrace2/babeltrace.h index 5e099a59..e2ea83cf 100644 --- a/include/babeltrace2/babeltrace.h +++ b/include/babeltrace2/babeltrace.h @@ -170,6 +170,7 @@ #undef __BT_FUNC_STATUS_NOT_FOUND #undef __BT_FUNC_STATUS_OK #undef __BT_FUNC_STATUS_OVERFLOW_ERROR +#undef __BT_FUNC_STATUS_USER_ERROR #undef __BT_IN_BABELTRACE_H #undef __BT_UPCAST #undef __BT_UPCAST_CONST diff --git a/include/babeltrace2/func-status.h b/include/babeltrace2/func-status.h index 0f1829af..444b219e 100644 --- a/include/babeltrace2/func-status.h +++ b/include/babeltrace2/func-status.h @@ -47,6 +47,11 @@ # define __BT_FUNC_STATUS_MEMORY_ERROR -12 #endif +/* User function error */ +#ifndef __BT_FUNC_STATUS_USER_ERROR +# define __BT_FUNC_STATUS_USER_ERROR -2 +#endif + /* General error */ #ifndef __BT_FUNC_STATUS_ERROR # define __BT_FUNC_STATUS_ERROR -1 diff --git a/include/babeltrace2/value-const.h b/include/babeltrace2/value-const.h index 0001ddd8..f42f60d8 100644 --- a/include/babeltrace2/value-const.h +++ b/include/babeltrace2/value-const.h @@ -164,11 +164,22 @@ bt_bool bt_value_map_is_empty(const bt_value *map_obj) extern const bt_value *bt_value_map_borrow_entry_value_const( const bt_value *map_obj, const char *key); -typedef bt_bool (* bt_value_map_foreach_entry_const_func)(const char *key, - const bt_value *object, void *data); +typedef enum bt_value_map_foreach_entry_const_func_status { + BT_VALUE_MAP_FOREACH_ENTRY_CONST_FUNC_STATUS_OK = __BT_FUNC_STATUS_OK, + BT_VALUE_MAP_FOREACH_ENTRY_CONST_FUNC_STATUS_ERROR = __BT_FUNC_STATUS_ERROR, + BT_VALUE_MAP_FOREACH_ENTRY_CONST_FUNC_STATUS_MEMORY_ERROR = __BT_FUNC_STATUS_MEMORY_ERROR, + BT_VALUE_MAP_FOREACH_ENTRY_CONST_FUNC_STATUS_INTERRUPT = __BT_FUNC_STATUS_INTERRUPTED, +} bt_value_map_foreach_entry_const_func_status; + +typedef bt_value_map_foreach_entry_const_func_status + (* bt_value_map_foreach_entry_const_func)(const char *key, + const bt_value *object, void *data); typedef enum bt_value_map_foreach_entry_const_status { BT_VALUE_MAP_FOREACH_ENTRY_CONST_STATUS_OK = __BT_FUNC_STATUS_OK, + BT_VALUE_MAP_FOREACH_ENTRY_CONST_STATUS_ERROR = __BT_FUNC_STATUS_ERROR, + BT_VALUE_MAP_FOREACH_ENTRY_CONST_STATUS_MEMORY_ERROR = __BT_FUNC_STATUS_MEMORY_ERROR, + BT_VALUE_MAP_FOREACH_ENTRY_CONST_STATUS_USER_ERROR = __BT_FUNC_STATUS_USER_ERROR, BT_VALUE_MAP_FOREACH_ENTRY_CONST_STATUS_INTERRUPTED = __BT_FUNC_STATUS_INTERRUPTED, } bt_value_map_foreach_entry_const_status; diff --git a/include/babeltrace2/value.h b/include/babeltrace2/value.h index e01b48fe..42e3cf96 100644 --- a/include/babeltrace2/value.h +++ b/include/babeltrace2/value.h @@ -126,11 +126,22 @@ extern bt_value *bt_value_map_create(void); extern bt_value *bt_value_map_borrow_entry_value( bt_value *map_obj, const char *key); -typedef bt_bool (* bt_value_map_foreach_entry_func)(const char *key, - bt_value *object, void *data); +typedef enum bt_value_map_foreach_entry_func_status { + BT_VALUE_MAP_FOREACH_ENTRY_FUNC_STATUS_OK = __BT_FUNC_STATUS_OK, + BT_VALUE_MAP_FOREACH_ENTRY_FUNC_STATUS_ERROR = __BT_FUNC_STATUS_ERROR, + BT_VALUE_MAP_FOREACH_ENTRY_FUNC_STATUS_MEMORY_ERROR = __BT_FUNC_STATUS_MEMORY_ERROR, + BT_VALUE_MAP_FOREACH_ENTRY_FUNC_STATUS_INTERRUPT = __BT_FUNC_STATUS_INTERRUPTED, +} bt_value_map_foreach_entry_func_status; + +typedef bt_value_map_foreach_entry_func_status + (* bt_value_map_foreach_entry_func)(const char *key, + bt_value *object, void *data); typedef enum bt_value_map_foreach_entry_status { BT_VALUE_MAP_FOREACH_ENTRY_STATUS_OK = __BT_FUNC_STATUS_OK, + BT_VALUE_MAP_FOREACH_ENTRY_STATUS_ERROR = __BT_FUNC_STATUS_ERROR, + BT_VALUE_MAP_FOREACH_ENTRY_STATUS_MEMORY_ERROR = __BT_FUNC_STATUS_MEMORY_ERROR, + BT_VALUE_MAP_FOREACH_ENTRY_STATUS_USER_ERROR = __BT_FUNC_STATUS_USER_ERROR, BT_VALUE_MAP_FOREACH_ENTRY_STATUS_INTERRUPTED = __BT_FUNC_STATUS_INTERRUPTED, } bt_value_map_foreach_entry_status; diff --git a/src/bindings/python/bt2/bt2/native_bt_value.i.h b/src/bindings/python/bt2/bt2/native_bt_value.i.h index 94645414..5f2b0e35 100644 --- a/src/bindings/python/bt2/bt2/native_bt_value.i.h +++ b/src/bindings/python/bt2/bt2/native_bt_value.i.h @@ -26,17 +26,16 @@ struct bt_value_map_get_keys_data { struct bt_value *keys; }; -static int bt_value_map_get_keys_cb(const char *key, const struct bt_value *object, void *data) +static bt_value_map_foreach_entry_const_func_status bt_value_map_get_keys_cb( + const char *key, const struct bt_value *object, void *data) { - bt_value_array_append_element_status status; + int status; struct bt_value_map_get_keys_data *priv_data = data; status = bt_value_array_append_string_element(priv_data->keys, key); - if (status != __BT_FUNC_STATUS_OK) { - return BT_FALSE; - } - - return BT_TRUE; + BT_ASSERT(status == __BT_FUNC_STATUS_OK || + status == __BT_FUNC_STATUS_MEMORY_ERROR); + return status; } static struct bt_value *bt_value_map_get_keys(const struct bt_value *map_obj) diff --git a/src/cli/babeltrace2.c b/src/cli/babeltrace2.c index cc793406..7dfeedbd 100644 --- a/src/cli/babeltrace2.c +++ b/src/cli/babeltrace2.c @@ -330,13 +330,14 @@ end: } static -bt_bool collect_map_keys(const char *key, const bt_value *object, void *data) +bt_value_map_foreach_entry_const_func_status collect_map_keys( + const char *key, const bt_value *object, void *data) { GPtrArray *map_keys = data; g_ptr_array_add(map_keys, (gpointer *) key); - return BT_TRUE; + return BT_VALUE_MAP_FOREACH_ENTRY_CONST_FUNC_STATUS_OK; } static diff --git a/src/common/common.h b/src/common/common.h index 96c09b6c..e931e2d1 100644 --- a/src/common/common.h +++ b/src/common/common.h @@ -671,6 +671,8 @@ const char *bt_common_func_status_string(int status) return "UNKNOWN_OBJECT"; case __BT_FUNC_STATUS_MEMORY_ERROR: return "MEMORY_ERROR"; + case __BT_FUNC_STATUS_USER_ERROR: + return "USER_ERROR"; case __BT_FUNC_STATUS_ERROR: return "ERROR"; case __BT_FUNC_STATUS_OK: diff --git a/src/lib/func-status.h b/src/lib/func-status.h index 1b9c5cdb..b974f84f 100644 --- a/src/lib/func-status.h +++ b/src/lib/func-status.h @@ -32,6 +32,7 @@ */ #define BT_FUNC_STATUS_AGAIN __BT_FUNC_STATUS_AGAIN #define BT_FUNC_STATUS_END __BT_FUNC_STATUS_END +#define BT_FUNC_STATUS_USER_ERROR __BT_FUNC_STATUS_USER_ERROR #define BT_FUNC_STATUS_ERROR __BT_FUNC_STATUS_ERROR #define BT_FUNC_STATUS_INTERRUPTED __BT_FUNC_STATUS_INTERRUPTED #define BT_FUNC_STATUS_UNKNOWN_OBJECT __BT_FUNC_STATUS_UNKNOWN_OBJECT diff --git a/src/lib/value.c b/src/lib/value.c index 15a97a82..2b23e80f 100644 --- a/src/lib/value.c +++ b/src/lib/value.c @@ -33,6 +33,7 @@ #include "common/common.h" #include "compat/glib.h" #include "lib/assert-pre.h" +#include "lib/assert-post.h" #include "lib/value.h" #include "common/assert.h" #include "func-status.h" @@ -1271,7 +1272,7 @@ enum bt_value_map_foreach_entry_status bt_value_map_foreach_entry( struct bt_value *map_obj, bt_value_map_foreach_entry_func func, void *data) { - enum bt_value_map_foreach_entry_status ret = BT_FUNC_STATUS_OK; + int status = BT_FUNC_STATUS_OK; gpointer key, element_obj; GHashTableIter iter; struct bt_value_map *typed_map_obj = BT_VALUE_TO_MAP(map_obj); @@ -1286,16 +1287,40 @@ enum bt_value_map_foreach_entry_status bt_value_map_foreach_entry( while (g_hash_table_iter_next(&iter, &key, &element_obj)) { const char *key_str = g_quark_to_string(GPOINTER_TO_UINT(key)); - if (!func(key_str, element_obj, data)) { - BT_LOGT("User interrupted the loop: key=\"%s\", " - "value-addr=%p, data=%p", - key_str, element_obj, data); - ret = BT_FUNC_STATUS_INTERRUPTED; + status = func(key_str, element_obj, data); + BT_ASSERT_POST_NO_ERROR_IF_NO_ERROR_STATUS(status); + if (status != BT_FUNC_STATUS_OK) { + if (status < 0) { + BT_LIB_LOGE_APPEND_CAUSE( + "User function failed while iterating " + "map value entries: " + "status=%s, key=\"%s\", " + "value-addr=%p, data=%p", + bt_common_func_status_string(status), + key_str, element_obj, data); + + if (status == BT_FUNC_STATUS_ERROR) { + /* + * User function error becomes a + * user error from this + * function's caller's + * perspective. + */ + status = BT_FUNC_STATUS_USER_ERROR; + } + } else { + BT_ASSERT(status == BT_FUNC_STATUS_INTERRUPTED); + BT_LOGT("User interrupted the loop: status=%s, " + "key=\"%s\", value-addr=%p, data=%p", + bt_common_func_status_string(status), + key_str, element_obj, data); + } + break; } } - return ret; + return status; } enum bt_value_map_foreach_entry_const_status bt_value_map_foreach_entry_const( @@ -1310,21 +1335,20 @@ enum bt_value_map_foreach_entry_const_status bt_value_map_foreach_entry_const( struct extend_map_element_data { struct bt_value *base_obj; - int status; }; static -bt_bool extend_map_element(const char *key, - const struct bt_value *extension_obj_elem, void *data) +bt_value_map_foreach_entry_const_func_status extend_map_element( + const char *key, const struct bt_value *extension_obj_elem, + void *data) { - bt_bool ret = BT_TRUE; + int status; struct extend_map_element_data *extend_data = data; struct bt_value *extension_obj_elem_copy = NULL; /* Copy object which is to replace the current one */ - extend_data->status = bt_value_copy(extension_obj_elem, - &extension_obj_elem_copy); - if (extend_data->status) { + status = bt_value_copy(extension_obj_elem, &extension_obj_elem_copy); + if (status) { BT_LIB_LOGE_APPEND_CAUSE("Cannot copy map element: %!+v", extension_obj_elem); goto error; @@ -1333,36 +1357,35 @@ bt_bool extend_map_element(const char *key, BT_ASSERT(extension_obj_elem_copy); /* Replace in base map value. */ - extend_data->status = bt_value_map_insert_entry( - extend_data->base_obj, key, + status = bt_value_map_insert_entry(extend_data->base_obj, key, (void *) extension_obj_elem_copy); - if (extend_data->status) { + if (status) { BT_LIB_LOGE_APPEND_CAUSE( "Cannot replace value in base map value: key=\"%s\", " "%![base-map-value-]+v, %![element-value-]+v", - key, extend_data->base_obj, - extension_obj_elem_copy); + key, extend_data->base_obj, extension_obj_elem_copy); goto error; } goto end; error: - BT_ASSERT(extend_data->status != BT_FUNC_STATUS_OK); - ret = BT_FALSE; + BT_ASSERT(status < 0); end: BT_OBJECT_PUT_REF_AND_RESET(extension_obj_elem_copy); - return ret; + BT_ASSERT(status == BT_FUNC_STATUS_OK || + status == BT_FUNC_STATUS_MEMORY_ERROR); + return status; } enum bt_value_map_extend_status bt_value_map_extend( struct bt_value *base_map_obj, const struct bt_value *extension_obj) { + int status = BT_FUNC_STATUS_OK; struct extend_map_element_data extend_data = { .base_obj = NULL, - .status = BT_FUNC_STATUS_OK, }; BT_ASSERT_PRE_NO_ERROR(); @@ -1379,15 +1402,16 @@ enum bt_value_map_extend_status bt_value_map_extend( * in the base map object. */ extend_data.base_obj = base_map_obj; - - if (bt_value_map_foreach_entry_const(extension_obj, extend_map_element, - &extend_data)) { + status = bt_value_map_foreach_entry_const(extension_obj, + extend_map_element, &extend_data); + if (status != BT_FUNC_STATUS_OK) { + BT_ASSERT(status == BT_FUNC_STATUS_MEMORY_ERROR); BT_LIB_LOGE_APPEND_CAUSE( "Cannot iterate on the extension object's elements: " "%![extension-value-]+v", extension_obj); } - return extend_data.status; + return status; } enum bt_value_copy_status bt_value_copy(const struct bt_value *object, diff --git a/src/plugins/common/param-validation/param-validation.c b/src/plugins/common/param-validation/param-validation.c index 4b6eed73..216f2742 100644 --- a/src/plugins/common/param-validation/param-validation.c +++ b/src/plugins/common/param-validation/param-validation.c @@ -156,8 +156,8 @@ enum bt_param_validation_status validate_value( struct bt_param_validation_context *ctx); static -bt_bool validate_map_value_entry(const char *key, - const bt_value *value, void *v_data) +bt_value_map_foreach_entry_const_func_status validate_map_value_entry( + const char *key, const bt_value *value, void *v_data) { struct validate_map_value_data *data = v_data; const struct bt_param_validation_map_value_entry_descr *entry = NULL; @@ -192,7 +192,9 @@ bt_bool validate_map_value_entry(const char *key, } /* Continue iterating if everything is good so far. */ - return data->status == BT_PARAM_VALIDATION_STATUS_OK; + return data->status == BT_PARAM_VALIDATION_STATUS_OK ? + BT_VALUE_MAP_FOREACH_ENTRY_CONST_FUNC_STATUS_OK : + BT_VALUE_MAP_FOREACH_ENTRY_CONST_FUNC_STATUS_INTERRUPT; } static diff --git a/src/plugins/text/details/write.c b/src/plugins/text/details/write.c index f5562689..b98efb78 100644 --- a/src/plugins/text/details/write.c +++ b/src/plugins/text/details/write.c @@ -370,14 +370,14 @@ gint compare_strings(const char **a, const char **b) } static -bt_bool map_value_foreach_add_key_to_array(const char *key, - const bt_value *object, void *data) +bt_value_map_foreach_entry_const_func_status map_value_foreach_add_key_to_array( + const char *key, const bt_value *object, void *data) { GPtrArray *keys = data; BT_ASSERT_DBG(keys); g_ptr_array_add(keys, (void *) key); - return BT_TRUE; + return BT_VALUE_MAP_FOREACH_ENTRY_CONST_FUNC_STATUS_OK; } static diff --git a/tests/lib/test_bt_values.c b/tests/lib/test_bt_values.c index c0a9f5a7..776e5f56 100644 --- a/tests/lib/test_bt_values.c +++ b/tests/lib/test_bt_values.c @@ -25,7 +25,7 @@ #include #include "tap/tap.h" -#define NR_TESTS 188 +#define NR_TESTS 190 static void test_null(void) @@ -368,18 +368,23 @@ void test_array(void) } static -bt_bool test_map_foreach_cb_count(const char *key, bt_value *object, +bt_value_map_foreach_entry_func_status test_map_foreach_cb_count( + const char *key, bt_value *object, void *data) { int *count = data; if (*count == 3) { - return BT_FALSE; + return BT_VALUE_MAP_FOREACH_ENTRY_FUNC_STATUS_INTERRUPT; + } else if (*count == 4) { + return BT_VALUE_MAP_FOREACH_ENTRY_FUNC_STATUS_ERROR; + } else if (*count == 5) { + return BT_VALUE_MAP_FOREACH_ENTRY_FUNC_STATUS_MEMORY_ERROR; } (*count)++; - return BT_TRUE; + return BT_VALUE_MAP_FOREACH_ENTRY_FUNC_STATUS_OK; } struct map_foreach_checklist { @@ -399,8 +404,8 @@ struct map_foreach_checklist { }; static -bt_bool test_map_foreach_cb_check(const char *key, bt_value *object, - void *data) +bt_value_map_foreach_entry_func_status test_map_foreach_cb_check( + const char *key, bt_value *object, void *data) { struct map_foreach_checklist *checklist = data; @@ -572,7 +577,7 @@ bt_bool test_map_foreach_cb_check(const char *key, bt_value *object, key); } - return BT_TRUE; + return BT_VALUE_MAP_FOREACH_ENTRY_FUNC_STATUS_OK; } static @@ -711,7 +716,21 @@ void test_map(void) ret = bt_value_map_foreach_entry(map_obj, test_map_foreach_cb_count, &count); ok(ret == BT_VALUE_MAP_FOREACH_ENTRY_STATUS_INTERRUPTED && count == 3, - "bt_value_map_foreach_entry() breaks the loop when the user function returns BT_FALSE"); + "bt_value_map_foreach_entry() breaks the loop when the user function returns BT_VALUE_MAP_FOREACH_ENTRY_FUNC_STATUS_INTERRUPT"); + + count = 4; + ret = bt_value_map_foreach_entry(map_obj, test_map_foreach_cb_count, + &count); + ok(ret == BT_VALUE_MAP_FOREACH_ENTRY_STATUS_USER_ERROR, + "bt_value_map_foreach_entry() fails when the user function returns BT_VALUE_MAP_FOREACH_ENTRY_FUNC_STATUS_ERROR"); + bt_current_thread_clear_error(); + + count = 5; + ret = bt_value_map_foreach_entry(map_obj, test_map_foreach_cb_count, + &count); + ok(ret == BT_VALUE_MAP_FOREACH_ENTRY_STATUS_MEMORY_ERROR, + "bt_value_map_foreach_entry() fails when the user function returns BT_VALUE_MAP_FOREACH_ENTRY_FUNC_STATUS_MEMORY_ERROR"); + bt_current_thread_clear_error(); memset(&checklist, 0, sizeof(checklist)); ret = bt_value_map_foreach_entry(map_obj, test_map_foreach_cb_check, -- 2.34.1