Skip to content

update(libsinsp): get_dynamic_field constness #2511

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+15 −11
Open

update(libsinsp): get_dynamic_field constness #2511

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions userspace/libsinsp/state/dynamic_struct.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,7 +261,7 @@ class dynamic_struct {
* @brief Accesses a field with the given accessor and reads its value.
*/
template<typename T, typename Val = T>
inline void get_dynamic_field(const field_accessor<T>& a, Val& out) {
inline void get_dynamic_field(const field_accessor<T>& a, Val& out) const {
_check_defsptr(a.info(), false);
get_dynamic_field(a.info(), reinterpret_cast<void*>(&out));
}
Expand Down Expand Up @@ -308,7 +308,7 @@ class dynamic_struct {
* according to the type definitions supported in libsinsp::state::typeinfo.
* For strings, "out" is considered of type const char**.
*/
virtual void get_dynamic_field(const field_info& i, void* out) {
virtual void get_dynamic_field(const field_info& i, void* out) const {
const auto* buf = _access_dynamic_field(i.m_index);
if(i.info().type_id() == SS_PLUGIN_ST_STRING) {
*((const char**)out) = ((const std::string*)buf)->c_str();
Expand Down Expand Up @@ -360,7 +360,7 @@ class dynamic_struct {
}
}

inline void* _access_dynamic_field(size_t index) {
inline void* _access_dynamic_field(size_t index) const {
if(!m_dynamic_fields) {
throw sinsp_exception("dynamic struct has no field definitions");
}
Expand Down Expand Up @@ -397,7 +397,7 @@ class dynamic_struct {
}
}

std::vector<void*> m_fields;
mutable std::vector<void*> m_fields;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reason behind this mutable here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We return a pointer to an item inside m_fields, but if the vector is too small, we resize it first (see _access_dynamic_field() above)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

get_dynamic_field calls _access_dynamic_field.
Dynamic fields are lazily constructed there, and inserted into m_fields.
Without the mutable we can't make the thing const.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about providing a separate const accessor to the field? In general, I would avoid using mutable as I think it is just a way of hiding non-constness.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That won't work, unless we make the field loading non-lazy, and I assume it was done like that on purpose (performance reasons? avoiding allocations until the field gets accessed?).
If you think of it as an internal cache, we're not really breaking the "OOP" constness of the class.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we e.g. return nullptr from the const accessor (and handle it in a higher layer by returning the default value)?

This would make it even lazier, if anything (the field would get allocated on write, not on read). I have a vague uneasy feeling about mutable (something something thread safety), but if the nullptr approach doesn't work for whatever reason, I guess this is the way to go

std::shared_ptr<field_infos> m_dynamic_fields;
};

Expand All @@ -409,15 +409,15 @@ class dynamic_struct {
template<>
inline void libsinsp::state::dynamic_struct::get_dynamic_field<std::string, const char*>(
const field_accessor<std::string>& a,
const char*& out) {
const char*& out) const {
_check_defsptr(a.info(), false);
get_dynamic_field(a.info(), reinterpret_cast<void*>(&out));
}

template<>
inline void libsinsp::state::dynamic_struct::get_dynamic_field<std::string, std::string>(
const field_accessor<std::string>& a,
std::string& out) {
std::string& out) const {
const char* s = NULL;
get_dynamic_field(a, s);
if(!s) {
Expand Down
10 changes: 7 additions & 3 deletions userspace/libsinsp/state/table_adapters.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,8 @@ class pair_table_entry_adapter : public libsinsp::state::table_entry {
inline void set_value(std::pair<Tfirst, Tsecond>* v) { m_value = v; }

protected:
virtual void get_dynamic_field(const dynamic_struct::field_info& i, void* out) override final {
virtual void get_dynamic_field(const dynamic_struct::field_info& i,
void* out) const override final {
if(i.index() > 1 || i.defs_id() != s_dynamic_fields_id) {
throw sinsp_exception(
"invalid field info passed to pair_table_entry_adapter::get_dynamic_field");
Expand Down Expand Up @@ -115,7 +116,9 @@ class pair_table_entry_adapter : public libsinsp::state::table_entry {
std::pair<Tfirst, Tsecond>* m_value;

template<typename T>
inline void get_dynamic_field(const dynamic_struct::field_info& i, const T* value, void* out) {
inline void get_dynamic_field(const dynamic_struct::field_info& i,
const T* value,
void* out) const {
if(i.info().type_id() == SS_PLUGIN_ST_STRING) {
*((const char**)out) = ((const std::string*)value)->c_str();
} else {
Expand Down Expand Up @@ -169,7 +172,8 @@ class value_table_entry_adapter : public libsinsp::state::table_entry {
inline void set_value(T* v) { m_value = v; }

protected:
virtual void get_dynamic_field(const dynamic_struct::field_info& i, void* out) override final {
virtual void get_dynamic_field(const dynamic_struct::field_info& i,
void* out) const override final {
if(i.index() != 0 || i.defs_id() != s_dynamic_fields_id) {
throw sinsp_exception(
"invalid field info passed to value_table_entry_adapter::get_dynamic_field");
Expand Down
2 changes: 1 addition & 1 deletion userspace/libsinsp/threadinfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -407,7 +407,7 @@ std::string sinsp_threadinfo::get_exepath() const {
return m_exepath;
}

std::string sinsp_threadinfo::get_container_id() {
std::string sinsp_threadinfo::get_container_id() const {
std::string container_id;

const auto accessor = m_params->thread_manager->get_field_accessor(
Expand Down
2 changes: 1 addition & 1 deletion userspace/libsinsp/threadinfo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ class SINSP_PUBLIC sinsp_threadinfo : public libsinsp::state::table_entry {
\brief Return the container_id associated with this thread, if the container plugins is
running, leveraging sinsp state table API.
*/
std::string get_container_id();
std::string get_container_id() const;

/*!
\brief Given the container_id associated with this thread, feetches the container user from
Expand Down
Loading