[systemd-devel] initializing _cleanup_* variables [was: [PATCH 01/12] fsckd daemon for inter-fsckd communication]

[Zbigniew Jędrzejewski-Szmek]

On Wed, Jan 28, 2015 at 04:06:30PM +0000, Dimitri John Ledkov wrote:
> On 28 January 2015 at 15:47, Martin Pitt <martin.pitt at ubuntu.com> wrote:
> > Hey Zbyszek,
> >
> > Zbigniew Jędrzejewski-Szmek [2015-01-28 15:37 +0100]:
> >> > +static int handle_requests(int socket_fd) {
> >> > +        Clients *first = NULL;
> >> > +        usec_t last_activity = 0;
> >> > +        int numdevices = 0, clear = 0;
> >> > +        double percent = 100;
> >> > +        _cleanup_fclose_ FILE *console = NULL;
> >> NULL not necessary (and you don't have it in similar circumstances above ;))
> >
> > How is that not necessary? Just because the very next line initializes
> > it, and there is no exit path before?
Yes.

> > Because in the general case it
> > certainly is necessary, otherwise the _cleanup_ handler will try to
> > free/close/clean up a random pointer value and crash.
Of course, but I think that in this case it is pretty obvious when
that the next line overwrites the NULL.

> > So IMHO it's a good defensive habit to always init _cleanup_* vars
> > with NULL. In the future someone might put some code with "return"
> > before the fopen() below, and then get a crash.
They can screw up in this and a thousand other ways. If there's a bunch
of other code in between, I agree that adding a defensive initialization
is good. In a simple case like this, I'm fine with skipping it.

Please note though, that my initial complaint was that there's an
inconsistency in the patch: one place had it, one didn't.

> > Or is there some gcc magic that I missed? (I thought only global
> > variables were guaranteed to be NULL, not stack vars).
No.
> 
> Well, during systemd build there are some -Wmaybe-uninitialized
> warnings.... in most cases it is code similar to the above case or
> like checking r || check uninitialised variable. Shall we fix all of
> them? In most cases it would be of limited gain, but at least it will
> be clear to see when -Wmaybe-uninitalized warnings catches a real
> thing.
gcc issues false positive warnings in two cases:
1. when the same conditional is used twice, and cannot change:

  bool cond = ...;
  int var;
  if (cond)
     var = ...;
  ...
  if (cond)
     use(var);

2. when there's a loop which must be executed at least one:
   int var;
   while(true) {
      var = ...;
      break;
   }
   use(var);

Adding workarounds is frowned upon, unless they are very simple.  We
try not to have warnings in the -O0 compilation mode. There's just too
many false warnings at higher optimization levels to fix all.

> (or is my compiler old and has false positives?)
Hard to say, not knowing the version :)
gcc5 issues approx. 100000 new warnings.

Zbyszek