Fixes for few el6 gcc warnings#371
Conversation
src/tools/sssctl/sssctl_cache.c: In function ‘sssctl_create_filter’: src/tools/sssctl/sssctl_cache.c:291: error: declaration of ‘attr_name’ shadows a global declaration src/tools/sssctl/sssctl_cache.c:90: error: shadowed declaration is here src/tools/sssctl/sssctl_cache.c: In function ‘sssctl_find_object’: src/tools/sssctl/sssctl_cache.c:342: error: declaration of ‘attr_name’ shadows a global declaration src/tools/sssctl/sssctl_cache.c:90: error: shadowed declaration is here src/tools/sssctl/sssctl_cache.c: In function ‘sssctl_fetch_object’: src/tools/sssctl/sssctl_cache.c:421: error: declaration of ‘attr_name’ shadows a global declaration src/tools/sssctl/sssctl_cache.c:90: error: shadowed declaration is here src/tools/sssctl/sssctl_cache.c: In function ‘sssctl_print_object’: src/tools/sssctl/sssctl_cache.c:477: error: declaration of ‘attr_name’ shadows a global declaration src/tools/sssctl/sssctl_cache.c:90: error: shadowed declaration is here
cc1: warnings being treated as errors /home/build/sssd/src/tests/sbus_codegen_tests.c: In function ‘test_signals’: /home/build/sssd/src/tests/sbus_codegen_tests.c:118: error: declaration of ‘signal’ shadows a global declaration /usr/include/signal.h:101: error: shadowed declaration is here make[3]: *** [src/tests/sbus_codegen_tests-sbus_codegen_tests.o] Error 1
src/tools/sssctl/sssctl_cache.c: In function ‘sssctl_print_object’: src/tools/sssctl/sssctl_cache.c:350: error: ‘entry’ may be used uninitialized in this function src/tools/sssctl/sssctl_cache.c:350: note: ‘entry’ was declared here src/tools/sssctl/sssctl_cache.c:427: error: ‘entry’ may be used uninitialized in this function src/tools/sssctl/sssctl_cache.c:427: note: ‘entry’ was declared here src/tools/sssctl/sssctl_cache.c:428: error: ‘dom’ may be used uninitialized in this function src/tools/sssctl/sssctl_cache.c:428: note: ‘dom’ was declared here
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
|
LGTM. Will check the warnings on el6 to see if they disappear with this patch. |
|
The patches fix the warnings. Just waiting for the CI before acking. |
|
@fidencio Sorry, I was not aware of the PRs you mentioned (my mistake). However I already reviewed this and I think it is better to have the patches rather then not have them. Because the patches in the PRs that you linked are (almost) identical to the patches in this patch-set, would you agree with the compromise of adding your name in signed-of-by tag and close the other PRs as duplicates? Changing the authorship with the two patches to you would IMO also be ok, because your PRs were older. I do not know what metadata would be more fair in this case. Putting the authorship metadata aside, this patchset just passed our internal CI and I do not see reason to not ACK it. It is better to avoid warnings on supported platforms. |
|
I'd prefer having my name as author (well, as I am the author of the pretty much identical non reviewed patches). Also, I'd prefer to have that patch set reviewed. |
|
Hmm.. it was a different CI result that came. So I am postponing the ACK for a few moments now (but really just waiting for the rawhide machine to finish). Lukas, can you please update the metadata of the two above mentioned patches to change the authorship to Fabiano? Thank you. |
|
Btw, sssctl: Fix warning may be used uninitialized was also part of my patch set and was rejected by @lslebodn for the following reason "I do not think that initialisation to NULL is the best solution for Wunitialized on RHLE6. My experience is that such bugs occurs when there is some suspicious code in functions which are So, NACK to that patch as well (or change my name as author). |
|
Just a note. The CI passed. Codewise I see no blockers for this patchset. I will keep the 'changes_requested' label solely for the requested metadata changes. |
You are not an author of patches. I wrote them from scratch. Unfortunately, your decision was not ideal in different PR and you closed it instead of splitting into two parts. One which could be pushed and another which need to be improved from performance POV, |
That PR was discussed many times face2face and i suggested it there. But I do not have any evidence. |
|
@jhrozek: Would you mind stepping up here? It does look like "content appropriation" to me and it's a really bad thing to see in an open source project, mainly happening inside the very same team. |
|
I can't tell if @lslebodn lslebodn wrote the patches from scratch or not but I assume it's easy to forget about a PR and write the same thing again. Similarly to @mzidek-rh I would prefer either both people in metadata or partly revive the old PR and attribute each patch to one person. |
4 participants
I noticed them as part of checking patch for #370