[sdbusplus] To generate a common header for public information of interfaces

[sdbusplus] To generate a common header for public information of interfaces

[Lei YU]

Currently, sdbus++ generates the server code separately, and in
phosphor-dbus-interfaces, the separated headers are installed, and the
separated cpp files are combined together, and compiled as a library.
The public information of the interfaces (e.g, the interface strings,
the enum strings) are either in the separated header or in the cpp
files.

The result is, when a phosphor service needs to use an interface, or
an enum, it has to define the interface string or the enum string
manually, and it happens everywhere.

How about
1. Making sdbusplus to generate a "common" header for each interface
including the public information;
2. Then phosphor-dbus-interfaces could generate a single header file
that includes all the public information of the interfaces;
3. Then the phosphor service could include a single header file, and
use the interface/enum strings it needs, without manually defining
them?

There is an initial concept implementation:
* https://github.com/mine260309/sdbusplus/commit/78cb63fb7e1ceb62165c71e15779f23f7e9f166c
* https://github.com/mine260309/phosphor-dbus-interfaces/commit/6079d25547f0143fc7562a1c7ee6beb888a66432

With the above changes, a service could directly use the generated
interface string, e.g.
`sdbusplus::xyz::openbmc_project::Software::ApplyTime::interface`.
In the future, the enum strings could be put there and thus we do not
have to manually write the long full qualified string.

Ideas and suggestions are welcome.

Re: [sdbusplus] To generate a common header for public information of interfaces

[William Kennington]

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

We should have common headers for this because I'd like to see us have a
typesafe client interface that is autogenerated with the server interface.
I wrote the code a while back to implement a typesafe sdbusplus client
interface but never finished the generator code that automatically defines
the types and DBus strings.

On Thu, Feb 13, 2020 at 6:39 AM Lei YU <mine260309@gmail.com> wrote:

> Currently, sdbus++ generates the server code separately, and in
> phosphor-dbus-interfaces, the separated headers are installed, and the
> separated cpp files are combined together, and compiled as a library.
> The public information of the interfaces (e.g, the interface strings,
> the enum strings) are either in the separated header or in the cpp
> files.
>
> The result is, when a phosphor service needs to use an interface, or
> an enum, it has to define the interface string or the enum string
> manually, and it happens everywhere.
>
> How about
> 1. Making sdbusplus to generate a "common" header for each interface
> including the public information;
> 2. Then phosphor-dbus-interfaces could generate a single header file
> that includes all the public information of the interfaces;
> 3. Then the phosphor service could include a single header file, and
> use the interface/enum strings it needs, without manually defining
> them?
>
> There is an initial concept implementation:
> *
> https://github.com/mine260309/sdbusplus/commit/78cb63fb7e1ceb62165c71e15779f23f7e9f166c
> *
> https://github.com/mine260309/phosphor-dbus-interfaces/commit/6079d25547f0143fc7562a1c7ee6beb888a66432
>
> With the above changes, a service could directly use the generated
> interface string, e.g.
> `sdbusplus::xyz::openbmc_project::Software::ApplyTime::interface`.
> In the future, the enum strings could be put there and thus we do not
> have to manually write the long full qualified string.
>
> Ideas and suggestions are welcome.
>

[-- Attachment #2: Type: text/html, Size: 2593 bytes --]

Re: [sdbusplus] To generate a common header for public information of interfaces

[Patrick Williams]

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

On Thu, Feb 13, 2020 at 10:38:37PM +0800, Lei YU wrote:
> Currently, sdbus++ generates the server code separately, and in
> phosphor-dbus-interfaces, the separated headers are installed, and the
> separated cpp files are combined together, and compiled as a library.
> The public information of the interfaces (e.g, the interface strings,
> the enum strings) are either in the separated header or in the cpp
> files.
> 
> The result is, when a phosphor service needs to use an interface, or
> an enum, it has to define the interface string or the enum string
> manually, and it happens everywhere.

I would say any case where this was done should have been fixed.  There
were already functions in sdbusplus to convert<Enum>ToString and
convertForMessage(<Enum>).  There are lots of cases where these are
being used today[1].  You recently made the interface public as well, so
we should begin converting these over.

I've also got commits pending, for merge soon, that make the enums be
supported as native types, so code should rarely even need to call the
"convert" functions anymore.  Another refactoring once that is merged.

> 
> How about
> 1. Making sdbusplus to generate a "common" header for each interface
> including the public information;
> 2. Then phosphor-dbus-interfaces could generate a single header file
> that includes all the public information of the interfaces;
> 3. Then the phosphor service could include a single header file, and
> use the interface/enum strings it needs, without manually defining
> them?

Currently only the 'server' interfaces are generated and some clients
are including the server header files even though they are a client.
The intention all along was to make proper 'client' bindings but there
hasn't been sufficient effort put into that yet.

I'm not sure if it is better or worse to have an explicit 'common'
header rather than the two separate 'server' and 'client' headers.  Is
there any reason to not simply get started on the client headers?

Some pro/cons of 'common':
    - Have an extra pass we have to run through 'sdbus++'.  This will
      probably make the templates more complex (3x code paths rather
      than 2x).

    + Explicitly consistent types between client and server bindings.

> 
> There is an initial concept implementation:
> * https://github.com/mine260309/sdbusplus/commit/78cb63fb7e1ceb62165c71e15779f23f7e9f166c
> * https://github.com/mine260309/phosphor-dbus-interfaces/commit/6079d25547f0143fc7562a1c7ee6beb888a66432
> 
> With the above changes, a service could directly use the generated
> interface string, e.g.
> `sdbusplus::xyz::openbmc_project::Software::ApplyTime::interface`.
> In the future, the enum strings could be put there and thus we do not
> have to manually write the long full qualified string.
> 
> Ideas and suggestions are welcome.

[1] https://github.com/search?q=org%3Aopenbmc+convertForMessage&type=Code

-- 
Patrick Williams

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

Re: [sdbusplus] To generate a common header for public information of interfaces

[Lei YU]

On Wed, Feb 19, 2020 at 4:39 AM Patrick Williams <patrick@stwcx.xyz> wrote:
>
> On Thu, Feb 13, 2020 at 10:38:37PM +0800, Lei YU wrote:
> > Currently, sdbus++ generates the server code separately, and in
> > phosphor-dbus-interfaces, the separated headers are installed, and the
> > separated cpp files are combined together, and compiled as a library.
> > The public information of the interfaces (e.g, the interface strings,
> > the enum strings) are either in the separated header or in the cpp
> > files.
> >
> > The result is, when a phosphor service needs to use an interface, or
> > an enum, it has to define the interface string or the enum string
> > manually, and it happens everywhere.
>
> I would say any case where this was done should have been fixed.  There
> were already functions in sdbusplus to convert<Enum>ToString and
> convertForMessage(<Enum>).  There are lots of cases where these are
> being used today[1].  You recently made the interface public as well, so
> we should begin converting these over.
>
> I've also got commits pending, for merge soon, that make the enums be
> supported as native types, so code should rarely even need to call the
> "convert" functions anymore.  Another refactoring once that is merged.

The idea of my proposal here is not to use "convertXXX" functions,
instead, it is to provide the constexpr strings that could be used by
client code.
E.g. the client code does not need to call any function, but directly use:

* xyz::...::server::MyServer::interface, instead of a user-defined string
  for the interface.
* xyz::...::MyServer::MyEnum::Enum1, instead of a user-defined string for
  the enum.

>
> >
> > How about
> > 1. Making sdbusplus to generate a "common" header for each interface
> > including the public information;
> > 2. Then phosphor-dbus-interfaces could generate a single header file
> > that includes all the public information of the interfaces;
> > 3. Then the phosphor service could include a single header file, and
> > use the interface/enum strings it needs, without manually defining
> > them?
>
> Currently only the 'server' interfaces are generated and some clients
> are including the server header files even though they are a client.
> The intention all along was to make proper 'client' bindings but there
> hasn't been sufficient effort put into that yet.
>
> I'm not sure if it is better or worse to have an explicit 'common'
> header rather than the two separate 'server' and 'client' headers.  Is
> there any reason to not simply get started on the client headers?
>
> Some pro/cons of 'common':
>     - Have an extra pass we have to run through 'sdbus++'.  This will
>       probably make the templates more complex (3x code paths rather
>       than 2x).
>
>     + Explicitly consistent types between client and server bindings.

Yup, the implementation is actually part of the "client" header, we
could rename the "common" to "client".
But preferably, we could combine all the client header into a single
header, which makes it easier for the client code to use.
If a client needs to set an enum property to a service, it then does
not have to include the server header nor the specific client header,
but include a single header.
Anyway, this part is done in phosphor-dbus-interface.

So we could say that sdbusplus generates part of the "client" header
instead of "common" header.

>
> >
> > There is an initial concept implementation:
> > * https://github.com/mine260309/sdbusplus/commit/78cb63fb7e1ceb62165c71e15779f23f7e9f166c
> > * https://github.com/mine260309/phosphor-dbus-interfaces/commit/6079d25547f0143fc7562a1c7ee6beb888a66432
> >
> > With the above changes, a service could directly use the generated
> > interface string, e.g.
> > `sdbusplus::xyz::openbmc_project::Software::ApplyTime::interface`.
> > In the future, the enum strings could be put there and thus we do not
> > have to manually write the long full qualified string.
> >
> > Ideas and suggestions are welcome.
>
> [1] https://github.com/search?q=org%3Aopenbmc+convertForMessage&type=Code
>
> --
> Patrick Williams

Re: [sdbusplus] To generate a common header for public information of interfaces

[Patrick Williams]

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

On Wed, Feb 19, 2020 at 10:59:40AM +0800, Lei YU wrote:
> On Wed, Feb 19, 2020 at 4:39 AM Patrick Williams <patrick@stwcx.xyz> wrote:
> >
> > On Thu, Feb 13, 2020 at 10:38:37PM +0800, Lei YU wrote:
> > I would say any case where this was done should have been fixed.  There
> > were already functions in sdbusplus to convert<Enum>ToString and
> > convertForMessage(<Enum>).  There are lots of cases where these are
> > being used today[1].  You recently made the interface public as well, so
> > we should begin converting these over.
> >
> > I've also got commits pending, for merge soon, that make the enums be
> > supported as native types, so code should rarely even need to call the
> > "convert" functions anymore.  Another refactoring once that is merged.
> 
> The idea of my proposal here is not to use "convertXXX" functions,
> instead, it is to provide the constexpr strings that could be used by
> client code.
> E.g. the client code does not need to call any function, but directly use:
> 
> * xyz::...::server::MyServer::interface, instead of a user-defined string
>   for the interface.
> * xyz::...::MyServer::MyEnum::Enum1, instead of a user-defined string for
>   the enum.

I don't see any reason any code should ever directly utilize the enum
strings.  We generate code already to safely convert them to and from
real C++ enum types.  Why would you want to use string comparison
instead?

Can you elaborate on why we should be enabling this usage pattern?

> Yup, the implementation is actually part of the "client" header, we
> could rename the "common" to "client".
> But preferably, we could combine all the client header into a single
> header, which makes it easier for the client code to use.
> If a client needs to set an enum property to a service, it then does
> not have to include the server header nor the specific client header,
> but include a single header.
> Anyway, this part is done in phosphor-dbus-interface.
> 
> So we could say that sdbusplus generates part of the "client" header
> instead of "common" header.

Agree, but we don't even need this code now if people were just
including the server header files (and there are many examples of
clients doing exactly this).  If we're not going to create client
headers now, this seems like wasted effort just for some slight syntax
improvement.

We could simply symlink <xyz/openbmc_project/foo/server.hpp> to
<xyz/openbmc_project/foo/client.hpp> and add a `namespace
xyz::openbmc_project::foo::client = xyz::openbmc_project:server;` and
get the same syntactic enhancement without nearly as much work.

-- 
Patrick Williams

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

Re: [sdbusplus] To generate a common header for public information of interfaces

[Lei YU]

> > The idea of my proposal here is not to use "convertXXX" functions,
> > instead, it is to provide the constexpr strings that could be used by
> > client code.
> > E.g. the client code does not need to call any function, but directly use:
> >
> > * xyz::...::server::MyServer::interface, instead of a user-defined string
> >   for the interface.
> > * xyz::...::MyServer::MyEnum::Enum1, instead of a user-defined string for
> >   the enum.
>
> I don't see any reason any code should ever directly utilize the enum
> strings.  We generate code already to safely convert them to and from
> real C++ enum types.  Why would you want to use string comparison
> instead?
>
> Can you elaborate on why we should be enabling this usage pattern?

There are cases for a service to use a "enum string" directly instead
of getting the string from enum by `convertForMessage()`
E.g.
* https://github.com/openbmc/phosphor-bmc-code-mgmt/blob/master/activation.hpp#L43
* https://github.com/openbmc/phosphor-host-ipmid/blob/master/chassishandler.cpp#L218

Some of the cases could be changed to use `convertForMessage()`, but
if one does want to directly use that, or use it as constexpr, why not
provide it directly?

>
> > Yup, the implementation is actually part of the "client" header, we
> > could rename the "common" to "client".
> > But preferably, we could combine all the client header into a single
> > header, which makes it easier for the client code to use.
> > If a client needs to set an enum property to a service, it then does
> > not have to include the server header nor the specific client header,
> > but include a single header.
> > Anyway, this part is done in phosphor-dbus-interface.
> >
> > So we could say that sdbusplus generates part of the "client" header
> > instead of "common" header.
>
> Agree, but we don't even need this code now if people were just
> including the server header files (and there are many examples of
> clients doing exactly this).  If we're not going to create client
> headers now, this seems like wasted effort just for some slight syntax
> improvement.
>
> We could simply symlink <xyz/openbmc_project/foo/server.hpp> to
> <xyz/openbmc_project/foo/client.hpp> and add a `namespace
> xyz::openbmc_project::foo::client = xyz::openbmc_project:server;` and
> get the same syntactic enhancement without nearly as much work.
>

I do not think it's good behavior for a client to include another
server's heaader files.
E.g. if I am writing client code and want to set a property on another
service (e.g. foo.bar), I do not really want to include a
"foo/bar/server.hpp", because it's not part of my client.
What I want is some constexpr strings, like interface name, property
name, enum string, etc.

Ideally, sdbusplus would generate the full client binding code. But we
do not have that now.
So I am proposing to move a bit forward to generate a little client
code that would benefit the client's usage. The client won't have to
write the full string manually anymore.


> --
> Patrick Williams

Re: [sdbusplus] To generate a common header for public information of interfaces

[Patrick Williams]

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

On Mon, Feb 24, 2020 at 11:25:46AM +0800, Lei YU wrote:
> > > The idea of my proposal here is not to use "convertXXX" functions,
> > > instead, it is to provide the constexpr strings that could be used by
> > > client code.
> > > E.g. the client code does not need to call any function, but directly use:
> > >
> > > * xyz::...::server::MyServer::interface, instead of a user-defined string
> > >   for the interface.
> > > * xyz::...::MyServer::MyEnum::Enum1, instead of a user-defined string for
> > >   the enum.
> >
> > I don't see any reason any code should ever directly utilize the enum
> > strings.  We generate code already to safely convert them to and from
> > real C++ enum types.  Why would you want to use string comparison
> > instead?
> >
> > Can you elaborate on why we should be enabling this usage pattern?
> 
> There are cases for a service to use a "enum string" directly instead
> of getting the string from enum by `convertForMessage()`
> E.g.
> * https://github.com/openbmc/phosphor-bmc-code-mgmt/blob/master/activation.hpp#L43
> * https://github.com/openbmc/phosphor-host-ipmid/blob/master/chassishandler.cpp#L218

Examples of poor code are not a use case.  Both of these examples are
trivially converted to convert<enum>ToString APIs.  We should do that
rather than facilitate unmaintainable code.

> 
> Some of the cases could be changed to use `convertForMessage()`, but
> if one does want to directly use that, or use it as constexpr, why not
> provide it directly?

Because it limits our future ability to change the string format.  When
people decided to reverse engineer what sdbusplus was doing and hard
code strings, they made it so we cannot change the format of what
sdbusplus is sending.  

There was a commit recently, as an example, that requested we add an
Intel-only "Management Engine" enumeration to an xyz.openbmc_project interface
rather than a com.intel one.  If we supported enumeration inheritance,
there would have been less need for this.  I don't know how we might
implement enumeration inheritance, but we might need to change the
"on-the-wire" string as a result.  Right now we're hamstrung by people
having hard coded strings.

> I do not think it's good behavior for a client to include another
> server's heaader files.
> E.g. if I am writing client code and want to set a property on another
> service (e.g. foo.bar), I do not really want to include a
> "foo/bar/server.hpp", because it's not part of my client.
> What I want is some constexpr strings, like interface name, property
> name, enum string, etc.

With the current state of things, programming choices are:
    1. Include a header poorly named "server" into a "client" code.
    2. Reimplement what is in that "server" header file.

Under no circumstances is a good approach or one that should be
encouraged.

> Ideally, sdbusplus would generate the full client binding code. But we
> do not have that now.
> So I am proposing to move a bit forward to generate a little client
> code that would benefit the client's usage. The client won't have to
> write the full string manually anymore.

So maybe we call it "client" rather than "common"?  Common implies that
there are header files that are okay to import under any case and I
don't see that to be the case with what you're proposing we define.

Again, I don't have any issue with the interface names.  I do take issue
with the enumeration strings because they shouldn't ever be used outside
of sdbusplus (or a similar dbus binding).

-- 
Patrick Williams

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

[sdbusplus] To generate client header (was: Re: To generate a common header for public information of interfaces)

[Lei YU]

> > > I don't see any reason any code should ever directly utilize the enum
> > > strings.  We generate code already to safely convert them to and from
> > > real C++ enum types.  Why would you want to use string comparison
> > > instead?
> > >
> > > Can you elaborate on why we should be enabling this usage pattern?
> >
> > There are cases for a service to use a "enum string" directly instead
> > of getting the string from enum by `convertForMessage()`
> > E.g.
> > * https://github.com/openbmc/phosphor-bmc-code-mgmt/blob/master/activation.hpp#L43
> > * https://github.com/openbmc/phosphor-host-ipmid/blob/master/chassishandler.cpp#L218
>
> Examples of poor code are not a use case.  Both of these examples are
> trivially converted to convert<enum>ToString APIs.  We should do that
> rather than facilitate unmaintainable code.

My concern to use "convertXXX" APIs is that it requires the client
code to link the server code, where logically all it needs is the enum
string.

>
> >
> > Some of the cases could be changed to use `convertForMessage()`, but
> > if one does want to directly use that, or use it as constexpr, why not
> > provide it directly?
>
> Because it limits our future ability to change the string format.  When
> people decided to reverse engineer what sdbusplus was doing and hard
> code strings, they made it so we cannot change the format of what
> sdbusplus is sending.
>
> There was a commit recently, as an example, that requested we add an
> Intel-only "Management Engine" enumeration to an xyz.openbmc_project interface
> rather than a com.intel one.  If we supported enumeration inheritance,
> there would have been less need for this.  I don't know how we might
> implement enumeration inheritance, but we might need to change the
> "on-the-wire" string as a result.  Right now we're hamstrung by people
> having hard coded strings.

This is exactly why I propose to provide enum strings in by the client header.
Currently, client code (poorly) uses "hard-coded" strings directly. If
we provide the constexpr strings in the client header, the client code
could be "refacted" to use the definitions from the client header.
Then sdbusplus is freely to update the string format without breaking
client code.

>
> > I do not think it's good behavior for a client to include another
> > server's heaader files.
> > E.g. if I am writing client code and want to set a property on another
> > service (e.g. foo.bar), I do not really want to include a
> > "foo/bar/server.hpp", because it's not part of my client.
> > What I want is some constexpr strings, like interface name, property
> > name, enum string, etc.
>
> With the current state of things, programming choices are:
>     1. Include a header poorly named "server" into a "client" code.
>     2. Reimplement what is in that "server" header file.
>
> Under no circumstances is a good approach or one that should be
> encouraged.
>
> > Ideally, sdbusplus would generate the full client binding code. But we
> > do not have that now.
> > So I am proposing to move a bit forward to generate a little client
> > code that would benefit the client's usage. The client won't have to
> > write the full string manually anymore.
>
> So maybe we call it "client" rather than "common"?  Common implies that
> there are header files that are okay to import under any case and I
> don't see that to be the case with what you're proposing we define.
>

Yeah, agreed.

> Again, I don't have any issue with the interface names.  I do take issue
> with the enumeration strings because they shouldn't ever be used outside
> of sdbusplus (or a similar dbus binding).
>

For the interface names, the patch is updated at:
https://gerrit.openbmc-project.xyz/c/openbmc/sdbusplus/+/29404
Let's treat it as a start point of client code.

> --
> Patrick Williams

Re: [sdbusplus] To generate client header (was: Re: To generate a common header for public information of interfaces)

[Patrick Williams]

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

On Tue, Feb 25, 2020 at 04:22:40PM +0800, Lei YU wrote:
> > Examples of poor code are not a use case.  Both of these examples are
> > trivially converted to convert<enum>ToString APIs.  We should do that
> > rather than facilitate unmaintainable code.
> 
> My concern to use "convertXXX" APIs is that it requires the client
> code to link the server code, where logically all it needs is the enum
> string.

I'm not sure if you're referring to an API level issue or ABI level
issue.  Almost anything at an API level can be dealt with by headers (as
you're proposing to generate) and light-weight indirects.  Being worried
about linking at an ABI level seems like a premature optimization (we
only make libphosphor-dbus.so today anyhow).

> This is exactly why I propose to provide enum strings in by the client header.
> Currently, client code (poorly) uses "hard-coded" strings directly. If
> we provide the constexpr strings in the client header, the client code
> could be "refacted" to use the definitions from the client header.
> Then sdbusplus is freely to update the string format without breaking
> client code.

Why not just fix them to use enums properly?  There is zero reason for
applications to be dealing in string manipulation for these.

> > So maybe we call it "client" rather than "common"?  Common implies that
> > there are header files that are okay to import under any case and I
> > don't see that to be the case with what you're proposing we define.
> >
> 
> Yeah, agreed.

:thumbsup:

> > Again, I don't have any issue with the interface names.  I do take issue
> > with the enumeration strings because they shouldn't ever be used outside
> > of sdbusplus (or a similar dbus binding).
> >
> 
> For the interface names, the patch is updated at:
> https://gerrit.openbmc-project.xyz/c/openbmc/sdbusplus/+/29404
> Let's treat it as a start point of client code.

I'll take a look soon.

-- 
Patrick Williams

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

Re: [sdbusplus] To generate client header (was: Re: To generate a common header for public information of interfaces)

[Lei YU]

On Tue, Feb 25, 2020 at 11:59 PM Patrick Williams <patrick@stwcx.xyz> wrote:
>
> On Tue, Feb 25, 2020 at 04:22:40PM +0800, Lei YU wrote:
> > > Examples of poor code are not a use case.  Both of these examples are
> > > trivially converted to convert<enum>ToString APIs.  We should do that
> > > rather than facilitate unmaintainable code.
> >
> > My concern to use "convertXXX" APIs is that it requires the client
> > code to link the server code, where logically all it needs is the enum
> > string.
>
> I'm not sure if you're referring to an API level issue or ABI level
> issue.  Almost anything at an API level can be dealt with by headers (as
> you're proposing to generate) and light-weight indirects.  Being worried
> about linking at an ABI level seems like a premature optimization (we
> only make libphosphor-dbus.so today anyhow).

I am referring to the code structure. Clean code should include the
headers it really needs, and should not include unnecessary headers.
If clientA's code needs to talk to serverB, it ideally includes
clientB.hpp instead of serverB.hpp.
So the proposal here is to make sdbusplus generate the client header
to be used by client code.

>
> > This is exactly why I propose to provide enum strings in by the client header.
> > Currently, client code (poorly) uses "hard-coded" strings directly. If
> > we provide the constexpr strings in the client header, the client code
> > could be "refacted" to use the definitions from the client header.
> > Then sdbusplus is freely to update the string format without breaking
> > client code.
>
> Why not just fix them to use enums properly?  There is zero reason for
> applications to be dealing in string manipulation for these.
>

It still does not resolve the case when user *does* want to use a enum
string as constexpr.
If sdbusplus could provide constexpr function to convert enum to
string, it will be fine.
Do you think if that's possible?

> > > So maybe we call it "client" rather than "common"?  Common implies that
> > > there are header files that are okay to import under any case and I
> > > don't see that to be the case with what you're proposing we define.
> > >
> >
> > Yeah, agreed.
>
> :thumbsup:
>
> > > Again, I don't have any issue with the interface names.  I do take issue
> > > with the enumeration strings because they shouldn't ever be used outside
> > > of sdbusplus (or a similar dbus binding).
> > >
> >
> > For the interface names, the patch is updated at:
> > https://gerrit.openbmc-project.xyz/c/openbmc/sdbusplus/+/29404
> > Let's treat it as a start point of client code.
>
> I'll take a look soon.
>
> --
> Patrick Williams

Re: [sdbusplus] To generate client header (was: Re: To generate a common header for public information of interfaces)

[Patrick Williams]

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

On Wed, Feb 26, 2020 at 10:46:46AM +0800, Lei YU wrote:
> On Tue, Feb 25, 2020 at 11:59 PM Patrick Williams <patrick@stwcx.xyz> wrote:
> >
> > On Tue, Feb 25, 2020 at 04:22:40PM +0800, Lei YU wrote:
> > > > Examples of poor code are not a use case.  Both of these examples are
> > > > trivially converted to convert<enum>ToString APIs.  We should do that
> > > > rather than facilitate unmaintainable code.
> > >
> > > My concern to use "convertXXX" APIs is that it requires the client
> > > code to link the server code, where logically all it needs is the enum
> > > string.
> >
> > I'm not sure if you're referring to an API level issue or ABI level
> > issue.  Almost anything at an API level can be dealt with by headers (as
> > you're proposing to generate) and light-weight indirects.  Being worried
> > about linking at an ABI level seems like a premature optimization (we
> > only make libphosphor-dbus.so today anyhow).
> 
> I am referring to the code structure. Clean code should include the
> headers it really needs, and should not include unnecessary headers.
> If clientA's code needs to talk to serverB, it ideally includes
> clientB.hpp instead of serverB.hpp.
> So the proposal here is to make sdbusplus generate the client header
> to be used by client code.

You said that "using the convertXXX APIs ... requires the client code to
link to the server code."  It doesn't.  Just like you can generate
strings in a client header, you can generate those same APIs in the
client header.  That's an implementation choice.

I'll say it again: I don't have any problem with generating client code.
I don't think that's where the high-value problem exists right now
(w.r.t. enums specifically) and I definitely don't think we should
continue to perpetuate bad practices.  That is my own issue.

> > > This is exactly why I propose to provide enum strings in by the client header.
> > > Currently, client code (poorly) uses "hard-coded" strings directly. If
> > > we provide the constexpr strings in the client header, the client code
> > > could be "refacted" to use the definitions from the client header.
> > > Then sdbusplus is freely to update the string format without breaking
> > > client code.
> >
> > Why not just fix them to use enums properly?  There is zero reason for
> > applications to be dealing in string manipulation for these.
> >
> 
> It still does not resolve the case when user *does* want to use a enum
> string as constexpr.
> If sdbusplus could provide constexpr function to convert enum to
> string, it will be fine.
> Do you think if that's possible?

I'm totally willing to be shown some neat code that uses them as an
argument for why it is an "ok" practice.  As of now, I haven't been
shown any, I haven't seen any, and I can't think of any.

When I originally implemented the sdbusplus enums, I chose sending them
as a string because it was best for humans if you were using something
like dbus-monitor.  I could have just as easily sent the raw enum value
or 'hash(string)'.  I get that because it is strings it feels more natural
to see it in code, but if I had chosen to implement them as
'hash(string)' I don't think you'd be arguing with me today that people
littering their code with magic hash values was a good idea.  Replace
"hash value" with "string that looks useful to humans" and hopefully
you'll see why I don't like their usage.

C has enumerations for a reason.  C++ makes them an even stronger part
of the type system.  The reason sdbusplus enums exist is because they
are useful in C/C++ and we wanted a safe way to transfer them between
processes.  Subverting that, in C/C++, with strings just isn't
beneficial.

-- 
Patrick Williams

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