Hi Jack, thank you very much for code review !
Jan On 03/26/09 18:12, Jack Schwartz wrote: > HI Jan. > > On 03/26/09 07:21, jan damborsky wrote: >> Hi Jack, >> >> thank you very much for your comments. >> Please see my response in line. >> >> Jan >> >> >> >> On 03/25/09 20:22, Jack Schwartz wrote: >>> >>> Hi Jan. >>> >>> Here are my comments, some echoing our phone conversation. >>> >>> *usr/src/cmd/auto-install/ai_manifest.rng:* >>> >>> lines 120-128: >>> >>> The code review shows that there can be multiple >>> <ai_uninstall_packages> clauses, but I think you only want one. You >>> want to handle multiple packages, not multiple ai_uninstall_packages >>> clauses. >>> >>> What you currently have works with the default.xml file, but the >>> packages will appear together in a single XML element. This >>> precludes the possibility of later adding (xml) attributes for >>> individual packages. >>> >>> The <ai_install_packages> clause looks pretty good. I'm glad that >>> the older <ai_packages> way of containing only a single package is >>> being replaced. The new <ai_install_packages> is now a single >>> clause, which can contain one or more individual xml elements each >>> of which represents a package. However, I would change it slightly >>> to be more like what DC does: >>> >>> How about replacing lines 113 - 115 with this: >>> >>> <element name="pkg"> >>> <attribute name="name"> >>> <text/> >>> </attribute> >>> </element> >>> >>> Then you can list the packages in the manifest like this: >>> >>> <ai_install_packages> >>> <pkg name="SUNWcsd"/> >>> <pkg name="SUNWcs"/> >>> <pkg name="babel_install/> >>> <pkg name="entire>/> >>> </ai_install_packages> >>> >>> I suggest something similar for <ai_uninstall_packages>: >>> >>> <optional> >>> <element name="ai_uninstall_packages> >>> <one_or_more> >>> <element name="pkg"> >>> <attribute name="name"> >>> <text/> >>> </attribute> >>> </element> >>> </one_or_more> >>> </element> >>> </optional> >>> >>> *usr/src/cmd/ai-webserver/default.xml*: OK, but if you change >>> ai_manifest.rng, this will have to change too, to this: >>> >>> <ai_install_packages> >>> <pkg name="SUNWcsd"/> >>> <pkg name="SUNWcs"/> >>> <pkg name="babel_install/> >>> <pkg name="entire>/> >>> </ai_install_packages> >>> <ai_uninstall_packages> >>> <pkg name="babel_install/> >>> <pkg name="slim_install/> >>> </ai_uninstall_packages> >> >> >> This is really good suggestion - thanks ! >> I have incorporated all those changes. >> >>> >>> *usr/src/cmd/auto-install/ai_manifest.defval.xml:* OK. XXX >>> >>> *usr/src/cmd/auto-install/ai_manifest.xml:* >>> >>> If you change ai_manifest.rng as I suggest, this file will need to >>> change too. Changes will be similar those suggested for default.xml >>> above. >> >> I have changed this as well. >> >>> >>> *usr/src/cmd/auto-install/auto_install.c:* >>> >>> 272, 277, 338, 346: This error checking seems overkill to me. The >>> fputs(3C) manpage says that fputs either returns the number of bytes >>> written if OK or else returns EOF (and sets errno). Seems good >>> enough to just check for EOF, rather than complicate the code with >>> calls to strlen() to check the byte count. >> >> This is a good point - I have changed this. >> >>> >>> 296, 304: AIM_PACKAGE_INSTALL_NAME and AIM_PACKAGE_NAME don't seem >>> descriptive enough. I would suggest AIM_PACKAGE_INSTALL_NAME and >>> AIM_OLD_PACKAGE_INSTALL_NAME or some such thing, to differentiate >>> new from old. Also, with this naming convention, the ...OLD... list >>> can be eventually removed cleanly (without traces of old vs new). >> >> Changed. >> >>> >>> 356: ret may be uninitialized on success here. You could initialize >>> ret to AUTO_INSTALL_SUCCESS on 245, and remove line 266. >> >> Changed. >> >>> >>> 239,265: Rather than pass "hardcode" in as an argument, why not >>> envelope lines 265-281 with #ifdef DEBUG. This saves a little bit >>> of memory, requires one less parameter to >>> create_package_list_file(), and since the user would have to >>> recompile the code anyhow to use "hardcode", you are not making >>> things any more difficult for that person. >> >> I am not sure if we could make this code >> as conditionally compiled. Looking at the code, >> auto-install engine can be invoked in two different >> ways: >> >> * auto-install -p <profile> >> * auto-install -d <slice_name> >> >> When the latter form is used, this is when >> package list is generated from hard coded values. >> I assume this mode is for testing, as it bypasses >> processing AI manifest and disk discovery code. > OK. >> >> >>> >>> *usr/src/cmd/auto-install/auto_install.h:** >>> * >>> 141, 143: If ai_manifest.rng changes per above, these lines would >>> change to: >>> >>> 141 #define AIM_PACKAGE_INSTALL_NAME >>> "ai_manifest/ai_install_packages/pkg/name" >>> >>> 143 #define AIM_PACKAGE_REMOVE_NAME >>> "ai_manifest/ai_uninstall_packages/pkg/name" >> >> Done. >> >>> >>> 164-168: I'm not convinced having a hardcoded list is necessary. I >>> also saw this in auto_install.c. How come it is needed? >> >> Please see my comments above. That said, I am not sure if this >> code is still used for testing purposes - I could file bug to >> track the request to evaluate this, since if it this code is >> no longer used, it should be removed. >> >>> >>> *usr/src/cmd/auto-install/auto_parse.c:* >>> >>> Nit: 589, 593, 595: Add a "_p" to all pointers, e.g. num_packages_p >>> and pkg_list_tag_p. >> >> Done. >> >>> >>> *usr/src/cmd/auto-install/auto_parse_manifest.c:* >>> >>> 114: OK. (I think this means that Python returned None (which is >>> something, not NULL, to C), and we're returning NULL to C callers if >>> Python returned None. Is my understanding correct? >> >> Yes - Python code returns None in case when validation >> of AI manifest fails. >> This was actually bug which prevented AI engine from >> correctly recognizing that failure and lead to the core >> dump when invalid manifest descriptor was later accessed. >> >>> >>> *usr/src/cmd/auto-install/svc/auto-installer:* >>> >>> 248: Please specify which log files in the message. >> >> This is a good point - I have added exact location of file which should >> be inspected in case of failure. >> >> I have retested those changes (this time with 110 bits) >> and updated the webrev accordingly: >> >> http://cr.opensolaris.org/~dambi/bug-7415/ >> >> > Looks good. > > One other nit I just saw: > auto-install.c: 294: Remove the word "one" from the message. > No need for me to see another webrev for just this change. > > Thanks, Jan. > > Jack >> >> >>> >>> *usr/src/lib/liborchestrator/perform_slim_install.c:* OK >>> >>> *usr/src/lib/libtransfer/transfermod.h: *OK >>> >>> *usr/src/lib/libtransfer_pymod/libtransfer.c: *OK >>> >>> Note: I went a little fast on the last three files, so another >>> person reviewing will also be helpful. >>> >>> Thanks, >>> Jack >>> >>> >>> On 03/24/09 06:57, jan damborsky wrote: >>>> Hi, >>>> >>>> could I please ask for reviewing changes for following bug ? >>>> >>>> 7415 'slim_install' is left on system when Automated Installer is >>>> used for the installation >>>> http://defect.opensolaris.org/bz/show_bug.cgi?id=7415 >>>> >>>> webrev: >>>> http://cr.opensolaris.org/~dambi/bug-7415 >>>> >>>> While I was there I removed GUI & AI installer specific >>>> code from transfer module and moved it to orchestrator. >>>> >>>> Thank you very much, >>>> Jan >>>> >>>> >>>> modules affected: >>>> ----------------- >>>> * libtransfer >>>> * liborchestrator >>>> * auto-install AI engine >>>> * auto-installer SMF start method >>>> * AI manifests (default & schema) >>>> >>>> testing done: >>>> ------------- >>>> >>>> SW: >>>> * AI images for x86&Sparc based on 109 built from >>>> http://ipkg.sfbay/dev >>>> * LiveCD iso image based on build 109 taken from nana >>>> >>>> test HW - AI clients: >>>> x86: Ultra 20 (1GB RWM) >>>> Sparc: T1000 (8GB RWM) - sun4v >>>> >>>> test procedures: >>>> * regression test - GUI installer tested with >>>> new liborchestrator & libtransfer >>>> >>>> * AI client - installation tried using >>>> - new AI image & old AI default manifest >>>> - succeeded, reduced set of languages installed, >>>> 'slim_install' not removed >>>> >>>> - new AI image & new AI default manifest >>>> - succeeded, full set of languages installed, verified >>>> that babel_install & slim_install removed from installed >>>> system >>>> >>>> - new AI image and invalid manifest (mangled tags) >>>> - failed, appropriate message displayed on console, >>>> auto-installer service went into 'maintenance' mode >>>> >>>> * AI server - new SUNWinstalladm-tools & SUNWauto-install-common >>>> installed >>>> - 'installadm add -m <manifest_name> -n <service_name>' tried with >>>> both >>>> old manifest (<ai_packages> tag) as well as new manifest >>>> (<ai_install_packages> & <ai_uninstall_packages> tags) >>>> >>>> >>>> _______________________________________________ >>>> caiman-discuss mailing list >>>> caiman-discuss at opensolaris.org >>>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >>> >> > -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://mail.opensolaris.org/pipermail/caiman-discuss/attachments/20090326/798c69f9/attachment.html>
