jan damborsky wrote: > Hello all, > > could I please ask for reviewing fix for following enhancement ? > > 3478 Automated Installer needs support for discovery of valid install services > http://defect.opensolaris.org/bz/show_bug.cgi?id=3478 > > Webrev: > http://cr.opensolaris.org/~dambi/bug-3478/ > > Since this code shares stuff with the 3477 which is currently > in process of code review, the webrev is generated against > slim source gate containing fix for 3477 in order to isolate > changes which are specific to this fix. > > Thank you, > Jan > > > Background: > AI service discovery engine (AISD) takes care of looking up > the install service which is later connected by AI service > choosing engine for obtaining AI and SC (System Configuration) > combined manifest. AISD is provided with name of the service > to look up and the timeout. If the valid install service is > found, address of the machine providing that service along > with port number is saved to the given location (file). > Please refer to AI design specification for more details. > > _______________________________________________ > caiman-discuss mailing list > caiman-discuss at opensolaris.org > http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
Jan, This looks good. My comments are below. Hope this helps. Joe usr/src/Makefile.master 1 line changed +-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-= looks good usr/src/Targetdirs 1 line changed +-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-= looks good usr/src/cmd/Makefile.targ 6 lines changed +-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-= looks good usr/src/cmd/auto-install/Makefile 6 lines changed +-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-= looks good usr/src/cmd/auto-install/README.test 60 lines changed +-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-= Issue/Question/Nit: ------------------- 88 (please refer to AI design specification for more details) Should it state wereh the AI design spec is? Maybe maybe not? Issue/Question/Nit: ------------------- Should something other than _dambi be used in the test procedure example? Suggested change from: 98 * Test procedure 99 [a] Publish service '_dambi' using dns-sd(1M) command 100 $ dns-sd -R _dambi _OSInstall._tcp local 8081 aiwebserver=tio:8081 101 102 [b] Run AISD to look up the service '_dambi' 103 $ ai_sd -n _dambi -o <service_list> To: 98 * Test procedure 99 [a] Publish service '_test_service' using dns-sd(1M) command 100 $ dns-sd -R _test_service _OSInstall._tcp local 8081 aiwebserver=tio:8081 101 102 [b] Run AISD to look up the service '_test_service' 103 $ ai_sd -n _test_service -o <service_list> Perhaps the usage can be put in this README.test file? New usr/src/cmd/auto-install/__init__.py 25 lines changed +-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-= Issue/Suggestion: ----------------- I suggest putting a comment in this file indicating why this empty file is needed. New usr/src/cmd/auto-install/ai_sd.py 435 lines changed +-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-= Issue: ------ 98 """ Metod: found() Comment is for method found() should be method __init__() Issue/Question: --------------- 202 cmd_stderr = None Please confirm it is correct to dump stderr? Issue/Question: --------------- Does svc_info_found = True need to be set @ line 239? Issue/Question: --------------- 291 (cmd_stdout, cmd_stderr) = cmd_popen.communicate() Could this hang here if the process is stuck? Would it be possible/safer to not wait? Issue/Question: --------------- 429 rc = main() Would it be possible/better to invoke parse_cli(sys.argv) and simply do away with main? Issue/Question: --------------- 434 sys.exit(99) Why sys.exit(99) on except, why not sys.exit(1) ? 434 sys.exit(99) usr/src/pkgdefs/SUNWauto-install/prototype_com 13 lines changed +-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
