1# OpenBMC Anti-patterns 2 3From [Wikipedia](https://en.wikipedia.org/wiki/Anti-pattern): 4 5 6"An anti-pattern is a common response to a recurring problem that is usually 7ineffective and risks being highly counterproductive." 8 9 10The developers of OpenBMC do not get 100% of decisions right 100% of the time. 11That, combined with the fact that software development is often an exercise in 12copying and pasting, results in mistakes happening over and over again. 13 14 15This page aims to document some of the anti-patterns that exist in OpenBMC to 16ease the job of those reviewing code. If an anti-pattern is spotted, rather 17that repeating the same explanations over and over, a link to this document can 18be provided. 19 20 21<!-- begin copy/paste on next line --> 22 23## Anti-pattern template [one line description] 24 25### Identification 26(1 paragraph) Describe how to spot the anti-pattern. 27 28### Description 29(1 paragraph) Describe the negative effects of the anti-pattern. 30 31### Background 32(1 paragraph) Describe why the anti-pattern exists. If you don't know, try 33running git blame and look at who wrote the code originally, and ask them on the 34mailing list or in Discord what their original intent was, so it can be documented 35here (and you may possibly discover it isn't as much of an anti-pattern as you 36thought). If you are unable to determine why the anti-pattern exists, put: 37"Unknown" here. 38 39### Resolution 40(1 paragraph) Describe the preferred way to solve the problem solved by the 41anti-pattern and the positive effects of solving it in the manner described. 42 43<!-- end copy/paste on previous line --> 44 45## Custom ArgumentParser object 46 47### Identification 48 49The ArgumentParser object is typically present to wrap calls to get options. 50It abstracts away the parsing and provides a `[]` operator to access the 51parameters. 52 53### Description 54 55Writing a custom ArgumentParser object creates nearly duplicate code in a 56repository. The ArgumentParser itself is the same, however, the options 57provided differ. Writing a custom argument parser re-invents the wheel on 58c++ command line argument parsing. 59 60### Background 61 62The ArgumentParser exists because it was in place early and then copied into 63each new repository as an easy way to handle argument parsing. 64 65### Resolution 66 67The CLI11 library was designed and implemented specifically to support modern 68argument parsing. It handles the cases seen in OpenBMC daemons and has some 69handy built-in validators, and allows easy customizations to validation. 70 71## Explicit AC_MSG_ERROR on PKG_CHECK_MODULES failure 72 73### Identification 74``` 75PKG_CHECK_MODULES( 76 [PHOSPHOR_LOGGING], 77 [phosphor-logging], 78 [], 79 [AC_MSG_ERROR([Could not find phosphor-logging...openbmc/phosphor-logging package required])]) 80``` 81 82### Description 83 84The autotools PKG_CHECK_MODULES macro provides the ability to specify an 85"if found" and "if not found" behavior. By default, the "if not found" 86behavior will list the package not found. In many cases, this is sufficient 87to a developer to know what package is missing. In most cases, it's another 88OpenBMC package. 89 90If the library sought's name isn't related to the package providing it, then 91the failure message should be set to something more useful to the developer. 92 93### Resolution 94 95Use the default macro behavior when it is clear that the missing package is 96another OpenBMC package. 97 98``` 99PKG_CHECK_MODULES([PHOSPHOR_LOGGING], [phosphor-logging]) 100``` 101 102## Explicit listing of shared library packages in RDEPENDS in bitbake metadata 103 104### Identification 105``` 106RDEPENDS_${PN} = "libsystemd" 107``` 108 109### Description 110Out of the box bitbake examines built applications, automatically adds runtime 111dependencies and thus ensures any library packages dependencies are 112automatically added to images, sdks, etc. There is no need to list them 113explicitly in a recipe. 114 115Dependencies change over time, and listing them explicitly is likely prone to 116errors - the net effect being unnecessary shared library packages being 117installed into images. 118 119Consult 120https://www.yoctoproject.org/docs/latest/mega-manual/mega-manual.html#var-RDEPENDS 121for information on when to use explicit runtime dependencies. 122 123### Background 124The initial bitbake metadata author for OpenBMC was not aware that bitbake 125added these dependencies automatically. Initial bitbake metadata therefore 126listed shared library dependencies explicitly, and was subsequently copy pasted. 127 128### Resolution 129Do not list shared library packages in RDEPENDS. This eliminates the 130possibility of installing unnecessary shared library packages due to 131unmaintained library dependency lists in bitbake metadata. 132 133## Use of /usr/bin/env in systemd service files 134 135### Identification 136In systemd unit files: 137``` 138[Service] 139 140ExecStart=/usr/bin/env some-application 141``` 142 143### Description 144Outside of OpenBMC, most applications that provide systemd unit files don't 145launch applications in this way. So if nothing else, this just looks strange 146and violates the [princple of least 147astonishment](https://en.wikipedia.org/wiki/Principle_of_least_astonishment). 148 149### Background 150This anti-pattern exists because a requirement exists to enable live patching of 151applications on read-only filesystems. Launching applications in this way was 152part of the implementation that satisfied the live patch requirement. For 153example: 154 155``` 156/usr/bin/phosphor-hwmon 157``` 158 159on a read-only filesystem becomes: 160 161``` 162/usr/local/bin/phosphor-hwmon` 163``` 164 165on a writeable /usr/local filesystem. 166 167### Resolution 168The /usr/bin/env method only enables live patching of applications. A method 169that supports live patching of any file in the read-only filesystem has emerged. 170Assuming a writeable filesystem exists _somewhere_ on the bmc, something like: 171 172``` 173mkdir -p /var/persist/usr 174mkdir -p /var/persist/work/usr 175mount -t overlay -o lowerdir=/usr,upperdir=/var/persist/usr,workdir=/var/persist/work/usr overlay /usr 176``` 177can enable live system patching without any additional requirements on how 178applications are launched from systemd service files. This is the preferred 179method for enabling live system patching as it allows OpenBMC developers to 180write systemd service files in the same way as most other projects. 181 182To undo existing instances of this anti-pattern remove /usr/bin/env from systemd 183service files and replace with the fully qualified path to the application being 184launched. For example, given an application in /usr/bin: 185 186``` 187sed -i s,/usr/bin/env ,/usr/bin/, foo.service 188``` 189 190## Placement of applications in /sbin or /usr/sbin 191 192### Identification 193OpenBMC applications that are installed in /usr/sbin. $sbindir in bitbake 194metadata, makefiles or configure scripts. 195 196### Description 197Installing OpenBMC applications in /usr/sbin violates the [principle of least 198astonishment](https://en.wikipedia.org/wiki/Principle_of_least_astonishment) and 199more importantly violates the 200[FHS](https://en.wikipedia.org/wiki/Filesystem_Hierarchy_Standard). 201 202### Background 203The sbin anti-pattern exists because the FHS was misinterpreted at an early point 204in OpenBMC project history, and has proliferated ever since. 205 206From the hier(7) man page: 207 208``` 209/usr/bin This is the primary directory for executable programs. Most programs 210executed by normal users which are not needed for booting or for repairing the 211system and which are not installed locally should be placed in this directory. 212 213/usr/sbin This directory contains program binaries for system administration 214which are not essential for the boot process, for mounting /usr, or for system 215repair. 216``` 217The FHS description for /usr/sbin refers to "system administration" but the 218de-facto interpretation of the system being administered refers to the OS 219installation and not a system in the OpenBMC sense of managed system. As such 220OpenBMC applications should be installed in /usr/bin. 221 222### Resolution 223Install OpenBMC applications in /usr/bin/. 224 225## Handling unexpected error codes and exceptions 226 227### Identification 228The anti-pattern is for an application to continue processing after it 229encounters unexpected conditions in the form of error codes and exceptions and 230not capturing the data needed to resolve the problem. 231 232Example C++ code: 233``` 234using InternalFailure = sdbusplus::xyz::openbmc_project::Common::Error::InternalFailure; 235try 236{ 237 ... use d-Bus APIs... 238} 239catch (InternalFailure& e) 240{ 241 phosphor::logging::commit<InternalFailure>(); 242} 243``` 244 245### Description 246Suppressing unexpected errors can lead an application to incorrect or erratic 247behavior which can affect the service it provides and the overall system. 248 249Writing a log entry instead of a core dump may not give enough information to 250debug a truly unexpected problem, so developers do not get a chance to 251investigate problems and the system's reliability does not improve over time. 252 253### Background 254Programmers want their application to continue processing when it encounters 255conditions it can recover from. Sometimes they try too hard and continue when 256it is not appropriate. 257 258Programmers also want to log what is happening in the application, so they 259write log entries that give debug data when something goes wrong, typically 260targeted for their use. They don't consider how the log entry is consumed 261by the BMC administrator or automated service tools. 262 263The `InternalFailure` in the [Phosphor logging README][] is overused. 264 265[Phosphor logging README]: https://github.com/openbmc/phosphor-logging/blob/master/README.md 266 267### Resolution 268 269Several items are needed: 2701. Check all places where a return code or errno value is given. Strongly 271 consider that a default error handler should throw an exception, for 272 example `std::system_error`. 2732. Have a good reason to handle specific error codes. See below. 2743. Have a good reason to handle specific exceptions. Allow other exceptions 275 to propagate. 2764. Document (in terms of impacts to other services) what happens when this 277 service fails, stops, or is restarted. Use that to inform the recovery 278 strategy. 279 280In the error handler: 281 2821. Consider what data (if any) should be logged. Determine who will consume 283 the log entry: BMC developers, administrator, or an analysis tool. Usually 284 the answer is more than one of these. 285 286 The following example log entries use an IPMI request to set network access 287 on, but the user input is invalid. 288 289 - BMC Developer: Reference internal applications, services, pids, etc. 290 the developer would be familiar with. 291 292 Example: `ipmid service successfully processed a network setting packet, 293 however the user input of USB0 is not a valid network interface to 294 configure.` 295 296 - Administrator: Reference the external interfaces of the BMC such as the 297 REST API. They can respond to feedback about invalid input, or a need 298 to restart the BMC. 299 300 Example: `The network interface of USB0 is not a valid option. Retry the 301 IPMI command with a valid interface.` 302 303 - Analyzer tool: Consider breaking the log down and including several 304 properties which an analyzer can leverage. For instance, tagging the 305 log with 'Internal' is not helpful. However, breaking that down into 306 something like [UserInput][IPMI][Network] tells at a quick glance that 307 the input received for configuring the network via an IPMI command was 308 invalid. Categorization and system impact are key things to focus on 309 when creating logs for an analysis application. 310 311 Example: `[UserInput][IPMI][Network][Config][Warning] Interface USB0 not 312 valid.` 313 3142. Determine if the application can fully recover from the condition. If not, 315 don't continue. Allow the system to determine if it writes a core dump or 316 restarts the service. If there are severe impacts when the service fails, 317 consider using a better error recovery mechanism. 318 319## Non-standard debug application options and logging 320 321### Identification 322An application uses non-standard methods on startup to indicate verbose 323logging and/or does not utilize standard systemd-journald debug levels for 324logging. 325 326### Description 327When debugging issues within OpenBMC that cross multiple applications, it's 328very difficult to enable the appropriate debug when different applications 329have different mechanisms for enabling debug. For example, different OpenBMC 330applications take the following as command line parameters to enable extra 331debug: 332- 0xff, --vv, -vv, -v, --verbose, <and more> 333 334Along these same lines, some applications then have their own internal methods 335of writing debug data. They use std::cout, printf, fprintf, ... 336Doing this causes other issues when trying to compare debug data across 337different applications that may be having their buffers flushed at different 338times (and picked up by journald). 339 340### Background 341Everyone has their own ways to debug. There was no real standard within 342OpenBMC on how to do it so everyone came up with whatever they were familiar 343with. 344 345### Resolution 346If an OpenBMC application is to support enhanced debug via a command line then 347it will support the standard "-v,--verbose" option. 348 349In general, OpenBMC developers should utilize the "debug" journald level for 350debug messages. This can be enabled/disabled globally and would apply to 351all applications. If a developer believes this would cause too much debug 352data in certain cases then they can protect these journald debug calls around 353a --verbose command line option. 354