aserver: constructor to initialize properties
Functions like 'emit_added()' do not work correctly if properties are
not initialized. Often, the initial values for the properties of an
interface are known, such as
- Software Version Interface
- Software ActivationProgress Interface
- Software ApplyTime Interface
- ...
How was this previously handled?
- Write a wrapper class for the specific aserver class.
This allows to initialize the properties, as they are protected
members. Example [2]
- Write a wrapper class and provide methods to get/set the variables.
This is done in [1]
- Set each property with a method call, which emits
'PropertyChanged' signal by default, not ideal.
This also does not work if the interface is ever refactored to have
additional properties.
To facilitate using these interfaces without having to write a wrapper
class to inherit from them, and to avoid having to manually set each
property, which can easily be forgotten, provide a constructor to
allow for passing a struct with values for all the properties.
Tested: Unit tests pass, but still needs to be build-tested against
openbmc/openbmc. The concern there is mostly due to an added template
parameter.
References:
[1] https://github.com/openbmc/sdbusplus/blob/da8574d5888b2c1622f5482a47adc7a12ffa0d0e/example/calculator-aserver.cpp#L44
[2] https://github.com/openbmc/phosphor-bmc-code-mgmt/blob/4983b138ea8fc70bd66fe4d30500e6252629fa5d/common/src/software.cpp#L29
Change-Id: I873cbca97ae16b19bfbf622303f4d52e2563a62c
Signed-off-by: Alexander Hansen <alexander.hansen@9elements.com>
Signed-off-by: Patrick Williams <patrick@stwcx.xyz>
diff --git a/include/sdbusplus/async/server.hpp b/include/sdbusplus/async/server.hpp
index ce42874..5f14500 100644
--- a/include/sdbusplus/async/server.hpp
+++ b/include/sdbusplus/async/server.hpp
@@ -28,6 +28,13 @@
explicit server(sdbusplus::async::context& ctx, const char* path) :
context_ref(ctx), Types<Instance, Self>(path)...
{}
+
+ // This constructor accepting one properties_t per interface:
+ explicit server(
+ sdbusplus::async::context& ctx, const char* path,
+ typename Types<Instance, Self>::properties_t... propValues) :
+ context_ref(ctx), Types<Instance, Self>(path, propValues)...
+ {}
};
} // namespace server
diff --git a/test/gen/server/Test2/meson.build b/test/gen/server/Test2/meson.build
new file mode 100644
index 0000000..683bce6
--- /dev/null
+++ b/test/gen/server/Test2/meson.build
@@ -0,0 +1,38 @@
+# Generated file; do not modify.
+
+sdbusplus_current_path = 'server/Test2'
+
+generated_sources += custom_target(
+ 'server/Test2__cpp'.underscorify(),
+ input: ['../../../yaml/server/Test2.interface.yaml'],
+ output: [
+ 'common.hpp',
+ 'server.hpp',
+ 'server.cpp',
+ 'aserver.hpp',
+ 'client.hpp',
+ ],
+ depend_files: sdbusplusplus_depfiles,
+ command: [
+ sdbuspp_gen_meson_prog,
+ '--command',
+ 'cpp',
+ '--output',
+ meson.current_build_dir(),
+ '--tool',
+ sdbusplusplus_prog,
+ '--directory',
+ meson.current_source_dir() / '../../../yaml',
+ 'server/Test2',
+ ],
+ install: should_generate_cpp,
+ install_dir: [
+ get_option('includedir') / sdbusplus_current_path,
+ get_option('includedir') / sdbusplus_current_path,
+ false,
+ get_option('includedir') / sdbusplus_current_path,
+ get_option('includedir') / sdbusplus_current_path,
+ ],
+ build_by_default: should_generate_cpp,
+)
+
diff --git a/test/gen/server/Test3/meson.build b/test/gen/server/Test3/meson.build
new file mode 100644
index 0000000..2da40be
--- /dev/null
+++ b/test/gen/server/Test3/meson.build
@@ -0,0 +1,38 @@
+# Generated file; do not modify.
+
+sdbusplus_current_path = 'server/Test3'
+
+generated_sources += custom_target(
+ 'server/Test3__cpp'.underscorify(),
+ input: ['../../../yaml/server/Test3.interface.yaml'],
+ output: [
+ 'common.hpp',
+ 'server.hpp',
+ 'server.cpp',
+ 'aserver.hpp',
+ 'client.hpp',
+ ],
+ depend_files: sdbusplusplus_depfiles,
+ command: [
+ sdbuspp_gen_meson_prog,
+ '--command',
+ 'cpp',
+ '--output',
+ meson.current_build_dir(),
+ '--tool',
+ sdbusplusplus_prog,
+ '--directory',
+ meson.current_source_dir() / '../../../yaml',
+ 'server/Test3',
+ ],
+ install: should_generate_cpp,
+ install_dir: [
+ get_option('includedir') / sdbusplus_current_path,
+ get_option('includedir') / sdbusplus_current_path,
+ false,
+ get_option('includedir') / sdbusplus_current_path,
+ get_option('includedir') / sdbusplus_current_path,
+ ],
+ build_by_default: should_generate_cpp,
+)
+
diff --git a/test/gen/server/meson.build b/test/gen/server/meson.build
index 34c9b4b..142903d 100644
--- a/test/gen/server/meson.build
+++ b/test/gen/server/meson.build
@@ -1,5 +1,7 @@
# Generated file; do not modify.
subdir('Test')
+subdir('Test2')
+subdir('Test3')
sdbusplus_current_path = 'server'
@@ -25,3 +27,47 @@
build_by_default: should_generate_markdown,
)
+generated_markdown += custom_target(
+ 'server/Test2__markdown'.underscorify(),
+ input: ['../../yaml/server/Test2.interface.yaml'],
+ output: ['Test2.md'],
+ depend_files: sdbusplusplus_depfiles,
+ command: [
+ sdbuspp_gen_meson_prog,
+ '--command',
+ 'markdown',
+ '--output',
+ meson.current_build_dir(),
+ '--tool',
+ sdbusplusplus_prog,
+ '--directory',
+ meson.current_source_dir() / '../../yaml',
+ 'server/Test2',
+ ],
+ install: should_generate_markdown,
+ install_dir: [inst_markdown_dir / sdbusplus_current_path],
+ build_by_default: should_generate_markdown,
+)
+
+generated_markdown += custom_target(
+ 'server/Test3__markdown'.underscorify(),
+ input: ['../../yaml/server/Test3.interface.yaml'],
+ output: ['Test3.md'],
+ depend_files: sdbusplusplus_depfiles,
+ command: [
+ sdbuspp_gen_meson_prog,
+ '--command',
+ 'markdown',
+ '--output',
+ meson.current_build_dir(),
+ '--tool',
+ sdbusplusplus_prog,
+ '--directory',
+ meson.current_source_dir() / '../../yaml',
+ 'server/Test3',
+ ],
+ install: should_generate_markdown,
+ install_dir: [inst_markdown_dir / sdbusplus_current_path],
+ build_by_default: should_generate_markdown,
+)
+
diff --git a/test/gen/test_aserver_emit_interfaces_added_signal.cpp b/test/gen/test_aserver_emit_interfaces_added_signal.cpp
new file mode 100644
index 0000000..acc6ddc
--- /dev/null
+++ b/test/gen/test_aserver_emit_interfaces_added_signal.cpp
@@ -0,0 +1,64 @@
+#include "server/Test/aserver.hpp"
+#include "server/Test/common.hpp"
+#include "server/Test2/aserver.hpp"
+#include "server/Test2/common.hpp"
+
+#include <sdbusplus/async/context.hpp>
+#include <sdbusplus/async/match.hpp>
+#include <sdbusplus/async/timer.hpp>
+
+class A : public sdbusplus::aserver::server::Test<A>
+{};
+
+auto waitForMatch(sdbusplus::async::context& ctx,
+ sdbusplus::async::match& ifcAdded) -> sdbusplus::async::task<>
+{
+ auto x = co_await ifcAdded.next();
+
+ ctx.request_stop();
+}
+
+auto shouldEmitSignal(sdbusplus::async::context& ctx)
+ -> sdbusplus::async::task<>
+{
+ co_await sdbusplus::async::sleep_for(ctx, std::chrono::seconds(1));
+
+ auto x =
+ std::variant<A::EnumOne, std::string, A::EnumTwo>(A::EnumOne::OneA);
+
+ sdbusplus::common::server::Test::properties_t props{
+ 0, 0, 0, 0, 0, 0, 0, std::string("/my/path"), 1.0, 1.0, 1.0, 1.0, x};
+
+ auto t2 = std::make_unique<sdbusplus::aserver::server::Test<A>>(
+ ctx, "/test/something", props);
+
+ t2->emit_added();
+
+ co_await sdbusplus::async::sleep_for(ctx, std::chrono::seconds(1));
+
+ co_return;
+}
+
+int main()
+{
+ sdbusplus::async::context ctx;
+
+ std::srand(std::chrono::system_clock::now().time_since_epoch().count());
+ std::string busName =
+ "xyz.openbmc_project.TestingInterfacesAdded" + std::to_string(rand());
+
+ ctx.request_name(busName.c_str());
+
+ sdbusplus::server::manager_t manager(ctx.get_bus(), "/test");
+
+ sdbusplus::async::match ifcAdded(
+ ctx, sdbusplus::bus::match::rules::interfacesAdded() +
+ sdbusplus::bus::match::rules::sender(busName));
+
+ ctx.spawn(waitForMatch(ctx, ifcAdded));
+ ctx.spawn(shouldEmitSignal(ctx));
+
+ ctx.run();
+
+ return 0;
+}
diff --git a/test/gen/test_aserver_multiple_interfaces.cpp b/test/gen/test_aserver_multiple_interfaces.cpp
new file mode 100644
index 0000000..03abd69
--- /dev/null
+++ b/test/gen/test_aserver_multiple_interfaces.cpp
@@ -0,0 +1,101 @@
+#include "server/Test/aserver.hpp"
+#include "server/Test/client.hpp"
+#include "server/Test2/aserver.hpp"
+#include "server/Test2/client.hpp"
+#include "server/Test3/aserver.hpp"
+
+#include <sdbusplus/async/context.hpp>
+#include <sdbusplus/async/match.hpp>
+#include <sdbusplus/async/timer.hpp>
+
+class A
+{};
+
+auto constructInterfaces(sdbusplus::async::context& ctx, std::string& busName)
+ -> sdbusplus::async::task<>
+{
+ auto x = std::variant<sdbusplus::common::server::Test::EnumOne, std::string,
+ sdbusplus::common::server::Test::EnumTwo>(
+ sdbusplus::common::server::Test::EnumOne::OneA);
+
+ sdbusplus::common::server::Test::properties_t props{
+ 832, 0, 0, 0, 0, 0, 0, std::string("/my/path"), 1.0, 1.0, 1.0, 1.0, x};
+
+ sdbusplus::common::server::Test2::properties_t props2{4200, 2};
+
+ sdbusplus::aserver::server::Test<A> a0(ctx, "/0");
+
+ sdbusplus::aserver::server::Test<A> a1(ctx, "/1", props);
+
+ // construct without properties
+ sdbusplus::async::server_t<A, sdbusplus::aserver::server::Test> a2(
+ ctx, "/2");
+
+ // construct Test with properties
+ auto a3 = std::make_unique<
+ sdbusplus::async::server_t<A, sdbusplus::aserver::server::Test>>(
+ ctx, "/3", props);
+
+ // construct Test2 with properties
+ sdbusplus::async::server_t<A, sdbusplus::aserver::server::Test2> a4(
+ ctx, "/4", props2);
+
+ // does not compile, (wronge type of properties!)
+ // sdbusplus::async::server_t<A, sdbusplus::aserver::server::Test2> ax(ctx,
+ // "/x", props);
+
+ // construct with both interfaces, no properties
+ sdbusplus::async::server_t<A, sdbusplus::aserver::server::Test,
+ sdbusplus::aserver::server::Test2>
+ a5(ctx, "/5");
+
+ // construct with both interfaces and properties
+ auto a6 = std::make_unique<
+ sdbusplus::async::server_t<A, sdbusplus::aserver::server::Test,
+ sdbusplus::aserver::server::Test2>>(
+ ctx, "/6", props, props2);
+
+ // does not compile, (wrong order of properties!)
+ // auto a7 = std::make_unique<
+ // sdbusplus::async::server_t<A, sdbusplus::aserver::server::Test,
+ // sdbusplus::aserver::server::Test2>>(
+ // ctx, "/7", props2, props);
+
+ auto a8 = std::make_unique<sdbusplus::async::server_t<
+ A, sdbusplus::aserver::server::Test, sdbusplus::aserver::server::Test2,
+ sdbusplus::aserver::server::Test3>>(ctx, "/8", props, props2,
+ std::nullopt);
+
+ auto client =
+ sdbusplus::client::server::Test(ctx).service(busName).path("/3");
+
+ assert(co_await client.some_value() == 832);
+
+ auto client2 =
+ sdbusplus::client::server::Test2(ctx).service(busName).path("/6");
+
+ assert(co_await client2.new_value() == 4200);
+
+ ctx.request_stop();
+
+ co_return;
+}
+
+int main()
+{
+ sdbusplus::async::context ctx;
+
+ std::srand(std::chrono::system_clock::now().time_since_epoch().count());
+ std::string busName = "xyz.openbmc_project.TestingMultipleInterfaces" +
+ std::to_string(rand());
+
+ ctx.request_name(busName.c_str());
+
+ sdbusplus::server::manager_t manager(ctx.get_bus(), "/test");
+
+ ctx.spawn(constructInterfaces(ctx, busName));
+
+ ctx.run();
+
+ return 0;
+}
diff --git a/test/meson.build b/test/meson.build
index aa07577..4fe9b15 100644
--- a/test/meson.build
+++ b/test/meson.build
@@ -117,6 +117,8 @@
uninit_tests = [
'test_server_no_uninitialized_value_constructor',
'test_aserver_no_uninitialized_value_constructor',
+ 'test_aserver_emit_interfaces_added_signal',
+ 'test_aserver_multiple_interfaces',
]
foreach t : uninit_tests
diff --git a/test/yaml/server/Test2.interface.yaml b/test/yaml/server/Test2.interface.yaml
new file mode 100644
index 0000000..d9fe45e
--- /dev/null
+++ b/test/yaml/server/Test2.interface.yaml
@@ -0,0 +1,9 @@
+description: >
+ A test interface
+properties:
+ - name: NewValue
+ type: int64
+ - name: OtherValue
+ type: int64
+ flags:
+ - const
diff --git a/test/yaml/server/Test3.interface.yaml b/test/yaml/server/Test3.interface.yaml
new file mode 100644
index 0000000..8516d31
--- /dev/null
+++ b/test/yaml/server/Test3.interface.yaml
@@ -0,0 +1,2 @@
+description: >
+ A test interface with no properties.
diff --git a/tools/sdbusplus/templates/interface.aserver.hpp.mako b/tools/sdbusplus/templates/interface.aserver.hpp.mako
index 75df5ad..496bad8 100644
--- a/tools/sdbusplus/templates/interface.aserver.hpp.mako
+++ b/tools/sdbusplus/templates/interface.aserver.hpp.mako
@@ -52,6 +52,16 @@
_context(), path, interface, _vtable, this)
{}
+ ${interface.classname}(
+ const char* path,
+ [[maybe_unused]] ${interface.classname}::properties_t props)
+ : ${interface.classname}(path)
+ {
+ % for p in interface.properties:
+ ${p.snake_case}_ = props.${p.snake_case};
+ % endfor
+ }
+
% for s in interface.signals:
${s.render(loader, "signal.aserver.emit.hpp.mako", signal=s, interface=interface)}
% endfor
diff --git a/tools/sdbusplus/templates/interface.common.hpp.mako b/tools/sdbusplus/templates/interface.common.hpp.mako
index 0c14038..a97a3c9 100644
--- a/tools/sdbusplus/templates/interface.common.hpp.mako
+++ b/tools/sdbusplus/templates/interface.common.hpp.mako
@@ -41,7 +41,9 @@
using PropertiesVariant = sdbusplus::utility::dedup_variant_t<
${",\n ".join(sorted(setOfPropertyTypes()))}>;
- % endif \
+ % else:
+ using properties_t = std::nullopt_t;
+ % endif
% for p in interface.paths:
% if p.description: