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

[Jim Meyering]

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;
    }