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. > > *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/ > > *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/cd5036c8/attachment.html>
