Hi, On Tue, Jul 14, 2020 at 01:47:06PM -0400, Selva Nair wrote: > Sorry for the long delay in getting back to this.. A few minor > nitpicks on style follows:
Thanks for the review. Indeed, my style fu was sort of absent when
I wrote this :-) (it's "mgetty+sendfax" style, with "I must remember
to use vim and :set expandtab!!!", but the braces always escape me...
and Antonio was busy elsewhere :) )
This...:
> On Tue, Jun 23, 2020 at 5:29 AM Gert Doering <g...@greenie.muc.de> wrote:
> > + /* client wants deferred auth
> > + */
> > + if ( strlen(ac_file_name) > 0 )
> > + {
> > + if (do_deferred_pam_auth(fd, ac_file_name,
> > + service, &up) < 0)
> > + {
> > + goto done;
>
> Do we have to abort in this case? This will exit the background
> process and cripple the server while this could be a temporary memory
> pressure causing the fork to fail. Why not just break and plough
> along? The core will fail to get a response via the ac_file, but that
> could happen if the grand-child fails as well -- the server is
> supposed to cope with such failures.
... is something I need to look more closely at. I remember that the
general approach to "mmmh, something failed!" here is "goto done",
but your argument about "yeah, fork() failed, so what - try again
later!" has merits.
> Apart from these minor issues that could be corrected or ignored at
> merge time, all look good.
This is actually a code change, so I think I'll send a v3 with all
the nitpicks fixed (uncrustify is my friend :-) ) and this one changed.
> We should put the usage info into README.auth-pam as that seems to be
> the only documentation of the plugin. Also an entry in changelog?
> Could be a separate patch.
When I do a v3 anyway, I can add this as well. Makes sense to have it
one commit.
gert
--
"If was one thing all people took for granted, was conviction that if you
feed honest figures into a computer, honest figures come out. Never doubted
it myself till I met a computer with a sense of humor."
Robert A. Heinlein, The Moon is a Harsh Mistress
Gert Doering - Munich, Germany g...@greenie.muc.de
signature.asc
Description: PGP signature
_______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
