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
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
This looks like a false positive to me. But the solution is non invasive, so ACK.
Michal
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?
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
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.
another way how to suppress warning is to initialize variabl monitor to NULL
LS
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.
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.
Michal
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.
Michal, Jakub it is your decision. (I don't mind) I just want to get rid of this warning.
LS
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?
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?
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..
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
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@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
On Fri, Oct 24, 2014 at 08:45:21PM +0200, Lukas Slebodnik wrote:
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@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
Ack from me as well. Let's not delay a resolution of a simple warning any longer.
Thank you both for the discussion.
On Fri, Oct 24, 2014 at 09:10:10PM +0200, Jakub Hrozek wrote:
warning is fixed.
I'm fine with this version. I let final ACK to jakub :-)
LS
Ack from me as well. Let's not delay a resolution of a simple warning any longer.
Thank you both for the discussion.
* master: cff89439b21f8573c6896b09cb1a8d5f9de3144c
On 10/23/2014 06:38 PM, 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
That is why I said, I think it is false positive, but the warning appears when compiling with -O2. So maybe some strange gcc optimization can cause trouble here under some circumstances. I think it is better to accept Lukas's patch (it at least removes the warning).
Michal
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Lukas Slebodnik -
Michal Židek