Hi Jack, I am not familiar with C to python interfaces, so, I just quickly browse though auto_ddu_lib.c and auto_parse_manifest.c. I don't see anything obviously wrong in those.
I reviewed all the other files in your webrev in detail. My comments to those files are below. If I don't list a file, that means I have no comment about it. ai_manifest.rng * line 171: why are these things "zeroOrMore"? Since "add_drivers" is already optional, I think that if they do specify the tag, then, they should at least specify 1 or more thing. ai_parse_manifest.py: * The ai_verify_manifest_filename() function is just used in ai_create_manifestserv() function, and it really only does one thing that can be done inside the ai_create_manifestserv() function itself, why have a separate function for it? * lines 103-105: the logic to create this temp manifest filename can be part of the schedule_validate() function. Since it is just a temp filename, I don't really see a point in having to pass it in, and having to duplicate the logic here as well as ManifestServ.py, lines 388-389. If you want people to have the flexibility to pass in a name, you can make that an optional argument. * line 110: since the last 3 arguments are optional, and you are just specifying the default values here, why not just not specify them? * The ai_start_manifest_server() function is another 1 liner that can be done inline in the ai_setup_manifestserv() function, why have it in a separate function? ManifestServ.py: * Since the manifest_name is passed in in the __init__() function of the ManifestServ object, why not just keep a copy of manifest_name in the object. That way, you don't need to maintain a copy of it in auto_parse.c. Additionally, you can just use the stored name to compute the schema used for validation, and the defval manifest, and you don't need to compute "valfile_base" separately outside, and pass it in. * line 423: Do you mean to put "raise err" here? auto_parse.c: *line 41: why not initialize manifest_filename to NULL here? * ai_setup_manifest_image() function: valfile_base is allocated but never freed in all the success or failure return points in this function * line 160: should manifest_filename be checked to make sure it is not null before you do "strdup" here. What if ai_create_anifest_image failed for some reason and the value didn't get set? ai_sparc_image.xml: ai_x86_image.xml: * In prior discussions, you mentioned that in order for things to work TODAY, before bug 13109 is fixed, you need to add a 50mb padding to the root archive. I don't see this padding added to the manifests. Is it the plan to not integrate these changes until bug 13109 if fixed? Thanks, --Karen On 02/ 9/10 06:24 PM, 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
