Using a struct[enum] as an sdbusplus D-Bus method arg?

Using a struct[enum] as an sdbusplus D-Bus method arg?

[Matt Spinler]

Hi,

I'm designing a phosphor-dbus-interfaces D-Bus method where I think the 
ideal interface has YAML like:

- name : CreateStuff
   parameters:
     - name: Data
       type: array[struct[enum[self.Types], uint32]]

But that doesn't compile using an enum in the struct, and fails because 
sdbus++ creates code like:

server.cpp:

     // Uses enum<self.Types>
     std::vector<std::tuple<enum<self.Types>, uint8_t>> data{};
     m.read(data);
     auto o = static_cast<Create*>(context);
     o->createStuff(data);



What I'm trying to do is a valid D-Bus definition, correct? Or should I 
just use a string instead
of an enum?

It doesn't seem very straightforward to fix, as the vector that gets 
passed to o->createStuff()
is a std::vector<std::tuple<Types, uint32_t>>, but the vector passed to 
m.read() is a
std::vector<std::tuple<std::string, uint32_t>>.

Any thoughts?

Thanks!

Re: Using a struct[enum] as an sdbusplus D-Bus method arg?

[Patrick Williams]

[-- Attachment #1: Type: text/plain, Size: 2119 bytes --]

On Wed, Jan 08, 2020 at 01:32:49PM -0600, Matt Spinler wrote:

Hi Matt,

> I'm designing a phosphor-dbus-interfaces D-Bus method where I think the
> ideal interface has YAML like:
> 
> - name : CreateStuff
>   parameters:
>     - name: Data
>       type: array[struct[enum[self.Types], uint32]]
> 
> But that doesn't compile using an enum in the struct, and fails because
> sdbus++ creates code like:
> 
> server.cpp:
> 
>     // Uses enum<self.Types>
>     std::vector<std::tuple<enum<self.Types>, uint8_t>> data{};
>     m.read(data);
>     auto o = static_cast<Create*>(context);
>     o->createStuff(data);
> 
> What I'm trying to do is a valid D-Bus definition, correct? Or should I just
> use a string instead
> of an enum?

Enums are not actually a DBus concept, but something we added to
sdbusplus specifically.  What happens is that at the DBus level we send
them between processes as fully-qualified strings[1].  In the sdbusplus
generated code we know that this was defined in the YAML as an enum and
so we look the string up in a map<string, enum> to get you the enum
value (and throw an exception if an invalid enum was given).

I wouldn't recommend using a string instead because you are just
bypassing this enum-string checking code.

> It doesn't seem very straightforward to fix, as the vector that gets passed
> to o->createStuff()
> is a std::vector<std::tuple<Types, uint32_t>>, but the vector passed to
> m.read() is a
> std::vector<std::tuple<std::string, uint32_t>>.

This is a bug / missing feature in the header generator.  I'd have to
look into the code but it seems like it is not re-cursing inside the
tuple correctly.  We handle this case fine for raw enums.  The
definition of data's type looks like a start of the problem
(enum<self.Types> doesn't seem correct to me).  It's possible though
that the m.read generated function is wrong in that it is missing the
conversion from self.Types <-> std::string.

[1] A fully-qualified string is something like
    xyz.openbmc-project.InterfaceClass.EnumType.EnumValue.
-- 
Patrick Williams

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Using a struct[enum] as an sdbusplus D-Bus method arg?

[Matt Spinler]

On 1/9/2020 10:22 AM, Patrick Williams wrote:
> On Wed, Jan 08, 2020 at 01:32:49PM -0600, Matt Spinler wrote:
>
> Hi Matt,
>
>> I'm designing a phosphor-dbus-interfaces D-Bus method where I think the
>> ideal interface has YAML like:
>>
>> - name : CreateStuff
>>    parameters:
>>      - name: Data
>>        type: array[struct[enum[self.Types], uint32]]
>>
>> But that doesn't compile using an enum in the struct, and fails because
>> sdbus++ creates code like:
>>
>> server.cpp:
>>
>>      // Uses enum<self.Types>
>>      std::vector<std::tuple<enum<self.Types>, uint8_t>> data{};
>>      m.read(data);
>>      auto o = static_cast<Create*>(context);
>>      o->createStuff(data);
>>
>> What I'm trying to do is a valid D-Bus definition, correct? Or should I just
>> use a string instead
>> of an enum?
> Enums are not actually a DBus concept, but something we added to
> sdbusplus specifically.  What happens is that at the DBus level we send
> them between processes as fully-qualified strings[1].  In the sdbusplus
> generated code we know that this was defined in the YAML as an enum and
> so we look the string up in a map<string, enum> to get you the enum
> value (and throw an exception if an invalid enum was given).
>
> I wouldn't recommend using a string instead because you are just
> bypassing this enum-string checking code.
>
>> It doesn't seem very straightforward to fix, as the vector that gets passed
>> to o->createStuff()
>> is a std::vector<std::tuple<Types, uint32_t>>, but the vector passed to
>> m.read() is a
>> std::vector<std::tuple<std::string, uint32_t>>.
> This is a bug / missing feature in the header generator.  I'd have to
> look into the code but it seems like it is not re-cursing inside the
> tuple correctly.  We handle this case fine for raw enums.  The
> definition of data's type looks like a start of the problem
> (enum<self.Types> doesn't seem correct to me).  It's possible though
> that the m.read generated function is wrong in that it is missing the
> conversion from self.Types <-> std::string.

Since the vectors are of different types, I think the code generated 
code would have to
do the equivalent of?

     std::vector<std::tuple<std::string, uint8_t>> data{};
     m.read(data);

     std::vector<std::tuple<Types, uint8_t>> apiData{};

     for (const auto& [arg0, arg1]: data)
        apiData.emplace_back(convertStringToEnum(arg0), arg1))
   
     o->createStuff(apiData);
  
That's a pretty big change from what's there today.  Think that's possible
to do generically?


> [1] A fully-qualified string is something like
>      xyz.openbmc-project.InterfaceClass.EnumType.EnumValue.

Re: Using a struct[enum] as an sdbusplus D-Bus method arg?

[Patrick Williams]

[-- Attachment #1: Type: text/plain, Size: 1661 bytes --]

On Thu, Jan 09, 2020 at 11:25:24AM -0600, Matt Spinler wrote:
> 
> 
> On 1/9/2020 10:22 AM, Patrick Williams wrote:
> > On Wed, Jan 08, 2020 at 01:32:49PM -0600, Matt Spinler wrote:

> Since the vectors are of different types, I think the code generated code
> would have to
> do the equivalent of?
> 
>     std::vector<std::tuple<std::string, uint8_t>> data{};
>     m.read(data);
> 
>     std::vector<std::tuple<Types, uint8_t>> apiData{};
> 
>     for (const auto& [arg0, arg1]: data)
>        apiData.emplace_back(convertStringToEnum(arg0), arg1))
>     o->createStuff(apiData);
> That's a pretty big change from what's there today.  Think that's possible
> to do generically?
> 

It seems like that is something that should have been done in the
`message::read` function.  First, since your initial data type is wrong
(that funny enum<...>), I think we need to figure out why this function
isn't working correctly:

https://github.com/openbmc/sdbusplus/blob/master/tools/sdbusplus/property.py#L72

It was suppose to have handled, through recursion, enums.  I would have
expected data's type to be vector<tuple<Interface::Types, uint8_t>>.
After that, I suspect you'll get compile errors that m.read doesn't know
how to handle an Interface::Types enum type.  This would require some
C++ template magic, but I think it could be handled there.  We need to
teach m.read to be able to call convertToMessage and convertFromMessage
if those exist.  I don't know why we originally did that lifting in the
from the generated code, except maybe that enums were bolted on later
and never fully complete?

-- 
Patrick Williams

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Using a struct[enum] as an sdbusplus D-Bus method arg?

[Matt Spinler]

On 1/9/2020 1:03 PM, Patrick Williams wrote:
> On Thu, Jan 09, 2020 at 11:25:24AM -0600, Matt Spinler wrote:
>> On 1/9/2020 10:22 AM, Patrick Williams wrote:
>>> On Wed, Jan 08, 2020 at 01:32:49PM -0600, Matt Spinler wrote:
>> Since the vectors are of different types, I think the code generated code
>> would have to
>> do the equivalent of?
>>
>>      std::vector<std::tuple<std::string, uint8_t>> data{};
>>      m.read(data);
>>
>>      std::vector<std::tuple<Types, uint8_t>> apiData{};
>>
>>      for (const auto& [arg0, arg1]: data)
>>         apiData.emplace_back(convertStringToEnum(arg0), arg1))
>>      o->createStuff(apiData);
>> That's a pretty big change from what's there today.  Think that's possible
>> to do generically?
>>
> It seems like that is something that should have been done in the
> `message::read` function.  First, since your initial data type is wrong
> (that funny enum<...>), I think we need to figure out why this function
> isn't working correctly:
>
> https://github.com/openbmc/sdbusplus/blob/master/tools/sdbusplus/property.py#L72
>
> It was suppose to have handled, through recursion, enums.  I would have
> expected data's type to be vector<tuple<Interface::Types, uint8_t>>.
> After that, I suspect you'll get compile errors that m.read doesn't know
> how to handle an Interface::Types enum type.  This would require some
> C++ template magic, but I think it could be handled there.  We need to
> teach m.read to be able to call convertToMessage and convertFromMessage
> if those exist.  I don't know why we originally did that lifting in the
> from the generated code, except maybe that enums were bolted on later
> and never fully complete?
>

To go from enums to strings, there is a convertForMessage() function, 
but to go from
strings back to the enums, which is what we need, the function has the 
enum name in it:
      convert<enum name>FromString(const std::string& s);

I don't think it's possible for message::read() to know that?

Re: Using a struct[enum] as an sdbusplus D-Bus method arg?

[Patrick Williams]

[-- Attachment #1: Type: text/plain, Size: 1064 bytes --]

On Mon, Jan 13, 2020 at 12:59:26PM -0600, Matt Spinler wrote:
> To go from enums to strings, there is a convertForMessage() function, but to
> go from
> strings back to the enums, which is what we need, the function has the enum
> name in it:
>      convert<enum name>FromString(const std::string& s);
> 
> I don't think it's possible for message::read() to know that?

This needs a pretty simple refactoring to rename the
convert<enum>FromString function to a template function.

    // in a header somewhere to forward declare.
    template <typename T> T convertFromString() = delete;

    // in the generated code for each enum.
    template <> enum_type convertFromString<enum_type>() { ... }

The message::read will be able map message::read<enum_type> to
convertFromString<enum_type>.  There is probably some SFINAE needed
using is_enum[1] so that we don't need to generate each
message::<enum_type> specialization but can use a generic one for all
enums.

[1] https://en.cppreference.com/w/cpp/types/is_enum
-- 
Patrick Williams

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Using a struct[enum] as an sdbusplus D-Bus method arg?

[Patrick Williams]

On Tue, Jan 14, 2020 at 11:41:17AM -0600, Patrick Williams wrote:
> On Mon, Jan 13, 2020 at 12:59:26PM -0600, Matt Spinler wrote:
> This needs a pretty simple refactoring to rename the
> convert<enum>FromString function to a template function.
>
> -- 
> Patrick Williams

This probably wasn't as simple as I lead on with this previous email,
but patches are up for sdbusplus ending with this change:
    Ib142482de90572e1bda2f3658f6aeec201c043de

https://gerrit.openbmc-project.xyz/c/openbmc/sdbusplus/+/28859

With this commit sequence you should be able to read / write any
container with sdbusplus-enums in it with the message::read / append
functions, which as a side-effect means the generated classes no
longer need all the conversion hoops they use to jump through.

-- 
Patrick Williams

Re: Using a struct[enum] as an sdbusplus D-Bus method arg?

[Matt Spinler]

On 1/28/2020 9:07 PM, Patrick Williams wrote:
> On Tue, Jan 14, 2020 at 11:41:17AM -0600, Patrick Williams wrote:
>> On Mon, Jan 13, 2020 at 12:59:26PM -0600, Matt Spinler wrote:
>> This needs a pretty simple refactoring to rename the
>> convert<enum>FromString function to a template function.
>>
>> -- 
>> Patrick Williams
> This probably wasn't as simple as I lead on with this previous email,
> but patches are up for sdbusplus ending with this change:
>      Ib142482de90572e1bda2f3658f6aeec201c043de
>
> https://gerrit.openbmc-project.xyz/c/openbmc/sdbusplus/+/28859
>
> With this commit sequence you should be able to read / write any
> container with sdbusplus-enums in it with the message::read / append
> functions, which as a side-effect means the generated classes no
> longer need all the conversion hoops they use to jump through.
>

Thanks!
I tried to build a flash (wspoon) image, and in the server.hpp generated 
for my interface,
https://gerrit.openbmc-project.xyz/c/openbmc/phosphor-dbus-interfaces/+/26544, 
it had:

#include <std/vector<std/message/tuple<FFDCFormat, uint8_t, uint8_t, 
sdbusplus.hpp>

Re: Using a struct[enum] as an sdbusplus D-Bus method arg?

[Patrick Williams]

[-- Attachment #1: Type: text/plain, Size: 458 bytes --]

On Wed, Jan 29, 2020 at 09:08:28AM -0600, Matt Spinler wrote:
> Thanks!
> I tried to build a flash (wspoon) image, and in the server.hpp generated for
> my interface,
> https://gerrit.openbmc-project.xyz/c/openbmc/phosphor-dbus-interfaces/+/26544,
> it had:
> 
> #include <std/vector<std/message/tuple<FFDCFormat, uint8_t, uint8_t,
> sdbusplus.hpp>

Latest version of the changes should fix this #include issue now, Matt.

-- 
Patrick Williams

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]