[sssd PR#140][opened] gcc7 related fixes

[fidencio]

URL: https://github.com/SSSD/sssd/pull/140
Author: lslebodn
 Title: #140: gcc7 related fixes
Action: opened

PR body:
"""

"""

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/140/head:pr140
git checkout pr140

[sssd PR#140][synchronized] gcc7 related fixes

[fidencio]

URL: https://github.com/SSSD/sssd/pull/140
Author: lslebodn
 Title: #140: gcc7 related fixes
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/140/head:pr140
git checkout pr140

[sssd PR#140][comment] gcc7 related fixes

[fidencio]

URL: https://github.com/SSSD/sssd/pull/140
Title: #140: gcc7 related fixes

lslebodn commented:
"""
http://sssd-ci.duckdns.org/logs/job/61/53/summary.html
"""

See the full comment at https://github.com/SSSD/sssd/pull/140#issuecomment-276127472

[sssd PR#140][+Changes requested] gcc7 related fixes

[fidencio]

URL: https://github.com/SSSD/sssd/pull/140
Title: #140: gcc7 related fixes

Label: +Changes requested

[sssd PR#140][comment] gcc7 related fixes

[fidencio]

URL: https://github.com/SSSD/sssd/pull/140
Title: #140: gcc7 related fixes

lslebodn commented:
"""
On (30/01/17 23:52), fidencio wrote:
> fidencio requested changes on this pull request.
>
> Neither the short log or the log are consistent between the patches.
> Please, let's try to follow the git-commit-template.
>
> Also, there are some questions in two of the patches, which are basically curiosity.

Could you be more specific?
There is just missing ticket in messages. Because ther is not a ticket.
> >  case 1:
>     k1 ^= tail[0];
>     k1 *= c1;
>     k1 = rotl(k1, 15);
>     k1 *= c2;
>     h1 ^= k1;
>    break;
>
> Why have you decided to not use SSS_ATTRIBUTE_FALLTHROUGH here as well?

because it does not chage semantic and looks better
then the attribute or comment.
> > @@ -434,8 +434,8 @@ static errno_t sssctl_fetch_object(TALLOC_CTX *mem_ctx,
>                                struct sss_domain_info **_dom)
>
> {
>     TALLOC_CTX *tmp_ctx;
> struct sysdb_attrs *entry;
> struct sss_domain_info *dom;
> struct sysdb_attrs *entry = NULL;
> struct sss_domain_info *dom = NULL;
>
> I'm curious why just this code path triggered the issue.
> We have similar code in a lot of other places (even in the same file).

Could you be more specific?
Because sssctl_find_object is called just once and there is something
smelly (hyper optimized) there.

LS

"""

See the full comment at https://github.com/SSSD/sssd/pull/140#issuecomment-276315279

[sssd PR#140][comment] gcc7 related fixes

[fidencio]

URL: https://github.com/SSSD/sssd/pull/140
Title: #140: gcc7 related fixes

fidencio commented:
"""
On Tue, Jan 31, 2017 at 10:29 AM, lslebodn  wrote:
> On (30/01/17 23:52), fidencio wrote:
> > fidencio requested changes on this pull request.
> >
> > Neither the short log or the log are consistent between the patches.
> > Please, let's try to follow the git-commit-template.
> >
> > Also, there are some questions in two of the patches, which are basically curiosity.
>
> Could you be more specific?
> There is just missing ticket in messages. Because ther is not a ticket.

* sssctl: Fix warning may be used uninitialized:
Component should be in capital letters according to the git-commit-template

* pam_sss: Suppress warning format-truncation
Same comment

* Suppres implicit-fallthrough from gcc 7
Some kind of comments are recognized by gcc 7
but they are ignored with -Wimplicit-fallthrough=5
and only attributes disable the warning.

Please, don't break the lines with 52 characters. Do it on 72 as recommended.
I know the recommendation is "do not exceed 72 (...)" and you don't
exceed. But we're not going to take this path as someone may write a
commit message with a word per-line just because it doesn't exceed the
72 characters :-)
> > > case 1:
> >
> > k1 ^= tail[0];
> > k1 *= c1;
> > k1 = rotl(k1, 15);
> > k1 *= c2;
> > h1 ^= k1;
> > break;
> >
> > Why have you decided to not use SSS_ATTRIBUTE_FALLTHROUGH here as well?
>
> because it does not chage semantic and looks better
> then the attribute or comment.

Fine by me.
> > > @@ -434,8 +434,8 @@ static errno_t sssctl_fetch_object(TALLOC_CTX *mem_ctx,
> >
> > struct sss_domain_info **_dom)
> > {
> > TALLOC_CTX *tmp_ctx;
> > struct sysdb_attrs *entry;
> > struct sss_domain_info *dom;
> > struct sysdb_attrs *entry = NULL;
> > struct sss_domain_info *dom = NULL;
> >
> > I'm curious why just this code path triggered the issue.
> > We have similar code in a lot of other places (even in the same file).
>
> Could you be more specific?
> Because sssctl_find_object is called just once and there is something
> smelly (hyper optimized) there.

Let me try to re-phrase. Is there any other part of the code where we
can be bite by the very same issue?

In a lot of places we don't initialize the output vars and may be the
case we end up hitting this very same "smelly hyper optimization" as
well.

Best Regards,
--
Fabiano Fidêncio

"""

See the full comment at https://github.com/SSSD/sssd/pull/140#issuecomment-276346797

[sssd PR#140][comment] gcc7 related fixes

[fidencio]

URL: https://github.com/SSSD/sssd/pull/140
Title: #140: gcc7 related fixes

lslebodn commented:
"""
On (31/01/17 04:06), fidencio wrote:
> On Tue, Jan 31, 2017 at 10:29 AM, lslebodn  wrote:
> > On (30/01/17 23:52), fidencio wrote:
> > > fidencio requested changes on this pull request.
> > >
> > > Neither the short log or the log are consistent between the patches.
> > > Please, let's try to follow the git-commit-template.
> > >
> > > Also, there are some questions in two of the patches, which are basically curiosity.
> >
> > Could you be more specific?
> > There is just missing ticket in messages. Because ther is not a ticket.
> sssctl: Fix warning may be used uninitialized:
>
> Component should be in capital letters according to the git-commit-template

git log says something different
sh$ git log --oneline | grep " SSSCTL:" | wc -l
1
sh$ git log --oneline | grep " sssctl:" | wc -l
26

And I cannot see a reason to change it.
sssctl looks better in lowercase. becaus eutility is sssctl and not SSSCTL
> pam_sss: Suppress warning format-truncation
>
> Same comment

the same here.
> Suppres implicit-fallthrough from gcc 7
>
> Some kind of comments are recognized by gcc 7
> but they are ignored with -Wimplicit-fallthrough=5
> and only attributes disable the warning.
>
> Please, don't break the lines with 52 characters. Do it on 72 as recommended.
> I know the recommendation is "do not exceed 72 (...)" and you don't
> exceed. But we're not going to take this path as someone may write a
> commit message with a word per-line just because it doesn't exceed the
> 72 characters :-)

If it is the only problem then I can change it before pushing patches :-)
> > > > @@ -434,8 +434,8 @@ static errno_t sssctl_fetch_object(TALLOC_CTX *mem_ctx,
> > >
> > > struct sss_domain_info **_dom)
> > > {
> > > TALLOC_CTX *tmp_ctx;
> > > struct sysdb_attrs *entry;
> > > struct sss_domain_info *dom;
> > > struct sysdb_attrs *entry = NULL;
> > > struct sss_domain_info *dom = NULL;
> > >
> > > I'm curious why just this code path triggered the issue.
> > > We have similar code in a lot of other places (even in the same file).
> >
> > Could you be more specific?
> > Because sssctl_find_object is called just once and there is something
> > smelly (hyper optimized) there.
>
> Let me try to re-phrase. Is there any other part of the code where we
> can be bite by the very same issue?
>
> In a lot of places we don't initialize the output vars and may be the
> case we end up hitting this very same "smelly hyper optimization" as
> well.

I cannot see any other warning related to uninitialized data.
Does it answer your question?

LS

"""

See the full comment at https://github.com/SSSD/sssd/pull/140#issuecomment-276361908

[sssd PR#140][comment] gcc7 related fixes

[fidencio]

URL: https://github.com/SSSD/sssd/pull/140
Title: #140: gcc7 related fixes

fidencio commented:
"""
On Tue, Jan 31, 2017 at 2:25 PM, lslebodn  wrote:
> On (31/01/17 04:06), fidencio wrote:
> > On Tue, Jan 31, 2017 at 10:29 AM, lslebodn
>
> wrote:
> > > On (30/01/17 23:52), fidencio wrote:
> > > > fidencio requested changes on this pull request.
> > > >
> > > > Neither the short log or the log are consistent between the patches.
> > > > Please, let's try to follow the git-commit-template.
> > > >
> > > > Also, there are some questions in two of the patches, which are
>
> basically curiosity.
> > > >
> > >
> > > Could you be more specific?
> > > There is just missing ticket in messages. Because ther is not a ticket.
> > sssctl: Fix warning may be used uninitialized:
> >
> > Component should be in capital letters according to the
>
> git-commit-template
> >
>
> git log says something different
> sh$ git log --oneline | grep " SSSCTL:" | wc -l
> 1
> sh$ git log --oneline | grep " sssctl:" | wc -l
> 26
>
> And I cannot see a reason to change it.
> sssctl looks better in lowercase. becaus eutility is sssctl and not SSSCTL

I'm in favour of updating the commit template then, as it hasn't been
followed.
But that's *not* related to this patchset.
> > pam_sss: Suppress warning format-truncation
> >
> > Same comment
>
> the same here.
> > Suppres implicit-fallthrough from gcc 7
> >
> > Some kind of comments are recognized by gcc 7
> > but they are ignored with -Wimplicit-fallthrough=5
> > and only attributes disable the warning.
> >
> > Please, don't break the lines with 52 characters. Do it on 72 as
>
> recommended.
> > I know the recommendation is "do not exceed 72 (...)" and you don't
> > exceed. But we're not going to take this path as someone may write a
> > commit message with a word per-line just because it doesn't exceed the
> > 72 characters :-)
>
> If it is the only problem then I can change it before pushing patches :-)

Please, do it.
> > > > > @@ -434,8 +434,8 @@ static errno_t sssctl_fetch_object(TALLOC_CTX
>
> *mem_ctx,
> > > > struct sss_domain_info **_dom)
> > > > {
> > > > TALLOC_CTX *tmp_ctx;
> > > > struct sysdb_attrs *entry;
> > > > struct sss_domain_info *dom;
> > > > struct sysdb_attrs *entry = NULL;
> > > > struct sss_domain_info *dom = NULL;
> > > >
> > > > I'm curious why just this code path triggered the issue.
> > > > We have similar code in a lot of other places (even in the same file).
> > >
> > > Could you be more specific?
> > > Because sssctl_find_object is called just once and there is something
> > > smelly (hyper optimized) there.
> >
> > Let me try to re-phrase. Is there any other part of the code where we
> > can be bite by the very same issue?
> >
> > In a lot of places we don't initialize the output vars and may be the
> > case we end up hitting this very same "smelly hyper optimization" as
> > well.
>
> I cannot see any other warning related to uninitialized data.
> Does it answer your question?

Kinda. Actually, it does, but the question was poorly formulated.
Shall we preventively change this in any other places? (And no, it doesn't
mean we should change it as part of this patch).
> LS
>
> —
> You are receiving this because you commented.
> Reply to this email directly, view it on GitHub
> https://github.com/SSSD/sssd/pull/140#issuecomment-276361908, or mute
> the thread
> https://github.com/notifications/unsubscribe-auth/AAG4emwVSas1ubIlJRDRvN75eP-ovHdCks5rXzY1gaJpZM4LxhX8
> .

"""

See the full comment at https://github.com/SSSD/sssd/pull/140#issuecomment-276366120

[sssd PR#140][comment] gcc7 related fixes

[fidencio]

URL: https://github.com/SSSD/sssd/pull/140
Title: #140: gcc7 related fixes

lslebodn commented:
"""
> > > Let me try to re-phrase. Is there any other part of the code where we
> > > can be bite by the very same issue?
> > >
> > > In a lot of places we don't initialize the output vars and may be the
> > > case we end up hitting this very same "smelly hyper optimization" as
> > > well.
> >
> > I cannot see any other warning related to uninitialized data.
> > Does it answer your question?
>
> Kinda. Actually, it does, but the question was poorly formulated.
> Shall we preventively change this in any other places? (And no, it doesn't
> mean we should change it as part of this patch).

compilers/static analysers might found real cases where variable
is used uninitialized. And initializing them to NULL will just hide bugs.

Wmaybe-uninitialized is a little bit complicated due to optimisations with
-O2/-03 and the only reasonable way is to initialize variable or to prevent
inlining of functions. Of course; we can rewrite the function but compiler
might do the same kind of optimisations it needn't be so simple.

LS

"""

See the full comment at https://github.com/SSSD/sssd/pull/140#issuecomment-276382605

[sssd PR#140][-Changes requested] gcc7 related fixes

[fidencio]

URL: https://github.com/SSSD/sssd/pull/140
Title: #140: gcc7 related fixes

Label: -Changes requested

[sssd PR#140][+Accepted] gcc7 related fixes

[fidencio]

URL: https://github.com/SSSD/sssd/pull/140
Title: #140: gcc7 related fixes

Label: +Accepted

[sssd PR#140][comment] gcc7 related fixes

[fidencio]

URL: https://github.com/SSSD/sssd/pull/140
Title: #140: gcc7 related fixes

fidencio commented:
"""
Okay, go for the patches!
"""

See the full comment at https://github.com/SSSD/sssd/pull/140#issuecomment-276422869

[sssd PR#140][comment] gcc7 related fixes

[fidencio]

URL: https://github.com/SSSD/sssd/pull/140
Title: #140: gcc7 related fixes

lslebodn commented:
"""
master:
* bf0b4eb335ec1fb4fdd925f5cf80490ec8b8c24e
* c587e9ae55c618c011bd4dde6a94fe5dc60fff01
* cbb0e683ff11d7800328da3991f3e75ef88f937f
* 2e505786d6d9d537f5b6631099862f6b93e2e687

"""

See the full comment at https://github.com/SSSD/sssd/pull/140#issuecomment-276653863

[sssd PR#140][+Pushed] gcc7 related fixes

[fidencio]

URL: https://github.com/SSSD/sssd/pull/140
Title: #140: gcc7 related fixes

Label: +Pushed

[sssd PR#140][closed] gcc7 related fixes

[fidencio]

URL: https://github.com/SSSD/sssd/pull/140
Author: lslebodn
 Title: #140: gcc7 related fixes
Action: closed

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/140/head:pr140
git checkout pr140