Cloudef / bemenu

Dynamic menu library and client program inspired by dmenu
GNU General Public License v3.0
1.23k stars 95 forks source link

lib: Improve error handling? #165

Open nmeum opened 3 years ago

nmeum commented 3 years ago

Currently, the API provided by the library provides the caller with no additional error information. Provided functions only allow the caller to determine that an error occurred (e.g. through specific return value) but do not indicate why this error occurred. I encountered this problem indirectly while debugging bemenu on Arch Linux where it failed to start for some reason. As it turns out, Arch Linux packages bemenu rendering backends in separate subpackages. If no backend is found bm_init fails but the client doesn't provide any error message (try with echo foo | env BEMENU_BACKEND=nonexistent bemenu). While the client could certainly write bm_init failed to standard error before terminating, it cannot state why it failed (i.e. no backend available) because the library doesn't propagate this information. I would find it desirable if the client would be able to output an error message ala. bm_init failed: no backend available. In order to implement this in the client, the library must do proper error handling.

I think there are two ways of implementing this:

  1. Changing the API of every library function, which potentially returns an error, to include an output function parameter for an error code.
  2. Adding a global error variable (think errno.h) with custom error codes and documenting which functions modify this global error variable.

The latter approach wouldn't require backwards incompatible changes to the existing API. In both cases one would define an enum with error reasons typedef enum { BACKEND_NOT_FOUND, … } bm_error and a function for converting a value of this type to a string, e.g. char *bm_error_str(bm_error). With the latter approach the bm_init calling code in the client could be changed to something along the following lines:

if (!bm_init()) {
  fprintf(stderr, "bm_init failed: %s\n", bm_error_str(bm_errno));
  return EXIT_FAILURE;
}

Is that something you would be interested in? If so, which approach do you prefer?

Cloudef commented 3 years ago

Considering there's not many error states in the library, both of the suggestions sound reasonable to me. API change is cleaner, and while I'm not huge fan of errno, considering how little error states and how the library works bm_errno could be enough.