This patch set fixes a bunch warnings by fidencio · Pull Request #377 · SSSD/sssd

fidencio · 2017-09-08T17:00:36Z

Those patches were part of PR#50, which got stuck for a long time and ended up closed as it didn't seem interesting for the project.

However, after some time another PR (PR#371) fixes the very same issues. So, I'd like to have my patches (at least) considered and, if possible, reviewed.

In any case, this PR is basically PR#50 without the patch that makes use of gnulib many warnings.

fidencio · 2017-09-09T13:02:56Z

retest this, please

lslebodn · 2017-09-13T08:36:31Z

 /* If INI_PARSE_IGNORE_NON_KVP is not defined, use 0 (no effect) */
 #ifndef INI_PARSE_IGNORE_NON_KVP
 #define INI_PARSE_IGNORE_NON_KVP 0
-#warning INI_PARSE_IGNORE_NON_KVP not defined.


I am not sure whether it is the latest version of patch set from #50 because this warning is intentionally here.
And was removed from #50

#50 (comment)

lslebodn · 2017-09-13T08:45:46Z

                                        password_or_responder(password),
                                        sss_krb5_prompter, kr, 0,
-                                        SSSD_KRB5_CHANGEPW_PRINCIPAL,
+                                        discard_const(SSSD_KRB5_CHANGEPW_PRINCIPAL),


It was agreen in #50 that this patch will not be included
#50 (comment)

lslebodn · 2017-09-13T08:47:26Z

 START_TEST(test_signals)
 {
-    const struct sbus_signal_meta *signal;
+    const struct sbus_signal_meta *sm_signal;


I would prefer ssm_singal (Sbus Signal Meta) or sgnl sig

And you kind of agreed in #50 (comment)

lslebodn · 2017-09-13T08:51:32Z

+#define SSSCTL_CACHE_CREATE {_("Cache entry creation date"), SYSDB_CREATE_TIME, attr_time_fn}
+#define SSSCTL_CACHE_UPDATE {_("Cache entry last update time"), SYSDB_LAST_UPDATE, attr_time_fn}
+#define SSSCTL_CACHE_EXPIRE {_("Cache entry expiration time"), SYSDB_CACHE_EXPIRE, attr_expire_fn}
+#define SSSCTL_CACHE_IFP    {_("Cached in InfoPipe"), SYSDB_IFP_CACHED, attr_yesno_fn}


Please check comment #50 (comment)

lslebodn · 2017-09-13T08:56:54Z

There are some patches with initialize some variables to NULL, 0 .. on rhle6
I assume they are caused by old gcc in e6 + combination with -O1. I was enabled due to coverage bug in el6 057f699. I would prefer to drop them

I'm fine with suppressing warnings on el6 with -02

lslebodn · 2017-09-13T08:57:37Z

Thank you for reviving this patchset with fixed warnings

fidencio · 2017-09-13T10:46:59Z

@lslebodn, for some reason when I reset the patchset I didn't have the latest version of the patches :-/

Okay, now I have it and will re-work according to your comments.
May I add your patches in PR #371 here as well? (of course, keeping your authorship)

fidencio · 2017-09-18T12:42:13Z

I have updated the patch set, now using the correct version of the patches done for PR #50. Also, I have added to this PR one patch from PR #371 that was not part of PR #50.

IMHO, PR #371 should be closed and this one should be reviewed instead.

lslebodn · 2017-09-25T12:49:47Z

    }
    assert_non_null(state->dom);
    assert_non_null(recent_dom_info);
-    recent_dom_info->next = NULL;


I could not see such warning in #371 and in was not part of previous patchset.
You might hide some bugs in test. Therefore NACK to this patch.

Rest of patch set is good and can be pushed

Okay, this patch has been dropped from this PR.

Signed-off-by: Fabiano Fidêncio <[email protected]>

Signed-off-by: Fabiano Fidêncio <[email protected]> Reviewed-by: Lukáš Slebodník <[email protected]>

This warning only happens when building SSSD on RHEL6. Signed-off-by: Fabiano Fidêncio <[email protected]> Reviewed-by: Lukáš Slebodník <[email protected]>

This warning only happens when building SSSD on RHEL6. Signed-off-by: Fabiano Fidêncio <[email protected]>

As done for the attr_name_fn to avoid "-Wshadow", let's be consistent and do the same for all the other sssctl_attr_fn functions. Signed-off-by: Fabiano Fidêncio <[email protected]>

This warning only happens when building SSSD on RHEL6. Signed-off-by: Fabiano Fidêncio <[email protected]>

Signed-off-by: Fabiano Fidêncio <[email protected]>

src/responder/common/cache_req/cache_req.c: In function 'cache_req_add_result': src/responder/common/cache_req/cache_req.c:587: error: declaration of 'index' shadows a global declaration /usr/include/string.h:489: error: shadowed declaration is here src/responder/nss/nss_cmd.c:387: error: declaration of 'index' shadows a global declaration /usr/include/string.h:489: error: shadowed declaration is here src/responder/nss/nss_cmd.c: In function 'nss_getent_get_result': src/responder/nss/nss_cmd.c:433: error: declaration of 'index' shadows a global declaration /usr/include/string.h:489: error: shadowed declaration is here src/responder/nss/nss_cmd.c: In function 'nss_endent': src/responder/nss/nss_cmd.c:671: error: declaration of 'index' shadows a global declaration /usr/include/string.h:489: error: shadowed declaration is here src/responder/nss/nss_protocol_netgr.c: In function 'nss_protocol_fill_netgrent': src/responder/nss/nss_protocol_netgr.c:113: error: declaration of 'index' shadows a global declaration /usr/include/string.h:489: error: shadowed declaration is here

fidencio · 2017-09-25T12:54:47Z

Patch set has been updated addressing @lslebodn's comments.

Thanks for the review.

lslebodn · 2017-09-25T13:07:19Z

On (25/09/17 12:54), fidencio wrote: fidencio commented on this pull request. > @@ -195,7 +195,15 @@ struct sdap_get_initgr_state *prepare_state(struct test_sdap_initgr_ctx *ctx, } assert_non_null(state->dom); assert_non_null(recent_dom_info); - recent_dom_info->next = NULL; Okay, this patch has been dropped from this PR.

Do you have an idea why you can see such warning? but others cannot LS

lslebodn · 2017-09-25T13:12:57Z

ACK

fidencio · 2017-09-25T13:15:01Z

@lslebodn, IIRC I saw that in our CI but not on my system.

So, in case I face it again, I'll add it as part of the PR #378.

lslebodn · 2017-09-25T16:25:31Z

retest this please

lslebodn · 2017-09-25T19:37:41Z

master:

fidencio force-pushed the wip/rhel6_warnings branch from cd5fdf2 to 3a03571 Compare September 8, 2017 17:02

This was referenced Sep 8, 2017

Fixes for few el6 gcc warnings #371

Closed

[RFC] Use GNULIB's compiler warning code #50

Closed

[RFC] Use GNULIB's compiler warning code #378

Closed

fidencio changed the title ~~This patch set fixes a bunch RHEL6 warnings~~ This patch set fixes a bunch warnings Sep 9, 2017

lslebodn reviewed Sep 13, 2017

View reviewed changes

fidencio force-pushed the wip/rhel6_warnings branch from 65e029b to 9d9aae9 Compare September 18, 2017 12:39

fidencio removed the Changes requested label Sep 18, 2017

lslebodn reviewed Sep 25, 2017

View reviewed changes

fidencio and others added 11 commits September 25, 2017 14:52

RESOLV: Fix "-Werror=null-dereference" caught by GCC

38c6441

Signed-off-by: Fabiano Fidêncio <[email protected]>

SIFP: Fix "-Wjump-misses-init" caught by GCC

6c470e8

Signed-off-by: Fabiano Fidêncio <[email protected]>

NSS: Fix "-Wold-style-definition" caught by GCC

5802725

Signed-off-by: Fabiano Fidêncio <[email protected]>

TESTS: Fix "-Werror=null-dereference" caught by GCC

d497cf9

Signed-off-by: Fabiano Fidêncio <[email protected]> Reviewed-by: Lukáš Slebodník <[email protected]>

TOOLS: Fix "-Wstack-protector" caught by GCC

856ed42

This warning only happens when building SSSD on RHEL6. Signed-off-by: Fabiano Fidêncio <[email protected]> Reviewed-by: Lukáš Slebodník <[email protected]>

SSSCTL: Fix "-Wshadow" warning caught by GCC

caabe5b

This warning only happens when building SSSD on RHEL6. Signed-off-by: Fabiano Fidêncio <[email protected]>

SSSCTL: Fix "-Wunitialized" caught by GCC

794dbab

This warning only happens when building SSSD on RHEL6. Signed-off-by: Fabiano Fidêncio <[email protected]>

SSSCTL: Use get_ prefix for the sssctl_attr_fn functions

6248e8b

As done for the attr_name_fn to avoid "-Wshadow", let's be consistent and do the same for all the other sssctl_attr_fn functions. Signed-off-by: Fabiano Fidêncio <[email protected]>

TESTS: Fix "-Wshadow" caught by GCC

cf3aea2

This warning only happens when building SSSD on RHEL6. Signed-off-by: Fabiano Fidêncio <[email protected]>

RESPONDER/COMMON: Fix "-Wold-style-definition" caught by GCC

d534b0f

Signed-off-by: Fabiano Fidêncio <[email protected]>

fidencio force-pushed the wip/rhel6_warnings branch from 9d9aae9 to 7487e28 Compare September 25, 2017 12:53

fidencio removed the Changes requested label Sep 25, 2017

Uh oh!

Conversation

fidencio commented Sep 8, 2017

Uh oh!

fidencio commented Sep 9, 2017

Uh oh!

lslebodn Sep 13, 2017

Choose a reason for hiding this comment

Uh oh!

lslebodn Sep 13, 2017

Choose a reason for hiding this comment

Uh oh!

lslebodn Sep 13, 2017

Choose a reason for hiding this comment

Uh oh!

lslebodn Sep 13, 2017

Choose a reason for hiding this comment

Uh oh!

lslebodn Sep 13, 2017

Choose a reason for hiding this comment

Uh oh!

lslebodn commented Sep 13, 2017

Uh oh!

lslebodn commented Sep 13, 2017

Uh oh!

fidencio commented Sep 13, 2017

Uh oh!

fidencio commented Sep 18, 2017

Uh oh!

lslebodn Sep 25, 2017

Choose a reason for hiding this comment

Uh oh!

fidencio Sep 25, 2017

Choose a reason for hiding this comment

Uh oh!

fidencio commented Sep 25, 2017

Uh oh!

lslebodn commented Sep 25, 2017 via email

Uh oh!

lslebodn commented Sep 25, 2017

Uh oh!

fidencio commented Sep 25, 2017

Uh oh!

lslebodn commented Sep 25, 2017

Uh oh!

lslebodn commented Sep 25, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants