Add reoccuring errors doc

There are a number of common coding, design, and implementation mistakes
that bmcweb users tend to make.  This attempts to document them so we
have a single source to point users to when making gerrit comments.  The
hope is that this alleviates some of the early mistakes that new users
tend to make.

Tested:
Documentation only.  No functional changes.

Signed-off-by: Ed Tanous <ed@tanous.net>
Change-Id: Icc0081c94403c937d9a1ce44b7d6e81a5716a32e

1 files changed, 181 insertions, 0 deletions

diff --git a/COMMON_ERRORS.md b/COMMON_ERRORS.md
new file mode 100644
index 0000000000..c68c86cdfe
--- /dev/null
+++ b/COMMON_ERRORS.md
@@ -0,0 +1,181 @@
+# Commonly recurring errors in bmcweb
+
+What follows is a list of common errors that new users to bmcweb tend to make
+when operating within its bounds for the first time.  If this is your first time
+developing in bmcweb, the maintainers highly recommend reading and understanding
+_all_ of common traps before continuing with any development.  Every single one
+of the examples below compile without warnings, but are incorrect in
+not-always-obvious ways, or impose a pattern that tends to cause hard to find
+bugs, or bugs that appear later.  Every one has been submitted to code review
+multiple times.
+
+### Directly dereferencing a pointer without checking for validity first
+```C++
+int myBadMethod(const nlohmann::json& j){
+    const int* myPtr = j.get_if<int>();
+    return *myPtr;
+}
+```
+This pointer is not guaranteed to be filled, and could be a null dereference.
+
+### String views aren't null terminated
+```C++
+int getIntFromString(const std::string_view s){
+    return std::atoi(s.data());
+}
+```
+This will give the right answer much of the time, but has the possibility to
+fail when string\_view is not null terminated.  Use from\_chars instead, which
+takes both a pointer and a length
+
+### Not handling input errors
+```C++
+int getIntFromString(const std::string& s){
+    return std::atoi(s.c_str());
+}
+```
+In the case where the string is not representable as an int, this will trigger
+undefined behavior at system level.  Code needs to check for validity of the
+string, ideally with something like from\_chars, and return the appropriate error
+code.
+
+### Walking off the end of a string
+```C++
+std::string getFilenameFromPath(const std::string& path){
+    size_t index = path.find("/");
+    if (index != std::string::npos){
+        // If the string ends with "/", this will walk off the end of the string.
+        return path.substr(pos + 1);
+    }
+    return "";
+}
+```
+
+### Using methods that throw (or not handling bad inputs)
+```C++
+int myBadMethod(nlohmann::json& j){
+    return j.get<int>();
+}
+```
+This method throws, and bad inputs will not be handled
+
+Commonly used methods that fall into this pattern:
+std::variant::get
+std::vector::at
+std::map::at
+std::set::at
+std::<generic container type>::at
+nlohmann::json::operator!=
+nlohmann::json::operator+=
+nlohmann::json::at
+nlohmann::json::get
+nlohmann::json::get\_ref
+nlohmann::json::get\_to
+std::filesystem::create\_directory
+std::filesystem::rename
+std::filesystem::file\_size
+std::stoi
+std::stol
+std::stoll
+
+#### special/strange case:
+
+nlohmann::json::parse by default throws on failure, but also accepts a optional
+argument that causes it to not throw.  Please consult the other examples in the
+code for how to handle errors.
+
+
+#### Special note: Boost
+there is a whole class of boost asio functions that provide both a method that
+throws on failure, and a method that accepts and returns an error code.  This is
+not a complete list, but users should verify in the boost docs when calling into
+asio methods, and prefer the one that returns an error code instead of throwing.
+
+boost::asio::ip::tcp::acceptor::bind();
+boost::asio::ip::tcp::acceptor::cancel();
+boost::asio::ip::tcp::acceptor::close();
+boost::asio::ip::tcp::acceptor::listen();
+boost::asio::ip::address::make\_address();
+
+### Blocking functions
+
+bmcweb uses a single reactor for all operations.  Blocking that reactor for any
+amount of time causes all other operations to stop.  The common blocking
+functions that tend to be called incorrectly are:
+
+sleep()
+boost::asio::ip::tcp::socket::read()
+boost::asio::ip::tcp::socket::read\_some()
+boost::asio::ip::tcp::socket::write()
+boost::asio::ip::tcp::socket::write\_some()
+boost::asio::ip::tcp::socket::connect()
+boost::asio::ip::tcp::socket::send()
+boost::asio::ip::tcp::socket::wait()
+boost::asio::steady\_timer::wait()
+
+Note: an exception is made for filesystem/disk IO read and write.  This is
+mostly due to not having great abstractions for it that mate well with the async
+system, the fact that most filesystem accesses are into tmpfs (and therefore
+should be "fast" most of the time) and in general how little the filesystem is
+used in practice.
+
+### Lack of locking between subsequent calls
+While global data structures are discouraged, they are sometimes required to
+store temporary state for operations that require it.  Given the single
+threaded nature of bmcweb, they are not required to be explicitly threadsafe,
+but they must be always left in a valid state, and checked for other uses
+before occupying.
+
+```C++
+std::optional<std::string> currentOperation;
+void firstCallbackInFlow(){
+    currentOperation = "Foo";
+}
+void secondCallbackInFlow(){
+    currentOperation.reset();
+}
+```
+
+In the above case, the first callback needs a check to ensure that
+currentOperation is not already being used.
+
+### Wildcard reference captures in lambdas
+```
+std::string x; auto mylambda = [&](){
+    x = "foo";
+}
+do_async_read(mylambda)
+```
+
+Numerous times, lifetime issues of const references have been injected into
+async bmcweb code.  While capturing by reference can be useful, given how
+difficult these types of bugs are to triage, bmcweb explicitly requires that all
+code captures variables by name explicitly, and calls out each variable being
+captured by value or by reference.  The above prototypes would change to
+[&x]()... Which makes clear that x is captured, and its lifetime needs tracked.
+
+
+### URLs should end in "/"
+```C++
+BMCWEB("/foo/bar");
+```
+Unless you explicitly have a reason not to (as there is one known exception
+where the behavior must differ) all URL handlers should end in "/".  The bmcweb
+route handler will detect routes ending in slash and generate routes for both
+the route ending in slash and the one without.  This allows both URLs to be
+used by users.  While many specifications do not require this, it resolves a
+whole class of bug that we've seen in the past.
+
+
+### URLs constructed in aggregate
+```C++
+std::string routeStart = "/redfish/v1";
+
+BMCWEB_ROUTE(routestart + "/SessionService/")
+```
+Very commonly, bmcweb maintainers and contributors alike have to do audits of
+all routes that are available, to verify things like security and documentation
+accuracy.  While these processes are largely manual, they can mostly be
+conducted by a simple grep statement to search for urls in question.  Doing the
+above makes the route handlers no longer greppable, and complicates bmcweb
+patchsets as a whole.