Susan Sohn wrote: > 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.
Yes I left that in there when it should have been removed. It's been removed. > > > 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. I was thinking that we didn't want to call that in the for loop but you're correct it will only get called once and only if the subcommand is correct. I've moved it. > 148 - please make this a fprintf to stderr. Also, why not make the text > a #define in installadm.h changed to fprintf. Unless I'm missing something this string is only used here so I'm not sure what moving it into installadm.h would buy us. > 179 - why do we try to enable the smf service if the subcommand failed? Because there may be other AI services that should be on and we want to attempt to start things up for those. > 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. Yes it is confusing. I've changed these names as you suggested. > 185 - what happens and what will the output be if a user hasn't created > any services yet, and starts out with "installadm help"? We'll remain in maintenance mode because there are no services. > 291, 294 - if there are no pg's, does that indicate no services and, if so, > should we go into maintenance? Yes I think we should we should. I've changed this so instead of returning we jump down to after the ai_free_pg_list(pg_list). > 300 - there is a constant you can use, SERVICE_STATUS, instead of "status" > 302 - Ditto for STATUS_ON Both fixed. > 311 - if the call to ai_read_property is not AI_SUCCESS, is this an error > that needs to be handled? I don't think so in this case. Either the property group doesn't exist or there was a memory allocation error but either way we end up doing the right thing in that we put the smf service into maintenance. > 324 - Should we print something out for the user, saying we're going > into maintenance? Or does smf do that for us? I'm not sure what you're asking from since we do print out a similar message at line 327. > 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) fixed the fprintf. However moving this line to a #define constant doesn't appear to buy us much for a string used once. > 337 - comment says it returns an error if it goes to maintenance, but > function returns void will fix... > 326,363,369,372,377 - use the libscf defined constants for these states Done. > 352 - good comments > 863,1085,1101,1151 - comment should no longer refer to "data file" changed "data file" to "property group". > > > usr/src/cmd/installadm/installadm.h > > 185 - remove this message, no longer used done. > 191 - might be better to rename this message > 192 - data -> properties changed data to property (or PROP) for both. > > > usr/src/cmd/installadm/installadm_util.c > > 32 - you might not need dirent.h anymore Done > 140 check_port_in_use could use more inline comments comments added... > 130,217,269 - comment should no longer refer to "data file" Fixed. > 191 - propert->proper should have been property. Fixed. > 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 done. > 401 - block comment should no longer refer to data file > Fixed. > > usr/src/cmd/installadm/server.xml > > usr/src/cmd/installadm/setup-service.sh > > usr/src/cmd/installadm/svc-install-server > > 45 - start -> enable fixed > 69 - what happens if the apache web server is already running? I think it's ok but I need to ask Jean about this to make sure I'm not missing something here. > > > Is there supposed to be a Makefile for installadm_test.c? Nope, it's just test stuff. Thanks for the comments! -evan > > 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 > >
