Problem Description

The int (*recv )(netdev_t *dev, void *buf, size_t len, void *info) member in netdev_driver_t (see Doxygen) is a textbook example of a misleading API:

1. The function does three completely different things depending on arguments:
   • Receive the message and return its size, if buf != NULL
   • Returns the message size of the incoming message if (buf == NULL) && (len == 0)
   • Drops the incoming message if (buf == NULL) && (len != 0)
2. The function name only reflects one of the three cases
3. One of the three cases is a corner case (the drop packet case only occurs under high load), so a bug is going to be unnoticed for quite some time

Symptopms

This misleading API already lead to a bug #9784. I predict similar bugs will show up in the future, if the API is not changed. Update: Many similar bugs were found, see #9832

Suggestions to Address the Problem

1. Split the function into three functions (e.g. recv(), get_size() and drop()). This would be the cleanest API, but the ROM size of the implementation is likely to increase, as all three will share some common code. (I assume the compiler will inline the common code when implemented in separate functions, so even when no duplicate C code is present, the ROM size will likely grow.)
   • Pros:
     • Cleanest API: Reviewers and implementers will no longer easily forget about the size/drop features
     • Checking for missing size/drop implementations possible by an assert
     • No increase in runtime overhead or RAM (barely measurable speedup expected by not longer needing conditional jumps)
     • Biggest increase in code readability (control flow simplified, functions get shorter, function names match their intention, header parsing will be moved to short static functions)
   • Cons:
     • Biggest ROM increase compared to other approaches expected
     • Biggest increase in lines of code
2. Rename the function, e.g. to recv_or_size_or_drop(). While this name is very clumsy, at least it is very obvious that this functions has to implement three different things.
   • Pro:
     • No increase in RAM/ROM usage and runtime overhead
   • Cons:
     • Only cosmetic, does not really address the problem
     • Unclean API
3. Add an additional parameter (e.g. an enum) to specify which of the three things the function should do. This would be much more obvious to the programmer, even though a bit wasteful
   • Pros:
     • No increase in ROM usage (and likely in RAM usage, if increased stack usage does not result in higher stack sizes being used)
     • Compiler helps implementer if both size and drop is forgotten (unused argument)
   • Cons:
     • Unclean API
     • Slight increase in runtime (barely measurable)
4. Like 1., but only split off drop and keep size in recv
   • Pros:
     • Only change that part of the API that was affected by bugs so far
     • Less ROM overhead and lines of code than 1.
   • Cons:
     • Still only slightly less unclean API
     • Compared to 1. most of the ROM / lines of code increase is already been paid, so why not pay the rest and get a clean API
5. Add static inline functions to increase readability: In the upper layers add wrappers to call the size and drop feature od the recv function more explicitly; in the lower layers to test for the drop/size/recv mode more readable
   • Pros:
     • No increase in RAM/ROM usage and runtime overhead
     • Correct recv implementations and upper layer code gets more readible
   • Cons:
     • Unclean API
     • The problem is (mostly) about incorrect lower layer implementations, this change does not help much here