sdbusplus reading InterfacesAdded issue: not all variants are created equal

[Ed Tanous]

On Thu, Dec 23, 2021 at 7:46 AM Paul Fertser <fercerpav at 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 at gmail.com