[PATCH] Null pointer dereferenced.

[Michal Židek]

https://fedorahosted.org/sssd/ticket/1674

Another bug found by Coverity. This patch changes indentation in part of 
the code (so it looks bigger than it actually is). I updated debug 
messages in the changed part with the new SSSDBG_* macros.

Patch is attached.

Thanks
Michal

[Pavel Březina]

On 11/27/2012 01:35 PM, Michal Židek wrote:
> https://fedorahosted.org/sssd/ticket/1674
>
> Another bug found by Coverity. This patch changes indentation in part of
> the code (so it looks bigger than it actually is). I updated debug
> messages in the changed part with the new SSSDBG_* macros.
>
> Patch is attached.
>
> Thanks
> Michal

Hi,
I'm sorry, but can you send those changes in separate patches please?
At least in two - one for the null pointer dereference and second for
the indentation and debug levels.

Thanks.

[Michal Židek]

On 11/27/2012 01:44 PM, Pavel Březina wrote:
> On 11/27/2012 01:35 PM, Michal Židek wrote:
> > https://fedorahosted.org/sssd/ticket/1674
> >
> > Another bug found by Coverity. This patch changes indentation in part of
> > the code (so it looks bigger than it actually is). I updated debug
> > messages in the changed part with the new SSSDBG_* macros.
> >
> > Patch is attached.
> >
> > Thanks
> > Michal
>
> Hi,
> I'm sorry, but can you send those changes in separate patches please?
> At least in two - one for the null pointer dereference and second for
> the indentation and debug levels.
>
> Thanks.

The indentation change is part of the null pointer dereference fix. So 
it would not make sense to split them. I changed the macros only because 
the lines with them were marked as changed already (due to the 
indentation change).

Michal

[Simo Sorce]

On Tue, 2012-11-27 at 13:59 +0100, Michal Židek wrote:
> On 11/27/2012 01:44 PM, Pavel Březina wrote:
> > On 11/27/2012 01:35 PM, Michal Židek wrote:
> > > https://fedorahosted.org/sssd/ticket/1674
> > >
> > > Another bug found by Coverity. This patch changes indentation in part of
> > > the code (so it looks bigger than it actually is). I updated debug
> > > messages in the changed part with the new SSSDBG_* macros.
> > >
> > > Patch is attached.
> > >
> > > Thanks
> > > Michal
> >
> > Hi,
> > I'm sorry, but can you send those changes in separate patches please?
> > At least in two - one for the null pointer dereference and second for
> > the indentation and debug levels.
> >
> > Thanks.
>
> The indentation change is part of the null pointer dereference fix. So 
> it would not make sense to split them. I changed the macros only because 
> the lines with them were marked as changed already (due to the 
> indentation change).

Why the fix requires changing indentation ?

I am not sure I understand what is required here ?

Is the required piece just the

+        if (!el) {
+            ret = EIO;
+            goto done;
+        }

block ?

If so why not just add an
} else {
    ret = EIO;
    goto done;
}

block at the end of the if (el) { } block and not re-indent everything ?

Simo.

[Stephen Gallagher]

On Tue 27 Nov 2012 09:28:56 AM EST, Simo Sorce wrote:
> On Tue, 2012-11-27 at 13:59 +0100, Michal Židek wrote:
> > On 11/27/2012 01:44 PM, Pavel Březina wrote:
> > > On 11/27/2012 01:35 PM, Michal Židek wrote:
> > > > https://fedorahosted.org/sssd/ticket/1674
> > > >
> > > > Another bug found by Coverity. This patch changes indentation in part of
> > > > the code (so it looks bigger than it actually is). I updated debug
> > > > messages in the changed part with the new SSSDBG_* macros.
> > > >
> > > > Patch is attached.
> > > >
> > > > Thanks
> > > > Michal
> > >
> > > Hi,
> > > I'm sorry, but can you send those changes in separate patches please?
> > > At least in two - one for the null pointer dereference and second for
> > > the indentation and debug levels.
> > >
> > > Thanks.
> >
> > The indentation change is part of the null pointer dereference fix. So
> > it would not make sense to split them. I changed the macros only because
> > the lines with them were marked as changed already (due to the
> > indentation change).
>
> Why the fix requires changing indentation ?
>
> I am not sure I understand what is required here ?
>
> Is the required piece just the
>    if (!el) {
>        ret = EIO;
>        goto done;
>    }
>
> block ?
>
> If so why not just add an
> } else {
>      ret = EIO;
>      goto done;
> }
>
> block at the end of the if (el) { } block and not re-indent everything ?
>
> Simo.

Actually, I agree with Michal here. Performing the NULL check first 
reads better (and is more in line with our coding style elsewhere in 
SSSD). The patch's real changes are viewable with 'git diff -w HEAD^' 
after applying it, where it's clear that he's only fixed up three debug 
messages and added the NULL check before the previously-indented 
section.

I'll ack this patch as-is. There is no other appropriate split for it.

[Pavel Březina]

On 11/27/2012 05:42 PM, Stephen Gallagher wrote:
> On Tue 27 Nov 2012 09:28:56 AM EST, Simo Sorce wrote:
> > On Tue, 2012-11-27 at 13:59 +0100, Michal Židek wrote:
> > > On 11/27/2012 01:44 PM, Pavel Březina wrote:
> > > > On 11/27/2012 01:35 PM, Michal Židek wrote:
> > > > > https://fedorahosted.org/sssd/ticket/1674
> > > > >
> > > > > Another bug found by Coverity. This patch changes indentation in
> > > > > part of
> > > > > the code (so it looks bigger than it actually is). I updated debug
> > > > > messages in the changed part with the new SSSDBG_* macros.
> > > > >
> > > > > Patch is attached.
> > > > >
> > > > > Thanks
> > > > > Michal
> > > >
> > > > Hi,
> > > > I'm sorry, but can you send those changes in separate patches please?
> > > > At least in two - one for the null pointer dereference and second for
> > > > the indentation and debug levels.
> > > >
> > > > Thanks.
> > >
> > > The indentation change is part of the null pointer dereference fix. So
> > > it would not make sense to split them. I changed the macros only because
> > > the lines with them were marked as changed already (due to the
> > > indentation change).
> >
> > Why the fix requires changing indentation ?
> >
> > I am not sure I understand what is required here ?
> >
> > Is the required piece just the
> >    if (!el) {
> >        ret = EIO;
> >        goto done;
> >    }
> >
> > block ?
> >
> > If so why not just add an
> > } else {
> >      ret = EIO;
> >      goto done;
> > }
> >
> > block at the end of the if (el) { } block and not re-indent everything ?
> >
> > Simo.
>
> 'git diff -w HEAD^'

This is awesome. Thanks.

[Jakub Hrozek]

On Tue, Nov 27, 2012 at 11:42:02AM -0500, Stephen Gallagher wrote:
> Actually, I agree with Michal here. Performing the NULL check first
> reads better (and is more in line with our coding style elsewhere in
> SSSD). The patch's real changes are viewable with 'git diff -w
> HEAD^' after applying it, where it's clear that he's only fixed up
> three debug messages and added the NULL check before the
> previously-indented section.
>
> I'll ack this patch as-is. There is no other appropriate split for it.

I agree. I like the code flow.

I also tested sysdb upgrade from 0.12 to 0.14 (aka from 6.3 to 6.4) and
it worked fine.

Ack

[Pavel Březina]

On 11/28/2012 01:47 PM, Jakub Hrozek wrote:
> On Tue, Nov 27, 2012 at 11:42:02AM -0500, Stephen Gallagher wrote:
> > Actually, I agree with Michal here. Performing the NULL check first
> > reads better (and is more in line with our coding style elsewhere in
> > SSSD). The patch's real changes are viewable with 'git diff -w
> > HEAD^' after applying it, where it's clear that he's only fixed up
> > three debug messages and added the NULL check before the
> > previously-indented section.
> >
> > I'll ack this patch as-is. There is no other appropriate split for it.
>
> I agree. I like the code flow.
>
> I also tested sysdb upgrade from 0.12 to 0.14 (aka from 6.3 to 6.4) and
> it worked fine.
>
> Ack

This was pushed to master and sssd-1-9.