* Re: sdbusplus reading InterfacesAdded issue: not all variants are created equal
2021-12-23 15:45 sdbusplus reading InterfacesAdded issue: not all variants are created equal Paul Fertser
@ 2021-12-23 16:24 ` Patrick Williams
2021-12-24 12:53 ` Paul Fertser
2021-12-24 19:21 ` Paul Fertser
2021-12-23 18:14 ` Ed Tanous
1 sibling, 2 replies; 6+ messages in thread
From: Patrick Williams @ 2021-12-23 16:24 UTC (permalink / raw)
To: Paul Fertser; +Cc: openbmc
[-- Attachment #1: Type: text/plain, Size: 3546 bytes --]
On Thu, Dec 23, 2021 at 06:45:26PM +0300, Paul Fertser wrote:
> Hello,
>
> While digging into current state of SNMP notifications (traps) support
> in OpenBMC I found some code I have no idea how to properly improve.
>
> phosphor-dbus-monitor has a handler that's meant to be called whenever
> a new log entry is created by monitoring InterfacesAdded signals on
> D-Bus logger paths. The processing is at
>
> https://github.com/openbmc/phosphor-dbus-monitor/blob/master/src/snmp_trap.cpp#L28
>
> It seems OK until you look into PathInterfacesAdded definition which
> includes a hard-coded std::variant<>:
> https://github.com/openbmc/phosphor-dbus-monitor/blob/master/src/data_types.hpp#L66
>
> This already raises suspicions and rightfully so: the interface we're
> specifically interested in, xyz.openbmc_project.Logging.Entry,
> includes AdditionalData property which should be of type
> std::vector<std::string> , but that's not in the list of the allowed
> hardcoded variants.
>
> If I'm trying to use the std::variant<> type suitable for
> Logging.Entry then msg.read() fails with InvalidEnum error, probably
> trying to parse data about other interfaces, and this is a bad idea
> anyway.
>
> So what is the correct method of using statically-typed sdbusplus APIs
> to parse such a "dynamic" reply?
The short answer is that there isn't currently. The code doesn't support either
something like 'std::any' or something like 'std::placeholders' to skip past
parsing of an entry.
Generally, you know the type of the signal you're trying to listen to, so this
wasn't very important. There are a few cases where the code interacting with
dbus is "generic" and it becomes important. I think in this case you're
expecting to be able to match against configuration in JSON? So we probably
need to do something to make the situation better.
In phosphor-inventory-manager they've had a similar problem and what we've ended
up having to do is extend the variant list for each new element type they want
to accept. I think they wrote a python script that generates it from the
phosphor-dbus-interfaces YAML information at build time.
There is something a little better than that available though. If you look at
the generated headers for phosphor-dbus-interface, you'll see that each class
has a PropertiesVariant. Here is the one for Logging.Entry:
using PropertiesVariant = sdbusplus::utility::dedup_variant_t<
Level,
bool,
std::string,
std::vector<std::string>,
uint32_t,
uint64_t>;
`dedup_variant_t` is a class that evaluates to a std::variant, but makes sure
that each type is only listed once. You could pretty easily add a
`merge_variant` on top of this that would be the union of all the variant types.
Then, the type you'd pass into `msg.read` would be:
using Value =
merge_variant<xyz::openbmc_project::Logging::Entry::PropertiesVariant,
...>;
Where we'd list all the interfaces we expect phosphor-dbus-monitor to be able to
handle.
You mentioned the InvalidEnum error. I'd have to see what you were doing there
but if you both an Enum-type and a std::string in the variant it should have
parsed as a std::string if the Enum-matching was unsuccessful and not thrown an
error. I thought I had a test case for that exact scenario as well. If you are
seeing that, it sounds like a bug.
--
Patrick Williams
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: sdbusplus reading InterfacesAdded issue: not all variants are created equal
2021-12-23 15:45 sdbusplus reading InterfacesAdded issue: not all variants are created equal Paul Fertser
2021-12-23 16:24 ` Patrick Williams
@ 2021-12-23 18:14 ` Ed Tanous
2021-12-24 12:48 ` Paul Fertser
1 sibling, 1 reply; 6+ messages in thread
From: Ed Tanous @ 2021-12-23 18:14 UTC (permalink / raw)
To: Paul Fertser; +Cc: openbmc
On Thu, Dec 23, 2021 at 7:46 AM Paul Fertser <fercerpav@gmail.com> wrote:
>
> Hello,
>
> While digging into current state of SNMP notifications (traps) support
> in OpenBMC I found some code I have no idea how to properly improve.
>
> phosphor-dbus-monitor has a handler that's meant to be called whenever
> a new log entry is created by monitoring InterfacesAdded signals on
> D-Bus logger paths. The processing is at
>
> https://github.com/openbmc/phosphor-dbus-monitor/blob/master/src/snmp_trap.cpp#L28
>
> It seems OK until you look into PathInterfacesAdded definition which
> includes a hard-coded std::variant<>:
> https://github.com/openbmc/phosphor-dbus-monitor/blob/master/src/data_types.hpp#L66
>
> This already raises suspicions and rightfully so: the interface we're
> specifically interested in, xyz.openbmc_project.Logging.Entry,
> includes AdditionalData property which should be of type
> std::vector<std::string> , but that's not in the list of the allowed
> hardcoded variants.
>
> If I'm trying to use the std::variant<> type suitable for
> Logging.Entry then msg.read() fails with InvalidEnum error, probably
> trying to parse data about other interfaces, and this is a bad idea
> anyway.
>
> So what is the correct method of using statically-typed sdbusplus APIs
> to parse such a "dynamic" reply?
I haven't looked at the code in question, but we hit the same thing in
bmcweb, and solved it by just doing the unpack manually from the
message, which lets you directly unpack into whatever structure you
want.
Take a look at the convertDbusToJSON function here.
https://github.com/openbmc/bmcweb/blob/d1a648140e3cadd0ea6b0014c4d61ec880742000/include/openbmc_dbus_rest.hpp#L1132
And the code for the InterfacesAdded call specifically:
https://github.com/openbmc/bmcweb/blob/9062d478d4dc89598e215e1538ba8fbb8db2cf10/include/dbus_monitor.hpp#L70
It's not ideal from a coding abstraction perspective, and in some
places we're calling directly into the sd-bus apis to open and close
the containers, but it does work for types that we've never seen
before, as we've defined the unpack behavior for all generic dbus
types (array, variant, struct, ect), not c++ types.
>
> --
> Be free, use free (http://www.gnu.org/philosophy/free-sw.html) software!
> mailto:fercerpav@gmail.com
^ permalink raw reply [flat|nested] 6+ messages in thread