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>
*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.
*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.
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).
356: ret may be uninitialized on success here. You could initialize ret
to AUTO_INSTALL_SUCCESS on 245, and remove line 266.
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.
*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"
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?
*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.
*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?
*usr/src/cmd/auto-install/svc/auto-installer:*
248: Please specify which log files in the message.
*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/20090325/6f7fff98/attachment.html>
