Jack,
Thanks for reviewing. This addresses your comments for:
ai_parse_manifest.py
auto_ddu_lib.c
auto_install.c
auto_install.h
auto_parse.c
install-tools/ManifestServ.py
install_utils/DefValProc.py
install_utils/ManifestServ.py
install_utils/TreeAcc.py
On 08/11/10 19:56, Jack Schwartz wrote:
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
--------------
(In general I was only planning to change text the user sees,
rather than variable names, unless the meaning of the variable
has completely changed. But these seems safe, reasonable
changes, so I'll go with them.)
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?
I'll go with PKGSPEC_NODEPATH
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
Done
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.
All done.
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.
Done.
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
Due to logic elsewhere (ManifestServ, I think), the manifest file and the
schema file need to have the same basename. So, since we went with
ai.dtd for the schema, we need to use ai.xml for the manifest.
(And no, I don't think it makes sense to change ManifestServ at this
stage so that files used purely within the apps can have more correct
names ;) )
(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);
I like this. Will fix.
420, 570, 620, 721, 1056: numberical -> numerical
Done.
774: Just curious: where does 191 come from?
/usr/include/sys/dktp/fdisk.h
#define SUNIXOS2 191 /* Solaris UNIX partition */
840: see it there is -> see if there is
Fixed
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()
I agree it would be nice to tidy up and re-arrange some of the code
in the way you suggest, but bear in mind that this module is due to
be significantly modified (in fact, probably completely re-written)
soon when AI is re-factored as part of CUD. So, I don't believe it
makes sense to make this kind of change to the code at this stage.
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
I'm not sure about this. The user might call the constructor with
full_init=False and later call set_defaults() or semantic_validate()
directly, which both access self.defval_manifest_name, so it should
be set, if possible. eg
ms = ManifestServ(manifest_name,
full_init=False,
dtd_schema=True)
...
ms.set_defaults()
ms.semantic_validate()
Admittedly, this is not a very logical thing to do, but I don't think
it makes sense to prevent self.defval_manifest_name being set here?
287: typo: validatei()
288: typo: sepcified -> specified
Fixed.
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
(...)):
Fixed.
Thanks,
- Dermot
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
------------------------------------------------------------------------
_______________________________________________
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
