openbmc/docs/anti-patterns.md

1 # OpenBMC Anti-patterns
3 From [Wikipedia](https://en.wikipedia.org/wiki/Anti-pattern):
5 "An anti-pattern is a common response to a recurring problem that is usually
12 This page aims to document some of the anti-patterns that exist in OpenBMC to
13 ease the job of those reviewing code. If an anti-pattern is spotted, rather that
17 <!-- begin copy/paste on next line -->
19 ## Anti-pattern template [one line description]
23 (1 paragraph) Describe how to spot the anti-pattern.
27 (1 paragraph) Describe the negative effects of the anti-pattern.
31 (1 paragraph) Describe why the anti-pattern exists. If you don't know, try
35 anti-pattern as you thought). If you are unable to determine why the
36 anti-pattern exists, put: "Unknown" here.
41 anti-pattern and the positive effects of solving it in the manner described.
43 <!-- end copy/paste on previous line -->
57 differ. Writing a custom argument parser re-invents the wheel on c++ command
69 handy built-in validators, and allows easy customizations to validation.
87 errors - the net effect being unnecessary shared library packages being
90 Consult [Yocto Project Mega-Manual][mega-manual-rdepends] for information on
93 [mega-manual-rdepends]:
94   https://www.yoctoproject.org/docs/latest/mega-manual/mega-manual.html#var-RDEPENDS
112 In systemd unit files:
117 ExecStart=/usr/bin/env some-application
122 Outside of OpenBMC, most applications that provide systemd unit files don't
129 This anti-pattern exists because a requirement exists to enable live patching of
130 applications on read-only filesystems. Launching applications in this way was
135 /usr/bin/phosphor-hwmon
138 on a read-only filesystem becomes:
141 /usr/local/bin/phosphor-hwmon`
149 that supports live patching of any file in the read-only filesystem has emerged.
153 mkdir -p /var/persist/usr
154 mkdir -p /var/persist/work/usr
155 mount -t overlay -o lowerdir=/usr,upperdir=/var/persist/usr,workdir=/var/persist/work/usr overlay /…
163 To undo existing instances of this anti-pattern remove /usr/bin/env from systemd
168 sed -i s,/usr/bin/env ,/usr/bin/, foo.service
190 1. Long-running daemons started by, for instance, systemd service files and
195 confusing and error-prone to users, but should instead be placed in a
200 we transition to having non-root access).
202 The sbin anti-pattern exists because the FHS was misinterpreted at an early
221 de-facto interpretation of the system being administered refers to the OS
233 ## Handling unexpected error codes and exceptions
237 The anti-pattern is for an application to continue processing after it
247   ... use d-Bus APIs...
278   https://github.com/openbmc/phosphor-logging/blob/master/README.md
302    - BMC Developer: Reference internal applications, services, pids, etc. the
308    - Administrator: Reference the external interfaces of the BMC such as the
315    - Analyzer tool: Consider breaking the log down and including several
331 ## Non-standard debug application options and logging
335 An application uses non-standard methods on startup to indicate verbose logging
336 and/or does not utilize standard systemd-journald debug levels for logging.
346 - 0xff, --vv, -vv, -v, --verbose, `<and more>`
362 it will support the standard "-v,--verbose" option.
368 --verbose command line option.
376 - <https://lore.kernel.org/openbmc/YV21cD3HOOGi7K2f@heinlein/>
377 - <https://lore.kernel.org/openbmc/CAH2-KxBV9_0Dt79Quy0f4HkXXPdHfBw9FsG=4KwdWXBYNEA-ew@mail.gmail.c…
378 - <https://lore.kernel.org/openbmc/YtPrcDzaxXiM6vYJ@heinlein.stwcx.org.github.beta.tailscale.net/>
396 that is more generally applicable (e.g. host power-on procedures).
420 inconsistent formatting with tools like `clang-format`, which causes significant
423 to unit test, and produce log messages that are difficult to read given their
440 than 10 lines, and call the appropriate function for handling non-trivial
458 …/CppCoreGuidelines/CppCoreGuidelines#f11-use-an-unnamed-lambda-if-you-need-a-simple-function-objec…
485 over-exposes the library's design and may lead to otherwise unnecessary API or
498 ## Ill-defined data structuring in `lg2` message strings
502 Attempts at encoding information into the journal's MESSAGE string that is at
515 [`lg2` is OpenBMC's preferred C++ logging interface][phosphor-logging-lg2] and
517 [systemd-journald provides structured logging][systemd-structured-logging],
518 which allows us to capture additional metadata alongside the provided message.
520 [phosphor-logging-lg2]:
521   https://github.com/openbmc/phosphor-logging/blob/master/docs/structured-logging.md
522 [systemd-structured-logging]:
523   https://0pointer.de/blog/projects/journal-submit.html
527 metadata with each message logged. However, the primary way the entries are
529 `MESSAGE` value for each log entry. This may lead to a misunderstanding that the
530 printed message is the only way to expose related metadata for investigating
534 into the journal's `MESSAGE` string. This aids with human inspection of the logs
536 it's not intended that this interpolation be used for ad-hoc, ill-defined
539 All key-value pairs provided to the `lg2` logging APIs are captured in the
541 into the `MESSAGE` string. It is always possible to recover the information
542 associated with the log event even if it's not captured in the `MESSAGE` string.
544 `phosphor-time-manager` demonstrates a reasonable use of the `lg2` APIs. One
545 logging instance in the code [is as follows][phosphor-time-manager-lg2]:
547 [phosphor-time-manager-lg2]:
548 …https://github.com/openbmc/phosphor-time-manager/blob/5ce9ac0e56440312997b25771507585905e8b360/man…
557 Sep 23 06:09:57 bmc phosphor-time-manager[373]: Time mode has been changed to xyz.openbmc_project.T…
564 # journalctl --identifier=phosphor-time-manager --boot --output=verbose | grep -v '^    _' | head -…
565 Sat 2023-09-23 06:09:57.645208 UTC [s=85c1cb5f8e02445aa110a5164c9c07f6;i=244;b=ffd111d3cdca41c8893b…
567     MESSAGE=Time mode has been changed to xyz.openbmc_project.Time.Synchronization.Method.NTP
569     CODE_FILE=/usr/src/debug/phosphor-time-manager/1.0+git/manager.cpp
573     SYSLOG_IDENTIFIER=phosphor-time-manager
577 the structured data, as well as being interpolated into `MESSAGE` as requested.
579 `MESSAGE` using the format string captured in the `LOG2_FMTMSG` metadata entry.
587 it][openbmc-gerrit-67994] added a number of ad-hoc, ill-defined attempts at
588 providing all the metadata through the `MESSAGE` entry.
590 [openbmc-gerrit-67994]: https://gerrit.openbmc.org/c/openbmc/pldm/+/67994
595 contain ad-hoc, ill-defined attempts at integrating metadata for automated