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. -Mark
