On Wed, Apr 09, 2008 at 11:07:21PM +0200, Mark Phalan wrote: > > On 9 Apr 2008, at 22:55, Will Fiveash wrote: >> 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? > > Simply to keep the amount of change low in order to make re-syncing with > MIT easier. I don't feel strongly about it though.
Let's give your changes back, yes? -- Will Fiveash Sun Microsystems Inc. http://opensolaris.org/os/project/kerberos/
