Hi Evan and Jean, Evan Layton wrote: > My apologies for the delay. I have fixed the problems with the > mis-merge, sanity tested the changes and updated the webrev. I've listed > out the files and split things up between the library and installadm. > Just pick a subset of files and let me know what you're looking at som > we make sure everyting gets covered. > > And thanks again to Sue for catching the mis-merge!!! > > Thanks! > -evan > > installadm files: > usr/src/cmd/installadm/Makefile > usr/src/cmd/installadm/installadm-common.sh
35 - This line was removed in a previous bug fix and should be deleted. I think this is still leftover from the mismerge. > usr/src/cmd/installadm/installadm.c 146 - perhaps move the call to ai_scf_init to line 173? And then you can delete the fini call at 202. 148 - please make this a fprintf to stderr. Also, why not make the text a #define in installadm.h 179 - why do we try to enable the smf service if the subcommand failed? 179/180 - because we are using the word service to refer to both install services and smf services, which is inherently confusing, you might want to consider renaming one or both of these functions so it is obvious what is going on. So something like check_for_enabled_install_services or smf_service_enable_attempt. 185 - what happens and what will the output be if a user hasn't created any services yet, and starts out with "installadm help"? 291, 294 - if there are no pg's, does that indicate no services and, if so, should we go into maintenance? 300 - there is a constant you can use, SERVICE_STATUS, instead of "status" 302 - Ditto for STATUS_ON 311 - if the call to ai_read_property is not AI_SUCCESS, is this an error that needs to be handled? 324 - Should we print something out for the user, saying we're going into maintenance? Or does smf do that for us? 327 - please make this a fprintf to stderr. Also, please make the text a #define in installadm.h (and a blank line would help readability here) 337 - comment says it returns an error if it goes to maintenance, but function returns void 326,363,369,372,377 - use the libscf defined constants for these states 352 - good comments 863,1085,1101,1151 - comment should no longer refer to "data file" > usr/src/cmd/installadm/installadm.h 185 - remove this message, no longer used 191 - might be better to rename this message 192 - data -> properties > usr/src/cmd/installadm/installadm_util.c 32 - you might not need dirent.h anymore 140 check_port_in_use could use more inline comments 130,217,269 - comment should no longer refer to "data file" 191 - propert->proper 266 - write_service_props should be renamed (and block comment updated), maybe to something like set_service_props. ditto for read_service_props - maybe something like get_service_props 401 - block comment should no longer refer to data file > usr/src/cmd/installadm/server.xml > usr/src/cmd/installadm/setup-service.sh > usr/src/cmd/installadm/svc-install-server 45 - start -> enable 69 - what happens if the apache web server is already running? Is there supposed to be a Makefile for installadm_test.c? That's it for now, Sue > usr/src/pkgdefs/SUNWinstall-libs/prototype_com > libaiscf Library files: > usr/src/lib/Makefile > usr/src/lib/libaiscf/Makefile > usr/src/lib/libaiscf/ai_create.c > usr/src/lib/libaiscf/ai_trans.c > usr/src/lib/libaiscf/ai_utils.c > > usr/src/lib/libaiscf/libaiscf.h > > > And if anybody is REALLY board here's the test files... > > optional files: > usr/src/cmd/installadm/test/installadm_test.c > usr/src/cmd/installadm/test/manual_tests > > > Evan Layton wrote: >> Sue pointed out that the webrev shows that I have a mis-merge so if >> you're looking at the changes please stop. I'll get the mis-merge >> fixed and send out a new webrev as soon as possible. >> >> Thanks, >> -evan >> >> >> Evan Layton wrote: >>> >>> We need to get the fix for 7218 code reviewed. There are around 2000 >>> lines of changes and new code so we'll need volunteers that can take >>> sections of the code to look at. I'm sending this out now so the link >>> is out there but I'll want to get things divided up tomorrow morning >>> and get folks looking at this tomorrow. >>> >>> The bug is 7218 >>> http://defect.opensolaris.org/bz/show_bug.cgi?id=7218 >>> >>> Webrev: >>> http://cr.opensolaris.org/~evanl/7218/ >>> >>> Thanks! >>> -evan >>> _______________________________________________ >>> caiman-discuss mailing list >>> caiman-discuss at opensolaris.org >>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >> >> _______________________________________________ >> caiman-discuss mailing list >> caiman-discuss at opensolaris.org >> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss > > _______________________________________________ > caiman-discuss mailing list > caiman-discuss at opensolaris.org > http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
