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