-
Notifications
You must be signed in to change notification settings - Fork 4
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.
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
Dbus connection contained in manager client or external #188
Conversation
2a54b55
to
d0a2863
Compare
@@ -30,11 +32,13 @@ struct config_property { | |||
|
|||
namespace dbus { | |||
static constexpr std::string_view property_name{ "config" }; | |||
} | |||
static constexpr std::string_view config_path{ "Config" }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed path of config, should we change the property_name
as well to capital first letter? @SindriTh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me, what does the DBUS naming standard say?
libs/ipc/inc/public/tfc/ipc.hpp
Outdated
auto new_state_filter = [this](value_t&& new_value) { | ||
// Here we get unfiltered new value and test whether the value matches the current value | ||
auto const& last_value = value(); | ||
// clang-format off | ||
PRAGMA_CLANG_WARNING_PUSH_OFF(-Wfloat-equal) | ||
if (!last_value.has_value() || new_value != last_value.value()) { | ||
PRAGMA_CLANG_WARNING_POP | ||
// clang-format on | ||
this->filters_(std::move(new_value)); | ||
return; | ||
} | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe this could be moved back, should we?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fyi filters cannot be moved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not really make sense to me.
You are comparing a value that is just off the wire with a value
that made it through the filters?
what if invert is applied?
Am I misunderstanding here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now caching unfiltered value with original new state filter within impl, added separate getter of unfiltered_value and value in ipc
void on_set(tfc::stx::invocable<value_t&&> auto&& callback) { | ||
interface_->register_method( | ||
std::string{ dbus::tags::tinker }, | ||
[callb = std::forward<decltype(callback)>(callback)](value_t const& set_value) { callb(value_t{ set_value }); }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sdbusplus does not support rvalue callback ...
: ipc_manager_client(std::make_shared<sdbusplus::asio::connection>(ctx, tfc::dbus::sd_bus_open_system())) {} | ||
: ipc_manager_client(std::make_shared<sdbusplus::asio::connection>(ctx, tfc::dbus::sd_bus_open_system())) { | ||
// If the owner of this client does not supply dbus connection we assume we are the sole owner of the connection | ||
connection_->request_name(tfc::dbus::make_dbus_process_name().c_str()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if manager client is spawned by context he makes the connection and requests name, not sure what happens if there are more than one who requests the same name? But I guess it won't happen in current code base
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I guess it won't happen in current code base
Famous last words
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
request_name, indicates that this routine can fail. Maybe we should try it and catch the error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it throws an error
if (r < 0)
{
throw exception::SdBusError(-r, "sd_bus_request_name");
}
but I don't see a reason catching it because we cannot continue if it fails right?
// If the provided interface is created by this class we initialize it here, | ||
// otherwise we assume it is taken care of elsewhere | ||
if (!interface_name_.empty()) { | ||
dbus_interface_->initialize(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So you create a interface to share with multiple parties.
then one of the parties initalizes the interface?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, similar to dbus connection
@@ -103,25 +110,44 @@ class slot { | |||
|
|||
auto operator=(slot const&) -> slot& = delete; | |||
|
|||
[[nodiscard]] auto value() const noexcept -> std::optional<value_t> const& { return slot_->value(); } | |||
[[nodiscard]] auto value() const noexcept -> std::optional<value_t> const& { return filters_.value(); } | |||
|
|||
[[nodiscard]] auto name() const noexcept -> std::string_view { return slot_->name(); } | |||
|
|||
[[nodiscard]] auto full_name() const noexcept -> std::string { return slot_->name_w_type(); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would look into calling this name_with_type or something more meaningful then full_name.
Maybe unique_name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say name_with_type
does not describe the whole truth as it is not strictly name with type, because it go prepended executable&process name
: ipc_manager_client(std::make_shared<sdbusplus::asio::connection>(ctx, tfc::dbus::sd_bus_open_system())) {} | ||
: ipc_manager_client(std::make_shared<sdbusplus::asio::connection>(ctx, tfc::dbus::sd_bus_open_system())) { | ||
// If the owner of this client does not supply dbus connection we assume we are the sole owner of the connection | ||
connection_->request_name(tfc::dbus::make_dbus_process_name().c_str()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
request_name, indicates that this routine can fail. Maybe we should try it and catch the error?
libs/ipc/inc/public/tfc/ipc.hpp
Outdated
auto new_state_filter = [this](value_t&& new_value) { | ||
// Here we get unfiltered new value and test whether the value matches the current value | ||
auto const& last_value = value(); | ||
// clang-format off | ||
PRAGMA_CLANG_WARNING_PUSH_OFF(-Wfloat-equal) | ||
if (!last_value.has_value() || new_value != last_value.value()) { | ||
PRAGMA_CLANG_WARNING_POP | ||
// clang-format on | ||
this->filters_(std::move(new_value)); | ||
return; | ||
} | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not really make sense to me.
You are comparing a value that is just off the wire with a value
that made it through the filters?
what if invert is applied?
Am I misunderstanding here.
223c03b
to
bb30e1c
Compare
d0103a0
to
f0f76eb
Compare
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
Already using this openbmc/sdbusplus#83 with sdbusplus patch, simplifying api