Symbol NV_CLONE used incorrectly

[krader1961]

While working on issue #1038 I realized there are far too many preprocessor constants named with a NV_ prefix that are simple ints. Why is this a problem? Because it makes it

a) hard to know which symbols can be combined meaningfully,

b) verify via static analysis that a given constant passed to a function is legal for that function.

For example, there are a bunch of preprocessor constants meant to be used only in conjunction with the nv_disc() function; e.g., NV_FIRST and NV_CLONE. So I decided to turn those symbols into a set of typedef'd enums. Lo and behold that identifies a bug in the code:

../src/cmd/ksh93/sh/subshell.c:329:35: error: use of undeclared identifier 'NV_CLONE'
        _nv_unset(mp, NV_RDONLY | NV_CLONE);

Note that this wouldn't have been caught by simply turning those symbols into an enum (because enums are ints). It was caught because I also deliberately renamed the symbols (e.g., NV_CLONE => NVDISC_CLONE) to catch such misuses. In this case it is pretty clear the intended expression was NV_RDONLY | NV_CLONED. Note that NV_CLONED is itself problematic because it is an alias for the horribly overloaded NV_MISC/NV_IDENT symbol.

If there aren't other bugs of this nature lurking in the code I will be very surprised. The name/value code is far too fragile due to its reliance on ints and constants named NV_something for completely unrelated purposes.

[krader1961]

There are four places that call nvdisc() with a literal zero for the "mode" parameter. Note that two of those places are in the src/cmd/ksh93/sh/bash.c module. The third is in nvdisc.c itself. The fourth is in nval.h. The only reason this works is that nvdisc() special-cases a mode of zero. WTF? An invalid "mode" should be a panic. If zero is meant to be an alias for NV_FIRST (currently defined to be one) for backward compatibility then that should be done in a more transparent manner.

[krader1961]

Another land mine.... Notice that NV_ASETSUB is defined twice: in nval.h and name.h. The two definitions are identical but defining constants in two different headers that serve the same purpose is the sort of thing that can cause hard to diagnose problems.

[krader1961]

I've decided to take a different approach to solving this issue. Instead of simply using enum for the constants I have decided to define types using this idiom: typedef struct { int val; } Nvxyz_t;. The problem with simple enums is that in C they are just ints. Which means the compiler won't complain if you use an int (e.g., 0) that is not one of the enum constants. Using a struct with a single val member does increase the verbosity. But it ensures the compiler (and presumably lint tools) will tell us if a function parameter is a simple int rather than the expected typedef. Whereas a simple enum typedef won't cause any type mismatch warnings.