sdbusplus reading InterfacesAdded issue: not all variants are created equal

sdbusplus reading InterfacesAdded issue: not all variants are created equal

[Paul Fertser]

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?

-- 
Be free, use free (http://www.gnu.org/philosophy/free-sw.html) software!
mailto:fercerpav@gmail.com

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

[Patrick Williams]

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

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

[Ed Tanous]

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

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

[Paul Fertser]

Hello Ed,

On Thu, Dec 23, 2021 at 10:14:02AM -0800, Ed Tanous wrote:
> > 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.

I see, you had to dynamically parse it by implementing
read*FromMessage ad-hoc functions as the nature of that case is
inherently dynamic.

Thank you for the example, I hope your mail will surface in the
Internet search results when the next dev starts scratching the
head. And then probably sdbusplus API will be extended to cover the
usecase if it's needed by more than bmcweb.

-- 
Be free, use free (http://www.gnu.org/philosophy/free-sw.html) software!
mailto:fercerpav@gmail.com

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

[Paul Fertser]

Hello Patrick,

Thank you for the prompt and elaborate response.

On Thu, Dec 23, 2021 at 10:24:30AM -0600, Patrick Williams wrote:
> > So what is the correct method of using statically-typed sdbusplus APIs
> > to parse such a "dynamic" reply?
> 
...
>  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,
>                       ...>;

Yep, I'll do that and submit a patch for phosphor-dbus-monitor after testing.

> 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.

It is a bug all right, in the development processes of our company
where we primarily work with an outdated fork and it doesn't include
your a22dbf40a115d5cd133e67500afa387b317cac14 commit :/

Thank you for the clear hints, I'm working on doing the right thing to
test this now.

-- 
Be free, use free (http://www.gnu.org/philosophy/free-sw.html) software!
mailto:fercerpav@gmail.com

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

[Paul Fertser]

On Thu, Dec 23, 2021 at 10:24:30AM -0600, Patrick Williams wrote:
> You could pretty easily add a `merge_variant` on top of this that
> would be the union of all the variant types.

This seems to work but I have near-zero clue about templates
meta-programming, please bear with me. Does this look suitable as an
another sdbusplus utility, should I send it for review after adding
docs?

template <typename T, typename... Unused>
struct merge_variant
{   
    using type = T;
};
template <typename D, typename... Done, typename... First, typename... Rest>
struct merge_variant<std::variant<D, Done...>, std::variant<First...>, Rest...>
    : public merge_variant<
          sdbusplus::utility::dedup_variant_t<D, Done..., First...>, Rest...>
{
};
template <typename... T>
using merge_variant_t = typename merge_variant<T...>::type;

-- 
Be free, use free (http://www.gnu.org/philosophy/free-sw.html) software!
mailto:fercerpav@gmail.com