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

[Patrick Williams]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20200224/9a85ff15/attachment.sig>