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 ? > > 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 :-) > > *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 '!='. > > *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. > > 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.
