Thank you Jan. The updated webrev looks good to me.
Joe jan damborsky wrote: > 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. > >
