Towards less magic strings.

[Patrick Williams]

Greetings,

One annoyance of mine has been the large proliferation of magic strings
related to dbus.  By a few simple calculations, it appears that we have
somewhere between 670 and 4200 instances of "xyz.openbmc_project" or
"/xyz/openbmc_project" sprinkled through the codebase:

```
$ find -type f -name "*.[ch]pp" -exec grep '"/\?xyz.openbmc_project' {} + | grep -v build | grep -v subprojects | grep -v .ccls-cache | wc
   4161   13869  465728
```

https://github.com/search?q=org%3Aopenbmc+%2F%22%28%5C%2F%29%3Fxyz.openbmc_project%2F+language%3AC%2B%2B&type=code

When I look at these, the vast majority of them falls into 1 of 4
different reasons:
    - A well-known service name.
    - A well-known path (or path prefix).
    - An interface name.
    - An enumeration value.

Today I merged support in the dbus YAML[1,2] to cover the first two
cases.  The other two cases have long been covered by the dbus YAML.
Thus, at this point, the vast majority of these magic strings can be
eliminated and derived from constants that show up in the
phosphor-dbus-interfaces headers.  I think it would be great if we
could see some refactoring towards eliminating these magics.

Below is a "howto" on each of these string types.

## Interface names

The interface xyz.openbmc_project.Foo.Bar has a number of header
files which are generated:
    - xyz/openbmc_project/Foo/Bar/common.hpp
    - xyz/openbmc_project/Foo/Bar/client.hpp
    - xyz/openbmc_project/Foo/Bar/server.hpp
    - xyz/openbmc_project/Foo/Bar/aserver.hpp

These headers will give you the constant:

    sdbusplus::common::xyz::openbmc_project::foo::Bar::interface

Any of the non-common headers also alias this into their own
namespace/types as appropriate.  Eg.

    sdbusplus::server::xyz::openbmc_project::foo::Bar::interface

## Enumeration values.

Any enumeration xyz.openbmc_project.Foo.Bar.Baz.{Value1, Value2} has
an `enum class Baz` defined in all the same header files.

    - xyz/openbmc_project/Foo/Bar/common.hpp

This enum class is intended to be used in your C++ code rather than
strings.  sdbusplus already has automatic conversion to/from the C++
enum-class and dbus-strings.  If for some reason (rare, but maybe for
serialization to a file) you want to explicitly do one of these
conversions, `sdbusplus::message::convert_to_string` and
`sdbusplus::message::convert_from_string` are what you should use.

## Service Names

I think it should be rare that you need a hard-coded service name in
your code, since typically you'll want to do some kind of mapper lookup
(based on the interface constants mentioned above).  There are a few
cases where we use explicit service names, such as mapper itself.
The [1] commit mentioned above documents a way to add these to the YAML.

Typically I expect to see a new YAML entry in Foo.Bar:

```yaml
service_names:
    default: xyz.openbmc_project.Foo.BarService
```

This will create `xyz::openbmc_project::foo::Bar::default_service`
constant.

If you have multiple services you want documented, the YAML and
generator supports that, but I'll be somewhat surprised to see any.

## Paths

Similar to the service names, we sometimes have static paths.  There are
3 classifications of these in my mind:

    - An explicit path to a singleton instance (typically called the
      "manager").
    - A path prefix to a collection of objects (I call this the
      "namespace").  You see this in logging, sensors, etc.
    - A path segment used to identify a purpose for a particular
      instance.

All 3 of these are now supported by the generator, with similar YAML:

```yaml
paths:
    - instance: /xyz/openbmc_project/Foo/Manager
    - namespace: /xyz/openbmc_project/Foo
      segments:
        - name: Bar
        - name: Baz
```

This will create the following constants:

- sdbusplus::common::xyz::openbmc_project::Foo::instance_path
- sdbusplus::common::xyz::openbmc_project::Foo::namespace_path::value
- sdbusplus::common::xyz::openbmc_project::Foo::namespace_path::bar
- sdbusplus::common::xyz::openbmc_project::Foo::namespace_path::baz

---

Hopefully these are useful and we can start to work towards eliminating
the vast majority of the magic-strings.  Please reach out if you'd like
more details or if something is not working correctly (I already fixed
one oversight while writing this email[3]).

[1]: https://gerrit.openbmc.org/c/openbmc/sdbusplus/+/66364
[2]: https://gerrit.openbmc.org/c/openbmc/sdbusplus/+/66429
[3]: https://gerrit.openbmc.org/c/openbmc/sdbusplus/+/66597

-- 
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/20230914/c5850209/attachment.sig>