From 2855df63ce69661356002974bf9069c24e5a85c2 Mon Sep 17 00:00:00 2001 From: Jon Siwek Date: Fri, 26 Mar 2021 18:57:42 -0700 Subject: [PATCH 1/4] Add RecordVal::AssignField() and use it in supervisor code This is a convenience method to assign a known record field value by field name. May also be useful to reduce warnings from static analysis (e.g. Coverity) about not checking for negative return values before assigning since that now flows through a [[noreturn]] error path. --- src/Val.h | 13 ++++++++++++ src/supervisor/Supervisor.cc | 40 +++++++++++++++++------------------ src/supervisor/supervisor.bif | 2 +- 3 files changed, 33 insertions(+), 22 deletions(-) diff --git a/src/Val.h b/src/Val.h index 4d4984db0a..9b402b0d2d 100644 --- a/src/Val.h +++ b/src/Val.h @@ -1152,6 +1152,19 @@ public: void Assign(int field, String* new_val) { Assign(field, new StringVal(new_val)); } + /** + * Assign a value of type @c T to a record field of the given name. + * A fatal error occurs if the no such field name exists. + */ + template + void AssignField(const char* field_name, T&& val) + { + int idx = GetType()->AsRecordType()->FieldOffset(field_name); + if ( idx < 0 ) + reporter->InternalError("missing record field: %s", field_name); + Assign(idx, std::forward(val)); + } + /** * Appends a value to the record's fields. The caller is responsible * for ensuring that fields are appended in the correct order and diff --git a/src/supervisor/Supervisor.cc b/src/supervisor/Supervisor.cc index ce3fe8d220..d751387a1e 100644 --- a/src/supervisor/Supervisor.cc +++ b/src/supervisor/Supervisor.cc @@ -1348,22 +1348,22 @@ RecordValPtr Supervisor::NodeConfig::ToRecord() const { const auto& rt = BifType::Record::Supervisor::NodeConfig; auto rval = make_intrusive(rt); - rval->Assign(rt->FieldOffset("name"), name); + rval->AssignField("name", name); if ( interface ) - rval->Assign(rt->FieldOffset("interface"), *interface); + rval->AssignField("interface", *interface); if ( directory ) - rval->Assign(rt->FieldOffset("directory"), *directory); + rval->AssignField("directory", *directory); if ( stdout_file ) - rval->Assign(rt->FieldOffset("stdout_file"), *stdout_file); + rval->AssignField("stdout_file", *stdout_file); if ( stderr_file ) - rval->Assign(rt->FieldOffset("stderr_file"), *stderr_file); + rval->AssignField("stderr_file", *stderr_file); if ( cpu_affinity ) - rval->Assign(rt->FieldOffset("cpu_affinity"), *cpu_affinity); + rval->AssignField("cpu_affinity", *cpu_affinity); auto st = rt->GetFieldType("scripts"); auto scripts_val = make_intrusive(std::move(st)); @@ -1371,11 +1371,11 @@ RecordValPtr Supervisor::NodeConfig::ToRecord() const for ( const auto& s : scripts ) scripts_val->Assign(scripts_val->Size(), make_intrusive(s)); - rval->Assign(rt->FieldOffset("scripts"), std::move(scripts_val)); + rval->AssignField("scripts", std::move(scripts_val)); auto tt = rt->GetFieldType("cluster"); auto cluster_val = make_intrusive(std::move(tt)); - rval->Assign(rt->FieldOffset("cluster"), cluster_val); + rval->AssignField("cluster", cluster_val); for ( const auto& e : cluster ) { @@ -1385,12 +1385,12 @@ RecordValPtr Supervisor::NodeConfig::ToRecord() const const auto& ept = BifType::Record::Supervisor::ClusterEndpoint; auto val = make_intrusive(ept); - val->Assign(ept->FieldOffset("role"), BifType::Enum::Supervisor::ClusterRole->GetEnumVal(ep.role)); - val->Assign(ept->FieldOffset("host"), make_intrusive(ep.host)); - val->Assign(ept->FieldOffset("p"), val_mgr->Port(ep.port, TRANSPORT_TCP)); + val->AssignField("role", BifType::Enum::Supervisor::ClusterRole->GetEnumVal(ep.role)); + val->AssignField("host", make_intrusive(ep.host)); + val->AssignField("p", val_mgr->Port(ep.port, TRANSPORT_TCP)); if ( ep.interface ) - val->Assign(ept->FieldOffset("interface"), *ep.interface); + val->AssignField("interface", *ep.interface); cluster_val->Assign(std::move(key), std::move(val)); } @@ -1403,10 +1403,10 @@ RecordValPtr SupervisorNode::ToRecord() const const auto& rt = BifType::Record::Supervisor::NodeStatus; auto rval = make_intrusive(rt); - rval->Assign(rt->FieldOffset("node"), config.ToRecord()); + rval->AssignField("node", config.ToRecord()); if ( pid ) - rval->Assign(rt->FieldOffset("pid"), pid); + rval->AssignField("pid", pid); return rval; } @@ -1458,17 +1458,15 @@ bool SupervisedNode::InitCluster() const auto val = make_intrusive(cluster_node_type); auto node_type = supervisor_role_to_cluster_node_type(ep.role); - val->Assign(cluster_node_type->FieldOffset("node_type"), std::move(node_type)); - val->Assign(cluster_node_type->FieldOffset("ip"), make_intrusive(ep.host)); - val->Assign(cluster_node_type->FieldOffset("p"), val_mgr->Port(ep.port, TRANSPORT_TCP)); + val->AssignField("node_type", std::move(node_type)); + val->AssignField("ip", make_intrusive(ep.host)); + val->AssignField("p", val_mgr->Port(ep.port, TRANSPORT_TCP)); if ( ep.interface ) - val->Assign(cluster_node_type->FieldOffset("interface"), - *ep.interface); + val->AssignField("interface", *ep.interface); if ( manager_name && ep.role != BifEnum::Supervisor::MANAGER ) - val->Assign(cluster_node_type->FieldOffset("manager"), - *manager_name); + val->AssignField("manager", *manager_name); cluster_nodes->Assign(std::move(key), std::move(val)); } diff --git a/src/supervisor/supervisor.bif b/src/supervisor/supervisor.bif index c143bc1dcf..3d65ab31a8 100644 --- a/src/supervisor/supervisor.bif +++ b/src/supervisor/supervisor.bif @@ -86,7 +86,7 @@ function Supervisor::__node%(%): Supervisor::NodeConfig zeek::emit_builtin_error("not a supervised process"); const auto& rt = zeek::BifType::Record::Supervisor::NodeConfig; auto rval = zeek::make_intrusive(rt); - rval->Assign(rt->FieldOffset("name"), ""); + rval->AssignField("name", ""); return rval; } From 7047eb92d8bf610570ef16d4a911916c73180a87 Mon Sep 17 00:00:00 2001 From: Jon Siwek Date: Fri, 26 Mar 2021 19:02:24 -0700 Subject: [PATCH 2/4] Change RecordVal::GetFieldAs() to use std::vector::operator[] Since the method claims it's up to the user to ensure the field exists before calling, the extra bounds-checking done by std::vector::at() isn't needed. --- src/Val.h | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/src/Val.h b/src/Val.h index 9b402b0d2d..bfbca2b6a6 100644 --- a/src/Val.h +++ b/src/Val.h @@ -1301,33 +1301,33 @@ public: if constexpr ( std::is_same_v || std::is_same_v || std::is_same_v ) - return record_val->at(field).int_val; + return record_val->operator[](field).int_val; else if constexpr ( std::is_same_v ) - return record_val->at(field).uint_val; + return record_val->operator[](field).uint_val; else if constexpr ( std::is_same_v || std::is_same_v || std::is_same_v ) - return record_val->at(field).double_val; + return record_val->operator[](field).double_val; else if constexpr ( std::is_same_v ) return val_mgr->Port(record_val->at(field).uint_val); else if constexpr ( std::is_same_v ) - return record_val->at(field).string_val->Get(); + return record_val->operator[](field).string_val->Get(); else if constexpr ( std::is_same_v ) - return record_val->at(field).addr_val->Get(); + return record_val->operator[](field).addr_val->Get(); else if constexpr ( std::is_same_v ) - return record_val->at(field).subnet_val->Get(); + return record_val->operator[](field).subnet_val->Get(); else if constexpr ( std::is_same_v ) - return *(record_val->at(field).file_val); + return *(record_val->operator[](field).file_val); else if constexpr ( std::is_same_v ) - return *(record_val->at(field).func_val); + return *(record_val->operator[](field).func_val); else if constexpr ( std::is_same_v ) - return record_val->at(field).re_val->Get(); + return record_val->operator[](field).re_val->Get(); else if constexpr ( std::is_same_v ) - return record_val->at(field).record_val; + return record_val->operator[](field).record_val; else if constexpr ( std::is_same_v ) - return record_val->at(field).vector_val; + return record_val->operator[](field).vector_val; else if constexpr ( std::is_same_v ) - return record_val->at(field).table_val->Get(); + return record_val->operator[](field).table_val->Get(); else { // It's an error to reach here, although because of @@ -1342,12 +1342,12 @@ public: T GetFieldAs(int field) const { if constexpr ( std::is_integral_v && std::is_signed_v ) - return record_val->at(field).int_val; + return record_val->operator[](field).int_val; else if constexpr ( std::is_integral_v && std::is_unsigned_v ) - return record_val->at(field).uint_val; + return record_val->operator[](field).uint_val; else if constexpr ( std::is_floating_point_v ) - return record_val->at(field).double_val; + return record_val->operator[](field).double_val; // Note: we could add other types here using type traits, // such as is_same_v, etc. From db975ac08ec45f35437abd125e4b7bac94b1a2c5 Mon Sep 17 00:00:00 2001 From: Jon Siwek Date: Fri, 26 Mar 2021 19:06:29 -0700 Subject: [PATCH 3/4] Fix maybe-uninitialized warning in ZVal::ToVal() Some compilers warn that 'v' may be used uninitialized but shouldn't be the case in practice since all cases are handled, making it impossible. --- src/ZVal.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/ZVal.cc b/src/ZVal.cc index 119d8c3f2e..0adaa70941 100644 --- a/src/ZVal.cc +++ b/src/ZVal.cc @@ -263,8 +263,9 @@ ValPtr ZVal::ToVal(const TypePtr& t) const case TYPE_TIMER: case TYPE_UNION: case TYPE_VOID: + default: v = nullptr; - reporter->InternalError("bad ret type return tag"); + reporter->InternalError("bad type in ZVal::ToVal: %s", type_name(t->Tag())); } if ( v ) From 43d9bda0077ef21ac80693f02158bc61321e2e3e Mon Sep 17 00:00:00 2001 From: Jon Siwek Date: Fri, 26 Mar 2021 21:59:30 -0700 Subject: [PATCH 4/4] Fix sign-compare compiler warning in coerce_to_record() --- src/Expr.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Expr.cc b/src/Expr.cc index 5778df4235..c23fdb6b16 100644 --- a/src/Expr.cc +++ b/src/Expr.cc @@ -3890,7 +3890,7 @@ ValPtr RecordCoerceExpr::Fold(Val* v) const RecordValPtr coerce_to_record(RecordTypePtr rt, Val* v, const std::vector& map) { - auto map_size = map.size(); + int map_size = map.size(); auto val = make_intrusive(rt); RecordType* val_type = val->GetType()->AsRecordType();