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
14 repeating the same explanations over and over, a link to this document can be
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 -->
50 abstracts away the parsing and provides a `[]` operator to access the
55 Writing a custom ArgumentParser object creates nearly duplicate code in a
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.
78     [phosphor-logging],
80     [AC_MSG_ERROR([Could not find phosphor-logging...openbmc/phosphor-logging package required])])
87 list the package not found. In many cases, this is sufficient to a developer to
99 PKG_CHECK_MODULES([PHOSPHOR_LOGGING], [phosphor-logging])
115 explicitly in a recipe.
118 errors - the net effect being unnecessary shared library packages being
122 https://www.yoctoproject.org/docs/latest/mega-manual/mega-manual.html#var-RDEPENDS
146 ExecStart=/usr/bin/env some-application
158 This anti-pattern exists because a requirement exists to enable live patching of
159 applications on read-only filesystems. Launching applications in this way was
160 part of the implementation that satisfied the live patch requirement. For
164 /usr/bin/phosphor-hwmon
167 on a read-only filesystem becomes:
170 /usr/local/bin/phosphor-hwmon`
173 on a writeable /usr/local filesystem.
177 The /usr/bin/env method only enables live patching of applications. A method
178 that supports live patching of any file in the read-only filesystem has emerged.
179 Assuming a writeable filesystem exists _somewhere_ on the bmc, something like:
182 mkdir -p /var/persist/usr
183 mkdir -p /var/persist/work/usr
184 mount -t overlay -o lowerdir=/usr,upperdir=/var/persist/usr,workdir=/var/persist/work/usr overlay /…
187 can enable live system patching without any additional requirements on how
189 method for enabling live system patching as it allows OpenBMC developers to
192 To undo existing instances of this anti-pattern remove /usr/bin/env from systemd
197 sed -i s,/usr/bin/env ,/usr/bin/, foo.service
219 1. Long-running daemons started by, for instance, systemd service files and
221 2. Utilities intended to be used by a user as a CLI.
224 confusing and error-prone to users, but should instead be placed in a
229 we transition to having non-root access).
231 The sbin anti-pattern exists because the FHS was misinterpreted at an early
250 de-facto interpretation of the system being administered refers to the OS
251 installation and not a system in the OpenBMC sense of managed system. As such
266 The anti-pattern is for an application to continue processing after it
276   ... use d-Bus APIs...
289 Writing a log entry instead of a core dump may not give enough information to
290 debug a truly unexpected problem, so developers do not get a chance to
307   https://github.com/openbmc/phosphor-logging/blob/master/README.md
313 1. Check all places where a return code or errno value is given. Strongly
314    consider that a default error handler should throw an exception, for example
316 2. Have a good reason to handle specific error codes. See below.
317 3. Have a good reason to handle specific exceptions. Allow other exceptions to
332    - BMC Developer: Reference internal applications, services, pids, etc. the
336 …`ipmid service successfully processed a network setting packet, however the user input of USB0 is …
338    - Administrator: Reference the external interfaces of the BMC such as the
339      REST API. They can respond to feedback about invalid input, or a need to
343 …`The network interface of USB0 is not a valid option. Retry the IPMI command with a valid interfac…
345    - Analyzer tool: Consider breaking the log down and including several
348      like [UserInput][ipmi][Network] tells at a quick glance that the input
357    don't continue. Allow the system to determine if it writes a core dump or
359    consider using a better error recovery mechanism.
361 ## Non-standard debug application options and logging
365 An application uses non-standard methods on startup to indicate verbose logging
366 and/or does not utilize standard systemd-journald debug levels for logging.
376 - 0xff, --vv, -vv, -v, --verbose, <and more>
391 If an OpenBMC application is to support enhanced debug via a command line then
392 it will support the standard "-v,--verbose" option.
396 applications. If a developer believes this would cause too much debug data in
397 certain cases then they can protect these journald debug calls around a
398 --verbose command line option.
404 Desire to expose a DBus interface to drive GPIOs, for example:
406 - https://lore.kernel.org/openbmc/YV21cD3HOOGi7K2f@heinlein/
407 - https://lore.kernel.org/openbmc/CAH2-KxBV9_0Dt79Quy0f4HkXXPdHfBw9FsG=4KwdWXBYNEA-ew@mail.gmail.co…
408 - https://lore.kernel.org/openbmc/YtPrcDzaxXiM6vYJ@heinlein.stwcx.org.github.beta.tailscale.net/
413 means with a shift in system design philosophy. As such, GPIOs are a (hardware)
425 question is probably best wrapped up as an implementation detail of a behaviour
426 that is more generally applicable (e.g. host power-on procedures).
450 inconsistent formatting with tools like `clang-format`, which causes significant
452 In addition, because they are declared in a function scope, they are difficult
459 Lambdas are a natural approach to implementing callbacks in the context of
466 Prefer to either use `std::bind_front` and a method or static function to handle
467 the return, or a lambda that is less than 10 lines of code to handle an error
469 `sdbusplus::asio::connection::async_method_call`, keep the lambda length less
470 than 10 lines, and call the appropriate function for handling non-trivial
488 …/CppCoreGuidelines/CppCoreGuidelines#f11-use-an-unnamed-lambda-if-you-need-a-simple-function-objec…
490 ## Placing internal headers in a parallel subtree
494 Declaring types and functions that do not participate in a public API of a
495 library in header files that do not live alongside their implementation in the
500 There's no reason to put internal headers in a parallel subtree (for example,
507 In C and C++, header files expose the public API of a library to its consumers
508 and need to be installed onto the developer's system in a location known to the
515 over-exposes the library's design and may lead to otherwise unnecessary API or
519 API or ABI in the sense of a library. Any header files in the source tree of
520 such projects have no need to be installed onto a developer's system and
528 ## Ill-defined data structuring in `lg2` message strings
533 most only plausible to parse using a regex while also reducing human
545 [`lg2` is OpenBMC's preferred C++ logging interface][phosphor-logging-lg2] and
547 [systemd-journald provides structured logging][systemd-structured-logging],
550 [phosphor-logging-lg2]:
551   https://github.com/openbmc/phosphor-logging/blob/master/docs/structured-logging.md
552 [systemd-structured-logging]:
553   https://0pointer.de/blog/projects/journal-submit.html
556 metadata associated with a log event. The journal captures a default set of
559 `MESSAGE` value for each log entry. This may lead to a misunderstanding that the
565 as it becomes possible to highlight important metadata for a log event. However,
566 it's not intended that this interpolation be used for ad-hoc, ill-defined
569 All key-value pairs provided to the `lg2` logging APIs are captured in the
574 `phosphor-time-manager` demonstrates a reasonable use of the `lg2` APIs. One
575 logging instance in the code [is as follows][phosphor-time-manager-lg2]:
577 [phosphor-time-manager-lg2]:
578 …https://github.com/openbmc/phosphor-time-manager/blob/5ce9ac0e56440312997b25771507585905e8b360/man…
587 Sep 23 06:09:57 bmc phosphor-time-manager[373]: Time mode has been changed to xyz.openbmc_project.T…
594 # journalctl --identifier=phosphor-time-manager --boot --output=verbose | grep -v '^    _' | head -…
595 Sat 2023-09-23 06:09:57.645208 UTC [s=85c1cb5f8e02445aa110a5164c9c07f6;i=244;b=ffd111d3cdca41c8893b…
599     CODE_FILE=/usr/src/debug/phosphor-time-manager/1.0+git/manager.cpp
603     SYSLOG_IDENTIFIER=phosphor-time-manager
611 `LOG2_FMTMSG` also provides a stable handle for identifying the existence of a
616 A variety of patches such as [PLDM:Catching exception precisely and printing
617 it][openbmc-gerrit-67994] added a number of ad-hoc, ill-defined attempts at
620 [openbmc-gerrit-67994]: https://gerrit.openbmc.org/c/openbmc/pldm/+/67994
625 contain ad-hoc, ill-defined attempts at integrating metadata for automated