On Wed, Apr 09, 2008 at 10:34:38PM +0200, Mark Phalan wrote: > > On 9 Apr 2008, at 22:19, Will Fiveash wrote: >> On Wed, Apr 09, 2008 at 10:35:39AM +0200, Mark Phalan wrote: >>> >>> On Tue, 2008-04-08 at 15:36 -0500, Will Fiveash wrote: >>>> On Wed, Apr 02, 2008 at 03:07:41PM +0200, Mark Phalan wrote: >>>>> >>>>> Need a code review for the following bug fix: >>>>> >>>>> 6680327 kdb5_util/kdb5_ldap_util core dumps and prints incorrect >>>>> progname on error paths >>>>> >>>>> webrev can be found here: >>>>> http://cr.opensolaris.org/~mbp/6680327-kdb5_util_dump/ >>>> >>>> Is there a reasonable way to put the "extern char *progname;" in some >>>> include file? >>> >>> Yes. Actually it looks like its already in kdb5_util.h (but not >>> kdb5_ldap_util.h). >>> >>> I've updated the webrev to remove the "extern char *progname" from >>> the .c files and added it to kdb5_ldap_util.h. >> >> In several of the .c files I still see: >> 69 +/* Solaris Kerberos */ >> 70 +extern char *progname; >> >> http://cr.opensolaris.org/~mbp/6680327-kdb5_util_dump/usr/src/cmd/krb5/kadmin/dbutil/kdb5_destroy.c.wdiff.html >> >> I'm assuming that for any file that either directly or indirectly >> includes one of the headers with the extern char *progname; definition, >> the redundant definition should be removed. >> >>>> Also, in which file is progname being assigned? >>> >>> progname gets assigned in kdb5_util.c:336 and kdb5_ldap_util.c:323. >> >> I don't see it. Are you sure you regenerated the webrev? > > Sorry. I had generated a new webrev rather than updating the old one but > forgot to mention it... > > http://cr.opensolaris.org/~mbp/6680327-kdb5_util_dump.1
That was the problem. My only remaining comment is in regards to: http://cr.opensolaris.org/~mbp/6680327-kdb5_util_dump.1/usr/src/cmd/krb5/kadmin/dbutil/dump.c.wdiff.html 2580 + programname = progname; - Why not s/programname/progname/ in dump.c? -- Will Fiveash Sun Microsystems Inc. http://opensolaris.org/os/project/kerberos/
