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
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index dee056b0d6..27865b14a0 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -379,7 +379,7 @@ set_authn_id(Port *port, const char *id)
ereport(LOG,
errmsg("connection authenticated: identity=\"%s\" method=%s "
"(%s:%d)",
- port->authn_id, hba_authname(port), HbaFileName,
+ port->authn_id, hba_authname(port->hba->auth_method), HbaFileName,
port->hba->linenumber));
}
}
diff --git a/src/backend/libpq/hba.c b/src/backend/libpq/hba.c
index b720b03e9a..60767f2957 100644
--- a/src/backend/libpq/hba.c
+++ b/src/backend/libpq/hba.c
@@ -2607,14 +2607,8 @@ fill_hba_line(Tuplestorestate *tuple_store, TupleDesc tupdesc,
else
nulls[index++] = true;
- /*
- * Make sure UserAuthName[] tracks additions to the UserAuth enum
- */
- StaticAssertStmt(lengthof(UserAuthName) == USER_AUTH_LAST + 1,
- "UserAuthName[] must match the UserAuth enum");
-
/* auth_method */
- values[index++] = CStringGetTextDatum(UserAuthName[hba->auth_method]);
+ values[index++] = CStringGetTextDatum(hba_authname(hba->auth_method));
/* options */
options = gethba_options(hba);
@@ -3150,18 +3144,13 @@ hba_getauthmethod(hbaPort *port)
* should not be freed.
*/
const char *
-hba_authname(hbaPort *port)
+hba_authname(UserAuth auth_method)
{
- UserAuth auth_method;
-
- Assert(port->hba);
- auth_method = port->hba->auth_method;
-
- if (auth_method < 0 || USER_AUTH_LAST < auth_method)
- {
- /* Should never happen. */
- elog(FATAL, "port has out-of-bounds UserAuth: %d", auth_method);
- }
+ /*
+ * Make sure UserAuthName[] tracks additions to the UserAuth enum
+ */
+ StaticAssertStmt(lengthof(UserAuthName) == USER_AUTH_LAST + 1,
+ "UserAuthName[] must match the UserAuth enum");
return UserAuthName[auth_method];
}
diff --git a/src/include/libpq/hba.h b/src/include/libpq/hba.h
index 63f2962139..8d9f3821b1 100644
--- a/src/include/libpq/hba.h
+++ b/src/include/libpq/hba.h
@@ -137,7 +137,7 @@ typedef struct Port hbaPort;
extern bool load_hba(void);
extern bool load_ident(void);
-extern const char *hba_authname(hbaPort *port);
+extern const char *hba_authname(UserAuth auth_method);
extern void hba_getauthmethod(hbaPort *port);
extern int check_usermap(const char *usermap_name,
const char *pg_role, const char *auth_user,
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
diff --git a/src/include/libpq/hba.h b/src/include/libpq/hba.h
index 63f2962139..6bd336899c 100644
--- a/src/include/libpq/hba.h
+++ b/src/include/libpq/hba.h
@@ -137,7 +137,7 @@ typedef struct Port hbaPort;
extern bool load_hba(void);
extern bool load_ident(void);
-extern const char *hba_authname(hbaPort *port);
+extern const char *hba_authname(HbaLine *hba);
extern void hba_getauthmethod(hbaPort *port);
extern int check_usermap(const char *usermap_name,
const char *pg_role, const char *auth_user,
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index dee056b0d6..e93404dd9e 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -379,7 +379,7 @@ set_authn_id(Port *port, const char *id)
ereport(LOG,
errmsg("connection authenticated: identity=\"%s\" method=%s "
"(%s:%d)",
- port->authn_id, hba_authname(port), HbaFileName,
+ port->authn_id, hba_authname(port->hba), HbaFileName,
port->hba->linenumber));
}
}
diff --git a/src/backend/libpq/hba.c b/src/backend/libpq/hba.c
index b720b03e9a..a6c88599a0 100644
--- a/src/backend/libpq/hba.c
+++ b/src/backend/libpq/hba.c
@@ -2607,14 +2607,9 @@ fill_hba_line(Tuplestorestate *tuple_store, TupleDesc tupdesc,
else
nulls[index++] = true;
- /*
- * Make sure UserAuthName[] tracks additions to the UserAuth enum
- */
- StaticAssertStmt(lengthof(UserAuthName) == USER_AUTH_LAST + 1,
- "UserAuthName[] must match the UserAuth enum");
/* auth_method */
- values[index++] = CStringGetTextDatum(UserAuthName[hba->auth_method]);
+ values[index++] = CStringGetTextDatum(hba_authname(hba));
/* options */
options = gethba_options(hba);
@@ -3150,18 +3145,13 @@ hba_getauthmethod(hbaPort *port)
* should not be freed.
*/
const char *
-hba_authname(hbaPort *port)
+hba_authname(HbaLine *hba)
{
- UserAuth auth_method;
+ /*
+ * Make sure UserAuthName[] tracks additions to the UserAuth enum.
+ */
+ StaticAssertStmt(lengthof(UserAuthName) == USER_AUTH_LAST + 1,
+ "UserAuthName[] must match the UserAuth enum");
- Assert(port->hba);
- auth_method = port->hba->auth_method;
-
- if (auth_method < 0 || USER_AUTH_LAST < auth_method)
- {
- /* Should never happen. */
- elog(FATAL, "port has out-of-bounds UserAuth: %d", auth_method);
- }
-
- return UserAuthName[auth_method];
+ return UserAuthName[hba->auth_method];
}
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/