Horton

Thread

Commits

  1. Refactor hba_authname

  2. Add some information about authenticated identity via log_connections

  1. hba.c:3160:18: warning: comparison of unsigned enum expression

    Erik Rijkers <er@xs4all.nl> — 2021-04-07T11:00:48Z

    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

  2. Re: hba.c:3160:18: warning: comparison of unsigned enum expression

    Magnus Hagander <magnus@hagander.net> — 2021-04-07T11:24:01Z

    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/

  3. Re: hba.c:3160:18: warning: comparison of unsigned enum expression

    Magnus Hagander <magnus@hagander.net> — 2021-04-07T11:32:37Z

    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/

  4. Re: hba.c:3160:18: warning: comparison of unsigned enum expression

    Michael Paquier <michael@paquier.xyz> — 2021-04-07T11:57:03Z

    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

  5. Re: hba.c:3160:18: warning: comparison of unsigned enum expression

    Magnus Hagander <magnus@hagander.net> — 2021-04-07T12:01:42Z

    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/

  6. Re: hba.c:3160:18: warning: comparison of unsigned enum expression

    Michael Paquier <michael@paquier.xyz> — 2021-04-07T12:17:45Z

    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

  7. Re: hba.c:3160:18: warning: comparison of unsigned enum expression

    Erik Rijkers <er@xs4all.nl> — 2021-04-07T12:18:13Z

    > 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

  8. Re: hba.c:3160:18: warning: comparison of unsigned enum expression

    Magnus Hagander <magnus@hagander.net> — 2021-04-07T12:20:25Z

    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/

  9. Re: hba.c:3160:18: warning: comparison of unsigned enum expression

    Michael Paquier <michael@paquier.xyz> — 2021-04-07T12:24:14Z

    On Wed, Apr 07, 2021 at 02:20:25PM +0200, Magnus Hagander wrote:
> Ok, I'll go ahead and push it. Thanks for confirming the fix!

Cool.  Thanks!
--
Michael