From a635e50750a3c1e8907b38c8311dc0b8bb0f09c3 Mon Sep 17 00:00:00 2001 From: Simon Marchi Date: Tue, 23 Jul 2019 23:54:13 -0400 Subject: [PATCH] lib: remove INVALID_PARAMS status This patch removes the INVALID_PARAMS status. It is currently meant to be used by component classes in query, if they don't get the parameters they expect (mandatory parameter not present, wrong type, wrong value). Since we now have a framework to describe errors precisely, this can be accomplished more simply by returning ERROR and appending an error cause. So this patch removes INVALID_PARAMS, replacing all of its uses with ERROR. It also remove other traces of that status, including in the Python bindings. - In lttng-live.c, we were logging a warning when getting invalid parameters. Since we are now returning an error, I think it makes sense to log an error. I also think it's something that deserves to be an error anyway. - Because queries now return ERROR without appending an error cause and the library's bt_query_executor_query function doesn't append either, we get in the case where we construct a bt2._Error and don't have an error for the current thread. I added some error cause appending in bt_query_executor_query to fix that, which I think is a valid improvement on its own. Component classes should ideally also append an error cause when they return ERROR, but that is left as an exercise for later. - The test_query_invalid_params test is removed. Changing raise bt2.InvalidParams to raise ValueError would just make the test redundant with test_query_gen_error. Change-Id: I8c9277aa5e55bb8cde9b23a3aedf217cbe6a5849 Signed-off-by: Simon Marchi Reviewed-on: https://review.lttng.org/c/babeltrace/+/1756 CI-Build: Philippe Proulx Tested-by: jenkins Reviewed-by: Philippe Proulx --- include/babeltrace2/babeltrace.h | 3 +-- include/babeltrace2/func-status.h | 5 ----- include/babeltrace2/graph/component-class.h | 1 - include/babeltrace2/graph/query-executor.h | 1 - src/bindings/python/bt2/bt2/__init__.py.in | 4 ---- .../python/bt2/bt2/native_bt_component_class.i | 8 -------- src/bindings/python/bt2/bt2/utils.py | 5 ----- src/cli/babeltrace2.c | 3 --- src/common/common.h | 2 -- src/lib/func-status.h | 1 - src/lib/graph/query-executor.c | 9 +++++++++ src/plugins/ctf/fs-src/query.c | 10 +++++----- src/plugins/ctf/lttng-live/lttng-live.c | 8 ++++---- .../bindings/python/bt2/test_query_executor.py | 17 +---------------- .../src.ctf.fs/query/test_query_trace_info.py | 8 ++++---- 15 files changed, 24 insertions(+), 61 deletions(-) diff --git a/include/babeltrace2/babeltrace.h b/include/babeltrace2/babeltrace.h index e0db1254..ded1b03f 100644 --- a/include/babeltrace2/babeltrace.h +++ b/include/babeltrace2/babeltrace.h @@ -159,14 +159,13 @@ /* Cancel private definitions */ #undef __BT_FUNC_STATUS_AGAIN #undef __BT_FUNC_STATUS_END -#undef __BT_FUNC_STATUS_END -#undef __BT_FUNC_STATUS_ERROR #undef __BT_FUNC_STATUS_ERROR #undef __BT_FUNC_STATUS_INTERRUPTED #undef __BT_FUNC_STATUS_INVALID_OBJECT #undef __BT_FUNC_STATUS_MEMORY_ERROR #undef __BT_FUNC_STATUS_NOT_FOUND #undef __BT_FUNC_STATUS_OK +#undef __BT_FUNC_STATUS_OVERFLOW #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 78a2f0a4..74fc1012 100644 --- a/include/babeltrace2/func-status.h +++ b/include/babeltrace2/func-status.h @@ -41,11 +41,6 @@ # define __BT_FUNC_STATUS_OVERFLOW -75 #endif -/* Invalid query parameters */ -#ifndef __BT_FUNC_STATUS_INVALID_PARAMS -# define __BT_FUNC_STATUS_INVALID_PARAMS -24 -#endif - /* Invalid query object */ #ifndef __BT_FUNC_STATUS_INVALID_OBJECT # define __BT_FUNC_STATUS_INVALID_OBJECT -23 diff --git a/include/babeltrace2/graph/component-class.h b/include/babeltrace2/graph/component-class.h index 63ad69f0..8a35146c 100644 --- a/include/babeltrace2/graph/component-class.h +++ b/include/babeltrace2/graph/component-class.h @@ -51,7 +51,6 @@ typedef enum bt_component_class_query_method_status { BT_COMPONENT_CLASS_QUERY_METHOD_STATUS_ERROR = __BT_FUNC_STATUS_ERROR, BT_COMPONENT_CLASS_QUERY_METHOD_STATUS_MEMORY_ERROR = __BT_FUNC_STATUS_MEMORY_ERROR, BT_COMPONENT_CLASS_QUERY_METHOD_STATUS_INVALID_OBJECT = __BT_FUNC_STATUS_INVALID_OBJECT, - BT_COMPONENT_CLASS_QUERY_METHOD_STATUS_INVALID_PARAMS = __BT_FUNC_STATUS_INVALID_PARAMS, } bt_component_class_query_method_status; typedef enum bt_component_class_message_iterator_init_method_status { diff --git a/include/babeltrace2/graph/query-executor.h b/include/babeltrace2/graph/query-executor.h index ef250462..cf6ec244 100644 --- a/include/babeltrace2/graph/query-executor.h +++ b/include/babeltrace2/graph/query-executor.h @@ -43,7 +43,6 @@ typedef enum bt_query_executor_query_status { BT_QUERY_EXECUTOR_QUERY_STATUS_ERROR = __BT_FUNC_STATUS_ERROR, BT_QUERY_EXECUTOR_QUERY_STATUS_MEMORY_ERROR = __BT_FUNC_STATUS_MEMORY_ERROR, BT_QUERY_EXECUTOR_QUERY_STATUS_INVALID_OBJECT = __BT_FUNC_STATUS_INVALID_OBJECT, - BT_QUERY_EXECUTOR_QUERY_STATUS_INVALID_PARAMS = __BT_FUNC_STATUS_INVALID_PARAMS, } bt_query_executor_query_status; extern diff --git a/src/bindings/python/bt2/bt2/__init__.py.in b/src/bindings/python/bt2/bt2/__init__.py.in index 01c87e51..b3a83a98 100644 --- a/src/bindings/python/bt2/bt2/__init__.py.in +++ b/src/bindings/python/bt2/bt2/__init__.py.in @@ -78,10 +78,6 @@ class InvalidObject(Exception): pass -class InvalidParams(Exception): - pass - - class OverflowError(OverflowError): pass diff --git a/src/bindings/python/bt2/bt2/native_bt_component_class.i b/src/bindings/python/bt2/bt2/native_bt_component_class.i index 63dcc0ba..bba59883 100644 --- a/src/bindings/python/bt2/bt2/native_bt_component_class.i +++ b/src/bindings/python/bt2/bt2/native_bt_component_class.i @@ -98,7 +98,6 @@ static PyObject *py_mod_bt2_exc_memory_error = NULL; static PyObject *py_mod_bt2_exc_try_again_type = NULL; static PyObject *py_mod_bt2_exc_stop_type = NULL; static PyObject *py_mod_bt2_exc_invalid_object_type = NULL; -static PyObject *py_mod_bt2_exc_invalid_params_type = NULL; static void bt_bt2_cc_init_from_bt2(void) @@ -128,9 +127,6 @@ void bt_bt2_cc_init_from_bt2(void) py_mod_bt2_exc_invalid_object_type = PyObject_GetAttrString(py_mod_bt2, "InvalidObject"); BT_ASSERT(py_mod_bt2_exc_invalid_object_type); - py_mod_bt2_exc_invalid_params_type = - PyObject_GetAttrString(py_mod_bt2, "InvalidParams"); - BT_ASSERT(py_mod_bt2_exc_invalid_params_type); } static @@ -154,7 +150,6 @@ void bt_bt2_cc_exit_handler(void) Py_XDECREF(py_mod_bt2_exc_try_again_type); Py_XDECREF(py_mod_bt2_exc_stop_type); Py_XDECREF(py_mod_bt2_exc_invalid_object_type); - Py_XDECREF(py_mod_bt2_exc_invalid_params_type); } @@ -395,9 +390,6 @@ int py_exc_to_status(bt_self_component_class *self_component_class, } else if (PyErr_GivenExceptionMatches(exc, py_mod_bt2_exc_invalid_object_type)) { status = __BT_FUNC_STATUS_INVALID_OBJECT; - } else if (PyErr_GivenExceptionMatches(exc, - py_mod_bt2_exc_invalid_params_type)) { - status = __BT_FUNC_STATUS_INVALID_PARAMS; } else { /* Unknown exception: convert to general error */ log_exception_and_maybe_append_error(BT_LOG_WARNING, true, diff --git a/src/bindings/python/bt2/bt2/utils.py b/src/bindings/python/bt2/bt2/utils.py index aa871042..abb9accf 100644 --- a/src/bindings/python/bt2/bt2/utils.py +++ b/src/bindings/python/bt2/bt2/utils.py @@ -161,10 +161,5 @@ def _handle_func_status(status, msg=None): raise bt2.InvalidObject else: raise bt2.InvalidObject(msg) - elif status == native_bt.__BT_FUNC_STATUS_INVALID_PARAMS: - if msg is None: - raise bt2.InvalidParams - else: - raise bt2.InvalidParams(msg) else: assert False diff --git a/src/cli/babeltrace2.c b/src/cli/babeltrace2.c index a0c6df11..bafda3b5 100644 --- a/src/cli/babeltrace2.c +++ b/src/cli/babeltrace2.c @@ -176,9 +176,6 @@ int query(struct bt_config *cfg, const bt_component_class *comp_cls, case BT_QUERY_EXECUTOR_QUERY_STATUS_INVALID_OBJECT: *fail_reason = "invalid or unknown query object"; goto error; - case BT_QUERY_EXECUTOR_QUERY_STATUS_INVALID_PARAMS: - *fail_reason = "invalid query parameters"; - goto error; case BT_QUERY_EXECUTOR_QUERY_STATUS_MEMORY_ERROR: *fail_reason = "not enough memory"; goto error; diff --git a/src/common/common.h b/src/common/common.h index 8429145c..3d5c0b5e 100644 --- a/src/common/common.h +++ b/src/common/common.h @@ -603,8 +603,6 @@ const char *bt_common_func_status_string(int status) switch (status) { case __BT_FUNC_STATUS_OVERFLOW: return "OVERFLOW"; - case __BT_FUNC_STATUS_INVALID_PARAMS: - return "INVALID_PARAMS"; case __BT_FUNC_STATUS_INVALID_OBJECT: return "INVALID_OBJECT"; case __BT_FUNC_STATUS_MEMORY_ERROR: diff --git a/src/lib/func-status.h b/src/lib/func-status.h index 658a82d7..25520ffa 100644 --- a/src/lib/func-status.h +++ b/src/lib/func-status.h @@ -35,7 +35,6 @@ #define BT_FUNC_STATUS_ERROR __BT_FUNC_STATUS_ERROR #define BT_FUNC_STATUS_INTERRUPTED __BT_FUNC_STATUS_INTERRUPTED #define BT_FUNC_STATUS_INVALID_OBJECT __BT_FUNC_STATUS_INVALID_OBJECT -#define BT_FUNC_STATUS_INVALID_PARAMS __BT_FUNC_STATUS_INVALID_PARAMS #define BT_FUNC_STATUS_MEMORY_ERROR __BT_FUNC_STATUS_MEMORY_ERROR #define BT_FUNC_STATUS_NOT_FOUND __BT_FUNC_STATUS_NOT_FOUND #define BT_FUNC_STATUS_OK __BT_FUNC_STATUS_OK diff --git a/src/lib/graph/query-executor.c b/src/lib/graph/query-executor.c index 03d420ec..ce6dd6d9 100644 --- a/src/lib/graph/query-executor.c +++ b/src/lib/graph/query-executor.c @@ -191,6 +191,15 @@ enum bt_query_executor_query_status bt_query_executor_query( "User method returned `BT_FUNC_STATUS_OK` without a result."); status = (int) query_status; + if (status < 0) { + BT_LIB_LOGW_APPEND_CAUSE( + "Component class's \"query\" method failed: " + "query-exec-addr=%p, %![cc-]+C, object=\"%s\", " + "%![params-]+v, log-level=%s", query_exec, comp_cls, + object, params, bt_common_logging_level_string(log_level)); + goto end; + } + end: return status; } diff --git a/src/plugins/ctf/fs-src/query.c b/src/plugins/ctf/fs-src/query.c index 872caddf..b2e5248d 100644 --- a/src/plugins/ctf/fs-src/query.c +++ b/src/plugins/ctf/fs-src/query.c @@ -74,20 +74,20 @@ bt_component_class_query_method_status metadata_info_query( if (!bt_value_is_map(params)) { BT_LOGE_STR("Query parameters is not a map value object."); - status = BT_COMPONENT_CLASS_QUERY_METHOD_STATUS_INVALID_PARAMS; + status = BT_COMPONENT_CLASS_QUERY_METHOD_STATUS_ERROR; goto error; } path_value = bt_value_map_borrow_entry_value_const(params, "path"); if (!path_value) { BT_LOGE_STR("Mandatory `path` parameter missing"); - status = BT_COMPONENT_CLASS_QUERY_METHOD_STATUS_INVALID_PARAMS; + status = BT_COMPONENT_CLASS_QUERY_METHOD_STATUS_ERROR; goto error; } if (!bt_value_is_string(path_value)) { BT_LOGE_STR("`path` parameter is required to be a string value"); - status = BT_COMPONENT_CLASS_QUERY_METHOD_STATUS_INVALID_PARAMS; + status = BT_COMPONENT_CLASS_QUERY_METHOD_STATUS_ERROR; goto error; } @@ -492,7 +492,7 @@ bt_component_class_query_method_status trace_info_query( if (!bt_value_is_map(params)) { BT_LOGE("Query parameters is not a map value object."); - status = BT_COMPONENT_CLASS_QUERY_METHOD_STATUS_INVALID_PARAMS; + status = BT_COMPONENT_CLASS_QUERY_METHOD_STATUS_ERROR; goto error; } @@ -502,7 +502,7 @@ bt_component_class_query_method_status trace_info_query( } if (!read_src_fs_parameters(params, &inputs_value, ctf_fs)) { - status = BT_COMPONENT_CLASS_QUERY_METHOD_STATUS_INVALID_PARAMS; + status = BT_COMPONENT_CLASS_QUERY_METHOD_STATUS_ERROR; goto error; } diff --git a/src/plugins/ctf/lttng-live/lttng-live.c b/src/plugins/ctf/lttng-live/lttng-live.c index 11b80f47..df5639b1 100644 --- a/src/plugins/ctf/lttng-live/lttng-live.c +++ b/src/plugins/ctf/lttng-live/lttng-live.c @@ -1399,15 +1399,15 @@ bt_component_class_query_method_status lttng_live_query_list_sessions( url_value = bt_value_map_borrow_entry_value_const(params, URL_PARAM); if (!url_value) { - BT_COMP_LOGW("Mandatory `%s` parameter missing", URL_PARAM); - status = BT_COMPONENT_CLASS_QUERY_METHOD_STATUS_INVALID_PARAMS; + BT_COMP_LOGE("Mandatory `%s` parameter missing", URL_PARAM); + status = BT_COMPONENT_CLASS_QUERY_METHOD_STATUS_ERROR; goto error; } if (!bt_value_is_string(url_value)) { - BT_COMP_LOGW("`%s` parameter is required to be a string value", + BT_COMP_LOGE("`%s` parameter is required to be a string value", URL_PARAM); - status = BT_COMPONENT_CLASS_QUERY_METHOD_STATUS_INVALID_PARAMS; + status = BT_COMPONENT_CLASS_QUERY_METHOD_STATUS_ERROR; goto error; } diff --git a/tests/bindings/python/bt2/test_query_executor.py b/tests/bindings/python/bt2/test_query_executor.py index bc20a719..a5d852f5 100644 --- a/tests/bindings/python/bt2/test_query_executor.py +++ b/tests/bindings/python/bt2/test_query_executor.py @@ -101,7 +101,7 @@ class QueryExecutorTestCase(unittest.TestCase): res = bt2.QueryExecutor().query(MySink, 'obj', [17, 23]) exc = ctx.exception - self.assertEqual(len(exc), 1) + self.assertEqual(len(exc), 2) cause = exc[0] self.assertIsInstance(cause, bt2.error._ComponentClassErrorCause) self.assertIn('raise ValueError', cause.message) @@ -153,21 +153,6 @@ class QueryExecutorTestCase(unittest.TestCase): with self.assertRaises(ValueError): res = bt2.QueryExecutor().query(MySink, 'obj', [17, 23], 12345) - def test_query_invalid_params(self): - class MySink(bt2._UserSinkComponent): - def _consume(self): - pass - - def _graph_is_configured(self): - pass - - @classmethod - def _query(cls, query_exec, obj, params, log_level): - raise bt2.InvalidParams - - with self.assertRaises(bt2.InvalidParams): - res = bt2.QueryExecutor().query(MySink, 'obj', [17, 23]) - def test_query_try_again(self): class MySink(bt2._UserSinkComponent): def _consume(self): diff --git a/tests/plugins/src.ctf.fs/query/test_query_trace_info.py b/tests/plugins/src.ctf.fs/query/test_query_trace_info.py index 0e1e3c15..fc55205b 100644 --- a/tests/plugins/src.ctf.fs/query/test_query_trace_info.py +++ b/tests/plugins/src.ctf.fs/query/test_query_trace_info.py @@ -102,7 +102,7 @@ class QueryTraceInfoClockOffsetTestCase(unittest.TestCase): self._check(trace, -2000000002) def test_clock_class_offset_s_wrong_type(self): - with self.assertRaises(bt2.InvalidParams): + with self.assertRaises(bt2._Error): self._executor.query( self._fs, 'trace-info', @@ -110,7 +110,7 @@ class QueryTraceInfoClockOffsetTestCase(unittest.TestCase): ) def test_clock_class_offset_s_wrong_type_none(self): - with self.assertRaises(bt2.InvalidParams): + with self.assertRaises(bt2._Error): self._executor.query( self._fs, 'trace-info', @@ -118,7 +118,7 @@ class QueryTraceInfoClockOffsetTestCase(unittest.TestCase): ) def test_clock_class_offset_ns_wrong_type(self): - with self.assertRaises(bt2.InvalidParams): + with self.assertRaises(bt2._Error): self._executor.query( self._fs, 'trace-info', @@ -126,7 +126,7 @@ class QueryTraceInfoClockOffsetTestCase(unittest.TestCase): ) def test_clock_class_offset_ns_wrong_type_none(self): - with self.assertRaises(bt2.InvalidParams): + with self.assertRaises(bt2._Error): self._executor.query( self._fs, 'trace-info', -- 2.34.1