Hi Paul.
Paul Cunningham said the following on Tuesday 24 February 2009 04:07 PM: > Srirama, > > This looks mainly good to me :-) Just a couple of comments, see below > ... Thanks for the review. Please see my replies inline. > > Paul > > Srirama Sharma wrote: >> >> Requesting a code review for Openwsman. >> >> Openwsman is a project intended to provide an open-source >> implementation of the Web Services Management specification >> (WS-Management) and to expose system management information on the >> underlying operating system using the WS-Management protocol. >> >> Webrev: http://cr.opensolaris.org/~srirama/openwsman/ >> Bug: http://bugs.opensolaris.org/bugdatabase/view_bug.do?bug_id=6797968 > > 1. usr/src/pkgdefs/SUNWopenwsmanr/depend > & usr/src/pkgdefs/SUNWopenwsmanu/depend > Shouldn't SUNWopenwsmanu depend on SUNWopenwsmanr, rather than > SUNWopenwsmanr depending on SUNWopenwsmanu? If that is the case > then you don't need SUNWopenwsmanr/depend and so you can add > "DATAFILES = depend" into SUNWopenwsmanr/Makefile so it > uses the default one The root package consists of SVC manifest and method files. If the user has just installed the root package (before installing the usr package) and then tries to enable the SMF service, it will exit with an error saying " /usr/sbin/openwsmand does not exist". This daemon binary is required to be present on the system and is installed as part of SUNWopenwsmanu package. Hence have added SUNWopenwsmanu (which delivers the daemon binary) as a dependency for the root package. In fact both user and root packages are interdependent. Because one can also see it other way round and say that the configuration files installed by root package is required for the daemon to be invoked and hence SUNWopenwsmanr has to be put as dependency in the SUNWopenwsnau/depend file as you suggest. Not sure if we can make these packages interdependent though ! > > 2. usr/src/pkgdefs/SUNWopenwsmanr/copyright > & usr/src/pkgdefs/SUNWopenwsmanu/copyright > You could concatenate the lines ... > 5 Copyright (c) 1988, 1993 The Regents of .... > 6 Copyright (c) 1990, 1993 The Regents of .... > to a single line (not that it really matters) .. > Copyright (c) 1988 - 1993 The Regents of .... AFAIK, When Copyright years are separated with a "," it doesn't specify a range of years and instead refers to those specific years. Hence I have included all the Copyright statements. > > 3. CDDL HEADER and top of files (eg. SUNWopenwsman?/depend) > The layout is very slight different in a few files (not > that is really matters); missing line space ... > > # CDDL HEADER END > # > > # > # Copyright 2009 Sun Microsystems, Inc. All rights reserved. Done. > > 4. Everything else looks good to me - well done > Thanks, Srirama
