Michael Paquier <michael.paqu...@gmail.com> writes: > 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.
I'm hearing block from both of you! Okay, if block is desired, I'll squish for v3. Sorry for the inconvenience. >> 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. My understanding is that frontend and backend code need to be separate (for linking), so it's automatically in two places. I really don't want to put encryption-related code in files called "auth.c" and "fe-auth.c" since those files are presumably for authentication, not encryption. I'm not sure what you mean about "rather generic set if routines"; GSSAPI is a RFC-standardized interface. I think I also don't understand the last half of your above paragraph. > 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. Sorry, what?
Description: PGP signature