On Sun, Oct 4, 2015 at 1:18 AM, Andres Freund <and...@anarazel.de> wrote: > Hi, > > I quickly read through the patch, trying to understand what exactly is > happening here. To me the way the patch is split doesn't make much sense > - I don't mind incremental patches, but right now the steps don't > individually make sense.
I agree with Andres. While I looked a bit at this patch, I just had a look at them a whole block and not individually. > On 2015-07-02 14:22:13 -0400, Robbie Harwood wrote: > [Andres' comments] Here are some comments on top of what Andres has mentioned. --- a/configure.in +++ b/configure.in @@ -636,6 +636,7 @@ PGAC_ARG_BOOL(with, gssapi, no, [build with GSSAPI support], krb_srvtab="FILE:\$(sysconfdir)/krb5.keytab" ]) AC_MSG_RESULT([$with_gssapi]) +AC_SUBST(with_gssapi) I think that using a new configure variable like that with a dedicated file fe-secure-gss.c and be-secure-gss.c has little sense done this way, and that it would be more adapted to get everything grouped in fe-auth.c for the frontend and auth.c for the backend, or move all the GSSAPI-related stuff in its own file. I can understand the move though: this is to imitate OpenSSL in a way somewhat similar to what has been done for it with a rather generic set if routines, but with GSSAPI that's a bit different, we do not have such a set of routines, hence based on this argument moving it to its own file has little sense. Now, a move that would make sense though is to move all the GSSAPI stuff in its own file, for example pg_GSS_recvauth and pg_GSS_error for the backend, and you should do the same for the frontend with all the pg_GSS_* routines. This should be as well a refactoring patch on top of the actual feature. diff --git a/src/interfaces/libpq/fe-secure-gss.c b/src/interfaces/libpq/fe-secure-gss.c new file mode 100644 index 0000000..afea9c3 --- /dev/null +++ b/src/interfaces/libpq/fe-secure-gss.c @@ -0,0 +1,92 @@ +#include <assert.h> You should add a proper header to those new files. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
