HI William. Here's a short update to a few items I responded to you about, after taking care of other's comments. Please see below.
Jack Schwartz wrote: > 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. This is no longer an issue. valfile_base has been removed altogether. >> >> 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. Filed bug: 14936 - more of auto-installer needs to make use of ai_free_manifest_value_list >> 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. I discovered usr/src/cmd/auto-install/ai_manifest.xml as an example manifest which had lots more comments than default.xml. I added my Driver Update comments to this file. >> 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. Updated code review will be out shortly. Thanks again, Jack > > 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 > >
