Re: svn commit: r244604 - head/usr.sbin/gssd

Garrett Cooper wrote:
> On Sun, Dec 30, 2012 at 4:49 PM, Rick Macklem <rmacklem_at_uoguelph.ca>
> wrote:
> > bf1783 wrote:
> >> >Author: rmacklem
> >> >Date: Sat Dec 22 23:21:17 2012
> >> >New Revision: 244604
> >> >URL: http://svnweb.freebsd.org/changeset/base/244604
> >> >
> >> >Log:
> >> >  It was reported via email that some sshds create kerberos
> >> >  credential cache files with names other than /tmp/krb5cc_<uid>.
> >> >  The gssd daemon does not know how to find these credential
> >> >  caches.
> >> >  This patch implements a new option "-s" that does a search for
> >> >  credential cache files, using roughly the same algorithm as the
> >> >  gssd daemon for Linux uses. The gssd behaviour is only changed
> >> >  if the new "-s" option is specified. It also implements two
> >> >  other
> >> >  new options related to the "-s" option.
> >> >
> >> >  Reported by: Piete.Brooks at cl.cam.ac.uk, Herbert Poeckl
> >> >  Tested by: Herbert Poeckl (admin at ist.tugraz.at), Illias A.
> >> >  Marinos
> >> >  MFC after: 2 weeks
> >>
> >> ...
> >>
> >> >+#include <krb5.h>
> >>
> >> Rick:
> >>
> >> This breaks world built WITHOUT_KERBEROS and WITH_GSSAPI.
> >>
> >> Regards,
> >> b.
> > Could you please test the attached patch.
> >
> > Also, if someone who is familiar with the build/Makefile side
> > of things could review this, it would be appreciated.
> 
> 1. I would name WITHOUT_KERBEROS to KERBEROS_SUPPORT in the sourcefile
> and CFLAGS to avoid potential confusion/noise with build logic.
> 
WITHOUT_KERBEROS is used other places, like telnetd. Were you aware of that?
(I just thought it would keep it consistent, but if you think it is better
 to use a different name, I don't care.)

> 2. This code should be revised per style(9):
> 
> +#else
> + fprintf(stderr, "This option not available when built"
> + " without MK_KERBEROS\n");
> + exit(1);
> 
> In particular:
> 
> errx(1, "This option requires Kerberos support");
> 
> Seems more succinct and addresses the actual item at hand.
> 
Yea, I'll switch it to errx(). I just cribbed the code further
down, that used fprintf().

> 3. This could be simplified as well potentially:
> 
> +.if ${MK_KERBEROS} != "no"
> DPADD= ${LIBGSSAPI} ${LIBKRB5} ${LIBHX509} ${LIBASN1} ${LIBROKEN}
> ${LIBCOM_ERR} ${LIBCRYPT} ${LIBCRYPTO}
> LDADD= -lgssapi -lkrb5 -lhx509 -lasn1 -lroken -lcom_err -lcrypt
> -lcrypto
> +.else
> +CFLAGS+= -DWITHOUT_KERBEROS
> +DPADD= ${LIBGSSAPI}
> +LDADD= -lgssapi
> +.endif
> 
> to this:
> 
> DPADD= ${LIBGSSAPI}
> LDADD= -lgssapi
> .if ${MK_KERBEROS} != "no"
> CFLAGS+= -DKERBEROS_SUPPORT
> DPADD+= ${LIBKRB5} ${LIBHX509} ${LIBASN1} ${LIBROKEN} ${LIBCOM_ERR}
> ${LIBCRYPT} ${LIBCRYPTO}
> LDADD+= -lkrb5 -lhx509 -lasn1 -lroken -lcom_err -lcrypt -lcrypto
> .endif
> 
Yea, I can do this change too. I think the latter is more readable.

Thanks, rick

> Thanks!
> -Garrett
> _______________________________________________
> freebsd-current_at_freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-current
> To unsubscribe, send any mail to
> "freebsd-current-unsubscribe_at_freebsd.org"