Hi everyone.
I reviewed files which relate to schema changes and Driver Update. I
didn't review files I didn't list below. Here are my comments.
verifyXML.py: OK
------------
auto-install/Makefile: OK
---------------------
ai_dtd:
------
How come target has * (which means zero or more)?
ai_manifest.xml:
---------------
I'd like to change the Driver Update section. Issues are outdated packages,
an incorrect searchall entry (already fixed per other's comments), and
some comments (which I admittedly wrote) that need to be clarified or
updated. I would also like the <search_all/> at the bottom not to be
commented.
Below is what I propose. I have attached a diff to help show the
changes since there are more than a few. For the diff, I left searchall
where it was before to aid in showing diffs; however, I move it to the
top as follows:
<add_drivers>
<!--
Driver Updates: This section is for adding driver packages
to the
boot environment before the installation takes place. The
installer can then access all devices on the system. The
packages installed in the boot environment will also be
installed
on the target.
A <searchall> entry performs a search for devices which are
missing their drivers. A repository publisher and location
may be specified, and that repository and its database will
be used. If no publisher and location is specified, the
configured repositories will be used.
(See pkg publisher command.) If <addall> is specified as
"true", then drivers the database says are third-party drivers
will be added like all others; otherwise third-party drivers
will not be added.
<search_all addall="true">
<source>
<publisher name="opensolaris.org">
<origin name="http://pkg.opensolaris.org/release";
</publisher>
</source>
<search_all/>
<software> entries are user-provided specifications of packages
needed in order to perform the install. types are P5I,
SVR4, DU.
A <software_data> action of "noinstall" inhibits adding to
target.
P5I: A pkg(5) P5I file, full path is in
source/publisher/origin.
Path may be to a local file or an http or ftp specification.
<software>
<software_data type="P5I"/>
<source>
<publisher>
<origin
name="http://pkg.opensolaris.org/release/p5i/0/driver/firewire.p5i"/>
</publisher>
</source>
</software>
SVR4: An SVR4 package spec. source/publisher/origin
corresponds
to the directory where the package is located. Name refers
to the
package name subdirectory (for expanded package) or datastream
file.
<software>
<software_data type="SVR4">
<name>my_disk_driver.d"/>
</software_data>
<source>
<publisher>
<origin name="/export/package_dir"/>
</publisher>
</source>
</software>
DU: An ITU (Install Time Update) or Driver Update image.
source/publisher/origin refers to the path just above the
image's
DU directory (if expanded) or the name of the .iso image. All
packages in the image will be added.
<software>
<software_data type="DU"/>
<source>
<publisher>
<origin name="/export/duimages/mydriver.iso"/>
</publisher>
</source>
</software>
-->
<search_all/>
</add_drivers>
default.xml
-----------
Copyright had 2008 removed. Was that intentional?
I plan on adding back <add_drivers> with some bugfixes I'm working on.
Do you want to add it back in a comment now instead, as is in the
current default.xml? If not, please let me know where in the file you
would want me to put it.
software.dtd:
-------------
83: A facet is an option...
89: I'm wondering about the value of the facet set attribute. If it
wasn't there, then if the facet was present it would be "true". To
temporarily disable the facet, one could just comment it out. This is
easier than setting an attribute to false.
ai_parse_manifest.py: OK
auto_ddu_lib.c
--------------
52: With these changes, "bundle" is no longer used in the manifest and
so has no meaning. I would change most references of "bundle".
"software_nodepath" isn't good as "software" is too general. I suggest
"PKGSPEC_NODEPATH" if you don't think that the "pkg" part implies IPS.
Something better?
Along the same lines as above, I suggest:
53: Change LOCN_NODEPATH to ORIGIN_NODEPATH
62: Change SEARCH_LOCN_NODEPATH to SEARCH_ORIGIN_NODEPATH
63: Change SEARCH_PUB_NODEPATH to SEARCH_PUBNAME_NODEPATH
Please change the following *throughout* the file.
- all bundle -> pkgspec (or whatever is chosen for line 52)
- all "location", locn, etc -> "origin"
For example:
839, 856, 859, etc: location -> origin
951: locnlen -> origin_len
952, 967, etc: locations -> origins
954, 967, etc: num_bundles -> num_pkgspecs
Be careful about bundle -> software as it doesn't always work. For example:
994: origin-software -> origin-pkgspec
1723: package <software> -> package specification
1734: package <software> -> package
Nit: 872: AND -> and
1062: addadd -> addall
1168: Message is incorrect (my bad). Should be "Only one origin allowed
per <search_all> entry.
auto_install.c
--------------
272: Nit: "AI manifest" is overused here, since it represents the input
file as well as the AI manifest portion of it. Suggest "input file"
instead of "AI manifest" here.
auto_install.h
--------------
This may be too big a change to make, but...
It seems inconsistent that AI_MANIFEST_FILE is defined as ai.xml,
SC_MANIFEST_FILE is defined as sc_manifest.xml, and the example AI input
manifest is called ai_manifest.xml. My guess is that AI_MANIFEST_FILE
is ai.xml and not ai_manifest.xml to not confuse it with the input
file. I think this can be clarified by something like the following:
AI input manifest: ai_manifest.xml
AI section of input manifest: ai_section.xml
SC section of input manifest: sc_section.xml
(I didn't check below line 100.)
auto_parse.c
------------
99-101 and 111-112: I suggest doing the following instead, for
maintainability and clarity:
dtd_xmllint = "/usr/bin/xmllint --noout --dtdvalid %s --dtdattr %s 2>&1"
cmd_ln = snprintf(NULL, 0, dtd_xmllint, schema, manifest);
cmd = (char *)malloc(cmd_ln + 1);
(void) snprintf(cmd, cmd_ln + 1, dtd_xmllint, schema, manifest);
420, 570, 620, 721, 1056: numberical -> numerical
774: Just curious: where does 191 come from?
840: see it there is -> see if there is
661, 816: I know original code was written this way, but...
I would like to see these functions return status in their function
return, and have the structures of returned info get passed in, rather
than the other way around. The reasons for this are:
- It is easier to check status:
if ((stat = func(&returned_info)) == AUTO_INSTALL_FAILURE) {
- The malloc'ed memory gets allocated and freed in the same function (the
caller).
- This would make these routines more consistent with other routines in
this module. examples:
ai_get_manifest_dump_device_info(), ai_get_manifest_swap_device_info() and
ai_get_manifest_disk_info()
install-tools/ManifestServ.py: OK
-----------------------------
install_utils/DefValProc.py: OK
---------------------------
install_utils/ManifestServ.py
-----------------------------
190-191: defval manifest name can be pushed into the else, since it
won't be used if dtd_schema is set
287: typo: validatei()
288: typo: sepcified -> specified
install_utils/TreeAcc.py
------------------------
Nit: Using an extra set of () and getting rid of the \ on lines
1354-1356 is more readable.
if ((...) and
(...) and
(...)):
Thanks,
Jack
On 08/05/10 15:07, 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
*** /tmp/old_add_drivers Wed Aug 11 09:49:31 2010
--- /tmp/new_add_drivers Wed Aug 11 11:25:33 2010
***************
*** 2,8 ****
<!--
! software_data are specifications of packages needed in order to
! perform the install. types are as follows:
! P5I: A pkg(5) P5I file, full path is in the "source" element.
Path may be to a local file or an http or ftp specification.
--- 2,14 ----
<!--
+ Driver Updates: This section is for adding driver packages to the
+ boot environment before the installation takes place. The
+ installer can then access all devices on the system. The
+ packages installed in the boot environment will also be installed
+ on the target.
! <software> entries are user-provided specifications of packages
! needed in order to perform the install. types are P5I, SVR4, DU.
! A <software_data> action of "noinstall" inhibits adding to target.
! P5I: A pkg(5) P5I file, full path is in source/publisher/origin.
Path may be to a local file or an http or ftp specification.
***************
*** 9,10 ****
--- 15,17 ----
<software>
+ <software_data type="P5I"/>
<source>
***************
*** 12,14 ****
<origin
! name="http://pkg.opensolaris.org/release/p5i/0/SUNW1394.p5i"/>
</publisher>
--- 19,21 ----
<origin
! name="http://pkg.opensolaris.org/release/p5i/0/driver/firewire.p5i"/>
</publisher>
***************
*** 15,17 ****
</source>
- <software_data type="P5I"/>
</software>
--- 22,23 ----
***************
*** 18,21 ****
! SVR4: An SVR4 package spec. location corresponds to what
! pkgadd -d would refer to. Name refers to the package name.
--- 24,29 ----
! SVR4: An SVR4 package spec. source/publisher/origin corresponds
! to the directory where the package is located. Name refers to the
! package name subdirectory (for expanded package) or datastream
! file.
***************
*** 22,23 ****
--- 30,34 ----
<software>
+ <software_data type="SVR4">
+ <name>my_disk_driver.d"/>
+ </software_data>
<source>
***************
*** 27,31 ****
</source>
- <software_data type="SVR4">
- <name>SUNW1394h"/>
- </software_data>
</software>
--- 38,39 ----
***************
*** 33,35 ****
DU: An ITU (Install Time Update) or Driver Update image.
! location refers to the path just above the image's DU directory.
--- 41,45 ----
DU: An ITU (Install Time Update) or Driver Update image.
! source/publisher/origin refers to the path just above the image's
! DU directory (if expanded) or the name of the .iso image. All
! packages in the image will be added.
***************
*** 36,37 ****
--- 46,48 ----
<software>
+ <software_data type="DU"/>
<source>
***************
*** 38,40 ****
<publisher>
! <origin name="/export/duimages/mydriver"/>
</publisher>
--- 49,51 ----
<publisher>
! <origin name="/export/duimages/mydriver.iso"/>
</publisher>
***************
*** 41,43 ****
</source>
- <software_data type="DU"/>
</software>
--- 52,53 ----
***************
*** 44,48 ****
! A <searchall> entry performs a search for devices which are
! missing their drivers. It attempts to locate the correct
! drivers via a database. A repository publisher and location
may be specified, and that repository and its database will
--- 54,57 ----
! A <search_all> entry performs a search for devices which are
! missing their drivers. A repository publisher and location
may be specified, and that repository and its database will
***************
*** 49,51 ****
be used. If no publisher and location is specified, the
! system's configured repositories will be used.
(See pkg publisher command.) If <addall> is specified as
--- 58,60 ----
be used. If no publisher and location is specified, the
! configured repositories will be used.
(See pkg publisher command.) If <addall> is specified as
***************
*** 55,60 ****
! <searchall addall="true">
<source>
! <publisher>
! <origin name="/export/duimages/mydriver"/>
</publisher>
--- 64,69 ----
! <search_all addall="true">
<source>
! <publisher name="opensolaris.org">
! <origin name="http://pkg.opensolaris.org/release";
</publisher>
***************
*** 61,66 ****
</source>
! </software>
! <searchall/>
!
-->
</add_drivers>
--- 70,76 ----
</source>
! <search_all/>
-->
+
+ <search_all/>
</add_drivers>
+
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
