Re: sdbusplus reading InterfacesAdded issue: not all variants are created equal

[Patrick Williams <patrick@stwcx.xyz>]

[-- 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 --]