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.