Joe, thank you very much for your help.
Jan Joseph J VLcek wrote: > 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. >> >> >
