hba.c:3160:18: warning: comparison of unsigned enum expression
Recently (last day or so), I get this warning from gcc 10.2:
-----
hba.c:3160:18: warning: comparison of unsigned enum expression < 0 is always false [-Wtautological-compare]
if (auth_method < 0 || USER_AUTH_LAST < auth_method)
~~~~~~~~~~~ ^ ~
1 warning generated.
-----
Erik
On Wed, Apr 7, 2021 at 1:01 PM Erik Rijkers <er@xs4all.nl> wrote:
Recently (last day or so), I get this warning from gcc 10.2:
-----
hba.c:3160:18: warning: comparison of unsigned enum expression < 0 is always false [-Wtautological-compare]
if (auth_method < 0 || USER_AUTH_LAST < auth_method)
~~~~~~~~~~~ ^ ~
1 warning generated.
-----
This one is from 9afffcb833d3c5e59a328a2af674fac7e7334fc1 (adding
Jacob and Michael to cc)
And it makes sense to give warning on that. AuthMethod is an enum. It
can by definition not have a value that's not in the enum. That check
simply seems wrong/unnecessary.
The only other use fo USER_AUTH_LAST is in fill_hba_line() which also
gets the name of the auth. That one uses :
StaticAssertStmt(lengthof(UserAuthName) == USER_AUTH_LAST + 1,
"UserAuthName[] must match the UserAuth enum");
Which seems like a more reasonable check.
But that also highlights -- shouldn't that function then also be made
to use hba_authname(), and the assert moved into the function? That
seems like the cleanest way?
--
Magnus Hagander
Me: https://www.hagander.net/
Work: https://www.redpill-linpro.com/
On Wed, Apr 7, 2021 at 1:24 PM Magnus Hagander <magnus@hagander.net> wrote:
On Wed, Apr 7, 2021 at 1:01 PM Erik Rijkers <er@xs4all.nl> wrote:
Recently (last day or so), I get this warning from gcc 10.2:
-----
hba.c:3160:18: warning: comparison of unsigned enum expression < 0 is always false [-Wtautological-compare]
if (auth_method < 0 || USER_AUTH_LAST < auth_method)
~~~~~~~~~~~ ^ ~
1 warning generated.
-----This one is from 9afffcb833d3c5e59a328a2af674fac7e7334fc1 (adding
Jacob and Michael to cc)And it makes sense to give warning on that. AuthMethod is an enum. It
can by definition not have a value that's not in the enum. That check
simply seems wrong/unnecessary.The only other use fo USER_AUTH_LAST is in fill_hba_line() which also
gets the name of the auth. That one uses :
StaticAssertStmt(lengthof(UserAuthName) == USER_AUTH_LAST + 1,
"UserAuthName[] must match the UserAuth enum");Which seems like a more reasonable check.
But that also highlights -- shouldn't that function then also be made
to use hba_authname(), and the assert moved into the function? That
seems like the cleanest way?
So to be clear, this is what I'm suggesting.
--
Magnus Hagander
Me: https://www.hagander.net/
Work: https://www.redpill-linpro.com/
Attachments:
hba_authname.patchtext/x-patch; charset=US-ASCII; name=hba_authname.patchDownload+9-20
On Wed, Apr 07, 2021 at 01:24:01PM +0200, Magnus Hagander wrote:
On Wed, Apr 7, 2021 at 1:01 PM Erik Rijkers <er@xs4all.nl> wrote:
Recently (last day or so), I get this warning from gcc 10.2:
Same compiler version here, but I did not get warned. Are you using
any particular flag?
But that also highlights -- shouldn't that function then also be made
to use hba_authname(), and the assert moved into the function? That
seems like the cleanest way?
Good idea, that's much cleaner this way. Do you like the attached?
--
Michael
Attachments:
gcc-hba-warning.patchtext/x-diff; charset=us-asciiDownload+10-20
On Wed, Apr 7, 2021 at 1:57 PM Michael Paquier <michael@paquier.xyz> wrote:
On Wed, Apr 07, 2021 at 01:24:01PM +0200, Magnus Hagander wrote:
On Wed, Apr 7, 2021 at 1:01 PM Erik Rijkers <er@xs4all.nl> wrote:
Recently (last day or so), I get this warning from gcc 10.2:
Same compiler version here, but I did not get warned. Are you using
any particular flag?But that also highlights -- shouldn't that function then also be made
to use hba_authname(), and the assert moved into the function? That
seems like the cleanest way?Good idea, that's much cleaner this way. Do you like the attached?
That's very close to mine (see one email later). Let's bikeshed about
the details. I think it's basically the same for current usecases, but
that taking the UserAuth as the parameter is cleaner and potentially
more useful for the future.
--
Magnus Hagander
Me: https://www.hagander.net/
Work: https://www.redpill-linpro.com/
On Wed, Apr 07, 2021 at 02:01:42PM +0200, Magnus Hagander wrote:
That's very close to mine (see one email later). Let's bikeshed about
the details. I think it's basically the same for current usecases, but
that taking the UserAuth as the parameter is cleaner and potentially
more useful for the future.
Missed it, sorry about that. Using UserAuth as argument is fine by
me. If you wish to apply that, please feel free. I am fine to do
that myself, but that will have to wait until tomorrow my time.
--
Michael
On 2021.04.07. 13:57 Michael Paquier <michael@paquier.xyz> wrote:
On Wed, Apr 07, 2021 at 01:24:01PM +0200, Magnus Hagander wrote:
On Wed, Apr 7, 2021 at 1:01 PM Erik Rijkers <er@xs4all.nl> wrote:
Recently (last day or so), I get this warning from gcc 10.2:
[gcc-hba-warning.patch]
FWIW, this fixes the warning.
(and no, I don't think I am using special gcc settings..)
Erik
On Wed, Apr 7, 2021 at 2:17 PM Michael Paquier <michael@paquier.xyz> wrote:
On Wed, Apr 07, 2021 at 02:01:42PM +0200, Magnus Hagander wrote:
That's very close to mine (see one email later). Let's bikeshed about
the details. I think it's basically the same for current usecases, but
that taking the UserAuth as the parameter is cleaner and potentially
more useful for the future.Missed it, sorry about that. Using UserAuth as argument is fine by
me. If you wish to apply that, please feel free. I am fine to do
that myself, but that will have to wait until tomorrow my time.
Ok, I'll go ahead and push it. Thanks for confirming the fix!
--
Magnus Hagander
Me: https://www.hagander.net/
Work: https://www.redpill-linpro.com/