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 IRC 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## Explicit AC_MSG_ERROR on PKG_CHECK_MODULES failure 46 47### Identification 48``` 49PKG_CHECK_MODULES( 50 [PHOSPHOR_LOGGING], 51 [phosphor-logging], 52 [], 53 [AC_MSG_ERROR([Could not find phosphor-logging...openbmc/phosphor-logging package required])]) 54``` 55 56### Description 57 58The autotools PKG_CHECK_MODULES macro provides the ability to specify an 59"if found" and "if not found" behavior. By default, the "if not found" 60behavior will list the package not found. In many cases, this is sufficient 61to a developer to know what package is missing. In most cases, it's another 62OpenBMC package. 63 64If the library sought's name isn't related to the package providing it, then 65the failure message should be set to something more useful to the developer. 66 67### Resolution 68 69Use the default macro behavior when it is clear that the missing package is 70another OpenBMC package. 71 72``` 73PKG_CHECK_MODULES([PHOSPHOR_LOGGING], [phosphor-logging]) 74``` 75 76## Explicit listing of shared library packages in RDEPENDS in bitbake metadata 77 78### Identification 79``` 80RDEPENDS_${PN} = "libsystemd" 81``` 82 83### Description 84Out of the box bitbake examines built applications, automatically adds runtime 85dependencies and thus ensures any library packages dependencies are 86automatically added to images, sdks, etc. There is no need to list them 87explicitly in a recipe. 88 89Dependencies change over time, and listing them explicitly is likely prone to 90errors - the net effect being unnecessary shared library packages being 91installed into images. 92 93Consult 94https://www.yoctoproject.org/docs/latest/mega-manual/mega-manual.html#var-RDEPENDS 95for information on when to use explicit runtime dependencies. 96 97### Background 98The initial bitbake metadata author for OpenBMC was not aware that bitbake 99added these dependencies automatically. Initial bitbake metadata therefore 100listed shared library dependencies explicitly, and was subsequently copy pasted. 101 102### Resolution 103Do not list shared library packages in RDEPENDS. This eliminates the 104possibility of installing unnecessary shared library packages due to 105unmaintained library dependency lists in bitbake metadata. 106 107## Use of /usr/bin/env in systemd service files 108 109### Identification 110In systemd unit files: 111``` 112[Service] 113 114ExecStart=/usr/bin/env some-application 115``` 116 117### Description 118Outside of OpenBMC, most applications that provide systemd unit files don't 119launch applications in this way. So if nothing else, this just looks strange 120and violates the [princple of least 121astonishment](https://en.wikipedia.org/wiki/Principle_of_least_astonishment). 122 123### Background 124This anti-pattern exists because a requirement exists to enable live patching of 125applications on read-only filesystems. Launching applications in this way was 126part of the implementation that satisfied the live patch requirement. For 127example: 128 129``` 130/usr/bin/phosphor-hwmon 131``` 132 133on a read-only filesystem becomes: 134 135``` 136/usr/local/bin/phosphor-hwmon` 137``` 138 139on a writeable /usr/local filesystem. 140 141### Resolution 142The /usr/bin/env method only enables live patching of applications. A method 143that supports live patching of any file in the read-only filesystem has emerged. 144Assuming a writeable filesystem exists _somewhere_ on the bmc, something like: 145 146``` 147mkdir -p /var/persist/usr 148mkdir -p /var/persist/work/usr 149mount -t overlay -o lowerdir=/usr,upperdir=/var/persist/usr,workdir=/var/persist/work/usr overlay /usr 150``` 151can enable live system patching without any additional requirements on how 152applications are launched from systemd service files. This is the preferred 153method for enabling live system patching as it allows OpenBMC developers to 154write systemd service files in the same way as most other projects. 155 156To undo existing instances of this anti-pattern remove /usr/bin/env from systemd 157service files and replace with the fully qualified path to the application being 158launched. For example, given an application in /usr/bin: 159 160``` 161sed -i s,/usr/bin/env ,/usr/bin/, foo.service 162``` 163 164## Placement of applications in /sbin or /usr/sbin 165 166### Identification 167OpenBMC applications that are installed in /usr/sbin. $sbindir in bitbake 168metadata, makefiles or configure scripts. 169 170### Description 171Installing OpenBMC applications in /usr/sbin violates the [principle of least 172astonishment](https://en.wikipedia.org/wiki/Principle_of_least_astonishment) and 173more importantly violates the 174[FHS](https://en.wikipedia.org/wiki/Filesystem_Hierarchy_Standard). 175 176### Background 177The sbin anti-pattern exists because the FHS was misinterpreted at an early point 178in OpenBMC project history, and has proliferated ever since. 179 180From the hier(7) man page: 181 182``` 183/usr/bin This is the primary directory for executable programs. Most programs 184executed by normal users which are not needed for booting or for repairing the 185system and which are not installed locally should be placed in this directory. 186 187/usr/sbin This directory contains program binaries for system administration 188which are not essential for the boot process, for mounting /usr, or for system 189repair. 190``` 191The FHS description for /usr/sbin refers to "system administration" but the 192de-facto interpretation of the system being administered refers to the OS 193installation and not a system in the OpenBMC sense of managed system. As such 194OpenBMC applications should be installed in /usr/bin. 195 196### Resolution 197Install OpenBMC applications in /usr/bin/. 198