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 <simon.marchi@efficios.com>
Reviewed-on: https://review.lttng.org/c/babeltrace/+/1756
CI-Build: Philippe Proulx <eeppeliteloop@gmail.com>
Tested-by: jenkins <jenkins@lttng.org>
Reviewed-by: Philippe Proulx <eeppeliteloop@gmail.com>

diff --git a/include/babeltrace2/babeltrace.h b/include/babeltrace2/babeltrace.h
index e0db1254e4a5551db646a1312cb7196275c7d1c3..ded1b03f83fc138478235c5b6d18a9480f1eb396 100644 (file)
--- 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 78a2f0a473c13f37d27bc3acb443e18f16598a4b..74fc101251359f93f1186871fb195748783ab9bc 100644 (file)
--- 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 63ad69f08fe9515c2d972d3dda48319c453d0760..8a35146c612e6c056a83afad1a44d6c8698834bc 100644 (file)
--- 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 ef250462cc5fa79776291468c608110a07780012..cf6ec244b07fcfb8879acb66de737d2287ab3ad2 100644 (file)
--- 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 01c87e51c238f932ccaf16803dccf6e019a2d64b..b3a83a981cace0847402bd713f4245ae5e831474 100644 (file)
--- 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 63dcc0ba29378f5e3612297fef2b6841364ea6da..bba59883329bf6afb1abef7de26c20ceb37003b1 100644 (file)
--- 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 aa871042bc136a73ad66ae908b62c148b190a573..abb9accfdee8b73ea3497737e99d96ff2c4f0590 100644 (file)
--- 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 a0c6df110e09af00a326906cd404877bdae10b34..bafda3b5697019d59258ef90ddebd6486b8392c8 100644 (file)
--- 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 8429145c5134c4ce5033a360934bda9ff654c220..3d5c0b5e29a1c5b2b95465be419a0375e7e31697 100644 (file)
--- 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 658a82d7ff4a0996820e978eb3603ec95cbef733..25520ffa59cbf338be442b327efc3c52fe8a76f4 100644 (file)
--- 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 03d420ece1fb55f5aca37f96400fa8bb134bb2c1..ce6dd6d9a3dbf2cbb4b5b5a7cc005fe4f65564ce 100644 (file)
--- 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 872caddf9b4dfdbc95c5a01ea894670d652aea63..b2e5248dc69c0a21607fa1df6aede774a4cb5e05 100644 (file)
--- 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 11b80f474d3c78c5986d21ab3c9cc89824bc7ccb..df5639b1ca1c38a5480985c8e3b667686a2281d0 100644 (file)
--- 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 bc20a719b3dca0000ee9a1fa123ed51041ddc0ef..a5d852f57c745f7b0fdef4a41bd551a1247086c7 100644 (file)
--- 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 0e1e3c150631f68432286d5c1bdee67eacf2c10d..fc55205b1b95daf87c94851ac9ca41b475facd3c 100644 (file)
--- 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',