From 5cc4af5231608d0d85d120c20bd9bbc9da9338fa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Luis=20Lafuente?= Date: Thu, 18 Apr 2024 20:05:59 +0200 Subject: [PATCH 1/6] Add isInitialized to nix::Value Add a method to check if a value has been initialized. This helps avoid segfaults when calling `type()`. Useful in the context of the new C API. Closes #10524 --- src/libexpr/value.hh | 9 ++++++++- tests/unit/libexpr/value/value.cc | 25 +++++++++++++++++++++++++ 2 files changed, 33 insertions(+), 1 deletion(-) create mode 100644 tests/unit/libexpr/value/value.cc diff --git a/src/libexpr/value.hh b/src/libexpr/value.hh index 7ed3fa5a9..a1003e16b 100644 --- a/src/libexpr/value.hh +++ b/src/libexpr/value.hh @@ -23,6 +23,7 @@ class BindingsBuilder; typedef enum { + tUnset, tInt = 1, tBool, tString, @@ -166,7 +167,7 @@ public: struct Value { private: - InternalType internalType; + InternalType internalType = tUnset; friend std::string showType(const Value & v); @@ -270,6 +271,7 @@ public: inline ValueType type(bool invalidIsThunk = false) const { switch (internalType) { + case tUnset: break; case tInt: return nInt; case tBool: return nBool; case tString: return nString; @@ -294,6 +296,11 @@ public: internalType = newType; } + inline bool isInitialized() + { + return internalType != tUnset; + } + inline void mkInt(NixInt n) { finishValue(tInt, { .integer = n }); diff --git a/tests/unit/libexpr/value/value.cc b/tests/unit/libexpr/value/value.cc new file mode 100644 index 000000000..49a896c87 --- /dev/null +++ b/tests/unit/libexpr/value/value.cc @@ -0,0 +1,25 @@ +#include "value.hh" + +#include "tests/libstore.hh" + +namespace nix { + +class ValueTest : public LibStoreTest +{}; + +TEST_F(ValueTest, unsetValue) +{ + Value unsetValue; + ASSERT_EQ(false, unsetValue.isInitialized()); + ASSERT_EQ(nThunk, unsetValue.type(true)); + ASSERT_DEATH(unsetValue.type(), ""); +} + +TEST_F(ValueTest, vInt) +{ + Value vInt; + vInt.mkInt(42); + ASSERT_EQ(true, vInt.isInitialized()); +} + +} // namespace nix From 9d7dee4a8f279ffd220d41d05a651761ed10d3be Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Luis=20Lafuente?= Date: Sat, 20 Apr 2024 14:15:05 +0200 Subject: [PATCH 2/6] nix::Value: Use more descriptive names --- src/libexpr/value.hh | 15 ++++++++++----- tests/unit/libexpr/value/value.cc | 4 ++-- 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/src/libexpr/value.hh b/src/libexpr/value.hh index a1003e16b..5795f04cf 100644 --- a/src/libexpr/value.hh +++ b/src/libexpr/value.hh @@ -23,7 +23,7 @@ class BindingsBuilder; typedef enum { - tUnset, + tUninitialized = 0, tInt = 1, tBool, tString, @@ -167,7 +167,7 @@ public: struct Value { private: - InternalType internalType = tUnset; + InternalType internalType = tUninitialized; friend std::string showType(const Value & v); @@ -271,7 +271,7 @@ public: inline ValueType type(bool invalidIsThunk = false) const { switch (internalType) { - case tUnset: break; + case tUninitialized: break; case tInt: return nInt; case tBool: return nBool; case tString: return nString; @@ -296,9 +296,14 @@ public: internalType = newType; } - inline bool isInitialized() + /** + * A value becomes valid when it is initialized. We don't use this + * in the evaluator; only in the bindings, where the slight extra + * cost is warranted because of inexperienced callers. + */ + inline bool isValid() const { - return internalType != tUnset; + return internalType != tUninitialized; } inline void mkInt(NixInt n) diff --git a/tests/unit/libexpr/value/value.cc b/tests/unit/libexpr/value/value.cc index 49a896c87..5762d5891 100644 --- a/tests/unit/libexpr/value/value.cc +++ b/tests/unit/libexpr/value/value.cc @@ -10,7 +10,7 @@ class ValueTest : public LibStoreTest TEST_F(ValueTest, unsetValue) { Value unsetValue; - ASSERT_EQ(false, unsetValue.isInitialized()); + ASSERT_EQ(false, unsetValue.isValid()); ASSERT_EQ(nThunk, unsetValue.type(true)); ASSERT_DEATH(unsetValue.type(), ""); } @@ -19,7 +19,7 @@ TEST_F(ValueTest, vInt) { Value vInt; vInt.mkInt(42); - ASSERT_EQ(true, vInt.isInitialized()); + ASSERT_EQ(true, vInt.isValid()); } } // namespace nix From ccad6e94e2d79241c0eab0d9f708bcd30155e840 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Luis=20Lafuente?= Date: Sat, 20 Apr 2024 22:09:32 +0200 Subject: [PATCH 3/6] C API: add (un)initialized value checks --- src/libexpr-c/nix_api_value.cc | 46 ++++++++- tests/unit/libexpr/nix_api_value.cc | 93 ++++++++++++++----- .../libutil-support/tests/nix_api_util.hh | 13 ++- 3 files changed, 128 insertions(+), 24 deletions(-) diff --git a/src/libexpr-c/nix_api_value.cc b/src/libexpr-c/nix_api_value.cc index 2550e975a..a3686022d 100644 --- a/src/libexpr-c/nix_api_value.cc +++ b/src/libexpr-c/nix_api_value.cc @@ -20,7 +20,7 @@ # include "gc_cpp.h" #endif -// Helper function to throw an exception if value is null +// Helper function to throw an exception if value is null or in an invalid state static const nix::Value & check_value_not_null(const Value * value) { if (!value) { @@ -37,6 +37,20 @@ static nix::Value & check_value_not_null(Value * value) return *((nix::Value *) value); } +static void check_value_initialized(const nix::Value & value) +{ + if (!value.isValid()) { + throw std::runtime_error("Uninitialized Value"); + } +} + +static void check_value_uninitialized(const nix::Value & value) +{ + if (value.isValid()) { + throw std::runtime_error("Value already initialized. Variables are immutable"); + } +} + /** * Helper function to convert calls from nix into C API. * @@ -112,6 +126,7 @@ ValueType nix_get_type(nix_c_context * context, const Value * value) context->last_err_code = NIX_OK; try { auto & v = check_value_not_null(value); + check_value_initialized(v); using namespace nix; switch (v.type()) { case nThunk: @@ -148,6 +163,7 @@ const char * nix_get_typename(nix_c_context * context, const Value * value) context->last_err_code = NIX_OK; try { auto & v = check_value_not_null(value); + check_value_initialized(v); auto s = nix::showType(v); return strdup(s.c_str()); } @@ -160,6 +176,7 @@ bool nix_get_bool(nix_c_context * context, const Value * value) context->last_err_code = NIX_OK; try { auto & v = check_value_not_null(value); + check_value_initialized(v); assert(v.type() == nix::nBool); return v.boolean(); } @@ -172,6 +189,7 @@ nix_err nix_get_string(nix_c_context * context, const Value * value, nix_get_str context->last_err_code = NIX_OK; try { auto & v = check_value_not_null(value); + check_value_initialized(v); assert(v.type() == nix::nString); call_nix_get_string_callback(v.c_str(), callback, user_data); } @@ -184,6 +202,7 @@ const char * nix_get_path_string(nix_c_context * context, const Value * value) context->last_err_code = NIX_OK; try { auto & v = check_value_not_null(value); + check_value_initialized(v); assert(v.type() == nix::nPath); // NOTE (from @yorickvP) // v._path.path should work but may not be how Eelco intended it. @@ -203,6 +222,7 @@ unsigned int nix_get_list_size(nix_c_context * context, const Value * value) context->last_err_code = NIX_OK; try { auto & v = check_value_not_null(value); + check_value_initialized(v); assert(v.type() == nix::nList); return v.listSize(); } @@ -215,6 +235,7 @@ unsigned int nix_get_attrs_size(nix_c_context * context, const Value * value) context->last_err_code = NIX_OK; try { auto & v = check_value_not_null(value); + check_value_initialized(v); assert(v.type() == nix::nAttrs); return v.attrs()->size(); } @@ -227,6 +248,7 @@ double nix_get_float(nix_c_context * context, const Value * value) context->last_err_code = NIX_OK; try { auto & v = check_value_not_null(value); + check_value_initialized(v); assert(v.type() == nix::nFloat); return v.fpoint(); } @@ -239,6 +261,7 @@ int64_t nix_get_int(nix_c_context * context, const Value * value) context->last_err_code = NIX_OK; try { auto & v = check_value_not_null(value); + check_value_initialized(v); assert(v.type() == nix::nInt); return v.integer(); } @@ -251,6 +274,7 @@ ExternalValue * nix_get_external(nix_c_context * context, Value * value) context->last_err_code = NIX_OK; try { auto & v = check_value_not_null(value); + check_value_initialized(v); assert(v.type() == nix::nExternal); return (ExternalValue *) v.external(); } @@ -263,6 +287,7 @@ Value * nix_get_list_byidx(nix_c_context * context, const Value * value, EvalSta context->last_err_code = NIX_OK; try { auto & v = check_value_not_null(value); + check_value_initialized(v); assert(v.type() == nix::nList); auto * p = v.listElems()[ix]; nix_gc_incref(nullptr, p); @@ -279,6 +304,7 @@ Value * nix_get_attr_byname(nix_c_context * context, const Value * value, EvalSt context->last_err_code = NIX_OK; try { auto & v = check_value_not_null(value); + check_value_initialized(v); assert(v.type() == nix::nAttrs); nix::Symbol s = state->state.symbols.create(name); auto attr = v.attrs()->get(s); @@ -299,6 +325,7 @@ bool nix_has_attr_byname(nix_c_context * context, const Value * value, EvalState context->last_err_code = NIX_OK; try { auto & v = check_value_not_null(value); + check_value_initialized(v); assert(v.type() == nix::nAttrs); nix::Symbol s = state->state.symbols.create(name); auto attr = v.attrs()->get(s); @@ -316,6 +343,7 @@ nix_get_attr_byidx(nix_c_context * context, const Value * value, EvalState * sta context->last_err_code = NIX_OK; try { auto & v = check_value_not_null(value); + check_value_initialized(v); const nix::Attr & a = (*v.attrs())[i]; *name = ((const std::string &) (state->state.symbols[a.name])).c_str(); nix_gc_incref(nullptr, a.value); @@ -331,6 +359,7 @@ const char * nix_get_attr_name_byidx(nix_c_context * context, const Value * valu context->last_err_code = NIX_OK; try { auto & v = check_value_not_null(value); + check_value_initialized(v); const nix::Attr & a = (*v.attrs())[i]; return ((const std::string &) (state->state.symbols[a.name])).c_str(); } @@ -343,6 +372,7 @@ nix_err nix_init_bool(nix_c_context * context, Value * value, bool b) context->last_err_code = NIX_OK; try { auto & v = check_value_not_null(value); + check_value_uninitialized(v); v.mkBool(b); } NIXC_CATCH_ERRS @@ -355,6 +385,7 @@ nix_err nix_init_string(nix_c_context * context, Value * value, const char * str context->last_err_code = NIX_OK; try { auto & v = check_value_not_null(value); + check_value_uninitialized(v); v.mkString(std::string_view(str)); } NIXC_CATCH_ERRS @@ -366,6 +397,7 @@ nix_err nix_init_path_string(nix_c_context * context, EvalState * s, Value * val context->last_err_code = NIX_OK; try { auto & v = check_value_not_null(value); + check_value_uninitialized(v); v.mkPath(s->state.rootPath(nix::CanonPath(str))); } NIXC_CATCH_ERRS @@ -377,6 +409,7 @@ nix_err nix_init_float(nix_c_context * context, Value * value, double d) context->last_err_code = NIX_OK; try { auto & v = check_value_not_null(value); + check_value_uninitialized(v); v.mkFloat(d); } NIXC_CATCH_ERRS @@ -388,6 +421,7 @@ nix_err nix_init_int(nix_c_context * context, Value * value, int64_t i) context->last_err_code = NIX_OK; try { auto & v = check_value_not_null(value); + check_value_uninitialized(v); v.mkInt(i); } NIXC_CATCH_ERRS @@ -399,6 +433,7 @@ nix_err nix_init_null(nix_c_context * context, Value * value) context->last_err_code = NIX_OK; try { auto & v = check_value_not_null(value); + check_value_uninitialized(v); v.mkNull(); } NIXC_CATCH_ERRS @@ -423,6 +458,7 @@ nix_err nix_init_external(nix_c_context * context, Value * value, ExternalValue context->last_err_code = NIX_OK; try { auto & v = check_value_not_null(value); + check_value_uninitialized(v); auto r = (nix::ExternalValueBase *) val; v.mkExternal(r); } @@ -450,6 +486,7 @@ nix_err nix_list_builder_insert(nix_c_context * context, ListBuilder * list_buil context->last_err_code = NIX_OK; try { auto & e = check_value_not_null(value); + check_value_initialized(e); list_builder->builder[index] = &e; } NIXC_CATCH_ERRS @@ -470,6 +507,7 @@ nix_err nix_make_list(nix_c_context * context, ListBuilder * list_builder, Value context->last_err_code = NIX_OK; try { auto & v = check_value_not_null(value); + check_value_uninitialized(v); v.mkList(list_builder->builder); } NIXC_CATCH_ERRS @@ -481,6 +519,7 @@ nix_err nix_init_primop(nix_c_context * context, Value * value, PrimOp * p) context->last_err_code = NIX_OK; try { auto & v = check_value_not_null(value); + check_value_uninitialized(v); v.mkPrimOp((nix::PrimOp *) p); } NIXC_CATCH_ERRS @@ -492,7 +531,9 @@ nix_err nix_copy_value(nix_c_context * context, Value * value, Value * source) context->last_err_code = NIX_OK; try { auto & v = check_value_not_null(value); + check_value_uninitialized(v); auto & s = check_value_not_null(source); + check_value_initialized(s); v = s; } NIXC_CATCH_ERRS @@ -504,6 +545,7 @@ nix_err nix_make_attrs(nix_c_context * context, Value * value, BindingsBuilder * context->last_err_code = NIX_OK; try { auto & v = check_value_not_null(value); + check_value_uninitialized(v); v.mkAttrs(b->builder); } NIXC_CATCH_ERRS @@ -530,6 +572,7 @@ nix_err nix_bindings_builder_insert(nix_c_context * context, BindingsBuilder * b context->last_err_code = NIX_OK; try { auto & v = check_value_not_null(value); + check_value_initialized(v); nix::Symbol s = bb->builder.state.symbols.create(name); bb->builder.insert(s, &v); } @@ -551,6 +594,7 @@ nix_realised_string * nix_string_realise(nix_c_context * context, EvalState * st context->last_err_code = NIX_OK; try { auto & v = check_value_not_null(value); + check_value_initialized(v); nix::NixStringContext stringContext; auto rawStr = state->state.coerceToString(nix::noPos, v, stringContext, "while realising a string").toOwned(); nix::StorePathSet storePaths; diff --git a/tests/unit/libexpr/nix_api_value.cc b/tests/unit/libexpr/nix_api_value.cc index ac0cdb9c4..9e922f8c7 100644 --- a/tests/unit/libexpr/nix_api_value.cc +++ b/tests/unit/libexpr/nix_api_value.cc @@ -14,11 +14,16 @@ namespace nixC { -TEST_F(nix_api_expr_test, nix_value_set_get_int) +TEST_F(nix_api_expr_test, nix_value_get_int_invalid) { ASSERT_EQ(0, nix_get_int(ctx, nullptr)); - ASSERT_DEATH(nix_get_int(ctx, value), ""); + assert_ctx_err(); + ASSERT_EQ(0, nix_get_int(ctx, value)); + assert_ctx_err(); +} +TEST_F(nix_api_expr_test, nix_value_set_get_int) +{ int myInt = 1; nix_init_int(ctx, value, myInt); @@ -27,11 +32,16 @@ TEST_F(nix_api_expr_test, nix_value_set_get_int) ASSERT_EQ(NIX_TYPE_INT, nix_get_type(ctx, value)); } -TEST_F(nix_api_expr_test, nix_value_set_get_float) +TEST_F(nix_api_expr_test, nix_value_set_get_float_invalid) { ASSERT_FLOAT_EQ(0.0, nix_get_float(ctx, nullptr)); - ASSERT_DEATH(nix_get_float(ctx, value), ""); + assert_ctx_err(); + ASSERT_FLOAT_EQ(0.0, nix_get_float(ctx, value)); + assert_ctx_err(); +} +TEST_F(nix_api_expr_test, nix_value_set_get_float) +{ float myDouble = 1.0; nix_init_float(ctx, value, myDouble); @@ -40,11 +50,16 @@ TEST_F(nix_api_expr_test, nix_value_set_get_float) ASSERT_EQ(NIX_TYPE_FLOAT, nix_get_type(ctx, value)); } -TEST_F(nix_api_expr_test, nix_value_set_get_bool) +TEST_F(nix_api_expr_test, nix_value_set_get_bool_invalid) { ASSERT_EQ(false, nix_get_bool(ctx, nullptr)); - ASSERT_DEATH(nix_get_bool(ctx, value), ""); + assert_ctx_err(); + ASSERT_EQ(false, nix_get_bool(ctx, value)); + assert_ctx_err(); +} +TEST_F(nix_api_expr_test, nix_value_set_get_bool) +{ bool myBool = true; nix_init_bool(ctx, value, myBool); @@ -53,12 +68,18 @@ TEST_F(nix_api_expr_test, nix_value_set_get_bool) ASSERT_EQ(NIX_TYPE_BOOL, nix_get_type(ctx, value)); } -TEST_F(nix_api_expr_test, nix_value_set_get_string) +TEST_F(nix_api_expr_test, nix_value_set_get_string_invalid) { std::string string_value; ASSERT_EQ(NIX_ERR_UNKNOWN, nix_get_string(ctx, nullptr, OBSERVE_STRING(string_value))); - ASSERT_DEATH(nix_get_string(ctx, value, OBSERVE_STRING(string_value)), ""); + assert_ctx_err(); + ASSERT_EQ(NIX_ERR_UNKNOWN, nix_get_string(ctx, value, OBSERVE_STRING(string_value))); + assert_ctx_err(); +} +TEST_F(nix_api_expr_test, nix_value_set_get_string) +{ + std::string string_value; const char * myString = "some string"; nix_init_string(ctx, value, myString); @@ -68,21 +89,29 @@ TEST_F(nix_api_expr_test, nix_value_set_get_string) ASSERT_EQ(NIX_TYPE_STRING, nix_get_type(ctx, value)); } +TEST_F(nix_api_expr_test, nix_value_set_get_null_invalid) +{ + ASSERT_EQ(NULL, nix_get_typename(ctx, value)); + assert_ctx_err(); +} + TEST_F(nix_api_expr_test, nix_value_set_get_null) { - ASSERT_DEATH(nix_get_typename(ctx, value), ""); - nix_init_null(ctx, value); ASSERT_STREQ("null", nix_get_typename(ctx, value)); ASSERT_EQ(NIX_TYPE_NULL, nix_get_type(ctx, value)); } -TEST_F(nix_api_expr_test, nix_value_set_get_path) +TEST_F(nix_api_expr_test, nix_value_set_get_path_invalid) { ASSERT_EQ(nullptr, nix_get_path_string(ctx, nullptr)); - ASSERT_DEATH(nix_get_path_string(ctx, value), ""); - + assert_ctx_err(); + ASSERT_EQ(nullptr, nix_get_path_string(ctx, value)); + assert_ctx_err(); +} +TEST_F(nix_api_expr_test, nix_value_set_get_path) +{ const char * p = "/nix/store/40s0qmrfb45vlh6610rk29ym318dswdr-myname"; nix_init_path_string(ctx, state, value, p); @@ -91,14 +120,21 @@ TEST_F(nix_api_expr_test, nix_value_set_get_path) ASSERT_EQ(NIX_TYPE_PATH, nix_get_type(ctx, value)); } -TEST_F(nix_api_expr_test, nix_build_and_init_list) +TEST_F(nix_api_expr_test, nix_build_and_init_list_invalid) { ASSERT_EQ(nullptr, nix_get_list_byidx(ctx, nullptr, state, 0)); + assert_ctx_err(); ASSERT_EQ(0, nix_get_list_size(ctx, nullptr)); + assert_ctx_err(); - ASSERT_DEATH(nix_get_list_byidx(ctx, value, state, 0), ""); - ASSERT_DEATH(nix_get_list_size(ctx, value), ""); + ASSERT_EQ(nullptr, nix_get_list_byidx(ctx, value, state, 0)); + assert_ctx_err(); + ASSERT_EQ(0, nix_get_list_size(ctx, value)); + assert_ctx_err(); +} +TEST_F(nix_api_expr_test, nix_build_and_init_list) +{ int size = 10; ListBuilder * builder = nix_make_list_builder(ctx, state, size); @@ -119,20 +155,33 @@ TEST_F(nix_api_expr_test, nix_build_and_init_list) nix_gc_decref(ctx, intValue); } -TEST_F(nix_api_expr_test, nix_build_and_init_attr) +TEST_F(nix_api_expr_test, nix_build_and_init_attr_invalid) { ASSERT_EQ(nullptr, nix_get_attr_byname(ctx, nullptr, state, 0)); + assert_ctx_err(); ASSERT_EQ(nullptr, nix_get_attr_byidx(ctx, nullptr, state, 0, nullptr)); + assert_ctx_err(); ASSERT_EQ(nullptr, nix_get_attr_name_byidx(ctx, nullptr, state, 0)); + assert_ctx_err(); ASSERT_EQ(0, nix_get_attrs_size(ctx, nullptr)); + assert_ctx_err(); ASSERT_EQ(false, nix_has_attr_byname(ctx, nullptr, state, "no-value")); + assert_ctx_err(); - ASSERT_DEATH(nix_get_attr_byname(ctx, value, state, 0), ""); - ASSERT_DEATH(nix_get_attr_byidx(ctx, value, state, 0, nullptr), ""); - ASSERT_DEATH(nix_get_attr_name_byidx(ctx, value, state, 0), ""); - ASSERT_DEATH(nix_get_attrs_size(ctx, value), ""); - ASSERT_DEATH(nix_has_attr_byname(ctx, value, state, "no-value"), ""); + ASSERT_EQ(nullptr, nix_get_attr_byname(ctx, value, state, 0)); + assert_ctx_err(); + ASSERT_EQ(nullptr, nix_get_attr_byidx(ctx, value, state, 0, nullptr)); + assert_ctx_err(); + ASSERT_EQ(nullptr, nix_get_attr_name_byidx(ctx, value, state, 0)); + assert_ctx_err(); + ASSERT_EQ(0, nix_get_attrs_size(ctx, value)); + assert_ctx_err(); + ASSERT_EQ(false, nix_has_attr_byname(ctx, value, state, "no-value")); + assert_ctx_err(); +} +TEST_F(nix_api_expr_test, nix_build_and_init_attr) +{ int size = 10; const char ** out_name = (const char **) malloc(sizeof(char *)); diff --git a/tests/unit/libutil-support/tests/nix_api_util.hh b/tests/unit/libutil-support/tests/nix_api_util.hh index 75d302bd6..efd200116 100644 --- a/tests/unit/libutil-support/tests/nix_api_util.hh +++ b/tests/unit/libutil-support/tests/nix_api_util.hh @@ -24,7 +24,9 @@ protected: nix_c_context * ctx; - inline void assert_ctx_ok() { + inline void assert_ctx_ok() + { + if (nix_err_code(ctx) == NIX_OK) { return; } @@ -33,5 +35,14 @@ protected: std::string msg(p, n); FAIL() << "nix_err_code(ctx) != NIX_OK, message: " << msg; } + + inline void assert_ctx_err() + { + if (nix_err_code(ctx) != NIX_OK) { + return; + } + FAIL() << "Got NIX_OK, but expected an error!"; + } }; + } From ff76dd2211d0acddc82a35bb0b1a884323c2a3cd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Luis=20Lafuente?= Date: Sun, 21 Apr 2024 20:24:56 +0200 Subject: [PATCH 4/6] C API: fix test, nix float is a double internally --- tests/unit/libexpr/nix_api_value.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/unit/libexpr/nix_api_value.cc b/tests/unit/libexpr/nix_api_value.cc index 9e922f8c7..7c8e82c50 100644 --- a/tests/unit/libexpr/nix_api_value.cc +++ b/tests/unit/libexpr/nix_api_value.cc @@ -34,18 +34,18 @@ TEST_F(nix_api_expr_test, nix_value_set_get_int) TEST_F(nix_api_expr_test, nix_value_set_get_float_invalid) { - ASSERT_FLOAT_EQ(0.0, nix_get_float(ctx, nullptr)); + ASSERT_DOUBLE_EQ(0.0, nix_get_float(ctx, nullptr)); assert_ctx_err(); - ASSERT_FLOAT_EQ(0.0, nix_get_float(ctx, value)); + ASSERT_DOUBLE_EQ(0.0, nix_get_float(ctx, value)); assert_ctx_err(); } TEST_F(nix_api_expr_test, nix_value_set_get_float) { - float myDouble = 1.0; + double myDouble = 1.0; nix_init_float(ctx, value, myDouble); - ASSERT_FLOAT_EQ(myDouble, nix_get_float(ctx, value)); + ASSERT_DOUBLE_EQ(myDouble, nix_get_float(ctx, value)); ASSERT_STREQ("a float", nix_get_typename(ctx, value)); ASSERT_EQ(NIX_TYPE_FLOAT, nix_get_type(ctx, value)); } From 8d70db3251f68597f332de1bffcc3f716ff30450 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Luis=20Lafuente?= Date: Sun, 21 Apr 2024 22:13:26 +0200 Subject: [PATCH 5/6] C API: add check_value_[in,out] helper functions --- src/libexpr-c/nix_api_value.cc | 109 ++++++++++++---------------- tests/unit/libexpr/nix_api_value.cc | 9 ++- 2 files changed, 54 insertions(+), 64 deletions(-) diff --git a/src/libexpr-c/nix_api_value.cc b/src/libexpr-c/nix_api_value.cc index a3686022d..0d61292c4 100644 --- a/src/libexpr-c/nix_api_value.cc +++ b/src/libexpr-c/nix_api_value.cc @@ -20,7 +20,7 @@ # include "gc_cpp.h" #endif -// Helper function to throw an exception if value is null or in an invalid state +// Internal helper functions to check [in] and [out] `Value *` parameters static const nix::Value & check_value_not_null(const Value * value) { if (!value) { @@ -37,18 +37,31 @@ static nix::Value & check_value_not_null(Value * value) return *((nix::Value *) value); } -static void check_value_initialized(const nix::Value & value) +static const nix::Value & check_value_in(const Value * value) { - if (!value.isValid()) { + auto & v = check_value_not_null(value); + if (!v.isValid()) { throw std::runtime_error("Uninitialized Value"); } + return v; } -static void check_value_uninitialized(const nix::Value & value) +static nix::Value & check_value_in(Value * value) { - if (value.isValid()) { + auto & v = check_value_not_null(value); + if (!v.isValid()) { + throw std::runtime_error("Uninitialized Value"); + } + return v; +} + +static nix::Value & check_value_out(Value * value) +{ + auto & v = check_value_not_null(value); + if (v.isValid()) { throw std::runtime_error("Value already initialized. Variables are immutable"); } + return v; } /** @@ -125,8 +138,7 @@ ValueType nix_get_type(nix_c_context * context, const Value * value) if (context) context->last_err_code = NIX_OK; try { - auto & v = check_value_not_null(value); - check_value_initialized(v); + auto & v = check_value_in(value); using namespace nix; switch (v.type()) { case nThunk: @@ -162,8 +174,7 @@ const char * nix_get_typename(nix_c_context * context, const Value * value) if (context) context->last_err_code = NIX_OK; try { - auto & v = check_value_not_null(value); - check_value_initialized(v); + auto & v = check_value_in(value); auto s = nix::showType(v); return strdup(s.c_str()); } @@ -175,8 +186,7 @@ bool nix_get_bool(nix_c_context * context, const Value * value) if (context) context->last_err_code = NIX_OK; try { - auto & v = check_value_not_null(value); - check_value_initialized(v); + auto & v = check_value_in(value); assert(v.type() == nix::nBool); return v.boolean(); } @@ -188,8 +198,7 @@ nix_err nix_get_string(nix_c_context * context, const Value * value, nix_get_str if (context) context->last_err_code = NIX_OK; try { - auto & v = check_value_not_null(value); - check_value_initialized(v); + auto & v = check_value_in(value); assert(v.type() == nix::nString); call_nix_get_string_callback(v.c_str(), callback, user_data); } @@ -201,8 +210,7 @@ const char * nix_get_path_string(nix_c_context * context, const Value * value) if (context) context->last_err_code = NIX_OK; try { - auto & v = check_value_not_null(value); - check_value_initialized(v); + auto & v = check_value_in(value); assert(v.type() == nix::nPath); // NOTE (from @yorickvP) // v._path.path should work but may not be how Eelco intended it. @@ -221,8 +229,7 @@ unsigned int nix_get_list_size(nix_c_context * context, const Value * value) if (context) context->last_err_code = NIX_OK; try { - auto & v = check_value_not_null(value); - check_value_initialized(v); + auto & v = check_value_in(value); assert(v.type() == nix::nList); return v.listSize(); } @@ -234,8 +241,7 @@ unsigned int nix_get_attrs_size(nix_c_context * context, const Value * value) if (context) context->last_err_code = NIX_OK; try { - auto & v = check_value_not_null(value); - check_value_initialized(v); + auto & v = check_value_in(value); assert(v.type() == nix::nAttrs); return v.attrs()->size(); } @@ -247,8 +253,7 @@ double nix_get_float(nix_c_context * context, const Value * value) if (context) context->last_err_code = NIX_OK; try { - auto & v = check_value_not_null(value); - check_value_initialized(v); + auto & v = check_value_in(value); assert(v.type() == nix::nFloat); return v.fpoint(); } @@ -260,8 +265,7 @@ int64_t nix_get_int(nix_c_context * context, const Value * value) if (context) context->last_err_code = NIX_OK; try { - auto & v = check_value_not_null(value); - check_value_initialized(v); + auto & v = check_value_in(value); assert(v.type() == nix::nInt); return v.integer(); } @@ -273,8 +277,7 @@ ExternalValue * nix_get_external(nix_c_context * context, Value * value) if (context) context->last_err_code = NIX_OK; try { - auto & v = check_value_not_null(value); - check_value_initialized(v); + auto & v = check_value_out(value); assert(v.type() == nix::nExternal); return (ExternalValue *) v.external(); } @@ -286,8 +289,7 @@ Value * nix_get_list_byidx(nix_c_context * context, const Value * value, EvalSta if (context) context->last_err_code = NIX_OK; try { - auto & v = check_value_not_null(value); - check_value_initialized(v); + auto & v = check_value_in(value); assert(v.type() == nix::nList); auto * p = v.listElems()[ix]; nix_gc_incref(nullptr, p); @@ -303,8 +305,7 @@ Value * nix_get_attr_byname(nix_c_context * context, const Value * value, EvalSt if (context) context->last_err_code = NIX_OK; try { - auto & v = check_value_not_null(value); - check_value_initialized(v); + auto & v = check_value_in(value); assert(v.type() == nix::nAttrs); nix::Symbol s = state->state.symbols.create(name); auto attr = v.attrs()->get(s); @@ -324,8 +325,7 @@ bool nix_has_attr_byname(nix_c_context * context, const Value * value, EvalState if (context) context->last_err_code = NIX_OK; try { - auto & v = check_value_not_null(value); - check_value_initialized(v); + auto & v = check_value_in(value); assert(v.type() == nix::nAttrs); nix::Symbol s = state->state.symbols.create(name); auto attr = v.attrs()->get(s); @@ -342,8 +342,7 @@ nix_get_attr_byidx(nix_c_context * context, const Value * value, EvalState * sta if (context) context->last_err_code = NIX_OK; try { - auto & v = check_value_not_null(value); - check_value_initialized(v); + auto & v = check_value_in(value); const nix::Attr & a = (*v.attrs())[i]; *name = ((const std::string &) (state->state.symbols[a.name])).c_str(); nix_gc_incref(nullptr, a.value); @@ -358,8 +357,7 @@ const char * nix_get_attr_name_byidx(nix_c_context * context, const Value * valu if (context) context->last_err_code = NIX_OK; try { - auto & v = check_value_not_null(value); - check_value_initialized(v); + auto & v = check_value_in(value); const nix::Attr & a = (*v.attrs())[i]; return ((const std::string &) (state->state.symbols[a.name])).c_str(); } @@ -371,8 +369,7 @@ nix_err nix_init_bool(nix_c_context * context, Value * value, bool b) if (context) context->last_err_code = NIX_OK; try { - auto & v = check_value_not_null(value); - check_value_uninitialized(v); + auto & v = check_value_out(value); v.mkBool(b); } NIXC_CATCH_ERRS @@ -384,8 +381,7 @@ nix_err nix_init_string(nix_c_context * context, Value * value, const char * str if (context) context->last_err_code = NIX_OK; try { - auto & v = check_value_not_null(value); - check_value_uninitialized(v); + auto & v = check_value_out(value); v.mkString(std::string_view(str)); } NIXC_CATCH_ERRS @@ -396,8 +392,7 @@ nix_err nix_init_path_string(nix_c_context * context, EvalState * s, Value * val if (context) context->last_err_code = NIX_OK; try { - auto & v = check_value_not_null(value); - check_value_uninitialized(v); + auto & v = check_value_out(value); v.mkPath(s->state.rootPath(nix::CanonPath(str))); } NIXC_CATCH_ERRS @@ -408,8 +403,7 @@ nix_err nix_init_float(nix_c_context * context, Value * value, double d) if (context) context->last_err_code = NIX_OK; try { - auto & v = check_value_not_null(value); - check_value_uninitialized(v); + auto & v = check_value_out(value); v.mkFloat(d); } NIXC_CATCH_ERRS @@ -420,8 +414,7 @@ nix_err nix_init_int(nix_c_context * context, Value * value, int64_t i) if (context) context->last_err_code = NIX_OK; try { - auto & v = check_value_not_null(value); - check_value_uninitialized(v); + auto & v = check_value_out(value); v.mkInt(i); } NIXC_CATCH_ERRS @@ -432,8 +425,7 @@ nix_err nix_init_null(nix_c_context * context, Value * value) if (context) context->last_err_code = NIX_OK; try { - auto & v = check_value_not_null(value); - check_value_uninitialized(v); + auto & v = check_value_out(value); v.mkNull(); } NIXC_CATCH_ERRS @@ -457,8 +449,7 @@ nix_err nix_init_external(nix_c_context * context, Value * value, ExternalValue if (context) context->last_err_code = NIX_OK; try { - auto & v = check_value_not_null(value); - check_value_uninitialized(v); + auto & v = check_value_out(value); auto r = (nix::ExternalValueBase *) val; v.mkExternal(r); } @@ -486,7 +477,6 @@ nix_err nix_list_builder_insert(nix_c_context * context, ListBuilder * list_buil context->last_err_code = NIX_OK; try { auto & e = check_value_not_null(value); - check_value_initialized(e); list_builder->builder[index] = &e; } NIXC_CATCH_ERRS @@ -506,8 +496,7 @@ nix_err nix_make_list(nix_c_context * context, ListBuilder * list_builder, Value if (context) context->last_err_code = NIX_OK; try { - auto & v = check_value_not_null(value); - check_value_uninitialized(v); + auto & v = check_value_out(value); v.mkList(list_builder->builder); } NIXC_CATCH_ERRS @@ -518,8 +507,7 @@ nix_err nix_init_primop(nix_c_context * context, Value * value, PrimOp * p) if (context) context->last_err_code = NIX_OK; try { - auto & v = check_value_not_null(value); - check_value_uninitialized(v); + auto & v = check_value_out(value); v.mkPrimOp((nix::PrimOp *) p); } NIXC_CATCH_ERRS @@ -530,10 +518,8 @@ nix_err nix_copy_value(nix_c_context * context, Value * value, Value * source) if (context) context->last_err_code = NIX_OK; try { - auto & v = check_value_not_null(value); - check_value_uninitialized(v); - auto & s = check_value_not_null(source); - check_value_initialized(s); + auto & v = check_value_out(value); + auto & s = check_value_in(source); v = s; } NIXC_CATCH_ERRS @@ -544,8 +530,7 @@ nix_err nix_make_attrs(nix_c_context * context, Value * value, BindingsBuilder * if (context) context->last_err_code = NIX_OK; try { - auto & v = check_value_not_null(value); - check_value_uninitialized(v); + auto & v = check_value_out(value); v.mkAttrs(b->builder); } NIXC_CATCH_ERRS @@ -572,7 +557,6 @@ nix_err nix_bindings_builder_insert(nix_c_context * context, BindingsBuilder * b context->last_err_code = NIX_OK; try { auto & v = check_value_not_null(value); - check_value_initialized(v); nix::Symbol s = bb->builder.state.symbols.create(name); bb->builder.insert(s, &v); } @@ -593,8 +577,7 @@ nix_realised_string * nix_string_realise(nix_c_context * context, EvalState * st if (context) context->last_err_code = NIX_OK; try { - auto & v = check_value_not_null(value); - check_value_initialized(v); + auto & v = check_value_in(value); nix::NixStringContext stringContext; auto rawStr = state->state.coerceToString(nix::noPos, v, stringContext, "while realising a string").toOwned(); nix::StorePathSet storePaths; diff --git a/tests/unit/libexpr/nix_api_value.cc b/tests/unit/libexpr/nix_api_value.cc index 7c8e82c50..6e209695e 100644 --- a/tests/unit/libexpr/nix_api_value.cc +++ b/tests/unit/libexpr/nix_api_value.cc @@ -139,13 +139,20 @@ TEST_F(nix_api_expr_test, nix_build_and_init_list) ListBuilder * builder = nix_make_list_builder(ctx, state, size); Value * intValue = nix_alloc_value(ctx, state); + Value * intValue2 = nix_alloc_value(ctx, state); + + // `init` and `insert` can be called in any order nix_init_int(ctx, intValue, 42); nix_list_builder_insert(ctx, builder, 0, intValue); + nix_list_builder_insert(ctx, builder, 1, intValue2); + nix_init_int(ctx, intValue2, 43); + nix_make_list(ctx, builder, value); nix_list_builder_free(builder); ASSERT_EQ(42, nix_get_int(ctx, nix_get_list_byidx(ctx, value, state, 0))); - ASSERT_EQ(nullptr, nix_get_list_byidx(ctx, value, state, 1)); + ASSERT_EQ(43, nix_get_int(ctx, nix_get_list_byidx(ctx, value, state, 1))); + ASSERT_EQ(nullptr, nix_get_list_byidx(ctx, value, state, 2)); ASSERT_EQ(10, nix_get_list_size(ctx, value)); ASSERT_STREQ("a list", nix_get_typename(ctx, value)); From 6acf02b32a12c9db052c9ca24a238882f12f10a9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Luis=20Lafuente?= Date: Sun, 21 Apr 2024 22:15:12 +0200 Subject: [PATCH 6/6] C API: source argument to nix_copy_value should be const --- src/libexpr-c/nix_api_value.cc | 2 +- src/libexpr-c/nix_api_value.h | 2 +- tests/unit/libexpr/nix_api_value.cc | 13 +++++++++++++ 3 files changed, 15 insertions(+), 2 deletions(-) diff --git a/src/libexpr-c/nix_api_value.cc b/src/libexpr-c/nix_api_value.cc index 0d61292c4..0366e5020 100644 --- a/src/libexpr-c/nix_api_value.cc +++ b/src/libexpr-c/nix_api_value.cc @@ -513,7 +513,7 @@ nix_err nix_init_primop(nix_c_context * context, Value * value, PrimOp * p) NIXC_CATCH_ERRS } -nix_err nix_copy_value(nix_c_context * context, Value * value, Value * source) +nix_err nix_copy_value(nix_c_context * context, Value * value, const Value * source) { if (context) context->last_err_code = NIX_OK; diff --git a/src/libexpr-c/nix_api_value.h b/src/libexpr-c/nix_api_value.h index d8bd77c33..b2b3439ef 100644 --- a/src/libexpr-c/nix_api_value.h +++ b/src/libexpr-c/nix_api_value.h @@ -422,7 +422,7 @@ nix_err nix_init_primop(nix_c_context * context, Value * value, PrimOp * op); * @param[in] source value to copy from * @return error code, NIX_OK on success. */ -nix_err nix_copy_value(nix_c_context * context, Value * value, Value * source); +nix_err nix_copy_value(nix_c_context * context, Value * value, const Value * source); /**@}*/ /** @brief Create a bindings builder diff --git a/tests/unit/libexpr/nix_api_value.cc b/tests/unit/libexpr/nix_api_value.cc index 6e209695e..6e1131e10 100644 --- a/tests/unit/libexpr/nix_api_value.cc +++ b/tests/unit/libexpr/nix_api_value.cc @@ -367,4 +367,17 @@ TEST_F(nix_api_expr_test, nix_value_init_apply_lazy_arg) nix_gc_decref(ctx, e); } +TEST_F(nix_api_expr_test, nix_copy_value) +{ + Value * source = nix_alloc_value(ctx, state); + + nix_init_int(ctx, source, 42); + nix_copy_value(ctx, value, source); + + ASSERT_EQ(42, nix_get_int(ctx, value)); + + // Clean up + nix_gc_decref(ctx, source); +} + }