Towards less magic strings.

[Bills, Jason M]

On 9/14/2023 12:36 AM, Patrick Williams wrote:
> 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]).

Thanks for this guide and improvements, Patrick!

I have seen a couple advantages to the magic strings that seem to make 
debugging easier between using something like busctl on a system and 
looking at the source code.

One is going from busctl to source, it's easy to copy the string from 
busctl and search the source for that string.

The other is going from source to busctl, it's easy to copy the strings 
from the source into busctl to see the results from D-Bus.

Do you have any tips on how to do these kinds of busctl<->source 
conversions using dbus-YAML that will help as we start removing the 
magic strings?

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