On 09/27/22 16:46, Richard W.M. Jones wrote: I think it boils down to the permitted multiplicity of a paramater: - exactly 1 (mandatory parameter) - 0 or 1 (optional parameter) - 1 or more (mandatory (non-empty) list) - 0 or more (optional list) First I think we should figure out what parameter has what multiplicity. Then, it should be documented for the end user (this can be generated, but either way, the documentation should be clear about the decisions). Considering "optional list" in particular, I see no semantic difference between vec==NULL and vec[0]==NULL. If an optional list is expected, both should be tolerated; if a mandatory (non-empty) list is expected, both are invalid. Once we decided / documented what parameters were valid, I think the most practical way to enfoce mandatory parameters (in case they are taken by address) and mandatory (non-empty) lists would be with assert(). (We should also make sure that NDEBUG is never defined -- some parts of libnbd and nbdkit already "#undef NDEBUG"; I'd go farther and just forbid building libnbd and nbdkit without assertions. Assertions cost a few CPU cycles, and I don't expect nbdkit to be CPU-bound ever. Assertions are worth the CPU costs.) assert() is good because: - it crashes (and yes, once we document the expectations, crashing a program from a library is fine), - static analyzers such as coverity understand it (to my knowledge), - gcc will not remove it (in the absence of NDEBUG, but for that, see above). I think the nonnull attribute is not worth it. It might catch statically provable NULL arguments, but cannot catch such that are not statically provable. By weakening the function's internall NULL checks, it introduces new problems for those statically-not-provable-but-still-NULL code paths. In fact, assert() *in combination* with attribute nonnull would be the best: issue warnings at build time in case the NULL arg is statically provable, use assert() to catch anything that might slip through dynamically. Unfortunately, attribute nonnull may not live up to the build-time-warning expectation (dependent on the gcc version), but it *does* eviscerate assert() -- if I understand correctly. So attribute nonnull does more harm than good, apparently. If *all* NULL args were statically provable, then attribute nonnull, with gcc 12+, would clearly win over assert() -- but not all such args are statically provable. Therefore we need assert(), and because attribute nonnull actually weakens assert(), we should *only* use assert(). ... And my argument would end here, in case we didn't generate the python bindings. If the python bindings were a separate project, I'd say that the symptom $ nbdsh -c 'h.connect_command([])' nbdsh: generator/states-connect.c:247: enter_STATE_CONNECT_COMMAND_START: Assertion `h->argv.ptr[0]' failed. Aborted (core dumped) was entirely valid (expected), and that it was up to the implementors of the python bindings to catch an empty list here, turn it into a python exception, lest the C function's invariants be violated. So, end-to-end, that would result in an assert() in the C function, and an "if" in the Python code. *But*. Given that we generate the python bindings... we might as well just move the "if" into the most deeply lying C code, in place of the assert()s, and then let the generator turn that error into a Python exception higher up, as usual. So, purely because of this centralized code generation, I guess I'm arguing for explicit "ifs" in the deepest (generated, or hand-written) C code, and avoiding the nonnull attribute. Again, I don't see a semantic difference here between vec==NULL and (vec != NULL && vec[0] == NULL). (There *is* a difference between "setting something to an empty list" vs. "not setting something at all", but we express that differently already, I hope! If we do distinguish "nonexistent" (~optional) from "empty", then I get to redo my whole argument...) Laszlo

On 09/27/22 20:59, Eric Blake wrote: Well, I don't know. In C, where we represent the string list with a NULL-terminated (char**), I really feel like there isn't a semantic difference between vec==NULL and vec[0]==NULL. It's a very rough representation anyway. But in Python and OCaml, where we can distinguish "None" from "[]", and "None" from "Some []" respectively, I'm not so sure myself. Python and OCaml seem to imbue the representation with more meaning / type information than C does. I think removing assertions from a library is the more courageous option! :) Is it better to produce garbage (or to set up a real, but long-distance crash) than to stop immediately, when we know something has gone wrong? I think the only alternative to live assertions is to code up some kind of recovery in every possible spot. That's a lot of work. (I've heard this argument wrt. kernel modules -- panicking the whole kernel due to hardware misbehavior in a driver is frowned upon, to say the least. So the kernel driver apparently needs to, at the least, disable itself dynamically, when a device behaves out of spec. That's a lot of work.) What projects consume libnbd primarily? I think in the context of a virt-v2v / virt-p2v conversion, it's better for the conversion to crash early, than to carry on with garbage data (for example). I'm out of ideas. I'd like to ask "can we get away with just modifying the documentation, and let the user deal with undefined behavior afterwards?" But given python and ocaml, I don't think this is a good idea, because those languages simply don't allow "undefined behavior" (definitely not to the extent that C does). In other words, python and ocaml promise the user so much more than C does that the python and ocaml bindings would have to be *much* thicker (contain much more logic and checking) than the C binding. I keep returning to my impression that the "thin" (?) binding generation is a very leaky abstraction. I apologize, I know that this is not constructive. I'm just out of ideas. Laszlo