Re: [libvirt] [PATCH] uml_conf.c: don't return an uninitialized pointer

4 Sep 2009

      Daniel Veillard wrote:
...
On Thu, Sep 03, 2009 at 01:39:25PM +0200, Jim Meyering wrote:
...
Jim Meyering wrote:
...
Daniel P. Berrange wrote:
...
...
...
Actually I did that first, but then un-did it in favor
of the change above.  Why?  because that initialization could
mask a failure to initialize in a new case.
With per-case initialization, we'd detect the bug at
compile/static-analysis time.  With the up-front unconditional
initialization, we cannot, and would have to rely on testing to find it.
It is a tradeoff, but I still prefer the initialization at time of
declaration as a safety net, and we do use this pattern pretty much
everywhere
Ok.  adjusted
Actually, I will now try to convince you that we should
do it the other way.  Not only is it better to have the compiler
tell us about this problem, but if ever someone were to add the
definition that seems to be missing in the default case, clang
would report the preceding initialization (the one you prefer)
as a "dead store" error.
[...]
...
And in every "case" of that switch, there is an assignment to "group".
That renders the preceding assignment useless, and hence clang calls it
a dead initialization.
Just to show you that they're not all so ambiguous, node_device_conf.c:116
has a "dead initialization" that is obviously worth fixing:
virNodeDevCapsDefPtr caps = def->caps;
    ...
    for (caps = def->caps; caps; caps = caps->next) {
Posting that separately.
I must admit I don't see a NULL initialization of a pointer for safety
when entering a routine as a bad thing, actually this could be
considered a standard feature in any highlevel language.
Then perhaps I didn't explain well.
Here's a small example:

    int
    foo (void)
    {
      int ret = 0;
      switch (f())
        {
        case 0:
          do_something ();
          ret = -1;
          break;

        case 1:
          do_something_else ();
          ret = -1;
          break;

        default:
          ret = -2;
          break;
        }

      return ret;
    }

In that function, the initialization of "ret" is officially unnecessary.
Or, make it more like the code in question, initializing to
the value used in the "default" case:

    int
    foo (void)
    {
      int ret = -2;
      switch (f())
        {
        case 0:
          do_something ();
          ret = -1;
          break;

        case 1:
          do_something_else ();
          ret = -1;
          break;

        default:
          break;
        }

      return ret;
    }

However, Dan argued that he wants to be sure "ret" is defined, in case
someone else (one of the case stmts) forgets.  However, that's precisely
the case in which you do *not* want there to be an overarching
definition, because with that "extra", initial assignment, the compiler
cannot warn you if you add a case like this:

        case 2:
          // ...do things, but forget to set "ret"
          break;

However, if you merely declare "ret", with no initial value,
adding that erroneous "case 2" would provoke a warning
about "ret" being used (via the return) undefined.

I prefer to use the compiler as a safety net, whenever possible.
I.e., imho, it is clear that this toy function should be written
like this, with no initial assignment to "ret":

    int
    foo (void)
    {
      int ret;
      switch (f())
        {
        case 0:
          do_something ();
          ret = -1;
          break;

        case 1:
          do_something_else ();
          ret = -1;
          break;

        default:
          ret = -2;
          break;
        }

      return ret;
    }