Hi Joe, thank you very much for your comments. Please see my response in line.
Jan Joseph J VLcek wrote: > 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/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? I think this is a good suggestion - I have added information about where/how this document could be obtained. > > 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> Done. > > Perhaps the usage can be put in this README.test file? Done. > > 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. Done. > > 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__() Corrected. > > Issue/Question: > --------------- > > 202 cmd_stderr = None > > Please confirm it is correct to dump stderr? Yes - 'stderr' of the process is captured for debugging purposes if the process failed/was terminated. > > Issue/Question: > --------------- > > Does svc_info_found = True need to be set @ line 239? I don't think so - svc_info_found is set to True at 225 when valid line containing information about service to be looked up is read. > > > 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? Good catch - you are right. I have changed the code, so that it doesn't wait for the process to finish. > > > Issue/Question: > --------------- > > 429 rc = main() > > Would it be possible/better to invoke parse_cli(sys.argv) and simply > do away with main? Done. > > Issue/Question: > --------------- > > 434 sys.exit(99) > > Why sys.exit(99) on except, why not sys.exit(1) ? Done. I have updated the webrev accordingly - it has been posted at the following location: http://cr.opensolaris.org/~dambi/bug-3478-cr/ Could you please take a look and let me know if the changes are fine ? Thank you very much.
