On 08/10/10 03:44 AM, Dermot McCluskey wrote:
Keith,
Thanks for reviewing.
This response addresses the comments for the files I worked on:
auto_ddu_lib.c:
auto_install.c:
auto_parse.c:
auto-installer:
On 08/09/10 20:11, Keith Mitchell wrote:
[...]
auto_ddu_lib.c:
61: The name change here (searchall -> search_all) doesn't seem to
have a matching change in the manifest XML.
The manifest was wrong and has been changed. The correct name,
as defined in ai.dtd, is <search_all>.
Ok.
auto_install.c:
395: See prior comment on package names and pkg:/ prepend.
re: pkg URIs. I will change lines 396-399 to:
package_list[0] = strdup("pkg:/SUNWcsd");
package_list[1] = strdup("pkg:/SUNWcs");
package_list[2] = strdup("pkg:/babel_install");
package_list[3] = strdup("pkg:/entire");
Is that correct?
Yes.
re: SUNWcs/SUNWcsd. They should be removed when our gate merges
in the fix for 1637, right?
If you have time to test the removal once build 146 is out, it should be
removed immediately. Otherwise, yes, add a note to bug 15507 to remove
these as well.
auto_parse.c:
156/160: Could you just add a "default:" after line 156?
Will do.
620, 1056: numberical -> numerical
Will fix.
auto-install/default.xml:
35-41: See prior comment on pkg names.
software.dtd:
55: im_type - for curiosity's sake, is there a reason this isn't
"img_type"?
auto-installer:
96-98: I don't suppose now would be a reasonable time to remove this
temporary solution?
Honestly, I'd prefer not to. Much of the AI client is due to be
re-written
when AI is refactored under CUD and I think that's the time to fix
things like this. For now, I'd just like to ensure that the AI client
does
the same thing it did before, but with the new schema.
Fair enough - was just curious.
Thanks,
Keith
Thanks,
- Dermot
text_live.xml, orchestrator-wrappers.c, orchestrator-wrappers.h:
Any particular reason these copyrights are being modified?
installadm.c:
1252-1308: Is there no way to skip processing the options here, and
instead, just pass everything directly to the called script? Same
question for the new do_set_criteria function. There's processing in
set_criteria.py to handle the option parsing, why do it twice?
test_manifest_serv.py:
69: Should there be assertions here? If this is relying on an
exception being raised, it'd be slightly preferred to enclose in a
try/except <Specific exception> clause, with a self.fail(<msg>) in
the except block. This differentiates between "expected" failures,
and completely unrelated errors.
installadm.1m.txt:
1-22: Based on other man pages (both in our gate and elsewhere),
should this even have a CDDL block?
On 08/ 5/10 03:07 PM, Ethan Quach wrote:
Hi all,
The following is the code review for the AI manifest schema changes,
and the installadm criteria changes. It is a rather large review, so
partial/piecewise review would be also be fine, just let us know what
you're reviewing. We've pre-requested reviews from some of you
already, but all comments welcomed by Aug 16th.
Webrev:
-------------
http://cr.opensolaris.org/~equach/webrev.ai-schema/
Bugs:
--------
16423 <http://defect.opensolaris.org/bz/show_bug.cgi?id=16423>
Updates to AI schema should be made
15449 <http://defect.opensolaris.org/bz/show_bug.cgi?id=15449>
installadm add validates combined manifest against image-specific
schema as well as schema in /usr/share/auto_install/
6975043 <http://bugs.opensolaris.org/view_bug.do?bug_id=6975043>
separate criteria and ai manifest
thanks,
-ethan
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
------------------------------------------------------------------------
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
