Jan Damborsky wrote: > Hi Sundar, > > thank you very much for your comments. > Please see my response in line. > > I have updated the webrev accordingly. > > Just differences from previous one can be seen at > http://cr.opensolaris.org/~dambi/ai-err-report-no-pep8-cr-diff/ > > And full updated webrev is available at > http://cr.opensolaris.org/~dambi/ai-err-report-no-pep8-cr-full/ > > Thank you, > Jan > > > > Sundar Yamunachari wrote: >> Jan, >> >> *usr/src/cmd/auto-install/auto_install.c*: >> >> 750-770: This is not a comment on your webrev. If the user password >> is not supplied, I don't think the user can login to the system. >> The default setup in opensolaris doesn't allow any user to login with >> null password (/etc/default/login: PASSREQ). We may >> have to file a bug not to allow or change the settings in the >> /etc/default/login file > > ok. Would you like me to file bug for this issue ? I can file a bug. > >> >> 1777: This error "Couldn't unmount target BE" still looks cryptic. >> can we come up with a more meaningful error message? > > Sure. I have changed the message to: > > "Couldn't unmount target boot environment. It is left mounted on > '/a'\n")); > > However, I am open to suggestions :-) There are other error messages from orchestrator and libbe if the failure happens. Please remove "It is left mounted on '/a'" and retain "Couldn't unmount target boot environment." > > >> >> *usr/src/cmd/auto-install/auto_parse.c*: >> >> 715: Can you add "from SC manifest" to the error message as follows? >> "Couldn't parse %s property from SC manifest" > > Added. > >> * >> usr/src/cmd/slim-install/svc/live-fs-root*: >> >> 380, 396: Can you add the whole path for solaris.zlib and >> solarismisc.zlib including $url? > > Added. > >> 396: server --> install server > > Changed. > >> >> *usr/src/lib/install_utils/install_utils.py*: >> >> Suggestion: >> 432-436: The checking of log to none can be made similar to other >> checking of log in lines 477-483, 490-495, and 517-522 >> if log == None construct could be changed to if log != None so that >> it will be consistent > > I have changed that code to be consistent as well as to reflect Keith's > suggestion - using 'is not' instead of '!='. That is even better. > >> >> *usr/src/lib/liborchestrator/om_misc.c*: >> >> 83, 87: Can we add more information to the error message like >> "Installing from the package repository failed" if it is AI and >> "Transferring the files from cd failed" in the case of livecd >> installer. If it is difficult to figure out the installer type, you >> can say "Transferring the files from the source failed. Please see >> the previous messages for more details" > > Since we would need to add additional mechanism for distinguishing > between installers in former case, I have changed the code according > to your > latter suggestion. okay.
Thanks, Sundar > >> >> 95: Can we change the error message to "One or more installation >> completion tasks failed. Please see the previous messages for more >> details" > > Changed. > >> >> 59- 96: What there are only 9 errors defined in the array >> om_failure_description_array? Are these the only API used by >> auo_install code? > > yes - those are exposed from the orchestrator to the consumer (AI, GUI) > in case of failure - e.g. by notify_error_status() function. Other error > codes are private to the orchestrator or can't be triggered by external > conditions (e.g. in case of unreachable IPS repo or when invalid data > were > provided). > >> >> *usr/src/lib/liborchestrator/orchestrator_api.h:* >> 285 int code; /* see definitions below for possible >> error codes */ >> What definitions the comments are referring here? > > I removed this comment, since it is misleading. > >
