On (24/10/14 20:30), Michal Židek wrote:
On 10/24/2014 07:13 PM, Jakub Hrozek wrote:
>On Thu, Oct 23, 2014 at 08:11:00PM +0200, Lukas Slebodnik wrote:
>>On (23/10/14 19:26), Michal Židek wrote:
>>>On 10/23/2014 07:09 PM, Jakub Hrozek wrote:
>>>>On Thu, Oct 23, 2014 at 07:02:56PM +0200, Lukas Slebodnik wrote:
>>>>>On (23/10/14 18:38), Jakub Hrozek wrote:
>>>>>>On Thu, Oct 23, 2014 at 06:35:07PM +0200, Jakub Hrozek wrote:
>>>>>>>On Thu, Oct 23, 2014 at 02:14:03PM +0200, Michal Židek wrote:
>>>>>>>>On 10/23/2014 11:36 AM, Lukas Slebodnik wrote:
>>>>>>>>>ehlo,
>>>>>>>>>
>>>>>>>>>src/monitor/monitor.c: In function ‘main’:
>>>>>>>>>src/monitor/monitor.c:2953:24: error: ‘monitor’ may be
used uninitialized
>>>>>>>>> in this function
[-Werror=maybe-uninitialized]
>>>>>>>>> monitor->is_daemon = !opt_interactive;
>>>>>>>>> ^
>>>>>>>>>cc1: all warnings being treated as errors
>>>>>>>>>
>>>>>>>>>This warning is caused be change
579e5d4b7a3ca161ea7518b2996905fa22c15995
>>>>>>>>>"MONITOR: Allow confdb to be accessed by nonroot
user"
>>>>>>>>>
>>>>>>>>>Patch is attached.
>>>>>>>>>
>>>>>>>>>LS
>>>>>>>>>
>>>>>>>
>>>>>>>Not sure I understand, under what circumstances can errno be 0
while
>>>>>>>chown returns 0?
>>>>>>
>>>>>>Err, while chown returns -1
>>>>>Try to explain it gcc with optimization (-O2), but usually gcc does
not lie.
>>>>
>>>>I know, but this code just makes wonder what's going on even now,
I'm
>>>>pretty sure I will have no idea a year from now...
>>>>
>>>>>
>>>>>another way how to suppress warning is to initialize variabl monitor
to NULL
>>>>
>>>>That sounds like a more readable option to me.
>>>
>>>Well... if it really is the case that gcc somehow reorganizes the
>>>optimized code in a way, that errno set by chown is not catched in
>>>the ret variable, then the initialization on NULL will cause crash
>>>(but yes it will suppress the warning). As I said, I think it is
>>>false positive, but I am in favour of the first solution, (the
>>>one I acked already) because it is safer.
>>>o
>
>Is there any way to look at what gcc generates in order to understand
>how we can get to the faulty situation?
We can investigate the assembly code, but I do not think
it is worth the time.
>
>>Michal, Jakub
>>it is your decision. (I don't mind)
>>I just want to get rid of this warning.
>>
>>LS
>
>I'm all for suppressing the warning. But for some reason, the ternary
>operator is not too readable to me..
>
>what about something like:
>
>ret = EINVAL; /* Init ret to suppress gcc warning with high -O level */
>if (errno) ret = errno;
>
>Would that be more readable?
Both ways work for me. The comment in your version is good way
to document why we do this.
>
>Please note I don't like the warning either, I just prefer to only
>upstram code that will be readable even in future when we look for some
>unrelated bug in that area..
Ok. I prepared the patch based on Jakub's suggestion
(with kind approval of Lukas) to speed up the resolution
on this issue.
Michal
From 66f65a3462daef32d9d75974809ddb6de1c940e0 Mon Sep 17 00:00:00
2001
From: Michal Zidek <mzidek(a)redhat.com>
Date: Thu, 23 Oct 2014 10:55:08 +0200
Subject: [PATCH] MONITOR: Fix warning may be used uninitialized
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
This warning is caused be change 579e5d4b7a3ca161ea7518b2996905fa22c15995
"MONITOR: Allow confdb to be accessed by nonroot user"
src/monitor/monitor.c: In function ‘main’:
src/monitor/monitor.c:2953:24: error: ‘monitor’ may be used uninitialized
in this function [-Werror=maybe-uninitialized]
monitor->is_daemon = !opt_interactive;
^
cc1: all warnings being treated as errors
---
src/monitor/monitor.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/monitor/monitor.c b/src/monitor/monitor.c
index 0dea327..b55757d 100644
--- a/src/monitor/monitor.c
+++ b/src/monitor/monitor.c
@@ -1723,7 +1723,9 @@ static errno_t load_configuration(TALLOC_CTX *mem_ctx,
* when SSSD runs as nonroot */
ret = chown(cdb_file, ctx->uid, ctx->gid);
if (ret != 0) {
- ret = errno;
+ /* Init ret to suppress gcc warning with high -O level */
+ ret = EINVAL;
+ if (errno) ret = errno;
DEBUG(SSSDBG_FATAL_FAILURE,
"chown failed for [%s]: [%d][%s].\n",
cdb_file, ret, sss_strerror(ret));
warning is fixed.
I'm fine with this version. I let final ACK to jakub :-)
LS