HI William. Thank you for your review.
William Schumann wrote: > Jack, > ai_manifest.rng: > You may want to avail yourself of the <interleave> tag to allow the > new elements to appear in any order: > 188 - optional attributes in "searchall" element - suggest putting > "publisher" and "location" in an interleave section, and to put both > options for "searchall" (187) in an interleave section so that the > optional attributes can appear in any order. > 173 - similarly for "bundle" element - if you don't care about the > ordering, add interleave tags > 210 - similarly for "ai_bundle_type_contents" within the "group" > section if you don't care if they put "type" or "name" first. > If you do indeed want to enforce tag ordering here, never mind. All of the above are attributes. Attributes can be placed in any order without an <interleave> tag. Elements need the <interleave> tag to relax ordering. Reference: http://www.relaxng.org/tutorial-20011203.html#IDA0FYR > > Suggest changing add_drivers to ai_add_drivers. The "ai_" prefix was > added to make top-level element names unique. Also, adding "ai_" will > make the name consistent with other elements at this level. OK. > > auto_install.c: > Suggest that after ai_du_get_and_install() failure, a stderr message > should appear with the log message. Good idea! > Suggest that after ai_setup_manifest(), stderr message should appear > before exiting. Yes. > > auto_install.h: > 384-5 tab before function name for consistency with surrounding code. OK. > > auto_parse.c: > release memory allocated for valfile_base in strdup() with free() Thanks. > > auto_parse_manifest.c: > 270 remove old comment if you are now indeed freeing this memory OK. BTW, Other parts of AI need to call ai_free_manifest_value_list but that will be beyond the scope of this project. I'll file and accept a bug to take care of that separately. > 313 sizeof should not be used in determining length of value list - > len will always be 1 here. The actual length of the list should be > used. Suggest using a NULL to terminate value_list (274 malloc > (list_ln+1)*sizeof..., 278 rv[list_ln] = NULL). Actually, the list itself is malloc'ed as (length * sizeof(char*)) -- see around line 270 -- so what I have is technically correct. That said, the list could be easier to traverse by its users if I NULL-terminated it. > > default.xml: > 30 Your comments here are far better than elsewhere in default.xml. > Consider adding a 1-line comment at the head to summarize the purpose > of the section - it is a bit unclear how it differs from the > ai_install_packages. Thanks. I replaced what I had with a much shorter comment that is more in line with the rest of the file. I've saved the more-complete contents so they can be added to the manpage. > 28,62 If you change add_drivers to ai_add_drivers, make the change > here also. Done. Thanks again for your review. I'll send out an update once I get through the other's comments later today. Thanks, Jack > > William > > On 02/10/10 03:24 AM, Jack Schwartz wrote: >> Hi everyone. >> >> Here is the code review for AI changes to incorporate Driver Update. >> >> Driver Update will allow the installer to install packages of needed >> drivers in its own boot environment before performing the >> installation, and then install those same packages on the target. >> Packages may be explicitly specified in the manifest. They can also >> be found via search, also initiated via the manifest. >> >> http://cr.opensolaris.org/~schwartz/100209.1/webrev/index.html >> >> This webrev is only of AI changes needed to call into the DDU >> library. The DDU team will be posting a separate webrev with their >> library changes. >> >> Testing is ongoing. Testing done: >> - Validation of correct and incorrect manifest entries. >> - Proper calling of DDU library, for explicit package entries >> - Proper calling of DDU library to complete a search and resulting >> installation. >> - Verification that packages are installed when library is >> correctly called, or else sending feedback to the DDU team about >> found library issues. >> - Verified that ManifestServ and DistroConstructor >> >> Please review and send comments/questions by Tues 2/23 COB. >> >> Thanks to the DDU team for helping me on their end, to get this done. >> >> Thanks, >> Jack >> _______________________________________________ >> caiman-discuss mailing list >> caiman-discuss at opensolaris.org >> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
